-
Notifications
You must be signed in to change notification settings - Fork 26
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
Allow Parsing NML Nodes Without Radius Attribute #4982
Conversation
@daniel-wer you mentioned in #4953 that the default radius should be 30, as that value is used in the front-end parser. Do you remember how this value was decided upon? |
@fm3 Very good that you're paying attention here! I had a look and there actually seems to be a lot of confusion around this value as well as two different defaults in the frontend (plus the also different default in the backend) 🙈 When uploading NMLs with nodes without a radius attribute, the default radius is 30. I cannot reconstruct how I chose this value, but it seems that I chose it somewhat randomly, because at the time I overlooked that the node radius is in nm and not in pixels (only visible if "Overrride Node Radius" is deactivated), so it heavily depends on the current zoom value, how large nodes appear. To complicate the matter, a certain minimum node size is assured by the shader to avoid tiny nodes. When creating the first node in a tracing, the radius is set to be When creating other nodes, the radius of the currently active node is copied. Practically speaking, I don't think this matters a lot, because most users have the "Overrride Node Radius" setting activated, but I would still like to unify and fix this. My intuition would be, for a dataset with scale 10x10x30 nm³, to set the default node radius to 5 nm, so that the node is 1vx in diameter in the dimension with the highest resolution. Happy to jump on a call about this and to contribute frontend fixes to this PR. |
@daniel-wer as discussed I changed the default radius (both in the NML parser and in the Task initial node) to 1.0f. If you are certain that this change won’t cause problems, feel free to add the front-end part here. |
To extend this point, the minimum node size assured by the shader is the "(Min.) ParticleSize" setting above the "Override Node Radius" setting. Most (if not all) users will have set this setting to a reasonable value and will likely have activated "Override Node Radius" as well (the minimum particle size is applied regardless). The unification of the default node radius in this PR shouldn't cause any problems, therefore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM apart from the changelog 🎉
@philippotto Could you have a quick look at the frontend code (see our discussion for why we think it's fine to unify and change the defaults) and approve if you agree and the code LGTY? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this clean up :)
URL of deployed dev instance (used for testing):
Steps to test:
Issues: