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.

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(viewer.js): Avoid rounding zoom factors that are too close to avoid duplicated resolution errors #107

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

igoroctaviano
Copy link
Collaborator

@igoroctaviano igoroctaviano commented Oct 31, 2023

Address Error caused by rounding the zoom factor:: ImagingDataCommons/slim#175

(level 0)
baseTotalPixelMatrixColumns / totalPixelMatrixColumns = 30920 / 30920 = 1
(level 1)
baseTotalPixelMatrixColumns / totalPixelMatrixColumns = 30920 / 1932 = 16.00414078674948 (16)
(level 2)
baseTotalPixelMatrixColumns / totalPixelMatrixColumns = 30920 / 1920 = 16.104166666666668 (16)

This way the resolutions array will be [16, 16, 1], and although its descending OpenLayers assertion doesn't allow equivalent numbers, the resolutions array should be composed of unique resolutions in descending order.

Removing the rounding here:

const zoomFactor = Math.round(

Results:
Screenshot 2023-10-30 at 19 21 13

@igoroctaviano igoroctaviano changed the title Avoid repeated resolutions caused by rounded zoom factors Avoid rounding zoom factors that are too close to avoid duplicated resolution errors Nov 2, 2023
@fedorov
Copy link
Member

fedorov commented Nov 21, 2023

@igoroctaviano following the discussion and review by @dclunie, let's hold this until we have a chance to review together.

@igoroctaviano igoroctaviano changed the title Avoid rounding zoom factors that are too close to avoid duplicated resolution errors fix(viewer.js): Avoid rounding zoom factors that are too close to avoid duplicated resolution errors May 8, 2024
@fedorov
Copy link
Member

fedorov commented Dec 10, 2024

I agree with @dclunie we should fix this...

@fedorov fedorov reopened this Dec 10, 2024
@fedorov
Copy link
Member

fedorov commented Dec 11, 2024

I don't understand why we have these close resolution factors to begin with. I understand that's what was in the data converted, but why?

In principle, this proposed solution looks harmless to me. @dclunie what are your thoughts?

@dclunie
Copy link

dclunie commented Dec 11, 2024

Why? I suspect it is because of the area of tissue size (which varies), and the scanner vendor strategy of downsampling as many times as is necessary until some limit is reached, then also including a single layer that is the apex of the pyramid as a thumbnail, and sometimes these coincide fairly closely. But I am just speculating, and haven't examined this issue that closely.

@igoroctaviano igoroctaviano merged commit 3fe1dda into master Dec 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants