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

[WIP] Overlays #3763

Closed
wants to merge 4 commits into from
Closed

[WIP] Overlays #3763

wants to merge 4 commits into from

Conversation

brisvag
Copy link
Contributor

@brisvag brisvag commented Dec 7, 2021

Description

Here's a [very work-in-progress] attempt at unifying overlays (see #3755 ).

Overlays are currently all broken (and a lot of the diff is for now simply removing stuff that would otherwise crash napari), except for Text, which is now using the new system. I thought this was a good point to create a PR and ask for feedback!

EDIT: current state:

  • text
  • scale_bar
  • axis (should be here?)
  • interaction box

Goals

  • move overlays to an Overlays evented model on the viewer
  • generate overlays similarly to how layers are generated (automatically from the model)
  • use them for everything (mainly selection boxes)

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

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.

@github-actions github-actions bot added the qt Relates to qt label Dec 7, 2021
Copy link
Contributor Author

@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.

I left a few comments to point out the important changes. Please ignore any changes specific to other non-text overlays: they are temporary, and broken still.

Comment on lines 383 to 395
def _add_overlay_visuals(self) -> None:
"""Add visuals for overlays."""

self.axes = VispyAxesOverlay(
self.viewer,
parent=self.view.scene,
order=1e6,
)
self.scale_bar = VispyScaleBarOverlay(
self.viewer,
parent=self.view,
order=1e6 + 1,
)
self.canvas.events.resize.connect(self.scale_bar._on_position_change)
self.text_overlay = VispyTextOverlay(
self.viewer,
parent=self.view,
order=1e6 + 2,
)
self.canvas.events.resize.connect(
self.text_overlay._on_position_change
)
self.interaction_box_visual = VispyInteractionBox(
self.viewer, parent=self.view.scene, order=1e6 + 3
)
self.interaction_box_mousebindings = InteractionBoxMouseBindings(
self.viewer, self.interaction_box_visual
)
for i, (name, overlay) in enumerate(self.viewer.overlays):
if name == 'visible':
continue
vispy_overlay = create_vispy_overlay(
overlay, viewer=self.viewer, parent=self.view, order=1e6 + i
)

# we must keep a reference of they will disappear
# TODO: using name here because evented models are unhashable?
self.overlay_to_visual[name] = vispy_overlay
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relevant changes in qt_viewer are here.

Comment on lines 4 to 28
class VispyBaseOverlay:
def __init__(self, overlay, viewer, parent=None, node=None, order=0):
super().__init__()
self.viewer = viewer
self.overlay = overlay
self.node = node
self.node.parent = parent # qt_viewer.view
self.node.order = order
self.node.transform = STTransform()

self.viewer.overlays.events.visible.connect(self._on_visible_change)
self.overlay.events.visible.connect(self._on_visible_change)
self.node.parent.events.resize.connect(self._on_position_change)

def _on_visible_change(self):
self.node.visible = (
self.viewer.overlays.visible and self.overlay.visible
)

def _on_position_change(self, event=None):
pass

def reset(self):
self._on_visible_change()
self._on_position_change()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the overlay equivalent of VispyBaseLayer.

napari/_vispy/overlays/text.py Show resolved Hide resolved
Comment on lines +87 to +115
def create_vispy_overlay(
overlay: EventedModel, **kwargs
) -> List[VispyBaseOverlay]:
"""
Create vispy visuals for each overlay contained in an Overlays model based on their type,

Parameters
----------
overlay : napari.components.overlays.VispyBaseOverlay
The overlay to create a visual for.
viewer : napari.components.ViewerModel
The viewer containing the overlays.

Returns
-------
vispy_overlays : napari._vispy.overlays.VispyBaseOverlay
Vispy overlay
"""
for overlay_type, visual_class in overlay_to_visual.items():
if isinstance(overlay, overlay_type):
return visual_class(overlay=overlay, **kwargs)

raise TypeError(
trans._(
'Could not find VispyOverlay for overlay of type {dtype}',
deferred=True,
dtype=type(overlay),
)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copyign the implementation of create_vispy_layer.

Comment on lines 11 to 39
class Overlays(EventedModel):
"""A collection of components that will draw on top of layer data.

Attributes
----------
interaction_box : InteractionBox
A box that can be used to select and transform layers or data within a layer.
axes:
Axes indicating world coordinate origin and orientation.
text:
Text overlay with arbitrary information.
scale_bar:
Scale bar indicating size in world coordinates.
visible:
Whether the overlays are visible.
"""

# fields
# interaction_box: InteractionBox = Field(
# default_factory=InteractionBox, allow_mutation=False
# )
# axes: Axes = Field(
# default_factory=Axes, allow_mutation=False
# )
text: Text = Field(default_factory=Text, allow_mutation=False)
# scale_bar: ScaleBar = Field(
# default_factory=ScaleBar, allow_mutation=False
# )
visible: bool = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core evented model. currently, order of fields also controls drawing order in vispy, but that's probably bad.

@alisterburt
Copy link
Contributor

looks cool! I see that the interaction box and axis visual are both overlays here, does this mean you've moved away from your strict definition of overlays as things which exist in the canvas coordinate system or is it just temporary?

@brisvag
Copy link
Contributor Author

brisvag commented Dec 8, 2021

looks cool! I see that the interaction box and axis visual are both overlays here, does this mean you've moved away from your strict definition of overlays as things which exist in the canvas coordinate system or is it just temporary?

They do exist only in the canvas coordinate system. Are you referring to the fact that Axis is affected by the camera orientation? In my mind, that does not matter. What (I think) should differentiate layers and overlays is:

  • layers contain Data
  • layers appear in the layer list
  • the contents of layers live in world space (i.e: they use an Affine transform) while overlays live in the canvas (i.e: they use STTransform)

EDIT: I just now started playign aroudn with axes (which I had never really sued before) and I see what you mean now. I assumed the axes were displayed in the corner of the canvas (much like many vispy examples do), not in the corner of the world space. Not sure how to taclke that, then. As is, it should probably NOT be an overlay, but one of your (@alisterburt, @kevinyamauchi ) 3D objects.

@brisvag
Copy link
Contributor Author

brisvag commented Dec 9, 2021

Interaction box is now partially working, in the sense that it's attached to the canvas and connected to the model via events.

What's missing is the connection to the mouse bindings, which appears to be broken by something in my chnages. @jojoelfe , could you check it out and see if you spot the issues?

To test out that the evented model works, and it's correctly connected to the visual:

import napari
import numpy as np

v = napari.Viewer()
il = v.add_image(np.random.rand(10, 10, 10))
v.overlays.interaction_box.update(dict(
    visible=True,
    points=np.array([[1, 1], [1, 2], [2, 1], [2, 2]]) * 300,
    show_handle=True,
    show_vertices=True,
))

however, you'll see that as soon as you click somewhere, it disappears. If you enable the transform mode, even stranger things happen (the box appears in the corner, tiny).

il.mode = 'transform'

@brisvag
Copy link
Contributor Author

brisvag commented Dec 10, 2021

On closer inspection, the interaction box mouse bindings are working correctly (i.e: you can drag the corner of an image just fine!); it's the visualization that's broken and somehow ends up squished in the corner...

if name == 'visible':
continue
vispy_overlay = create_vispy_overlay(
overlay, viewer=self.viewer, parent=self.view, order=1e6 + i
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem might be that for the interaction_box the parent should be view.scene and not view. I guess that means it wants to live in world coordinates and not in canvas coordinates.

@jojoelfe
Copy link
Contributor

I think when I was planning the interaction_box the plan was to have it in canvas coordinates, but in practice it was much easier to have it in world coordinates, because there are data_to_world functions, but no easy to access data_to_canvas or world_to_canvas functions. #3783 should help there.

The decision was also easy because the main practical reason to have it in canvas coordinates is that control-point size should be independent of camera zoom, but you get that behavior for free from .scaling = False of the marker visual.

@brisvag
Copy link
Contributor Author

brisvag commented Dec 10, 2021

Oh, I didntt realize! Yeah that makes sense. However, it seems like we should definitely move towards canvas coordinates for something like this...

@sofroniewn
Copy link
Contributor

hi @brisvag what are your thoughts on picking this up again? i can start giving it some review. In general though, I think the addition of VispyBaseOverlay and Overlays will be good for the code base. This is also something that @andy-sweet might be able to help review too

@brisvag
Copy link
Contributor Author

brisvag commented Jan 25, 2022

I'm happy to pick it up again, but we need some discussion before we can move forward I think. As you can see from the comments above, there are a few things that make the transition harder than I thought. Specifically, the axis and interaction box are currently tied to the world coordinates, which makes them unsuitable for the Overlay abstraction.

While I'd be ok with leaving Axes out of this if most people prefer the current behaviour (that is, they are fixed in a point in world coordinates rather than being in a corner of the canvas), I think we really need to figure out how to get InteractionBox "fixed" to work as an overlay. It seems that this requires some refactoring on the transform logic, for which I'll definitely need help, maybe from @andy-sweet !

@brisvag brisvag mentioned this pull request Mar 7, 2022
@sofroniewn sofroniewn marked this pull request as draft April 3, 2022 04:29
@Carreau
Copy link
Contributor

Carreau commented Jun 28, 2022

This PR have not been active for quite some time.

In order to keep the list of PRs short and easier to work with, I am going to tag this PR as stale and close it.

Feel free to reopen and remove the "stale" label when work on this PR resumes.
If you do not have the permissions to do so, feel free to request help from one of the maintainer to reopen and remove the stale label .

@Carreau Carreau closed this Jun 28, 2022
@Carreau Carreau added the stale PR that is not currently being worked on label Jun 28, 2022
@brisvag brisvag mentioned this pull request Aug 2, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qt Relates to qt stale PR that is not currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants