Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 21, 2024
Merged

fix: dzi viewer would loop forever due to some faulty math #43

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b569d13
fix a bug, in which passing a very small camera would cause the dzi u…
froyo-np Nov 19, 2024
2e49dc7
switch to a closed form computation - it passed all the tests, what c…
froyo-np Nov 20, 2024
1fbb17c
Merge branch 'main' into noah/fix-impossible-dzi-math
froyo-np Nov 20, 2024
1f3c11e
Merge branch 'main' into noah/fix-impossible-dzi-math
froyo-np Nov 21, 2024
f86312f
fmt
froyo-np Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/dzi/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@alleninstitute/vis-dzi",
"version": "0.0.4",
"version": "0.0.5",
"contributors": [
{
"name": "Lane Sawyer",
Expand Down Expand Up @@ -55,4 +55,4 @@
"lodash": "^4.17.21",
"regl": "^2.1.0"
}
}
}
11 changes: 11 additions & 0 deletions packages/dzi/src/loader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ describe('tiling math', () => {
const pretend_max_image_width = 512;
expect(firstSuitableLayer(pretend_max_image_width, 4096)).toEqual(9);
});
it('never picks a layer that cant exist (negative layer indexes)', () => {
expect(firstSuitableLayer(512, 0.00001)).toEqual(0);
});
});
it('divide 512 into 2 chunks', () => {
const intervals = tileWithOverlap(512, 256, 1);
Expand Down Expand Up @@ -51,6 +54,14 @@ describe('tiling math', () => {
const size = imageSizeAtLayer(highsmith, 9);
expect(size).toEqual([220, 289]);
});
it('tilesInLayer does not loop indefinitly when given a nonsensical layer index', () => {
const size = imageSizeAtLayer(highsmith, -1);
expect(size).toEqual(imageSizeAtLayer(highsmith, 0));
});
it('tilesInLayer does not loop indefinitly when given a NAN layer index', () => {
const size = imageSizeAtLayer(highsmith, Number.NaN);
expect(size).toEqual(imageSizeAtLayer(highsmith, 0));
});
it('matches observed image dimensions (https://openseadragon.github.io/example-images/highsmith/highsmith.dzi) at layer 9', () => {
const tiles = tilesInLayer(highsmith, 9);
expect(tiles.length).toBe(2);
Expand Down
25 changes: 17 additions & 8 deletions packages/dzi/src/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Collaborator Author

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.

return Math.min(biggestRealLayer, idealLayer);
return Math.max(0, Math.min(biggestRealLayer, idealLayer));
}

/**
Expand Down Expand Up @@ -121,14 +121,23 @@ export function tileWithOverlap(total: number, step: number, overlap: number): I
function boxFromRowCol(row: Interval, col: Interval) {
return Box2D.create([col.min, row.min], [col.max, row.max]);
}

const logBaseHalf = (x: number) => Math.log2(x) / Math.log2(0.5);

export function imageSizeAtLayer(dzi: DziImage, layer: number) {
const { size } = dzi;
const layerMaxSize = 2 ** layer;
let total: vec2 = [size.width, size.height];
while (total[0] > layerMaxSize || total[1] > layerMaxSize) {
total = Vec2.ceil(Vec2.scale(total, 1 / 2));
}
return total;
const { size: dim } = dzi;
const layerMaxSize = 2 ** (isFinite(layer) ? Math.max(0, layer) : 0);
const size: vec2 = [dim.width, dim.height];
// the question is how many times do we need to divide size
// by 2 to make it less than layerMaxSize?
// solve for N, X = the larger the image dimensions:
// X * (0.5^N) <= maxLayerSize ...
// 0.5^N = maxLayerSize/X ...
// log_0.5(maxLayerSize/X) = N
const bigger = Math.max(size[0], size[1]);
const N = Math.ceil(logBaseHalf(layerMaxSize / bigger))
return Vec2.ceil(Vec2.scale(size, 0.5 ** N));

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

}
export function tilesInLayer(dzi: DziImage, layer: number): box2D[][] {
const { overlap, tileSize } = dzi;
Expand Down
Loading