-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: dzi viewer would loop forever due to some faulty math #43
Conversation
…tility to pick a layer index less than zero. This would cause a while-loop to run indefinitely, causing a hang. added unit tests, sanitize the layer math, and protect imageSizeAtLayer() from bogus inputs
@@ -93,7 +93,7 @@ export function getVisibleTiles(dzi: DziImage, camera: { view: box2D; screenSize | |||
export function firstSuitableLayer(imageWidth: number, screenWidth: number) { | |||
const idealLayer = Math.ceil(Math.log2(screenWidth)); | |||
const biggestRealLayer = Math.ceil(Math.log2(imageWidth)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the log of numbers smaller than 1 can be negative. In many circumstances, the width passed in as what is needed could be very small, and in that case, the result would be a layer at a negative index, which of course does not exist.
packages/dzi/src/loader.ts
Outdated
let total: vec2 = [size.width, size.height]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue here is that the while loop below will use ceil after dividing the total by 2 - that means that if maxLayerSize was a number less than 1 (for example 2**-1 == 0.5), then the progressive dividing of total would never reach the exit condition, resulting in an infinite loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using 0 as a fallback value is fine, because 2**0 = 1
packages/dzi/src/loader.ts
Outdated
let total: vec2 = [size.width, size.height]; | ||
|
||
while (total[0] > layerMaxSize || total[1] > layerMaxSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo:
if we didn't use the ceil at each step, this while-loop could be replaced with a closed-form expression... its not clear to me if that would give us the correct answer for all scenarios though - this has to match the (rather poorly documented) specification of the DZI format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay for tests to make sure we don't break this when you're on leave 😁
And the fix makes sense!
WIP Steps
What
Fix some poor mathy assumptions in code dealing with Level-of-detail determination in the dzi loader.
How
Replace this txt describing what kind of technical overlaying code changes were introduced here.
Screenshots
This section is optional if there are no visible changes
PR Checklist
main
?