Skip to content

Clipping plane slider layer controls #29

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

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

AnniekStok
Copy link
Collaborator

@AnniekStok AnniekStok commented Mar 21, 2025

Proposed Change

Implementation of UI controls for the 'experimental clipping planes' feature. Both image and labels layer controls show tools to set the plane and clipping plane values (only when a layer is displayed in 3D and has >=3 dimensions). By clicking the 'clipping plane' checkbox, users can (de)activate the clipping planes. The planeNormalButtons are used for setting both the plane and the clipping plane normals simultaneously. I added the 'experimental_clipping_planes' event to the Points and Tracks layers as well. They do not have UI tools, but this allows the clipping plane settings to be synced across image/labels/points/tracks layers with the 'link_layers' function (or in the UI by selecting layers and right-click > link layers). The clipping plane event is linked for all layers from the TracksLayerGroup by default, but this can be deactivated with TracksLayerGroup.unlink_clipping_planes() function.

image

Checklist

Go through these things before merge. Actions should run automatically to test them, but for information on how to run locally, see CONTRIBUTING.md.

  • I have added tests that prove that my feature works in various situations or tests the bugfix (if applicable).
  • I have checked that the tests pass and I maintained or improved test coverage (if applicable).
  • I have written docstrings and checked that they render correctly in the documentation build.

Further Comments

  • what should we do when the user clicks on a node in the tree view that is not visible because it is clipped? Just accept that you cannot see it, or somehow show it anyway?
  • optionally include an image layer in the TracksLayerGroup so that the clipping plane can be linked.
  • for now we decided to allow 2D+time volume view and clipping planes, even though it looks a bit funny.

@AnniekStok AnniekStok added the feature New feature or request label Mar 21, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2025

Codecov Report

Attention: Patch coverage is 95.19520% with 16 lines in your changes missing coverage. Please review.

Project coverage is 89.48%. Comparing base (81925e1) to head (599425e).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
finn/layers/image/_image_key_bindings.py 33.33% 6 Missing ⚠️
...layer_controls/qt_layer_clipping_plane_controls.py 97.26% 4 Missing ⚠️
finn/_qt/layer_controls/qt_image_controls.py 87.50% 2 Missing ⚠️
finn/_qt/layer_controls/qt_labels_controls.py 84.61% 2 Missing ⚠️
...qt/layer_controls/_tests/test_qt_layer_controls.py 0.00% 1 Missing ⚠️
...rack_data_views/views/layers/tracks_layer_group.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   89.37%   89.48%   +0.10%     
==========================================
  Files         651      650       -1     
  Lines       59932    60162     +230     
==========================================
+ Hits        53565    53835     +270     
+ Misses       6367     6327      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AnniekStok AnniekStok marked this pull request as ready for review March 28, 2025 13:15
@AnniekStok AnniekStok requested a review from cmalinmayor March 28, 2025 13:17
Copy link
Contributor

@cmalinmayor cmalinmayor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay - I finally went through and read all the code changes. Left some comments that are mostly for my understanding or about renaming stuff. Overall I love it!! The test of the linking that you added made me very happy 😄. Once the renaming questions are decided we can merge!

@AnniekStok
Copy link
Collaborator Author

Thanks for your review and comments! I tried to address them as much as possible. I just could not find a better way to add the widget while maintaining a consistent layout, and I am not sure about the action manager, what do you think we should do here?

@cmalinmayor
Copy link
Contributor

All good about the per-row addition - I just was wondering if there was a better way, but I believe you that there is not!

@cmalinmayor cmalinmayor self-requested a review April 24, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants