Overlay backend refactor #4907

brisvag · 2022-08-04T15:17:57Z

Description

This is intended as a precursor to #4894. Most changes in this PR should be refactor only, with minimal changes to the public API. This way, we can discuss api details without this whole mess of diff in the way.

In short, these changes can be divided in 3 main branches:

create _vispy/overlays/ and refactor the current code in VispyTextOverlay & Co. to share a similar structure as the VispyLayers
separate as much as possible from the "glue code" in _vispy/overlays/ the "visual code", which now lives in _vispy/visuals/
Do a similar unification on the model side of things inside napari/components/overlays

Some quirks:

InteractionBox was left mostly untouched: this is because I think it should be unified with the BoundingBox from Layer bounding box #4849 and Overlays 2.0 #4894 and ultimately live on the Layer as a LayerOverlay. But that will come in a separate PR.
some extra api was gained by the overlays: you can now set opacity for all of them, and every CanvasPosition is now available to both Text and ScaleBar, instead of restricting the scale bar to just the corner.

Type of change

Bug-fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

References

How has this been tested?

example: the test suite for my feature covers cases x, y, and z
example: all tests pass with my change
example: I check if my changes works with both PySide and PyQt backends
as there are small differences between the two Qt bindings.

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
If I included new strings, I have used trans. to make them localizable.
For more information see our translations guide.

codecov · 2022-08-04T15:33:34Z

Codecov Report

Merging #4907 (889ab0a) into main (1f02839) will increase coverage by 0.01%.
The diff coverage is 93.02%.

@@            Coverage Diff             @@
##             main    #4907      +/-   ##
==========================================
+ Coverage   87.76%   87.77%   +0.01%     
==========================================
  Files         578      583       +5     
  Lines       49371    49373       +2     
==========================================
+ Hits        43330    43338       +8     
+ Misses       6041     6035       -6

Impacted Files	Coverage Δ
.../components/overlays/_interaction_box_constants.py	`100.00% <ø> (ø)`
napari/_vispy/visuals/axes.py	`81.57% <81.57%> (ø)`
napari/_vispy/overlays/base.py	`92.06% <92.06%> (ø)`
napari/_qt/qt_viewer.py	`78.64% <100.00%> (ø)`
napari/_tests/test_interactive_transforms.py	`97.40% <100.00%> (ø)`
napari/_vispy/__init__.py	`100.00% <100.00%> (ø)`
...apari/_vispy/_tests/test_vispy_scale_bar_visual.py	`100.00% <100.00%> (ø)`
napari/_vispy/_tests/test_vispy_text_visual.py	`100.00% <100.00%> (ø)`
napari/_vispy/overlays/axes.py	`100.00% <100.00%> (+21.79%)`	⬆️
napari/_vispy/overlays/interaction_box.py	`94.56% <100.00%> (ø)`
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sofroniewn

Thanks for splitting this out @brisvag - I think this is close. I just flagged two things for discussion - one is if we need all the different overlay model classes right away, and the other is if we can put some of these model classes behind something private so they are less exposed.

vispy changes look good.

sofroniewn · 2022-08-05T00:07:56Z

napari/components/overlays/base.py

+from .._viewer_constants import CanvasPosition
+
+
+class Overlay(EventedModel):


We've got quite a lot of classes here - not sure if this is a pattern @jni would want us to go too right away. Maybe we just start with Overlay with everything on it and drop CanvasOverlay / SceneOverlay

Ah ok I just saw in #4894

CanvasOverlay: things that live in canvas space (ScaleBar, TextOverlay, InteractionBox, the welcome screen, and in the future maybe ColorBars and other HUD-like visuals) and thus should be children of canvas.view
SceneOverlay: things that live in scene space -- also referred to as "displayed coordinates"; this is the 2D or 3D slices being rendered -- and thus should be children of the canvas.view.scene

So I see how these are two different types of thing - still not quite clear to me they need their own classes, but maybe. If so it would be good to have some nice doc strings for the them, and maybe make _Overlay or OverlayBase so that it was clear it was not something to be used directly

It's definitely reasonable to make them private for now.

The reason for having different classes is mainly because in #4894 I used isinstance checks to choose how to parent the visual. This can probably be done with an attribute just as well, so I'm not particularly attached to class separation; I just found it more intuitive.

sofroniewn · 2022-08-05T00:08:45Z

napari/components/overlays/__init__.py

@@ -0,0 +1,5 @@
+from .axes import AxesOverlay


Could overlays be _overlays and private? I don't think we think users will import and uses these objects publicaly, so we could hide them which might make iterating and refactoring easier

Aren't the equivalents (e.g. napari.components.axes) public right now though? And they have (and will continue to have?) a pretty prominent/public entry point (i.e. Viewer.axes or maybe later Viewer.overlays.axes that will need to publicly documented?

Or do you just want to make this module private and import these publicly?

Not that I mind making the public API smaller, but wanted to check.

I don't mind either way. I think probably the path of least resistance is to do this change with 0.5 and not deal with all the possible deprecations.

andy-sweet

This mostly looks great and I think could be merged pretty soon. Are there any high level parts you're more uncertain about or want particular feedback on? Or do you think this could also go in pretty soon?

napari/components/overlays/base.py

napari/_qt/qt_viewer.py

napari/components/overlays/base.py

andy-sweet · 2022-09-01T21:05:55Z

napari/components/overlays/__init__.py

@@ -0,0 +1,5 @@
+from .axes import AxesOverlay


Aren't the equivalents (e.g. napari.components.axes) public right now though? And they have (and will continue to have?) a pretty prominent/public entry point (i.e. Viewer.axes or maybe later Viewer.overlays.axes that will need to publicly documented?

Or do you just want to make this module private and import these publicly?

Not that I mind making the public API smaller, but wanted to check.

andy-sweet · 2022-09-01T21:09:23Z

napari/components/overlays/base.py

+
+
+class SceneOverlay(Overlay):
+    # TODO: should transform live here?


Can you give an example of a scene overlay transform? Maybe the interaction box transform?

I'm not convinced of what should be our "default" here; for example, the InteractionBox (or BoundingBoxOverlay from #4849 which will probably be merged with it) do not need a transform that is directly settable by the user (despite having an affine themselves), since they derive it from the layer, and should never be offset manually. On the other hand, stuff like the manipulators from alisterburt/napari-threedee should have an easily settable transform. It's probably something the subclass should expose, though.

Given we don't have a huge number of overlays, how about taking the suggestion of @sofroniewn and just removing CanvasOverlay and SceneOverlay as distinct subclasses for now? They only add the position attribute in CanvasOverlay, which could just be added to the small number of canvas overlays we have right now.

I guess having distinction might introduce some nice clear semantics, but I also think that could come later.

Ah right, I guess you explained that in: https://github.com/napari/napari/pull/4907/files#r946860221

I don't feel particularly strongly here.

brisvag · 2022-09-02T14:51:19Z

This mostly looks great and I think could be merged pretty soon. Are there any high level parts you're more uncertain about or want particular feedback on? Or do you think this could also go in pretty soon?

I think this is close to mergeable. I can see people having issues with the half-changed InteractionBox, but as I mentioned at the top, I don't think it's worth tackling yet, since I plan to merge it with BoundingBox.

andy-sweet · 2022-09-02T16:08:34Z

I don't mind either way. I think probably the path of least resistance is to do this change with 0.5 and not deal with all the possible deprecations.

It seems like the biggest blocker here is around defining a release plan around 0.5, which is unclear to me. It would be great if we knew what we want to be in 0.5 and when main can accept 0.5 things that might still need some work before release.

alisterburt

@andy-sweet and @brisvag do we want to try to push for merging this in 0.4.17 or would you rather leave until 0.5.0? I'm happy either way, excited to see work on overlays move forwards!

FWIW - I'm very pro the clear semantics of CanvasOverlay and SceneOverlay despite the proliferation of classes, I agree with @sofroniewn's suggestion of renaming what is currently Overlay to OverlayBase to avoid confusion.

Longer term adding custom overlays is something we might want to support via the plugin API but this shouldn't be rushed so private for now seems a safe bet

brisvag · 2022-09-05T09:30:19Z

@andy-sweet and @brisvag do we want to try to push for merging this in 0.4.17 or would you rather leave until 0.5.0? I'm happy either way, excited to see work on overlays move forwards!

I don't see the benefit of merging this without the followup, so I would say it's better to get all in for 0.5 :)

I agree that we should keep the class separation. Note that while the "class proliferation" seems unnecessary here, its usefulness will be more clear in the followup PR. Remember that this PR is intended as a way to split out refactors and non-breaking changes from #4894 in order to ease code review.

andy-sweet · 2022-09-06T14:50:36Z

I don't see the benefit of merging this without the followup, so I would say it's better to get all in for 0.5 :)

Agreed.

…/vispy-overlays

github-actions · 2022-09-28T08:50:43Z

Click here to download the docs artifacts
docs
(zip file)

brisvag · 2022-09-30T11:25:56Z

I think I have addressed all the comments. @sofroniewn, @andy-sweet and @alisterburt, this is ready to be merged if you are happy with it. I can then focus on the real work on #4894.

…/vispy-overlays

andy-sweet

Agreed that this is a priority in terms of merging. Go for it!

brisvag added 4 commits August 4, 2022 15:53

overlay backend refactor

321bb2e

fix tests

d3d0953

fix scale bar

ce2d3eb

fix other overlay visuals initialization

142eee6

github-actions bot assigned brisvag Aug 4, 2022

github-actions bot added qt Relates to qt tests Something related to our tests labels Aug 4, 2022

brisvag changed the title ~~overlay backend refactor~~ Overlay vispy backend refactor Aug 4, 2022

brisvag changed the title ~~Overlay vispy backend refactor~~ Overlay backend refactor Aug 4, 2022

sofroniewn reviewed Aug 5, 2022

View reviewed changes

andy-sweet reviewed Sep 1, 2022

View reviewed changes

brisvag added 3 commits September 2, 2022 15:29

remove useless hash function

dafbd55

remove confusing comment

554085d

add docstrings to overlays

04d93d2

alisterburt reviewed Sep 4, 2022

View reviewed changes

Merge branch 'main' of https://github.com/napari/napari into refactor…

c3bc251

…/vispy-overlays

brisvag added 2 commits September 30, 2022 13:29

Merge branch 'main' of https://github.com/napari/napari into refactor…

592a7b9

…/vispy-overlays

better naming

889ab0a

andy-sweet approved these changes Sep 30, 2022

View reviewed changes

brisvag merged commit 887a1d6 into napari:main Oct 3, 2022

brisvag deleted the refactor/vispy-overlays branch October 3, 2022 09:17

brisvag added refactor Refactoring code base vispy Vispy integration labels Oct 3, 2022

brisvag mentioned this pull request Jan 18, 2023

Overlays 2.0 #4894

Merged

7 tasks

andy-sweet mentioned this pull request Feb 8, 2023

viewer axes point in basically random directions 😂 #5536

Closed

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

Czaki added this to the 0.4.18 milestone Jun 13, 2023

Czaki added maintenance PR with maintance changes, triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process labels Jun 13, 2023

andy-sweet mentioned this pull request Jun 14, 2023

Fix vispy axes labels #5565

Merged

1 task

Czaki pushed a commit that referenced this pull request Jun 16, 2023

Overlay backend refactor (#4907)

98b1e44

Czaki pushed a commit that referenced this pull request Jun 17, 2023

Overlay backend refactor (#4907)

6a661d0

Czaki pushed a commit that referenced this pull request Jun 18, 2023

Overlay backend refactor (#4907)

3e4b4a9

Czaki pushed a commit that referenced this pull request Jun 19, 2023

Overlay backend refactor (#4907)

f059b1f

Czaki pushed a commit that referenced this pull request Jun 21, 2023

Overlay backend refactor (#4907)

fc708c4

Czaki pushed a commit that referenced this pull request Jun 21, 2023

Overlay backend refactor (#4907)

6b1d77d

Czaki pushed a commit that referenced this pull request Jun 21, 2023

Overlay backend refactor (#4907)

42f38b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overlay backend refactor #4907

Overlay backend refactor #4907

brisvag commented Aug 4, 2022

codecov bot commented Aug 4, 2022 •

edited

sofroniewn left a comment

sofroniewn Aug 5, 2022

sofroniewn Aug 5, 2022

brisvag Aug 16, 2022

sofroniewn Aug 5, 2022

andy-sweet Sep 1, 2022 •

edited

brisvag Sep 2, 2022

andy-sweet left a comment

andy-sweet Sep 1, 2022 •

edited

andy-sweet Sep 1, 2022

brisvag Sep 2, 2022

andy-sweet Sep 2, 2022

andy-sweet Sep 2, 2022

brisvag commented Sep 2, 2022

andy-sweet commented Sep 2, 2022

alisterburt left a comment

brisvag commented Sep 5, 2022

andy-sweet commented Sep 6, 2022

github-actions bot commented Sep 28, 2022

brisvag commented Sep 30, 2022

andy-sweet left a comment

		from .._viewer_constants import CanvasPosition


		class Overlay(EventedModel):



		class SceneOverlay(Overlay):
		# TODO: should transform live here?

Overlay backend refactor #4907

Overlay backend refactor #4907

Conversation

brisvag commented Aug 4, 2022

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Aug 4, 2022 • edited

Codecov Report

sofroniewn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet left a comment

Choose a reason for hiding this comment

andy-sweet Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag commented Sep 2, 2022

andy-sweet commented Sep 2, 2022

alisterburt left a comment

Choose a reason for hiding this comment

brisvag commented Sep 5, 2022

andy-sweet commented Sep 6, 2022

github-actions bot commented Sep 28, 2022

brisvag commented Sep 30, 2022

andy-sweet left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 4, 2022 •

edited

andy-sweet Sep 1, 2022 •

edited

andy-sweet Sep 1, 2022 •

edited