-
Notifications
You must be signed in to change notification settings - Fork 441
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
Picking overhaul #4254
Picking overhaul #4254
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4254 +/- ##
==========================================
- Coverage 95.82% 95.77% -0.06%
==========================================
Files 127 127
Lines 21105 21357 +252
==========================================
+ Hits 20224 20454 +230
- Misses 881 903 +22 |
…to feat/picking-overhaul
@banesullivan, don't take this as a complete review, but local testing (both unit and functional) of this branch on a closed source library that makes heavy use of the existing picking API works with no issues. Full review will likely happen over the weekend, but don't wait for me if you'd like to merge sooner. |
@pyvista/developers, ready for review if anyone has the bandwidth |
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.
There are several new methods in plotting/picking.py
and plotting/render_window_interactor
that codecov is warning about.
* Face: pick a single face of a cell on the mesh | ||
* Edge: pick a single edge of a cell on the mesh | ||
* Point: pick a single point on the mesh |
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.
* Face: pick a single face of a cell on the mesh | |
* Edge: pick a single edge of a cell on the mesh | |
* Point: pick a single point on the mesh | |
* Point: pick a single point on the mesh (equivalent to :func:`enable_point_picking() <pyvista.Plotter.enable_point_picking>`.) |
What about helper methods for enable_face_picking
and enable_edge_picking
?
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 also see a method enable_rectangle_picking
, is there a reason it's left out of this group of examples?
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.
This is actually a bit different than enable_point_picking
. enable_point_picking
picks points under the cursor: sometimes in free space, sometimes on a mesh, and if the right picker is chosen, sometimes it will snap to points. enable_element_picking
specifically snaps to points on a mesh. This is why I left it out.
enable_rectangle_picking
isn't meant to be used directly, so I don't want to advertise it. enable_rectangle_through_picking
and enable_rectangle_visible_picking
are the interfaces meant to be used for this type of picking. Further, rectangle selection picks under some sort of area or volume whereas these methods are point-based picking
Co-authored-by: Anne Haley <anne.haley@kitware.com>
Co-authored-by: Anne Haley <anne.haley@kitware.com>
Co-authored-by: Anne Haley <anne.haley@kitware.com>
Co-authored-by: Anne Haley <anne.haley@kitware.com>
Thanks for the review, @annehaley!
This is a big PR, so things are falling through the cracks. Coverage stands at 93.19% for the diff alone (above my bar) but this is a big PR, so the missing ~7% is still significant. I'm unsure if I have the bandwidth to get us to 100% right now. I'll take another look at the missing coverage and see if anything seems problematic |
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.
Functional testing passes on my end, and integration testing with a closed source application that uses picking (on windows and linux) works. Made one minor commit as it would fail when running it interactively. Otherwise, LGTM.
An internal overhaul to the picking interface to improve stability and useability with the following upgrades:
The redesign splits picking into two modes:
All additional picking methods build on those two interfaces.
Backward compatibility is maintained for the most part (some callbacks did change). Specifically, I need to document what changed:
use_mesh
argument for point picking has been removedResolves
Resolves #1012
Resolves #3564
Resolves #2578
Resolves #3027 - makes it easier to choose which picker to use
Resolves #3703
Resolves #3915
Resolves #4381
Resolves #2520
To do
picker
setter/getter interface for tab-completion (not just hardcoded strings)