Instrument View 2.0: Add rectangular widget #40513
Conversation
GuiMacielPereira
added
ISIS: Core
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe changes introduce an implicit-function-based masking system to replace the previous cylinder-specific approach. A new WidgetType enum and RectangleWidgetNoRotation class enable support for both cylindrical and rectangular selection shapes. The FullInstrumentViewWindow is refactored to use a shape selector dropdown and toggle mechanism for adding/removing widgets, while the FullInstrumentViewPresenter is updated to compute masks using implicit function evaluation rather than VTK cylinder functions. The presenter now obtains implicit functions from the view widget and evaluates them at detector mesh points to determine mask membership. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py (1)
247-253: Consider using NumPy for performance on large detector meshes.The list comprehension iterating over
self._detector_mesh.pointsworks correctly, but for instruments with many detectors (potentially tens of thousands of points), this Python loop may become a bottleneck compared to a vectorized NumPy approach.🔎 Suggested vectorized approach
def on_add_mask_clicked(self) -> None: implicit_function = self._view.get_current_widget_implicit_function() if not implicit_function: return - mask = [(implicit_function.EvaluateFunction(pt) < 0) for pt in self._detector_mesh.points] + # Evaluate implicit function for all points + mask = np.array([implicit_function.EvaluateFunction(pt) < 0 for pt in self._detector_mesh.points]) new_key = self._model.add_new_detector_mask(mask) self._view.set_new_mask_key(new_key)Note: VTK's
EvaluateFunctiondoesn't support batch evaluation natively, so the loop is unavoidable. However, wrapping the result innp.array()ensures consistent array type for downstream operations. If performance becomes critical, consider using VTK'svtkImplicitFunctionToImageStencilor similar batch evaluation utilities.qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py (1)
76-85: Document interaction state constants for improved maintainability.The magic numbers 8 (Rotating) and 7 (Translating) used in
SetInteractionState()are correct for vtkBoxWidget2, but would benefit from named constants or inline comments explaining their meaning. This mirrors the established pattern withCylinderWidgetNoRotationand improves code clarity for future maintainers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py(1 hunks)qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: MialLewis
Repo: mantidproject/mantid PR: 39893
File: qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py:33-38
Timestamp: 2025-09-03T14:53:35.361Z
Learning: In the SliceViewer masking system (qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py), all classes that inherit from SelectionMaskingBase are guaranteed to set _selector during initialization, and this selector persists for the lifetime of the object. There are no ordering issues between disconnect() and clear() methods - disconnect() only calls _selector.disconnect_events() without nullifying the _selector reference, so artists remain accessible for removal.
Learnt from: MialLewis
Repo: mantidproject/mantid PR: 39893
File: qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py:33-38
Timestamp: 2025-09-03T14:53:35.361Z
Learning: In the SliceViewer masking system (qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py), all classes that inherit from SelectionMaskingBase are guaranteed to set _selector during initialization, and this selector persists for the lifetime of the object. There are no ordering issues between disconnect() and clear() methods - disconnect() only calls _selector.disconnect() without nullifying the _selector reference, so artists remain accessible for removal.
Learnt from: MialLewis
Repo: mantidproject/mantid PR: 39893
File: qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/presenter.py:220-224
Timestamp: 2025-09-03T14:59:18.497Z
Learning: In the SliceViewer masking system (qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py), the clear_and_disconnect() method properly handles selector cleanup without memory leaks. The method ensures all selectors and their matplotlib artists are properly destroyed, making additional cleanup steps unnecessary before calling clear_model().
📚 Learning: 2025-09-03T14:59:18.497Z
Learnt from: MialLewis
Repo: mantidproject/mantid PR: 39893
File: qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/presenter.py:220-224
Timestamp: 2025-09-03T14:59:18.497Z
Learning: In the SliceViewer masking system (qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py), the clear_and_disconnect() method properly handles selector cleanup without memory leaks. The method ensures all selectors and their matplotlib artists are properly destroyed, making additional cleanup steps unnecessary before calling clear_model().
Applied to files:
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py
📚 Learning: 2025-09-03T14:59:18.497Z
Learnt from: MialLewis
Repo: mantidproject/mantid PR: 39893
File: qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/presenter.py:220-224
Timestamp: 2025-09-03T14:59:18.497Z
Learning: In the SliceViewer masking system (qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py), the clear_and_disconnect() method comprehensively handles cleanup: it sets selectors inactive, calls disconnect() to remove event handlers via disconnect_events(), calls clear() which removes matplotlib artists by iterating through _selector.artists and calling artist.remove(), and processes all selectors in the collection. No additional cleanup steps are needed before calling clear_model().
Applied to files:
qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py
🧬 Code graph analysis (2)
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py (2)
qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py (1)
get_current_widget_implicit_function(719-729)Framework/PythonInterface/mantid/simpleapi.py (1)
EvaluateFunction(374-383)
qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py (2)
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py (1)
on_add_cylinder_clicked(244-245)qt/python/instrumentview/instrumentview/Projections/ProjectionType.py (1)
ProjectionType(10-18)
🔇 Additional comments (7)
qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py (7)
59-61: LGTM!Clean enum definition with meaningful user-facing values. The choice of "Circle" and "Rectangle" for the UI makes sense given the 2D projection context.
466-475: LGTM!The toggle logic cleanly handles widget creation/deletion based on shape selection and properly manages the "Add Mask" button state.
477-480: LGTM!Correctly handles the projection change behavior: untoggles the button (triggering cleanup) when switching projections, and disables the widget for 3D views as specified in the PR requirements.
690-709: LGTM!The rectangular widget creation follows the established pattern from
add_cylinder_widget. Initial placement at the screen center (1/3 to 2/3 bounds) is reasonable for user convenience.Minor nit: the explicit
returnat line 709 is unnecessary but harmless.
719-729: LGTM!Clean implementation that properly handles both widget types and returns the appropriate VTK implicit function. The
isinstancechecks provide clear type discrimination.
227-240: LGTM!The UI setup correctly implements the dropdown and toggle button approach mentioned in the PR objectives. Using
WidgetTypeenum values for the combo box ensures consistency.
462-464: LGTM!Signal connections are correctly set up, and the initial disabled state of the "Add Mask" button is appropriate since no widget exists yet.
| world_x, world_y, _world_z, world_w = renderer.GetWorldPoint() | ||
| return world_x / world_w, world_y / world_w, world_y / world_w |
There was a problem hiding this comment.
Bug: Third return component uses world_y instead of world_z.
Line 717 returns world_y / world_w as the third component instead of the z-coordinate. While callers currently discard this value (using _z pattern), this should still be fixed for correctness.
🔎 Proposed fix
- world_x, world_y, _world_z, world_w = renderer.GetWorldPoint()
- return world_x / world_w, world_y / world_w, world_y / world_w
+ world_x, world_y, world_z, world_w = renderer.GetWorldPoint()
+ return world_x / world_w, world_y / world_w, world_z / world_w📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| world_x, world_y, _world_z, world_w = renderer.GetWorldPoint() | |
| return world_x / world_w, world_y / world_w, world_y / world_w | |
| world_x, world_y, world_z, world_w = renderer.GetWorldPoint() | |
| return world_x / world_w, world_y / world_w, world_z / world_w |
🤖 Prompt for AI Agents
In qt/python/instrumentview/instrumentview/FullInstrumentViewWindow.py around
lines 716-717, the function unprojects world coordinates and currently returns
the third component as world_y / world_w; change that third returned value to
the z-coordinate (use _world_z / world_w as _world_z is the variable from
renderer.GetWorldPoint()) so the return tuple is (world_x/world_w,
world_y/world_w, _world_z/world_w). Ensure no other callers rely on the
incorrect value.
jclarkeSTFC
left a comment
jclarkeSTFC
left a comment
There was a problem hiding this comment.
This looks good, I like the rectangle select and the drop down for the shapes. I think it needs some tests on the code in the view, mainly that the sizes are set appropriately.
I also managed to get it to hang , but only once, I think it's the same problem that I had with the add peaks PR. I think all the algorithms that add to the ADS should not do that, e.g CloneWorkspace. We should call CloneWorkspace with StoreInADS=False, then put that workspace in the ADS ourselves if required. This should prevent the ADS callback causing a deadlock.
|
|
||
| def _on_interaction(self): | ||
| # Replace rotation state with translation state | ||
| if self.GetRepresentation().GetInteractionState() == 8: |
There was a problem hiding this comment.
What are these numbers? E.g. 8, 7
There was a problem hiding this comment.
They're the representation of the rotation state and translation state in vtk, I've changed the comments so hopefully it'll be clear now
| cylinder = vtkCylinder() | ||
| widget.GetCylinderRepresentation().GetCylinder(cylinder) | ||
| mask = [(cylinder.FunctionValue(pt) < 0) for pt in self._detector_mesh.points] | ||
| mask = [(implicit_function.EvaluateFunction(pt) < 0) for pt in self._detector_mesh.points] |
There was a problem hiding this comment.
Maybe the rabbit is right about this line, potential numpy speed up?
There was a problem hiding this comment.
But the rabbit also agreed that the loop needs to be done:
Note: VTK's EvaluateFunction doesn't support batch evaluation natively, so the loop is unavoidable.
Wrapping it up in an array is redundant as it already gets converted down the line
jclarkeSTFC
left a comment
jclarkeSTFC
left a comment
There was a problem hiding this comment.
Thanks for making those changes, although when I click overwrite mask I get this error:
Traceback (most recent call last):
File "C:\Mantid\Clean\mantid\qt\python\instrumentview\instrumentview\FullInstrumentViewPresenter.py", line 265, in on_overwrite_mask_clicked
self._model.overwrite_mask_to_current_workspace()
File "C:\Mantid\Clean\mantid\qt\python\instrumentview\instrumentview\FullInstrumentViewModel.py", line 452, in overwrite_mask_to_current_workspace
MaskDetectors(ws, MaskedWorkspace=self.mask_ws)
File "C:\Mantid\Clean\mantid\Framework\PythonInterface\mantid\simpleapi.py", line 1095, in __call__
raise RuntimeError(msg) from e
RuntimeError: MaskDetectors-v1: Add Data Object with empty nameFor the VTK interaction state integers, I think an enum would be best. I had a quick look but couldn't find where those integers are in the VTK docs, did you have to look in their code?
One other minor thing, is it possible to make the rectangle pan when dragging the dot in the middle? I mean like how the circle works.
This reverts commit ca5f27c. Need to store workspace in ADS for MaskWorkspaces() to work, otherwise throws an error.
|
For the enum, the types for the cylinder are defined here: https://vtk.org/doc/nightly/html/classvtkImplicitCylinderRepresentation.html#afca5ba71d1dfb07012c60d0738312968 and for the rectangle are in https://vtk.org/doc/nightly/html/classvtkBoxRepresentation.html: 
In this case it looks like it doesn't even have a name. For moving the rectangle in the middle, it's complicated because the middle of the rectangle is a handler used for resizing, same as the sphere handlers you see on all sides, so I couldn't find a solution to restrict the sphere handlers of only 2 out of the 6 faces of the cube. |
jclarkeSTFC
left a comment
jclarkeSTFC
left a comment
There was a problem hiding this comment.
Thanks, hanging looks to be fixed now. One problem left is that if there is a workspace in the ADS called, e.g. Mask 1, then the name collision in the masking interface when adding a mask results in some strange behaviour, probably needs a check for unique name when creating the masks? Minor at the moment, but I'm thinking in the future it could cause a bigger problem.
| def on_toggle_add_mask(self, checked): | ||
| if checked: | ||
| if self._shape_options.currentText() == WidgetType.Cylinder.value: | ||
| self._presenter.on_add_cylinder_clicked() |
There was a problem hiding this comment.
I think on_add_cylinder_clicked on the presenter can be replaced with add_cylinder_widget, like the rectangle does, then the method on the presenter can be deleted.
There was a problem hiding this comment.
Well spotted, the reason I had it in the presenter was because I needed the bounds from the mesh but turns out I can just get the bounds from the plotter directly, so I can remove the need to go through the presenter 👍
jclarkeSTFC
left a comment
jclarkeSTFC
left a comment
There was a problem hiding this comment.
Working well, looks good
Description of work
This PR:
I opted for using a dropdown instead of icons for now. This can be changed in future PRs if users prefer icons.
Closes #40048
To test:
Add Shapeand check that widget is cancelledAdd Shapetoggled, change to 3D projection, buttons should get disabledAdd Shapetoggled, change to any non-3D projection, button should get untoggledReviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
mantid-developersormantid-contributorsteams, add a review commentrerun cito authorize/rerun the CIGatekeeper
As per the gatekeeping guidelines: