Add NAP for multicanvas #249

aganders3 · 2023-10-13T19:09:50Z

Particular thanks to @brisvag and @melonora for previous work and discussions on this, and for agreeing to be co-authors. I am happy to absorb any criticism and direct praise their way 😄.

Thanks also to @andy-sweet and @Czaki, both of whom provided helpful discussion while I worked on this NAP.

Of course I expect many changes to this and look forward to feedback.

andy-sweet

I took a first pass through this and left a few comments. Overall, I'm very excited to see something like this get approval and get implemented!

andy-sweet · 2023-10-13T20:26:48Z

docs/naps/8-multiple-canvases.md

+* Specific UI implementations will be explored as part of this work, but I expect UX and UI will be formalized later (possibly in a separate NAP).
+* Supporting alternative frontend (Qt) and backend (Vispy) frameworks. While this work should not make such tasks more difficult in the future, explicit consideration is out-of-scope until further progress is made in these areas.
+* [Non-goals also in NAP-3](https://napari.org/dev/naps/3-spaces.html#non-goals) are related but also considered out-of-scope here
+    * Separation of rendering information from the Layer(Data) model


Would this benefit the work here or do you think of it as mostly orthogonal? Would a simultaneous effort help or hinder progress here?

I think layergroups here could potentially provide a cleaner API, but overall the API is similar to current grid mode at least for multichannel view, thus could be an orthogonal effort.

I don't think it's entirely orthogonal, but perhaps mostly. I think it depends on the scope of features we want. For example just below I propose for all canvases to share a common layer list, with independent slicing, camera, dimensionality, and layer visibility. I think this would be more relevant if supporting (for example) different colormaps for the same data in two canvases.

andy-sweet · 2023-10-13T20:37:05Z

docs/naps/8-multiple-canvases.md

+* `ViewerModel` will own a *list* of `CanvasModel` objects
+* `QtViewer` will own a *list* of `VispyCanvas` objects
+* `VispyLayer` subclasses will hold references to their `Layer` (for rendering information) as well as a `LayerSlice` (for data)
+* The LayerSlicer will need to update the sync and async callbacks[^async-only] to pass a canvas parameter where the resulting sliced data will be stored, rather than storing it on the layer itself. Slice task cancellation logic will need to be revisited accordingly.


One alternative here that came to mind is that each canvas (or maybe vispy canvas) could own its own layer slicer - that way you don't need to pass in the extra canvas parameter and you can do cancellation per canvas (which I think is desirable).

With lots of canvas, you might introduce too much concurrency, but in most cases, maybe that won't be too bad. And we may be able to serialize it with a shared resource/controller if needed.

Thanks - that's an interesting idea I hadn't considered. I was initially thinking the current LayerSlicer may expand to use a pool instead of a single thread.

I think you're right passing the full canvas is probably more than necessary, and trying to pass a canvas ID or something feels a bit hacky to me. One benefit of just passing the canvas object is that it would bring with it the camera, which might be useful for finding corner_pixels without waiting for the draw.

andy-sweet · 2023-10-13T20:38:46Z

docs/naps/8-multiple-canvases.md

+
+> Note: in both diagrams, the `on_draw()` method on the `VispyCanvas` breaks MVC convention (view layer talks directly to the model layer). This is a separate/known issue and I believe is mostly only true for multiscale image layers right now. Changing this is considered out of scope for this project at this time.
+
+Slice state for each layer is currently stored on the Layer model. Again, see [NAP-4 for previous discussion](https://napari.org/stable/naps/4-async-slicing.html#existing-slice-state). This NAP proposes to move this state off the Layer instance, into a specific Layer Slice instance. This is what will allow multiple slices of a single layer to be visualized simultaneously. *Table 1* lists the attributes related to slice state that will be moved in this work from each Layer class into corresponding Layer Slice classes.


Maybe I misread the out-of-scope section, but my impression was that moving slice state off the layer was a non-goal - is that wrong?

Or maybe this is the proposed approach that incidentally achieves that non-goal?

Anyway, I'm generally in favor of that move, but just wanted to clarify the proposal and intent here.

Do you mean this non-goal?

Normalizing slice data for different layer types, though this may benefit in the course of this work.

That's not really what I meant there, but also I think it's ambiguous enough warrant changing or removing. I was trying to say it is a non-goal to create a uniform/generic concept (or Protocol) of a "slice" - it's okay for the slices to be quite specific for each layer type.

No, I meant this one.

Separation of rendering information from the Layer(Data) model

But maybe you meant that only with respect to NAP-3.

Anyway, I also agree that a generic slice class/protocol should be a non-goal. Though we might consider during/after.

I see - I will try to make this more clear. The goal here is indeed to move the slice state off the layer. The non-goal was meant to indicate that the remaining attributes of the layer will not be separated. So for example this work does not necessarily make it easier to share data between two layers with different colormaps.

andy-sweet · 2023-10-13T20:42:11Z

docs/naps/8-multiple-canvases.md

+
+## Implementation
+
+1. Introduce minimally disruptive `_canvases` attribute on the ViewerModel


I'm fully in support of this ordering where implementation of architecture changes happen first to support new functionality later.

One mistake on the async slicing project is that I didn't do enough to prioritize things that way, which meant that things didn't end up quite as I'd hoped.

docs/naps/8-multiple-canvases.md

andy-sweet · 2023-10-13T20:59:47Z

docs/naps/8-multiple-canvases.md

+> [name=Ashley A]
+
+### UI Design and Architecture
+Specific UI design and architecture remains to be determined. This will be explored as part of step 4 in the [Implementation Plan](#Implementation). UI design needs additional refinement and exploration, and this is expected to continue after basic/core implementation propsed in this NAP is complete. UI changes may also be described in a separate NAP along with a discussion of convenience functions and affordances for common operations. Some placeholder or experimental code will be used in the meantime as a prototype implementation.


Is there any prototype code available to look at and play with? And/or some videos showing some of the proposals here at least kind-of in action?

Both could help to ground some of the proposals and discussion here.

My prototype/experimentation code is pretty early, but is here:
aganders3/napari#13

andy-sweet · 2023-10-13T21:00:53Z

docs/naps/8-multiple-canvases.md

+> Note: some layers include "seleciton" information (Points, Shapes, and Tracks). How selection is handled in multicanvas view is TBD at this point.
+
+***Table 1** - Layer attributes that hold slice data. These attributes will be moved from the Layer onto individual Layer Slice objects (one Layer Slice per Layer per Canvas).*
+> TODO: make this a list or figure out a way to center it [name=Ashley A]


I regretted adding something like this to the async slicing NAP, because it's just too much detail, so another option might be to remove it from the main NAP document and maybe link to it or shove it into an appendix.

I found it helpful to write this out, but if it's too detailed I'm happy to move or remove it. It might just fit better in a PR with actual code changes.

Co-authored-by: Andy Sweet <[email protected]>

aganders3 · 2023-10-14T00:36:47Z

I took a first pass through this and left a few comments. Overall, I'm very excited to see something like this get approval and get implemented!

Thanks! I appreciate the support and feedback.

Czaki

The number 8 is for telemetry (#226), also please add a new NAP to the table of contents.

brisvag

Generally, great work on the NAP! Other than comments and clarifications here and there, I have one main disagreement, and it seems it comes from a misunderstanding when we discussed before, because I thing this is quite important. You say:

* All canvases shall share a common layer list and (unsliced) layer data.

I think we need to ensure from the get-go that canvases can hold different layer lists, but can also share some layers. This is key for example for applications like highly multiplexed data (@melonora can add some more specifics here if needed). I fear that leaving this out will make it harder to support it afterwards.

Anyways, approving as draft as per normal NAP procedure :)

docs/naps/9-multiple-canvases.md

brisvag · 2023-10-19T12:09:24Z

docs/naps/9-multiple-canvases.md

+* The application shall natively display multiple canvases simultaneously.
+    * There shall be a minimum of one canvas (current status) per viewer.
+    * There shall be a maximum of 8 canvases (tentative) per viewer.
+* All canvases shall share a common layer list and (unsliced) layer data.


This is my only disagreement with the current form of the NAP, I wrote more details in the general review comment.

docs/naps/9-multiple-canvases.md

brisvag · 2023-10-19T12:55:44Z

docs/naps/9-multiple-canvases.md

+* What kind of cross-reference displays or tools should there be?
+    * through-plane slice indicators
+    * three-point slice definition


This is hard to do unless we assume a shared space between canvases, which I'm not sure we should do. Initially, I think we should leave this to the user.

Creating an orthoview example as part of this would would be a great way to test these ideas.

docs/naps/9-multiple-canvases.md

aganders3 · 2023-10-20T01:13:52Z

Thanks for the review and great comments @brisvag! I will try to respond to each and update accordingly, but generally your comments cleared up a few things for me. We're not too far apart on anything here :).

Co-authored-by: Lorenzo Gaifas <[email protected]>

aganders3 · 2023-11-22T21:15:49Z

I updated a bit but not ready for your re-review @brisvag. I should be able to finish addressing your comments next week, but you can ignore GitHub notifications for now :)

aganders3 · 2023-12-01T20:09:57Z

I have one main disagreement, and it seems it comes from a misunderstanding when we discussed before, because I thing this is quite important. You say:

All canvases shall share a common layer list and (unsliced) layer data.

I think we need to ensure from the get-go that canvases can hold different layer lists, but can also share some layers. This is key for example for applications like highly multiplexed data (@melonora can add some more specifics here if needed). I fear that leaving this out will make it harder to support it afterwards.

My way of addressing this use case was specifying layers would gain a per-canvas visibility setting. I think this is also where misalignment on #249 (comment) comes from; using a common layer list would also inherently share space between canvases. If you want a truly independent space, I don't see that much benefit over opening a separate viewer window.

Changing to fully independent layer lists would definitely offer more flexibility, and may actually make this implementation simpler in some ways. My concern is that it will make for a more difficult UI design, and may make it more difficult to implement certain common use cases later (for example, where you do want a common space between canvases). That said, I am definitely still open to this approach. You (and @melonora) may have a better understanding of the use-cases that would benefit.

changed

Initial draft of multicanvas nap

b8f22f6

github-actions bot assigned aganders3 Oct 13, 2023

github-actions bot added the documentation Improvements or additions to documentation label Oct 13, 2023

andy-sweet reviewed Oct 13, 2023

View reviewed changes

Fix typos

9c2df8d

Co-authored-by: Andy Sweet <[email protected]>

Czaki previously requested changes Oct 14, 2023

View reviewed changes

Update NAP number, add to TOC

7171040

brisvag requested a review from Czaki October 19, 2023 11:47

brisvag approved these changes Oct 19, 2023

View reviewed changes

psobolewskiPhD added this to the 0.4.19 milestone Oct 26, 2023

aganders3 and others added 3 commits November 20, 2023 13:39

Merge branch 'main' into multicanvas-nap

a83bd0e

typo

d2552ad

Co-authored-by: Lorenzo Gaifas <[email protected]>

Incorporate some review comments.

daffed5

Merge branch 'main' into multicanvas-nap

fe76b1f

brisvag mentioned this pull request Dec 12, 2023

example: add cursor-overlay example napari/napari#6510

Open

Czaki added 2 commits January 31, 2024 15:23

Merge branch 'main' into multicanvas-nap

e020cca

fix merge bug

21373de

Czaki merged commit 7a204c0 into napari:main Jan 31, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NAP for multicanvas #249

Add NAP for multicanvas #249

aganders3 commented Oct 13, 2023

andy-sweet left a comment

andy-sweet Oct 13, 2023

melonora Oct 13, 2023

aganders3 Oct 14, 2023

andy-sweet Oct 13, 2023

aganders3 Oct 14, 2023

andy-sweet Oct 13, 2023

aganders3 Oct 14, 2023 •

edited

Loading

andy-sweet Oct 23, 2023

aganders3 Dec 1, 2023

andy-sweet Oct 13, 2023

andy-sweet Oct 13, 2023

aganders3 Oct 18, 2023

andy-sweet Oct 13, 2023

aganders3 Oct 14, 2023

aganders3 commented Oct 14, 2023

Czaki left a comment

brisvag left a comment

brisvag Oct 19, 2023

brisvag Oct 19, 2023

aganders3 commented Oct 20, 2023

aganders3 commented Nov 22, 2023

aganders3 commented Dec 1, 2023


		> Note: in both diagrams, the `on_draw()` method on the `VispyCanvas` breaks MVC convention (view layer talks directly to the model layer). This is a separate/known issue and I believe is mostly only true for multiscale image layers right now. Changing this is considered out of scope for this project at this time.

		Slice state for each layer is currently stored on the Layer model. Again, see [NAP-4 for previous discussion](https://napari.org/stable/naps/4-async-slicing.html#existing-slice-state). This NAP proposes to move this state off the Layer instance, into a specific Layer Slice instance. This is what will allow multiple slices of a single layer to be visualized simultaneously. Table 1 lists the attributes related to slice state that will be moved in this work from each Layer class into corresponding Layer Slice classes.


		## Implementation

		1. Introduce minimally disruptive `_canvases` attribute on the ViewerModel

Add NAP for multicanvas #249

Add NAP for multicanvas #249

Conversation

aganders3 commented Oct 13, 2023

andy-sweet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 Oct 14, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 commented Oct 14, 2023

Czaki left a comment

Choose a reason for hiding this comment

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aganders3 commented Oct 20, 2023

aganders3 commented Nov 22, 2023

aganders3 commented Dec 1, 2023

aganders3 Oct 14, 2023 •

edited

Loading