Grid mode using vispy ViewBox and linked cameras #7870
Conversation
Co-authored-by: Wouter-Michiel Vierdag <w-mv@hotmail.com>
for more information, see https://pre-commit.ci
443e246 to
da63dc6
Compare
|
Todo from pairing with @melonora:
|
just kidding, the updated constraints were good, I'm just dumb :P |
add test to ensure cameras are updated correctly on grid switch
|
Great great work! Again, wish I had more time than a drive by. Earlier problem is fixed. Mostly everything is smoothed. Reordering layers results in miscalculation of overlay position (until something like a mouse scroll event, which fixes it): It also appears that removing layers such that there is an odd number left, but not even, also has the miscalculation. Adding layers (odd or even) does not result in the issue. I'm sure that's clue to where the layer updates aren't being called correctly, can look later tonight. Tests are so close now! |
src/napari/_vispy/canvas.py
Outdated
| for napari_layer, vispy_layer in self.layer_to_visual.items(): | ||
| row, col = self.viewer.grid.position( | ||
| # FIXME: the use of `len(self.viewer.layers) - 1 - idx` should be removed | ||
| # see https://github.com/napari/napari/pull/7870#issuecomment-2965031040 | ||
| len(self.viewer.layers) | ||
| - 1 | ||
| - self.viewer.layers.index(napari_layer), | ||
| len(self.viewer.layers), | ||
| ) | ||
| view = self.grid[row, col] | ||
| # any border_color != None will add a padding of +1 | ||
| # see https://github.com/vispy/vispy/issues/1492 | ||
| view.border_width = 0 | ||
| view.border_color = None | ||
|
|
||
| camera = VispyCamera(view, self.viewer.camera, self.viewer.dims) |
There was a problem hiding this comment.
is this creating a new camera for each layer? wouldn't that mean with something like stride =2 that there are 2 cameras in the same grid? I dunno if that's the problem with the comment I had about overlay positioning or not
There was a problem hiding this comment.
Ah, true, nice catch! I updated it to loop over viewboxes instead, I'm sure multiple cameras probably cause some of the weird issues we encountered so far 😓
|
I'm finding something odd with additive blending and stride, specifically negative stride. Actually, it also happens with even number of layers if the negative stride is odd, e.g. with lilly: Otherwise things appear to be working other than the issues noted above by Tim which can be triggered by changing stride. |
|
@TimMonko both your issues should be fixed! @psobolewskiPhD I think I had missed 1 spot where the inverted order of the grid causes issues. Can you check again? |
|
@brisvag indeed, you also fixed a severe issue with multiscale layers, which are now properly rendered. 🎉 |
Unfortunately does not solve #8033. What test were you trying to solve? I don't see any in the recent commits that are screenshot related, just multiscale. The screenshot test in |
|
Ok, I've figured out the multiscale test fails. The I'm approving :) I tried really hard to break this, and couldn't. |
Uhm, I thought I solved it in 1372ed9, which is why I thought it might also solve your problem, by updating the canvas before screenshotting. But I might have misunderstood the test, I thought the divide by zero was failing due to the image being denegerate. Maybe we should revert that commit then? Yeah those multiscale are the same as mentioned in these comments, I should mark xfail! EDIT: actually, these might be more fixable than the above. Unfortunately, I'm not gonna be able to come back to this for a couple of days, sorry for the bad timing 😅 I'll let you decide what to do! |
|
The plot thickens. Apparently the screenshot Edit: and it thickens more. it still fails with
Maybe it's flaky? I'll try rerunning. Locally it passes for me. Edit2: Yes it's flaky, so it's not this PR, per-se. It is in the sense we change the draw mechanism, but not a blocker to the PR. I'll still put in a resolution or x-fail here. |
|
Ok, so here's my final decision. This test is actually still an issue on main (localy), and is the same issue as #8033. Without fully processing (like in the tests), we get just the canvas (hence the divide by zero, because the data doesn't ever show up). Hopefully this just means we are slightly slower in displaying the image, but it must be ever so slight, since this is the only test issue. And I mean the change in event processing must be so slight since itss flaky, so I'm not worried about performance. But I do think trying to fix #8033 is high because now it also probably effects others OSes. For that reason, I'm going to add the processing into this test only. I'm guessing this is probably the root of so much of our screenshot test flakiness.... Once the whole loop is processed is loaded, the figures/screenshots look good (as expected) |
| center = tuple(np.round(np.divide(screenshot.shape[:2], 2)).astype(int)) | ||
| np.testing.assert_almost_equal(screenshot[center], [0, 255, 255, 255]) |
There was a problem hiding this comment.
maybe here and above, comment out the code and add a note, as you did with other tests above, so that we know to revive the test as soon as we can?
There was a problem hiding this comment.
Will do, good catch! Didn't realize Lorenzo commented these out too ... another lead on how this issue is happening. 😬
There was a problem hiding this comment.
or, why does it need commented out? the test passes?
There was a problem hiding this comment.
ok actually I'm just confused on what I'm looking at. Is this the most recent commit? It's not what I see locally and I think it's the most recent commit. But this does look like a diff that has this code removed.
References and relevant issues
Feature: Highlight currently selected layer in grid mode #2954EDIT: postponed to a followup PRDescription
This PR replaces the current transform-based grid mode with a viewbox-based one using linked cameras.
Additionally, viewer canvas overlays (such as
scale_bar) now have agriddedproperty which, when set toTrue, will make it so the overlay appears in every grid box instead that on the general canvas.