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

Noah/fix omezarr tile math mistake #45

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

froyo-np
Copy link
Collaborator

@froyo-np froyo-np commented Dec 2, 2024

What

  • fix a mistake that would cause the visibility determination part of the ome-zarr loader to return tiles that were malformed, causing various visual glitches.

How

  • tiles in an image layer must not exceed the total dimensions of that image layer.
  • TODO: a simple Unit test

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

  • If possible add screenshots of the visible additions in the UI.
  • If there are changes in the UI, add Before and After Screenshots for quick overview.
  • If there was a Figma design, add a link to that here as well.
  • Hint : Drag and Drop any images you want to add to the PR. Also you can create a gif of an interactive version and add that!

PR Checklist

  • Is your PR title following our conventional commit naming recommendations?
  • Have you filled in the PR Description Template?
  • Is your branch up to date with the latest in main?
  • Do the CI checks pass successfully?
  • Have you smoke tested the example applications?
  • Did you check that the changes meet accessibility standards?
  • Have you tested the application on these browsers?
    • Chrome (Fully supported)
    • Firefox (Major bug fixes supported)
    • Safari (Major bug fixes supported)

…he size of the image layer as a whole. add a super-quick pan to the example app. update the version.
@froyo-np froyo-np requested a review from a team as a code owner December 2, 2024 20:21
@froyo-np froyo-np requested review from TheMooseman, crispipear and lanesawyer and removed request for crispipear December 2, 2024 20:21
@@ -30,7 +30,7 @@ function visitTilesWithin(idealTilePx: vec2, size: vec2, bounds: box2D, visit: (
for (let y = Math.floor(boundsInTiles.minCorner[1]); y < Math.ceil(boundsInTiles.maxCorner[1]); y += 1) {
// all tiles visited are always within both the bounds, and the image itself
const lo = Vec2.mul([x, y], idealTilePx);
const hi = Vec2.add(lo, idealTilePx);
const hi = Vec2.min(size, Vec2.add(lo, idealTilePx));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the fix - it limits the visited tiles by the total size of the image. The bug here caused a bunch of math later in the process to be incorrect, because the tiles extended past what was actually possible.

Copy link
Contributor

@TheMooseman TheMooseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working for me, not seeing the artifacting I saw in my PR!

@froyo-np froyo-np merged commit 11ca0dc into main Dec 2, 2024
5 checks passed
@froyo-np froyo-np deleted the noah/fix-omezarr-tile-math-mistake branch December 2, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants