Add arrow display styles to Vectors layer #5740
Conversation
jules-vanaret
changed the title
|
That looks great! |
There was a problem hiding this comment.
Awesome work! Love that you added the arrows as well, this is so much better than what we have :)
General suggestions:
- we should use an enum instead of strings to encode the different styles. You can look at how
Blendingworks for examples on how to do that (base layer controls and base layer model).- I would also prefer
linetorectanglewhich I find more intuitive
- I would also prefer
- I would just call it
display_styleor even juststyleormoderather thanedge_display_style... in fact I think we should ditchedgefrom everything in vectors, but that's another topic ^^' Surely @jni has opinions here
Other than that, I think having some more comments and docstrings in the new code would help reviewing, otherwise the math is a bit hard to follow :)
|
Quick note so I don't forget: make sure that |
There was a problem hiding this comment.
Awesome work! Thanks for the beatiful comments and docstrings, super valuable :) Sorry this slipped through the cracks, I hope you still have time to follow up. Don't hesitate to ping me directly if this happens!
One general issue: It seems that the length parameter interacts only with the position of the tip of the vector. This results in the arrow style being quite deformed by the length change. Try this code and change the length to 10. Even worse, if you go smaller than 1 the arrow folds onto itself xD
import napari
import numpy as np
v = napari.Viewer(ndisplay=3)
a = np.array([[0, 0, 0], [10, 10, 10]])
v.add_vectors(a, vector_style='arrow')Other than that, I think we're basically ready to merge!
|
Thank you very much for your review, and no problem for the delay; there seem to be a lot of PRs these past months, which is great for the community ! :) Great catch on the arrow head issue with length. This was due to a simple missing 'length' factor. Nevertheless, even with the fix, I think arrow-styled vectors look quite ugly when the length parameters is set below the typical width of the vectors. I could make the width vary along with the length, but I don't think this a good idea. I noticed that a test was not passing. I'm quite unfamiliar with testing suites, but it seems to be related to one testing scripts for the 'test_generate_vector_meshes' and 'test_generate_vector_meshes2D' functions. I modified it, but I am not so confident about my changes... |
|
WHOO ALL GREEN! 🚀 |
jni
changed the title
|
Thank you so much for your work here @jules-vanaret! This has been weighing on my mind for so long you don't even know! 🙏 🚀 🌔 |
|
Oops, I was on vacation so I missed the conclusion of the PR ! 😅 For the sake of completeness regarding the Vectors layer, I have a few suggestions for follow-up PRs:
If any of these ideas are of particular interest to the community, please let me know! |
Always good to keep those updated, if we have missed something in the review 😅 The rest are all good options for followup PRs. I wouldn't call any of them high priority (for myself or from what I've seen discussed in the past), so feel free to tackle whichever is most blocking for your own work, and I'll be happy to review! |
@jules-vanaret currently, this change is unreleased, so a quick followup PR would not need to go through the deprecation pathway! Currently I don't have this slated to go into 0.4.18 (that is a special release off a branch that requires manual cherry-picking of changes to that branch), so we would have time, but I'm quite tempted to add it — if you think you could change that in the next ~24h I could add both and there would be no need for a deprecation! Having said that 0.5 will contain lots of deprecations (that's why we are making 0.4.18), so one more doesn't bother me. 😅
Definitely not urgent, but a PR would definitely be appreciated! 😊
As above — performance fixes (especially for relatively "rare" events) can be made in subsequent PRs — and will be much appreciated. 😊 But they are not essential.
I like this! For all of the issues in that list @jules-vanaret, if you are not fixing them more or less immediately, it would be good to create separate issues so we keep track of them! 🙏 |
No, we're talking about renaming the |
|
Thank you both for the feedback. I have tons of things to do this week so I probably won't be able to work on anything immediately. I'll try to create the new issues rapidly though. |
# Fixes/Closes Closes napari#4149 # Description This is a follow-up of the discussion napari#4149 in which adding different display style to the vectors of the Vectors layer was requested. As suggested by users @brisvag and @jni, I implemented a ComboBox in the GUI with 3 choices: 1. `rectangle` which is the old style of vectors, and currently the default value 2. `triangle` which displays vectors as triangles 3. `arrow` which displays a "classical" vector shape, with a rectangular shaft and a triangular head  Most of this contribution comes from additional functions to compute vectors meshes in _vispy/layers/vectors.py. Because changes are mainly in the function `generate_vector_meshes_2D`, there was no need to change anything in the functions `_on_data_change` or `generate_vector_meshes`, i.e it works natively with the 3D visualization. The Vectors layer now provides a parameter `edge_display_style`. Right now the possible values are: `[rectangle, triangle, arrow]` --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com> Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>
Ah, sorry, I get it now! But on the plus side this wasn't changed here, right? |
Absolutely, I haven't touched it at all! We simply pointed out at some point in the discussion that it might be a good idea to slowly change its name. :) |
# Fixes/Closes Closes #4149 # Description This is a follow-up of the discussion #4149 in which adding different display style to the vectors of the Vectors layer was requested. As suggested by users @brisvag and @jni, I implemented a ComboBox in the GUI with 3 choices: 1. `rectangle` which is the old style of vectors, and currently the default value 2. `triangle` which displays vectors as triangles 3. `arrow` which displays a "classical" vector shape, with a rectangular shaft and a triangular head  Most of this contribution comes from additional functions to compute vectors meshes in _vispy/layers/vectors.py. Because changes are mainly in the function `generate_vector_meshes_2D`, there was no need to change anything in the functions `_on_data_change` or `generate_vector_meshes`, i.e it works natively with the 3D visualization. The Vectors layer now provides a parameter `edge_display_style`. Right now the possible values are: `[rectangle, triangle, arrow]` --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com> Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/announcement-napari-0-4-18-released/83322/1 |
# Description This is a follow-up micro-PR to the discussion in #5740 about adding last comments to the Vectors layer code. ## Type of change - [x] Enhancement (non-breaking improvements of an existing feature) - [x] Documentation (update of docstrings and deprecation information) ## Final checklist: - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas
Fixes/Closes
Closes #4149
Description
This is a follow-up of the discussion #4149 in which adding different display style to the vectors of the Vectors layer was requested. As suggested by users @brisvag and @jni, I implemented a ComboBox in the GUI with 3 choices:
rectanglewhich is the old style of vectors, and currently the default valuetrianglewhich displays vectors as trianglesarrowwhich displays a "classical" vector shape, with a rectangular shaft and a triangular headMost of this contribution comes from additional functions to compute vectors meshes in _vispy/layers/vectors.py. Because changes are mainly in the function
generate_vector_meshes_2D, there was no need to change anything in the functions_on_data_changeorgenerate_vector_meshes, i.e it works natively with the 3D visualization.The Vectors layer now provides a
stringparameteredge_display_style. Right now I check that any input string is one of[rectangle, triangle, arrow]in the setter ofedge_display_style(in layers/vectors/vectors.py), but it does not feel like the right place. Also, I am not sure of my use oftrans..I have not included any real tests, and I am pretty sure that I have forgotten to update the description of at least one function...
Type of change
Final checklist:
trans.to make them localizable.