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

Again fix translate of overlays (this time with tests) #6927

Merged
merged 7 commits into from
Jun 23, 2024

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented May 23, 2024

References and relevant issues

closes #6897

fix again bug from #6633

Description

This PR is fixing again problems with transforms of overlays, this time it contains basic tests.

@Czaki Czaki added the bugfix PR with bugfix label May 23, 2024
@Czaki Czaki added this to the 0.5.0 milestone May 23, 2024
@github-actions github-actions bot added the tests Something related to our tests label May 23, 2024
@Czaki
Copy link
Collaborator Author

Czaki commented May 23, 2024

@psobolewskiPhD @brisvag During fixing this I hit multiple walls (before I decided to use corner pixels). Could you please try to play with yours datasets and validate if it looks correctly?

@napari/core-devs testing this is not trivial, but looks really required. Maybe one of you will have idea how to improve tests.

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.76%. Comparing base (6d207bf) to head (0fafb5c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6927      +/-   ##
==========================================
- Coverage   92.82%   92.76%   -0.07%     
==========================================
  Files         613      613              
  Lines       55640    55681      +41     
==========================================
+ Hits        51647    51651       +4     
- Misses       3993     4030      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jni
Copy link
Member

jni commented May 24, 2024

Sorry, I missed the earlier discussions — can you tell me what a "child node" is in the context of multiscales? Are they tiles? Or...?

@jni
Copy link
Member

jni commented May 24, 2024

Adding docstrings to the tests would be really handy. "Issue #6897 showed that we were offsetting overlays for multiscale layers incorrectly because [...]. This test checks against that by creating a multiscale layer then [...], which previously would have done [...] but now does [...]."

@andy-sweet
Copy link
Member

@napari/core-devs testing this is not trivial, but looks really required. Maybe one of you will have idea how to improve tests.

Not sure if it's helpful, but in general, I like the vispy integration tests like the ones you wrote. They exercise a critical large integration of napari's code, without bringing in the whole viewer. So I think a little complexity/maintenance is worth it. Will try to take a more detailed look later today or tomorrow!

@Czaki
Copy link
Collaborator Author

Czaki commented May 24, 2024

Sorry, I missed the earlier discussions — can you tell me what a "child node" is in the context of multiscales? Are they tiles? Or...?

Child nodes are overlays at this moment. I'm not familiar with async rendering.

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Possible followup: I think the "nice" way to solve this would be to have multiscale layers be an custom empty node in the vispy scenegraph with overloaded bounds, so that from the outside it looks like the full normal-sized image. Then, the tiles should be children of this.

@Czaki
Copy link
Collaborator Author

Czaki commented May 24, 2024

Possible followup: I think the "nice" way to solve this would be to have multiscale layers be an custom empty node in the vispy scenegraph with overloaded bounds, so that from the outside it looks like the full normal-sized image. Then, the tiles should be children of this.

Maybe even have ImageMultiscale and LabelsMultiscale multiscale classes that will be selected in Image.__new__ and Labels.__new__ if multiscale data are passed so we could have a clean independent implementation without multiple if. And then we could implement Vispy visual (or another backend) as you described.

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Sorry, I missed the earlier discussions — can you tell me what a "child node" is in the context of multiscales? Are they tiles? Or...?

@jni, child_node here always refers to vispy nodes that are children of the actual "data" node, i.e: overlays. In the case of multiscale, the data node is the tile. The child is still an overlay. The reason why this needs special treatment is that overlays like bounding box and transform box are relying on the parent node's transforms and bounds() method to know "how big" the data is, and in the case of multiscale this needs to basically undo the tiling step, IIRC.

napari/_vispy/layers/base.py Outdated Show resolved Hide resolved
Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Ok I've updated the code comment to be (I think) a bit clearer, otherwise looks good to me, thank you @Czaki and @brisvag for clarifying for me, and sorry that this fell off my radar.

@jni jni added the ready to merge Last chance for comments! Will be merged in ~24h label Jun 22, 2024
@jni jni merged commit e53d666 into napari:main Jun 23, 2024
37 checks passed
@jni jni deleted the fix_rotate_shift branch June 23, 2024 13:41
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform mode of Labels layer pivots wrong when non-empty Shapes/Points present
4 participants