-
Notifications
You must be signed in to change notification settings - Fork 24
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
Render patterns in volume segments to increase distinguishability #4730
Conversation
@daniel-wer I think, you can delay the review until we decide that the feature itself is really ready for merging. The feedback until now wasn't that enthusiastic. |
…pattern; add variable for density
…erns; adapt zoom-dependent scaling
…nossos into segmentation-patterns
frontend/javascripts/oxalis/view/right-menu/mapping_info_view.js
Outdated
Show resolved
Hide resolved
@daniel-wer Ok, I think this is ready-for-review-ish. I still need to adapt the table in the "segmentation tab" so that the new color generation is used there, too. But I think the rest is ready to review and test. Thanks to @valentin-pinkau's suggestion, I also revised the pseudo-random generation of pattern features to use primitive roots which gives us an elegant approach without collisions within these sequences. I added a detailed explanation to the source code and hope this makes sense :) |
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.
Absolutely great work! I can only imagine how hard it was to write and debug all of this shader code, but it works very well!
I reviewed most of the code and added some questions and suggestions. I didn't review the part involving the scaling of the world coordinates in detail (lines 107+ in segmentation.glsl) as my brain shut off trying. I'm not sure whether it's needed, though as it works very well 👍
I would propose to add a general description of what kinds of patterns, angles and colors we're using now - a little high-level description of what's going on.
I'm not sure whether it's only me, but I find it very hard to distinguish most of the patterns with the default pattern opacity of 40 (in datasets with a full segmentation layer). A value like 70 works a lot better for me. Not a big deal, as it's configurable, but I wanted to throw this suggestion in here :)
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.js
Show resolved
Hide resolved
One more thing: The isosurface color seems to still be the old color as well. I don't think the isosurfaces need the patterns, but the color should be equal :) |
…mentation-patterns
… be more robust against numerical instability
I added some explaining comments there and hope it's easier to grasp now :)
Good idea! Added that now.
Yeah, I noticed that the ideal segmentation pattern opacity value highly depends on the actual segmentation opacity value (so, 40 works well for me when the volume opacity is higher). This is not really ideal, but I couldn't come up with a quick solution which improves this. In the end, most users should fine tune this, anyway, I think.
Yeah, that was the last missing piece (also affected the segmentation tab table). It was a bit tricky to get right, since the color calculation suffers from imprecisions. In the end, I ensured that the JS implementation suffers from the same imprecisions 🙈 Since it's consistent, it seems fine to me. I also changed the color count so that the used primitive root is 2 instead of 3, since this yields better precision. @MichaelBuessemeyer Since @daniel-wer is on vacation, could you give this PR a final pass (test it and also look over the code)? Thank you! |
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.
Thanks for this awesome PR. It really improves the distinguishability of the segments 🤓
I let some comments about a few parts I do not understand and I would place a few methods in another file.
The merge this when the questions and comments are resolved 😄
frontend/javascripts/oxalis/view/right-menu/mapping_info_view.js
Outdated
Show resolved
Hide resolved
@MichaelBuessemeyer Thanks for your feedback! I just incorporated it. Could you have another look? |
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.
I got one last thing: I mentioned to maybe move the none-shader implementation of getElementOfPermutation
, colormapJet
and rgb2hsv
to another js file. What do you think about this?
After this is resolved, let's merge it 🚢
Alright, the last thing that might need to be done is updating the screenshots 🖼️. After that let's merge this awesomeness 🕺 |
URL of deployed dev instance (used for testing):
Steps to test:
Issues: