Instrument View: Fix picking when detector shapes are drawn#41127
Instrument View: Fix picking when detector shapes are drawn#41127GuiMacielPereira wants to merge 7 commits intomainfrom
Conversation
This ensures that tiles overlap and there is no conflict with picking
GuiMacielPereira
added
the
Epic
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughRenderer initialization in the instrument view was refactored from lazy/cached construction to eager instantiation. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py`:
- Around line 81-82: Recreate the SideBySideShapeRenderer after the model's
projection/ workspace state is synchronized so it captures current
bank_groups_by_detector_id instead of the empty initial value: replace
persistent construction sites that set self._sbs_shape_renderer =
SideBySideShapeRenderer(...) (e.g. in FullInstrumentViewPresenter where
_shape_renderer and _sbs_shape_renderer are built) with lazy re-creation logic
and call that re-creation from the projection-sync / workspace-replacement path
(the code that runs after FullInstrumentViewModel.setup() and in the same place
_on_show_shapes_toggled() reads the renderer) so that SideBySideShapeRenderer is
re-instantiated with the up-to-date self._model.bank_groups_by_detector_id
whenever the active projection or workspace is updated.
In `@qt/python/instrumentview/instrumentview/renderers/shape_renderer.py`:
- Around line 391-398: In the SIDE_BY_SIDE branch the group_scales are taken
from per_detector_scales and thus omit each detector's native scale
(ComponentInfo.scaleFactor from self._det_scales), causing incorrect vertex
sizes and overlap math; modify the SIDE_BY_SIDE branch so group_scales combines
the per-detector component scales with the per-detector projection scales (the
same way non-side-by-side uses scales[det_indices[group_indices]]), i.e. use
self._det_scales (or the appropriate per-detector scale source)
multiplied/applied to per_detector_scales when building group_scales for
ProjectionType.SIDE_BY_SIDE so _compute_bank_projection_scales and rendered
vertices get the detector native scale. Ensure rotate_mask, group_pos
(display_positions), and indexing by det_indices/group_indices remain
consistent.
In
`@qt/python/instrumentview/instrumentview/renderers/side_by_side_shape_renderer.py`:
- Around line 140-142: When bank_local_groups is empty, do NOT set
per_det_scale[:] = 1.0; instead implement a uniform nearest-neighbour fallback
so overlap-avoidance remains active: for each detector index in per_det_scale,
find the nearest detector that has a valid computed scale (using detector
positions / bank membership available in the renderer) and copy that scale into
per_det_scale; leave per_det_rotate unchanged. Update the logic in
side_by_side_shape_renderer (around the bank_local_groups check referencing
per_det_scale and per_det_rotate) so that if no local groups exist you populate
per_det_scale from nearest-valid neighbours and only if no neighbours exist use
a safe default, rather than unconditionally assigning 1.0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 995dc9b1-d862-4b8d-9bc5-3be3a35a1c3b
📒 Files selected for processing (6)
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.pyqt/python/instrumentview/instrumentview/renderers/shape_renderer.pyqt/python/instrumentview/instrumentview/renderers/side_by_side_shape_renderer.pyqt/python/instrumentview/instrumentview/renderers/test/test_renderers.pyqt/python/instrumentview/instrumentview/renderers/test/test_side_by_side_shape_renderer.pyqt/python/instrumentview/instrumentview/test/test_presenter.py
qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py
Outdated
Show resolved
Hide resolved
qt/python/instrumentview/instrumentview/renderers/shape_renderer.py
Outdated
Show resolved
Hide resolved
| if not bank_local_groups: | ||
| uniform = self._compute_projection_scale(det_indices, projected_positions) | ||
| per_det_scale[:] = uniform | ||
| per_det_scale[:] = 1.0 | ||
| return per_det_scale, per_det_rotate |
There was a problem hiding this comment.
Don't fall back to unit-size tiles when bank grouping is unavailable.
Returning 1.0 here bypasses the overlap-avoidance logic and restores full-size detector shapes whenever bank groups are missing, stale, or incomplete. That path is reachable from the new eager presenter construction, so side-by-side picking can still regress to the old ambiguous overlap behaviour unless this keeps a uniform nearest-neighbour fallback instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@qt/python/instrumentview/instrumentview/renderers/side_by_side_shape_renderer.py`
around lines 140 - 142, When bank_local_groups is empty, do NOT set
per_det_scale[:] = 1.0; instead implement a uniform nearest-neighbour fallback
so overlap-avoidance remains active: for each detector index in per_det_scale,
find the nearest detector that has a valid computed scale (using detector
positions / bank membership available in the renderer) and copy that scale into
per_det_scale; leave per_det_rotate unchanged. Update the logic in
side_by_side_shape_renderer (around the bank_local_groups check referencing
per_det_scale and per_det_rotate) so that if no local groups exist you populate
per_det_scale from nearest-valid neighbours and only if no neighbours exist use
a safe default, rather than unconditionally assigning 1.0.
There was a problem hiding this comment.
@jclarkeSTFC I removed the function to calculate the nearest-neighbour scaling, mostly because I thought that scaling only affects the size of the detector shapes, so it shouldn't be that important? Do you know more about these cases where banks are missing and bank_local_groups is an empty list?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think that it shouldn't be possible for bank_local_groups to be empty, because in the side-by-side projection, any detectors not in a grid in the IDF are put into a custom flat bank that arranges them in a grid, i.e. if there are some detectors then there will always be at least one bank.
Unit test results2 863 tests 2 863 ✅ 12h 28m 27s ⏱️ Results for commit db160d9. ♻️ This comment has been updated with latest results. |
System test results808 tests 792 ✅ 2h 47m 58s ⏱️ Results for commit db160d9. ♻️ This comment has been updated with latest results. |
Took out bank_groups_by_detector_id out of initializer because it should be called every time that build_mesh is called, otherwise risk freezing it for one projection and it's wrong if the projection gets updated. Also reduced the dependency of renderes on model
Easier to call and to use, better encapsulation of the different projections now all under the same class Projection
jclarkeSTFC
left a comment
jclarkeSTFC
left a comment
There was a problem hiding this comment.
This looks good, the picked detectors are highlighted in a better way and the code is simpler, was working fine for me.
| root_position: np.ndarray, | ||
| detector_positions: list[DetectorPosition] | np.ndarray, | ||
| axis: np.ndarray, | ||
| **kwargs, |
There was a problem hiding this comment.
Did you leave these comments here deliberately, to see what kwargs is supposed to have?
| workspace = self._create_mock_workspace(n_detectors=1) | ||
| self.renderer.precompute(workspace) | ||
| self.renderer = ShapeRenderer(workspace) | ||
| self.renderer.precompute() |
There was a problem hiding this comment.
I think these three lines can go into a method that takes n_detectors as an argument, they appear quite a few times.
2 participants
Description of work
Drawing the detector shapes causes the projection in 2d to often overlap shapes, leading to inconsistent picking. This PR uses a trick to assign each detector shape in 2d a small height in Z, with the detectors closer to the centre of the mesh having the lowest Z height and the detectors away from the centre have increasing Z height.
The idea is to make tiles overlap like a roof, so that picking will always choose the tile that is in front of another.
I also did some general cleaning of the tiling implementation during assembling the mesh.
Closes #xxxx.
To test:
Reviewer
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: