Prevent Shapes corruption when drawing tiny polygons with lasso #7914
Conversation
|
Should I add tests? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7914 +/- ##
==========================================
+ Coverage 92.90% 92.92% +0.01%
==========================================
Files 643 643
Lines 60663 60673 +10
==========================================
+ Hits 56360 56381 +21
+ Misses 4303 4292 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hmm, I'd wait to add tests until we like the implementation, speaking of which: This PR does prevent the current error message(s) in #7914, but I'm not sure if this is the behavior that we want. Below shown with default RDP epsilon, notice that small shapes dissappear (even some that are a few pixels in size) as well as the long line structures that @coreyelowsky see users cause the error on.
However, personally I like this behavior as I think it will result in only thoughtful shapes being drawn. And this won't be a regression from current behavior, so the only thing this PR ultimately does is prevent the error -- as previously the shape also wouldn't be drawn, and the layer would break. |
I think that it is a practical solution for 0.6.1. All others should land in |
There was a problem hiding this comment.
I actually agree with @TimMonko's original point that this doesn't actually fix the issue. To me, it feels more broken than before, because it's silently breaking. Since this is not a bug introduced in 0.6.0 but actually dates back to the original implementation of lasso, I don't think it's critical to get this in 0.6.1, and I'd rather we took a bit more time to think about a better solution.
|
Is there a lasso section in the docs? I think the best thing to do for 0.6.1 is to add an admonition in the docs about the RDP setting for small lasso shapes. |
|
There actually is extensive documentation for the lasso. |
|
So the question is what are the units on our RDP epsilon? |
Without compiled backend may be huge. Units are layer units. 1 is one pixel on image layer. |
Did you try to reproduce the original bug? It just blocks the Shapes layer without a way to fix. You need to remove it and start drawing from scratch... Ad I think that I do not see a good way to fix it. Because we point that, we remove every point that is less than 0.5 pixel from the line to smooth it. And it works as described. It is something like set black background in #7904. |
We call these world coordinates. (ie if the image has a scale, it's not 1 pixel, it's 1 "world unit".) |
At least, there should be a warning. My point is that in the previous case, it was broken, but it was broken enough that you would probably come to the website and raise an issue, and you could then get help. With the silent break it just looks like the feature is broken, and you might just conclude that napari is not the right tool. |
|
So one fix would be to use screen pixels for the epsilon, thus accounting for zoom? |
|
I have added a warning. It is calling napari-2025-05-14_08.18.05.mp4
To do this, we need to do total refactor of whole napari code. We already push too much information about rendering state into the layer. Also, the problem could be triggered on any scale if user draws a straight line. napari-2025-05-14_08.31.52.mp4 |
Doesn't using warnings.warn surface the warning automagically? Alternatively, with the new logger, maybe a log entry is a better idea… |
napari/layers/shapes/shapes.py
Outdated
| 'Polygon must have at least 3 vertices. With current rep epsilon value {eps},' | ||
| ' the {removed_vertices} vertices were removed.', | ||
| 'Cannot create polygon.', |
There was a problem hiding this comment.
Whether we end up using show_warning, warnings.warn, or logging.log (could we hook up log warnings and errors to our notifications?), here is my suggestion for the message:
Polygons must have three vertices. Lasso polygons are simplified using an algorithm called RDP, which may cause polygons smaller than the RDP epsilon parameter to disappear entirely. You can change this parameter in napari > Settings > Experimental > RDP epsilon.
Maybe this is better in the docs and then we can just link to the docs:
Simplified polygon has too few vertices. See [link] for details.
A warning is emitted one time. We want to show information every time user meet the problem. Logging is not currently displayed to user. And it is out of the scope of this PR to do this. |
|
Thanks for the great notes from the community meeting! Looks like y'all talked about sov much. What's the sense of how we want to move forward with the lasso bug for 0.6.1. I see merit to both sides, but would prefer a solution that doesn't break someone's hard work in a shapes layer, but does give them feedback on what might be happening. It looks like a warning has been added. Are we happy with that implementation or need to iterate more? Happy to play later when I can. |
There was a problem hiding this comment.
@TimMonko yeah @Czaki convinced me that the bug is severe enough (totally broken Shapes layer, which could lose hours of manual annotation work) that it should get fixed, even if we're not happy with the messaging infrastructure being used currently. Let's get this in to 0.6.1 now that there is a warning, and then iterate in later versions.
This is what the warning looks like when you expand the toast, I think it's pretty good:
|
(I still like my idea linking to the docs but I won't get a chance to update them tonight. I think let's just ship this and iterate docs, message, and code in 0.6.2+. But whatever folks want to do while I'm asleep is ok by me 😂) |
|
I have tested and it looks like adding a link will require to update |
|
Good good, glad to see we are moving forward with this. I'll add this to rc1 tonight. |
|
@jni for me its cut off :/ |
napari/layers/shapes/shapes.py
Outdated
Show resolved
Co-authored-by: Tim Monko <timmonko@gmail.com>
|
Nice condensification @TimMonko! I've committed it. 👍 |
References and relevant issues
Closes #7903
Description
Fix bug in adding shapes, by first removing points based on epsilon, then checking if there are enough point to create a polygon.