[TRCL-664][feat] SPARC GUI: support rotated RoA #3311
Conversation
pieleric
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for rotated Region of Acquisition (RoA) in the SPARC GUI, enabling vector-scanned e-beam to acquire data at arbitrary rotation angles. The implementation provides rotation controls when the scanner supports the scanPath attribute.
Key changes:
- New utility functions
rotate_rect()andseparate_rect_rotation()for rectangle rotation transformations - Enhanced
RepetitionSelectOverlayandRectangleEditingMixinwith rotation support and improved editing controls - Updated pixel/spectrum overlays to handle rotated spectral data using stream coordinate transformations
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/util/init.py | Added rotate_rect() and separate_rect_rotation() utility functions for rectangle rotation transformations |
| src/odemis/util/test/util_test.py | Added comprehensive test cases for rotation functions covering various angles and edge cases |
| src/odemis/gui/model/tab_gui_data.py | Added roa_rotation VA to enable rotation control when scanner supports vector scanning |
| src/odemis/gui/cont/tabs/sparc_acquisition_tab.py | Updated spot position initialization to use global RoA instead of stream-specific ROI |
| src/odemis/gui/cont/acquisition/sparc_acq.py | Added validation to prevent acquisition when rotated RoA exceeds FoV boundaries |
| src/odemis/gui/cont/stream_bar.py | Added rotation synchronization between tab data model and streams with ROI |
| src/odemis/gui/cont/tabs/analysis_tab.py | Refactored to use set_stream() instead of set_data_properties() for pixel overlays |
| src/odemis/gui/comp/overlay/repetition_select.py | Major refactor to support rotation: added physical coordinates tracking, rotation handling, edit knobs, and side labels |
| src/odemis/gui/comp/overlay/rectangle.py | Updated to use renamed start_creation() method and removed unused center parameter |
| src/odemis/gui/comp/overlay/pixel_select.py | Updated to use stream-based coordinate transformations and handle rotated pixels |
| src/odemis/gui/comp/overlay/spectrum_line_select.py | Updated to use cairo_polygon() for drawing rotated pixel rectangles |
| src/odemis/gui/comp/overlay/world_select.py | Added TODO comment about refactoring overlay class hierarchy |
| src/odemis/gui/comp/overlay/base.py | Enhanced RectangleEditingMixin with rotation support, normalized rectangle handling, PixelDataMixin refactored to use stream coordinates, added cairo_polygon() helper |
| src/odemis/gui/comp/overlay/view_select.py | Removed unused ViewSelectOverlay class |
| src/odemis/gui/comp/miccanvas.py | Updated RepetitionSelectOverlay initialization to pass rotation parameter |
| src/odemis/gui/test/comp_overlay_test.py | Updated tests to use corners instead of rectangles, added rotation tests, refactored spectrum stream creation |
| src/odemis/acq/stream/_base.py | Enhanced documentation for getPixelCoordinates() and getPhysicalCoordinates() |
| plugins/secom_cl.py | Set roa_rotation = None to indicate no rotation support for SECOM-CL acquisitions |
Comments suppressed due to low confidence (2)
src/odemis/gui/cont/stream_bar.py:981
- The rotation subscription added in
_connectROIat line 963 is not properly unsubscribed in_disconnectROI. This could lead to a memory leak or unexpected behavior when streams are removed. Theroa_rotation.unsubscribe(self._on_roa_rotation)call should be added in the_disconnectROImethod, similar to how the ROA subscription is handled.
def _disconnectROI(self, stream):
"""
Remove ROI subscriptions for the stream.
It's fine to call for a stream which is not connected to ROI.
stream (Stream): the stream being removed
"""
if stream in self._roi_listeners:
logging.debug("Removing %s from ROI subscriptions", stream)
# Removing the callable from the roi_listeners should be sufficient,
# as the callable should be unreferenced and free'd, which should drop
# it from the subscriber... but let's make everything explicit.
stream.roi.unsubscribe(self._roi_listeners[stream])
del self._roi_listeners[stream]
src/odemis/gui/comp/overlay/rectangle.py:81
- This class does not call WorldOverlay.init during initialization. (RectangleOverlay.init may be missing a call to a base class init)
class RectangleOverlay(EditableShape, RectangleEditingMixin, WorldOverlay):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughAdds vector-rotation support for ROA across the GUI, streams, overlays, acquisition controller, and utilities. A new optional public attribute Sequence Diagram(s)sequenceDiagram
actor User
participant GUI as SparcAcquisitionGUIData
participant StreamBar as StreamBarController
participant Stream as Stream
participant Overlay as Repetition/Pixel/Spectrum Overlay
participant Acq as SparcAcquiController
participant Util as util.rotate_rect / separate_rect_rotation
User->>GUI: set RoA and/or roa_rotation
GUI->>StreamBar: publish roa and roa_rotation
StreamBar->>Stream: init stream rotation & subscribe to updates
Stream->>Overlay: provide metadata via set_stream (includes rotation)
Overlay->>Util: compute rotated physical corners (rotate_rect)
Overlay->>Acq: emit on_roa / on_rotation events
Acq->>Util: validate rotated corners within [0,1] (separate_rect_rotation)
Acq-->>User: enable/disable Acquire UI and show warnings
Possibly related PRs
🚥 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 unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/odemis/gui/cont/stream_bar.py (1)
953-968: Guardroa_rotationaccess and avoid repeated subscriptionsThe rotation wiring in
_connectROIis conceptually right, but the implementation is brittle:
if self._tab_data_model.roa_rotation:will raiseAttributeErrorif a tab defines.roabut not.roa_rotation.- Calling
self._tab_data_model.roa_rotation.subscribe(self._on_roa_rotation)on every_connectROIrisks duplicate subscriptions and unnecessary callbacks.Consider:
- Resolving the rotation VA once (e.g. in
__init__) viagetattr(tab_data, "roa_rotation", None).- Subscribing
_on_roa_rotationonly once when that VA exists, and only using it in_connectROIto seedstream.rotation.value.Example adjustment:
Proposed safer rotation hookup
@@ class StreamBarController(object): - self._tab_data_model = tab_data + self._tab_data_model = tab_data + # Optional global ROA rotation VA + self._roa_rotation_va = getattr(tab_data, "roa_rotation", None) @@ def _connectROI(self, stream): - stream.roi.value = self._tab_data_model.roa.value - self._tab_data_model.roa.subscribe(self._onROA) - - if self._tab_data_model.roa_rotation: - stream.rotation.value = self._tab_data_model.roa_rotation.value - self._tab_data_model.roa_rotation.subscribe(self._on_roa_rotation) + stream.roi.value = self._tab_data_model.roa.value + self._tab_data_model.roa.subscribe(self._onROA) + + if self._roa_rotation_va is not None: + stream.rotation.value = self._roa_rotation_va.value + # Subscribe once (idempotent if VA de-duplicates callbacks) + self._roa_rotation_va.subscribe(self._on_roa_rotation)src/odemis/acq/stream/_base.py (1)
1259-1267: Clarify coordinate docstrings to match actual behaviorThe new docstrings are slightly misleading relative to the current implementations:
getPixelCoordinates(...)returns integer indices whencheck_bbox=True(afterfloor+ bounds check), and floating coordinates only whencheck_bbox=False. The text currently describes float-in-pixel semantics unconditionally.getPhysicalCoordinates(...)accepts floats (and works fine with sub‑pixel values likei+0.5), but the wording could better stress that pixel coordinates are in image‑index units (0 at top‑left pixel border, 1 per pixel step), not normalized 0–1 over the whole image.Consider tightening the docstrings along these lines:
Suggested docstring tweak
- :returns: the position in pixel coordinates, as float, going from +(0, 0) for the top-left of - the pixel to +(1.0, 1.0) for the bottom-right of the pixel. So you can use int() to get the pixel index. - It returns None if it's outside of the image and check_boundary is True + :returns: the position in pixel coordinates. + * If ``check_bbox`` is True (default), returns integer pixel indices ``(ix, iy)`` or + ``None`` when outside the image. + * If ``check_bbox`` is False, returns float pixel coordinates, where each unit + corresponds to one pixel (fractional part describes the position inside a pixel).This keeps callers from assuming float returns in all cases.
Also applies to: 1297-1303
🧹 Nitpick comments (8)
src/odemis/gui/comp/overlay/world_select.py (1)
39-40: Consider tracking these refactoring TODOs in your issue tracker.The TODO comments document substantial architectural changes that would benefit from formal tracking and prioritization. This ensures they don't get lost and can be planned appropriately.
src/odemis/gui/comp/overlay/base.py (1)
1965-2051: Respect non-square pixels inPixelDataMixin.pixel_to_rect
pixel_to_rectcurrently uses the X pixel size for both width and height:b_pixel_size = (self._phys_pxs[0] * scale, self._phys_pxs[0] * scale)This assumes square pixels and slightly distorts the highlighted area when X/Y pixel sizes differ. Since you now centralized this helper, it’s a good moment to make it fully general:
Suggested fix for asymmetric pixel sizes
- b_pixel_size = (self._phys_pxs[0] * scale, self._phys_pxs[0] * scale) + b_pixel_size = (self._phys_pxs[0] * scale, self._phys_pxs[1] * scale)That keeps behavior identical for square pixels while producing correct geometry for anisotropic pixels.
src/odemis/gui/comp/overlay/rectangle.py (1)
89-157: Constructor API change is consistent but might want optionalcan_rotateexposureThe new
__init__(self, cnvs, colour=..., show_selection_points=True)correctly matchesRectangleEditingMixin.__init__(self, colour)and keeps rotation enabled by default. If you foresee non-rotatable rectangles in the future, consider threading acan_rotateparameter through this constructor (defaulting toTrue) so callers don’t need to reach into the mixin directly.src/odemis/util/test/util_test.py (2)
587-591: Consider addingstrict=Truetozip()for safety.The static analyzer flags
zip()without explicitstrict=parameter. Sinceexpectedandactuallengths are asserted equal on line 588, addingstrict=Truewould make the intent clearer and catch any future bugs if the assertion is accidentally removed.🔎 Proposed fix
def _assert_corners_equal(self, expected: List[Tuple], actual:List[Tuple]): assert len(expected) == len(actual) - for e, a in zip(expected, actual): + for e, a in zip(expected, actual, strict=True): self.assertAlmostEqual(e[0], a[0]) self.assertAlmostEqual(e[1], a[1])
701-705: Unused variableres_angleis intentional but could be clearer.The static analyzer flags
res_angleas unused. Since this test only verifies the rectangle result for undefined ROI, consider using_to indicate the variable is intentionally ignored.🔎 Proposed fix
def test_undefined_roi(self): corners = [(2, 3)] * 4 - res_rect, res_angle = util.separate_rect_rotation(corners) + res_rect, _ = util.separate_rect_rotation(corners) self.assertEqual(res_rect, UNDEFINED_ROI)src/odemis/gui/comp/overlay/repetition_select.py (3)
166-199: Corner ordering differs fromWorldSelectOverlay.get_physical_sel.The
get_physical_selmethod returns corners in a reordered sequence (4, 3, 2, 1) to account for Y-axis inversion between view and physical coordinates. This differs from the baseWorldSelectOverlay.get_physical_selwhich returns a simple (l, t, r, b) rect. Ensure callers are aware of this change in return type (4 corners vs. 4-tuple rect).The docstring could be enhanced to clarify the corner ordering.
🔎 Suggested docstring enhancement
def get_physical_sel(self) -> Optional[List[Tuple[float, float]]]: """ Return the selected rectangle in physical coordinates - :return: Physical position in m of the 4 corners, or None if no selection + :return: Physical position in m of the 4 corners (ordered for rotate_rect compatibility: + bottom-left, bottom-right, top-right, top-left), or None if no selection """
280-310: Rotation conversion logic is correct but could use a comment.The rotation conversion
2 * math.pi - rotationcorrectly handles the Y-axis inversion between physical coordinates (Y up) and view coordinates (Y down). A brief inline comment explaining this would improve maintainability.🔎 Suggested comment
def on_roa(self, roa: Optional[Tuple[float, float, float, float]]): """ Update the ROA overlay with the new roa VA data roi (tuple of 4 floats): left, top, right, bottom position relative to the SEM image """ - phys_rot = 2 * math.pi - self.rotation # Y inverted => clockwise rotation becomes counter-clockwise + # Convert from view rotation to physical rotation: + # Y is inverted between coordinate systems, so counter-clockwise in view + # becomes clockwise in physical coordinates (and vice versa) + phys_rot = 2 * math.pi - self.rotation
338-373: Large TODO block indicates incomplete rotation clipping implementation.The TODO comments (lines 343-356) describe a significant missing feature: clipping the rotated rectangle to the scanner FOV. Currently, this is handled by the GUI controller disabling the "acquire" button when the ROA is out of bounds, but this is noted as a temporary workaround.
Consider tracking this as a follow-up issue to ensure it's addressed.
Would you like me to open an issue to track the implementation of proper rotation-aware ROA clipping to the scanner FOV?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
plugins/secom_cl.pysrc/odemis/acq/stream/_base.pysrc/odemis/gui/comp/miccanvas.pysrc/odemis/gui/comp/overlay/base.pysrc/odemis/gui/comp/overlay/pixel_select.pysrc/odemis/gui/comp/overlay/rectangle.pysrc/odemis/gui/comp/overlay/repetition_select.pysrc/odemis/gui/comp/overlay/spectrum_line_select.pysrc/odemis/gui/comp/overlay/view_select.pysrc/odemis/gui/comp/overlay/world_select.pysrc/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/test/comp_overlay_test.pysrc/odemis/util/__init__.pysrc/odemis/util/test/util_test.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/comp/overlay/view_select.py
🧰 Additional context used
🧬 Code graph analysis (11)
src/odemis/gui/comp/overlay/spectrum_line_select.py (1)
src/odemis/gui/comp/overlay/base.py (2)
cairo_polygon(61-73)pixel_to_rect(2029-2050)
src/odemis/gui/cont/tabs/analysis_tab.py (3)
src/odemis/gui/comp/overlay/base.py (1)
set_stream(1985-1992)src/odemis/gui/comp/overlay/pixel_select.py (1)
connect_selection(51-62)src/odemis/gui/comp/overlay/spectrum_line_select.py (1)
connect_selection(186-212)
src/odemis/gui/comp/overlay/repetition_select.py (3)
src/odemis/gui/comp/overlay/base.py (18)
Vec(295-342)cairo_polygon(61-73)text(99-100)text(103-105)pos(117-118)pos(121-123)align(135-136)align(139-141)clear_selection(773-784)clear_selection(1238-1253)clear_selection(1729-1734)x(322-323)y(326-327)rotate(329-342)draw(159-292)draw(2060-2061)draw(2078-2079)draw(2102-2139)src/odemis/gui/comp/overlay/world_select.py (5)
clear_selection(70-74)_phys_to_view(86-94)get_physical_sel(96-107)set_physical_sel(109-121)draw(123-185)src/odemis/util/__init__.py (2)
rotate_rect(430-466)separate_rect_rotation(469-514)
src/odemis/gui/cont/acquisition/sparc_acq.py (2)
src/odemis/gui/cont/stream_bar.py (1)
_onROA(1014-1036)src/odemis/util/__init__.py (1)
rotate_rect(430-466)
src/odemis/gui/model/tab_gui_data.py (2)
src/odemis/model/_components.py (2)
model(570-571)hasVA(63-69)src/odemis/model/_vattributes.py (1)
FloatContinuous(1072-1082)
src/odemis/gui/comp/overlay/rectangle.py (2)
src/odemis/gui/comp/overlay/shapes.py (1)
EditableShape(47-170)src/odemis/gui/comp/overlay/base.py (2)
RectangleEditingMixin(1125-1662)start_creation(1207-1214)
src/odemis/gui/comp/miccanvas.py (1)
src/odemis/gui/comp/overlay/repetition_select.py (1)
RepetitionSelectOverlay(42-631)
src/odemis/gui/comp/overlay/pixel_select.py (1)
src/odemis/gui/comp/overlay/base.py (4)
DragMixin(564-659)cairo_polygon(61-73)pixel_to_rect(2029-2050)dragging(644-646)
src/odemis/gui/comp/overlay/base.py (2)
src/odemis/acq/stream/_base.py (4)
Stream(91-1370)_find_metadata(988-1028)getPixelCoordinates(1259-1295)getPhysicalCoordinates(1297-1322)src/odemis/util/__init__.py (2)
normalize_rect(387-403)rotate_rect(430-466)
src/odemis/gui/test/comp_overlay_test.py (3)
src/odemis/util/comp.py (1)
get_fov_rect(98-113)src/odemis/util/testing.py (1)
assert_tuple_almost_equal(221-236)src/odemis/util/__init__.py (1)
rotate_rect(430-466)
src/odemis/util/test/util_test.py (2)
src/odemis/util/__init__.py (5)
to_str_escape(178-194)BackgroundWorker(824-892)rotate_rect(430-466)separate_rect_rotation(469-514)rot_almost_equal(128-144)src/odemis/util/testing.py (1)
assert_tuple_almost_equal(221-236)
🪛 Ruff (0.14.8)
src/odemis/gui/comp/overlay/base.py
1568-1568: Avoid specifying long messages outside the exception class
(TRY003)
src/odemis/gui/test/comp_overlay_test.py
638-638: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
721-721: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
734-734: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
747-747: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
src/odemis/util/test/util_test.py
589-589: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
661-661: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
678-678: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
698-698: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
703-703: Unpacked variable res_angle is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (python)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (28)
plugins/secom_cl.py (1)
888-896: Explicitly disabling ROA rotation for SECOM CL is appropriateSetting
dlg._dmodel.roa_rotation = Noneclearly signals that this dialog does not support rotation and keepsDblMicroscopeCanvas.setViewhappy when it passestab_data.roa_rotationintoRepetitionSelectOverlay. No further changes needed here.src/odemis/gui/comp/overlay/pixel_select.py (1)
25-35: Polygon-based pixel rendering integrates cleanly with PixelDataMixinUsing
cairo_polygon(ctx, self.pixel_to_rect(...))for both selected and hover pixels, with distinctselect_colorvscolour, keeps the drawing logic simple and correctly leverages the rotation-awarePixelDataMixin.pixel_to_rect. Skipping hover fill whileself.draggingavoids visual flicker during selection updates. This refactor looks solid.Also applies to: 48-50, 162-180
src/odemis/gui/model/tab_gui_data.py (1)
568-603: Conditionalroa_rotationVA design matches hardware capabilityExposing
roa_rotationas aFloatContinuousonly whenmain.ebeamsupportsscanPath, and otherwise setting it toNone, is a reasonable way to gate rotation support on hardware capability. As long as callers treatNoneas “no rotation possible” (as inmiccanvasand the SECOM CL plugin), this is a good fit.Please double‑check that all other consumers of
SparcAcquisitionGUIDatanow null‑checkroa_rotationbefore subscribing or writing to it.src/odemis/gui/comp/overlay/base.py (1)
61-75: Geometry / rotation infrastructure looks good; mind API consumersThe new
cairo_polygonhelper, extendedVecarithmetic, and rotation-awareRectangleEditingMixin(withcan_rotateand rotation knob handling) form a coherent base for rotated ROIs. Internals (_calc_center, _calc_rotation, rotation knob placement) are consistent, and the selection mode handling (CREATE/EDIT/DRAG/ROTATION) is clear.However,
RectangleEditingMixin’s public-ish API changed (start_selection/update_selection→start_creation/update_creation, newcan_rotateparameter). Any subclasses or external code that previously called the old methods or relied on always-rotatable rectangles must be updated accordingly.Please grep for
RectangleEditingMixinsubclasses and ensure:
- They no longer call
start_selection/update_selectiondirectly.- They pass an appropriate
can_rotateflag (e.g.can_rotate=rotation is not None) when calling__init__.Also applies to: 295-343, 1125-1200, 1398-1460, 1554-1569
src/odemis/gui/comp/overlay/rectangle.py (1)
341-369: Use ofstart_creation()aligns correctly with new mixin APISwitching from
start_selection()tostart_creation()matches the updatedRectangleEditingMixininterface and keeps the “only create when no selection exists” behavior intact. No functional issues spotted.src/odemis/gui/cont/tabs/sparc_acquisition_tab.py (1)
381-394: Spot initialization from ROA center looks correctUsing
tab_data_model.roa.value(withUNDEFINED_ROIfallback) to place the initial spot at the ROA center, or FoV center when undefined, is consistent with the ROA-driven workflow and keeps coordinates normalized. No issues found.src/odemis/gui/comp/overlay/spectrum_line_select.py (2)
25-33: Imports forwxandcairo_polygonare appropriateBringing in
wx(for cursor control) andcairo_polygontogether withPixelDataMixin/Vecmatches their usage later in the file and avoids NameErrors.
279-295: Rotation-aware pixel rendering properly implemented with correct initializationThe change to use
pixel_to_rect(point, scale)+cairo_polygon(ctx, corners)correctly renders rotation-aware per-pixel footprints in buffer space. The code properly depends onset_stream()initialization: analysis_tab.py callsol.set_stream(spec_stream)before draw operations, and test code follows the same pattern, ensuring_data_resolution,_phys_pxs, and rotation attributes are populated.src/odemis/gui/cont/tabs/analysis_tab.py (1)
436-454: Stream binding for pixel/line overlays is wired correctlyCalling
ol.set_stream(spec_stream)beforeconnect_selection(...)for bothpixel_overlayandline_overlayis the right way to feed resolution/physical/rotation metadata into the overlays. The existing “first spectrum stream only” limitation is unchanged and documented in the comment.src/odemis/gui/cont/stream_bar.py (1)
1038-1049: ROA rotation propagation to ROI streams is logically sound
_on_roa_rotationcorrectly broadcasts the new angle to all streams that participate in ROI synchronisation (those tracked in_roi_listeners) and doesn’t attempt to write back to the global ROA, avoiding feedback loops. This assumes all such streams expose a writable.rotationVA, which matches the new rotation-capable settings streams.src/odemis/util/test/util_test.py (2)
593-627: LGTM!The
RotateRectTestCasetest class is well-structured with good coverage of rotation scenarios: zero angle, 90° rotation about center, custom center rotation, and negative angles. The helper method_rotate_pointcorrectly implements the rotation formula for verification.
630-679: LGTM!The
SeparateRectRotationTestCaseprovides comprehensive round-trip testing betweenrotate_rectandseparate_rect_rotation. The tests verify that the functions are inverses of each other, which is the correct approach for validating coordinate transformation utilities.src/odemis/gui/cont/acquisition/sparc_acq.py (3)
39-39: LGTM!The
utilmodule import is correctly added to support therotate_rectfunction usage for rotation bounds checking.
141-161: LGTM!The ROA source is correctly updated to use
tab_data.roadirectly, and the conditional subscription toroa_rotationproperly triggers ROA updates when rotation changes. The comment clearly explains the purpose of the rotation subscription.
302-342: Verify rotation bounds check handles edge cases correctly.The rotation bounds check at lines 321-328 rotates the ROA and verifies all corners are within [0, 1]. However,
roa_rotationis checked for truthiness, which would beFalseif the rotation value is0. This seems intentional (skip check when no rotation), but verify this is the desired behavior.Also, the rotation is applied directly to the ROA coordinates. Ensure that the rotation center used by
rotate_rect(defaulting to rectangle center) aligns with how the acquisition system interprets the rotation.src/odemis/util/__init__.py (2)
430-466: LGTM!The
rotate_rectfunction correctly implements 2D rotation around an optional center point. The math is correct: translate to origin, apply rotation matrix, translate back. The docstring clearly documents the corner ordering (clockwise from minx, miny).
469-514: LGTM!The
separate_rect_rotationfunction correctly invertsrotate_rectby computing the rotation angle from corner vectors and then un-rotating to get the axis-aligned rectangle. The edge case handling for degenerate rectangles (corners 0 & 1 identical) is well-documented and falls back to using corners 0 & 3 with a 90° offset.src/odemis/gui/test/comp_overlay_test.py (4)
622-639: LGTM!The test correctly creates a rotation model and passes it to
RepetitionSelectOverlay. The ROI comparison is properly updated to use rotated corners withutil.rotate_rectandassert_tuple_almost_equalfor floating-point comparison.
686-748: LGTM!The
test_roa_select_overlay_vatest is well-updated to verify rotation functionality. It properly tests:
- Undefined ROA returns None
- Full FoV with no rotation
- Half FoV with no rotation
- Half FoV with 0.1 rad rotation
The corner comparisons correctly use
util.rotate_rectto compute expected rotated positions.
783-795: LGTM!The
_create_spectrum_streamhelper method is a good refactoring that reduces code duplication across spectrum-related overlay tests. The metadata setup (pixel size, position, rotation, wavelength list) provides a realistic test fixture.
816-816: LGTM!The overlay tests are correctly updated to use
set_stream()instead of the previous property-based approach, aligning with the new streaming data API introduced in this PR.src/odemis/gui/comp/overlay/repetition_select.py (7)
42-66: LGTM!The class inheritance change from
WorldSelectOverlaytoWorldOverlayandRectangleEditingMixinis a clean refactoring. Thecan_rotateparameter is correctly derived from whether a rotation VA is provided, and both parent classes are properly initialized.
79-107: LGTM!The physical corner tracking fields (p_point1-4) and side length labels are well-initialized. The label configuration with white text on black background provides good visibility for the dimension display.
142-165: Verify coordinate conversion consistency.The
_view_to_physand_phys_to_viewmethods convert between view and physical coordinates. Ensure that these are called symmetrically after any coordinate modifications to keepv_point*andp_point*in sync. Missing synchronization could lead to inconsistent state.
402-458: LGTM!The
_draw_pointsmethod correctly handles the rotated rectangle by computing point positions via interpolation between the corner vertices. The performance optimization to fall back to a solid fill for very dense grids (>50K points or <4px spacing) is a good approach.
460-500: LGTM!The
_draw_gridmethod correctly draws grid lines by interpolating between adjacent corners, which naturally handles rotation. The fallback to solid fill for dense grids (<3px spacing) maintains visual clarity.
557-596: LGTM!The
_draw_side_labelsmethod correctly computes side lengths using physical coordinates and positions labels with appropriate rotation to align with each side. The approach of finding corners by x+y sum/difference is a clever way to identify the bottom-left and bottom-right corners regardless of rotation.
597-631: LGTM!The
drawmethod correctly validates that all four points are defined before rendering, handles the degenerate case where the rectangle hasn't been fully defined yet, and appropriately skips repetition drawing during edit/create modes to improve interactivity.
pieleric
requested review from
nandishjpatel
and removed request for
tmoerkerken
pieleric
force-pushed
the
feat-sparc-gui-support-rotated-roa
branch
from
e3d37be to
75c27fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/odemis/acq/stream/_base.py:
- Around line 1264-1266: The docstring mistakenly mentions parameter name
check_boundary; update the docstring to refer to the actual parameter check_bbox
(the parameter defined as check_bbox) and adjust the sentence to state that the
function returns None when the point is outside the image and check_bbox is
True; ensure any other occurrences in the same docstring use check_bbox
consistently so the documentation matches the function signature (refer to the
parameter check_bbox in the current function's docstring).
🧹 Nitpick comments (1)
src/odemis/gui/test/comp_overlay_test.py (1)
622-639: Consider addingstrict=Trueto zip() for corner comparison.The comparison loops at lines 638-639 (and similar ones at 721-722, 734-735, 747-748) iterate over two lists of corners that should always have exactly 4 elements. Adding
strict=Truewould catch any unexpected mismatch during testing.🔧 Suggested fix
- for o, b in zip(corners, corners_back): - testing.assert_tuple_almost_equal(o, b) + for o, b in zip(corners, corners_back, strict=True): + testing.assert_tuple_almost_equal(o, b)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
plugins/secom_cl.pysrc/odemis/acq/stream/_base.pysrc/odemis/gui/comp/miccanvas.pysrc/odemis/gui/comp/overlay/base.pysrc/odemis/gui/comp/overlay/pixel_select.pysrc/odemis/gui/comp/overlay/rectangle.pysrc/odemis/gui/comp/overlay/repetition_select.pysrc/odemis/gui/comp/overlay/spectrum_line_select.pysrc/odemis/gui/comp/overlay/world_select.pysrc/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/test/comp_overlay_test.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/odemis/gui/cont/tabs/sparc_acquisition_tab.py
- src/odemis/gui/comp/overlay/spectrum_line_select.py
- src/odemis/gui/cont/stream_bar.py
- plugins/secom_cl.py
- src/odemis/gui/cont/tabs/analysis_tab.py
- src/odemis/gui/comp/overlay/pixel_select.py
- src/odemis/gui/comp/overlay/world_select.py
- src/odemis/gui/cont/acquisition/sparc_acq.py
- src/odemis/gui/comp/miccanvas.py
- src/odemis/gui/comp/overlay/rectangle.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/gui/comp/overlay/repetition_select.pysrc/odemis/gui/test/comp_overlay_test.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/acq/stream/_base.pysrc/odemis/gui/comp/overlay/base.py
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/comp/overlay/base.py
🧬 Code graph analysis (2)
src/odemis/gui/model/tab_gui_data.py (2)
src/odemis/model/_components.py (2)
model(570-571)hasVA(63-69)src/odemis/model/_vattributes.py (1)
FloatContinuous(1072-1082)
src/odemis/gui/comp/overlay/base.py (3)
src/odemis/acq/stream/_base.py (4)
Stream(91-1370)_find_metadata(988-1028)getPixelCoordinates(1259-1295)getPhysicalCoordinates(1297-1322)src/odemis/gui/comp/overlay/shapes.py (1)
move_to(112-114)src/odemis/util/__init__.py (2)
normalize_rect(387-403)rotate_rect(430-466)
🪛 Ruff (0.14.10)
src/odemis/gui/test/comp_overlay_test.py
638-638: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
721-721: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
734-734: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
747-747: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
src/odemis/gui/comp/overlay/base.py
1577-1577: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (22)
src/odemis/acq/stream/_base.py (1)
1300-1301: LGTM!The updated docstring for
pixel_posaccurately describes the coordinate system and aligns with the return value documentation ofgetPixelCoordinates.src/odemis/gui/model/tab_gui_data.py (1)
598-602: LGTM! Clean conditional initialization of rotation capability.The pattern of checking for
scanPathVA to determine rotation support is appropriate, and settingroa_rotationtoNonewhen not supported provides a clear signal to consumers. The range(0, 2*pi)is correct for rotation angles.src/odemis/gui/test/comp_overlay_test.py (3)
737-749: Good rotation test coverage.The test properly validates that rotation changes propagate through the overlay, computing expected corners with
util.rotate_rectand comparing against the actual selection. This ensures the rotation integration works end-to-end.
783-795: Well-structured test helper.The
_create_spectrum_streamhelper provides a clean, reusable way to create test streams with proper metadata. The rotation metadata at line 791 enables testing the rotation-aware pixel coordinate transformations.
816-816: API improvement:set_streamis cleaner than individual property setters.Using
set_stream()provides all required metadata (resolution, pixel size, rotation) from a single source of truth, reducing the risk of inconsistent state.src/odemis/gui/comp/overlay/repetition_select.py (7)
42-107: Good architecture: Mixin-based approach for rotation support.The refactor to inherit from
WorldOverlayandRectangleEditingMixinis a clean design that reuses the rotation-aware rectangle editing logic from FAST-EM. The dual tracking of view coordinates (v_point*) and physical coordinates (p_point*) is necessary for proper world-space representation.
142-164: Coordinate synchronization methods are correctly implemented.The bidirectional conversion between view and physical coordinates properly handles the optional point state and uses the canvas's coordinate transformation methods.
166-199: Selection accessors handle coordinate system differences correctly.The point reordering in
get_physical_sel()(returning[p_point4, p_point3, p_point2, p_point1]) compensates for Y-axis inversion between view and physical coordinates. The guard inset_physical_sel()preventing updates during active selection prevents race conditions.
280-298: ROA callback correctly handles rotation and coordinate systems.The rotation inversion (
2 * math.pi - self.rotation) properly accounts for the Y-axis direction difference between physical (Y-up) and view (Y-down) coordinates. The conditional handling of scanner-relative vs. absolute coordinates is well-structured.
339-374: ROA update logic is sound, with documented limitation for rotated clipping.The unsubscribe/subscribe pattern at lines 366-370 correctly prevents feedback loops when updating the ROA VA. The TODO comments (lines 343-356) acknowledge the complexity of clipping rotated rectangles to the scanner FOV - this is a reasonable scope limitation for this PR.
402-500: Well-optimized rotation-aware repetition rendering.The drawing methods handle rotated rectangles correctly by interpolating positions between corners rather than using axis-aligned calculations. The fallback to solid fill for dense grids (>50K points or <3px spacing) is a good performance optimization that prevents UI lag.
557-595: Side labels provide useful dimension feedback.The label positioning logic correctly identifies the "bottom" and "right" sides of a rotated rectangle using coordinate sum/difference comparisons. Computing lengths from physical coordinates ensures accurate measurements regardless of view scale.
src/odemis/gui/comp/overlay/base.py (10)
61-73: Useful utility for drawing polygons.The
cairo_polygonhelper cleanly encapsulates the common pattern of drawing a closed polygon path. The early return for empty points prevents errors.
312-319: Vec division operator enables cleaner center calculations.The
__truediv__method allows expressions like(self.v_point1 + self.v_point3) / 2for computing rectangle centers, improving code readability.
1156-1199: Good design:can_rotateflag separates user interaction from programmatic control.The
can_rotateparameter allows disabling user rotation while still permitting programmatic rotation. This is useful for cases where rotation should be controlled externally (e.g., linked to a VA).
1207-1214: Method rename improves clarity.Renaming
start_selectiontostart_creationbetter distinguishes rectangle creation from the "SEL_MODE_*" states used during editing. Resetting rotation to 0 for new rectangles ensures consistent initial state.
1534-1561: Rectangle normalization correctly handles rotated rectangles.The approach of unrotating, normalizing, and re-rotating ensures points are always in consistent clockwise order regardless of how the user dragged corners. This is essential for correct hover detection and edge editing.
1324-1382: Rotation editing implementation is robust.The rotation methods correctly:
- Compute rotation by comparing angles relative to center
- Rotate all 4 points together about the center
- Support Ctrl-snapping to 45° increments for precise alignment
- Handle edge cases (no points defined)
The Ctrl-snapping feature (lines 1350-1361) is a nice UX improvement for aligning rectangles.
1449-1532: Edge calculation correctly gates rotation hover oncan_rotate.The conditional addition of
HOVER_ROTATIONedges (lines 1459-1468) ensures the rotation knob only appears when rotation is enabled. The midpoint and vertex bounding boxes provide proper hit-testing areas for edge and corner editing.
2048-2069:pixel_to_rectcorrectly produces rotation-aware pixel boundaries.The method computes pixel corners by building an axis-aligned rectangle and then rotating it. The negated rotation (
-self._rotation) correctly compensates for the Y-axis direction difference in physical coordinates.
1576-1578: Minor: Inline exception message is acceptable here.The static analysis hint about
TRY003(exception messages) is a false positive for this case - the message is short and provides necessary context for debugging. No change needed.
2004-2012:set_streamprovides a cleaner API than individual property setters and follows the same pattern as Stream's public methods.Extracting metadata from the Stream object ensures consistency. The
_find_metadatacall mirrors the approach used in Stream's publicgetPixelCoordinates()method, making this a standard internal pattern rather than problematic coupling.
pieleric
force-pushed
the
feat-sparc-gui-support-rotated-roa
branch
from
75c27fa to
c238c6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/stream_bar.py (1)
953-968: Add rotation VA guard and prevent duplicate subscriptions to roa_rotation.The code subscribes to
roa_rotationinside_connectROI(), which gets called once per ROI-capable stream. This causes the same callback_on_roa_rotation()to be registered multiple times, multiplying its execution on each rotation change.Additionally, line 962 and line 1048 directly access
stream.rotation.valuewithout checking if the stream has arotationVA. The codebase shows the correct pattern elsewhere (e.g.,src/odemis/acq/stream/_sync.py:127-128usesif model.hasVA(s, "rotation")). Finally, the_on_roa_rotation()method lacks a return type hint (-> None) per the Python 3.10+ guideline.Proposed fix (subscribe once; guard rotation VA; add return type)
def _connectROI(self, stream): """ Connect the .roi of the (repetition) stream to the global ROA """ # First, start with the same ROI as the global ROA stream.roi.value = self._tab_data_model.roa.value self._tab_data_model.roa.subscribe(self._onROA) rotation_va = getattr(self._tab_data_model, "roa_rotation", None) if rotation_va is not None and model.hasVA(stream, "rotation"): try: stream.rotation.value = rotation_va.value except Exception: logging.exception("Failed to initialize rotation for %s", stream.name.value) # Subscribe only once per controller, not once per stream if not getattr(self, "_roa_rotation_subscribed", False): rotation_va.subscribe(self._on_roa_rotation) self._roa_rotation_subscribed = True listener = functools.partial(self._onStreamROI, stream) stream.roi.subscribe(listener) self._roi_listeners[stream] = listener -def _on_roa_rotation(self, angle: float): +def _on_roa_rotation(self, angle: float) -> None: """ Called when the ROA rotation is changed. Copies the angle to all the (settings) streams with ROI. :param angle: rotation angle in radians (0 -> 2pi) """ - for s in self._tab_data_model.streams.value: - if s not in self._roi_listeners: - continue - # The streams always accept the rotation as-is, so no need to read it back - s.rotation.value = angle + for s in self._roi_listeners.keys(): + if not model.hasVA(s, "rotation"): + continue + try: + # The streams always accept the rotation as-is, so no need to read it back + s.rotation.value = angle + except Exception: + logging.exception("Failed to apply rotation for %s", s.name.value)Also applies to: lines 1038–1049
🤖 Fix all issues with AI agents
In `@src/odemis/gui/comp/overlay/base.py`:
- Around line 2061-2062: The code sets b_pixel_size using the X component twice;
update the assignment for b_pixel_size to use both components of self._phys_pxs
so width uses self._phys_pxs[0] and height uses self._phys_pxs[1] (i.e., replace
the second self._phys_pxs[0] with self._phys_pxs[1]) so non-square pixel sizes
are handled correctly where b_pixel_size is computed.
♻️ Duplicate comments (1)
src/odemis/gui/comp/miccanvas.py (1)
246-251: Guardtab_data.roa_rotationto avoid AttributeError across tabs.
This is the same crash risk noted previously: sometab_datamodels may haveTOOL_ROAbut noroa_rotationattribute, and this direct access will blow up at runtime.Proposed fix
if guimodel.TOOL_ROA in tools_possible: # Get the region of interest and link it to the ROA overlay self._roa = tab_data.roa - self.roa_overlay = RepetitionSelectOverlay(self, self._roa, tab_data.fovComp, - tab_data.roa_rotation) + rotation = getattr(tab_data, "roa_rotation", None) + self.roa_overlay = RepetitionSelectOverlay(self, self._roa, tab_data.fovComp, rotation) self.add_world_overlay(self.roa_overlay)
🧹 Nitpick comments (5)
src/odemis/gui/test/comp_overlay_test.py (1)
638-639: Consider addingstrict=Trueto zip() calls for defensive testing.Static analysis flags these
zip()calls without explicitstrict=parameter. While the lengths are inherently matched (both are 4 corners from rectangles), addingstrict=Truewould provide better error detection if the underlying functions ever change:-for o, b in zip(corners, corners_back): +for o, b in zip(corners, corners_back, strict=True):This is optional for test code since the lengths are guaranteed to match.
Also applies to: 721-722, 734-735, 747-748
src/odemis/gui/comp/overlay/repetition_select.py (1)
390-391: Consider using a more intuitive cursor for rotation.
wx.CURSOR_MAGNIFIERis typically associated with zooming. A cursor likewx.CURSOR_HANDmight be more intuitive for rotation, though this is a minor UX detail.src/odemis/gui/comp/overlay/base.py (3)
1533-1547: Minor docstring inconsistency.The docstring mentions
:param point1:,:param point3:, and:param rotation:(lines 1538-1540) but these aren't method parameters - they're internal instance attributes. Consider simplifying:📝 Suggested docstring fix
def _normalize_rectangle(self) -> None: """ Given 4 points of a rectangle, return them in the order point1, point2, point3, point4 where point1 is the top-left point (min, min) and the other points are in clockwise order. - - :param point1: Vec - :param point3: Vec - :param rotation: The rotation of the rectangle in radians - :return: point1, point2, point3, point4 in clockwise order + + Updates v_point1-4 to be in clockwise order. 1 -------------- 2 | | | |
1792-1800: Optional: Duplicated snap-to-45° logic.The Ctrl-snap logic here is identical to
RectangleEditingMixin.update_rotation(lines 1350-1361). Consider extracting to a shared helper function if this pattern is used elsewhere.
2003-2010: Consider making_find_metadatapublic or providing a public wrapper.Line 2007 accesses
stream._find_metadata(), a private method. While this is the current API for retrieving merged metadata from a stream, there is no public alternative available. This pattern is used elsewhere in the codebase (e.g., plugins, other GUI components), suggesting the method should be made public (renamed tofind_metadata) or wrapped in a public accessor if privacy is intentional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
plugins/secom_cl.pysrc/odemis/acq/stream/_base.pysrc/odemis/gui/comp/miccanvas.pysrc/odemis/gui/comp/overlay/base.pysrc/odemis/gui/comp/overlay/pixel_select.pysrc/odemis/gui/comp/overlay/rectangle.pysrc/odemis/gui/comp/overlay/repetition_select.pysrc/odemis/gui/comp/overlay/spectrum_line_select.pysrc/odemis/gui/comp/overlay/world_select.pysrc/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/test/comp_overlay_test.py
✅ Files skipped from review due to trivial changes (1)
- src/odemis/gui/comp/overlay/world_select.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/acq/stream/_base.py
- src/odemis/gui/model/tab_gui_data.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for function parameters and return types in Python code
Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)
Ensure code is valid for Python 3.10 and above
Clean up code at the end of a task using autopep8 with the command:autopep8 --in-place --select W291,W292,W293,W391
Files:
src/odemis/gui/comp/overlay/repetition_select.pysrc/odemis/gui/cont/stream_bar.pyplugins/secom_cl.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/comp/miccanvas.pysrc/odemis/gui/comp/overlay/rectangle.pysrc/odemis/gui/test/comp_overlay_test.pysrc/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/comp/overlay/pixel_select.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/comp/overlay/spectrum_line_select.pysrc/odemis/gui/comp/overlay/base.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/gui/comp/overlay/repetition_select.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/analysis_tab.pysrc/odemis/gui/comp/miccanvas.pysrc/odemis/gui/comp/overlay/rectangle.pysrc/odemis/gui/test/comp_overlay_test.pysrc/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/comp/overlay/pixel_select.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/comp/overlay/spectrum_line_select.pysrc/odemis/gui/comp/overlay/base.py
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/comp/overlay/base.py
🧬 Code graph analysis (8)
src/odemis/gui/comp/overlay/repetition_select.py (1)
src/odemis/util/__init__.py (2)
rotate_rect(430-466)separate_rect_rotation(469-514)
src/odemis/gui/cont/stream_bar.py (1)
src/odemis/acq/stream/_sync.py (2)
streams(227-228)streams(2589-2590)
src/odemis/gui/comp/miccanvas.py (1)
src/odemis/gui/comp/overlay/repetition_select.py (1)
RepetitionSelectOverlay(42-636)
src/odemis/gui/test/comp_overlay_test.py (3)
src/odemis/util/comp.py (2)
compute_scanner_fov(33-57)get_fov_rect(98-113)src/odemis/util/testing.py (1)
assert_array_not_equal(204-218)src/odemis/util/__init__.py (1)
rotate_rect(430-466)
src/odemis/gui/cont/acquisition/sparc_acq.py (2)
src/odemis/gui/cont/stream_bar.py (1)
_onROA(1014-1036)src/odemis/util/__init__.py (1)
rotate_rect(430-466)
src/odemis/gui/comp/overlay/pixel_select.py (1)
src/odemis/gui/comp/overlay/base.py (4)
PixelDataMixin(1983-2068)cairo_polygon(61-73)pixel_to_rect(2047-2068)is_over_pixel_data(2019-2028)
src/odemis/gui/comp/overlay/spectrum_line_select.py (1)
src/odemis/gui/comp/overlay/base.py (3)
Vec(295-342)cairo_polygon(61-73)pixel_to_rect(2047-2068)
src/odemis/gui/comp/overlay/base.py (3)
src/odemis/acq/stream/_base.py (2)
getPixelCoordinates(1259-1295)getPhysicalCoordinates(1297-1322)src/odemis/gui/comp/overlay/rectangle.py (1)
move_to(222-232)src/odemis/util/__init__.py (2)
normalize_rect(387-403)rotate_rect(430-466)
🪛 Ruff (0.14.11)
src/odemis/gui/test/comp_overlay_test.py
638-638: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
721-721: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
734-734: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
747-747: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
src/odemis/gui/comp/overlay/base.py
1576-1576: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (30)
plugins/secom_cl.py (1)
894-897: Good explicit “no rotation supported” signal for this dialog model.
Settingdlg._dmodel.roa_rotation = Nonecleanly opts this plugin out of the new rotation feature and should keep overlay behavior consistent.src/odemis/gui/cont/tabs/sparc_acquisition_tab.py (1)
381-393: Nice alignment: spot initialization now uses the tab’s global ROA.
Usingself.tab_data_model.roa.valuekeeps spot placement consistent with the GUI’s ROA source (including cases where ROA is deliberately decoupled from a specific stream).src/odemis/gui/comp/overlay/spectrum_line_select.py (1)
25-35: LGTM: rotation-aware pixel rendering viapixel_to_rect()+cairo_polygon().
This is a clean swap from axis-aligned rectangles to a shared polygon trace that can represent rotated pixels.Also applies to: 287-295
src/odemis/gui/cont/tabs/analysis_tab.py (1)
442-453: LGTM! Clean migration to stream-based data binding.The switch from explicit data property binding to
set_stream()is consistent with thePixelDataMixinchanges that now extract resolution, pixel size, and rotation from the stream's metadata. This simplifies the overlay binding and naturally supports rotation throughMD_ROTATIONin the stream metadata.src/odemis/gui/comp/overlay/pixel_select.py (1)
160-178: LGTM! Clean refactor to polygon-based rendering with rotation support.The draw method now correctly uses
pixel_to_rect()(which returns rotation-aware corners fromPixelDataMixin) andcairo_polygon()for tracing. The pattern of tracing all polygons before a singlefill()call is efficient for batch rendering.src/odemis/gui/cont/acquisition/sparc_acq.py (3)
158-161: LGTM! Proper subscription to rotation changes.The conditional subscription to
roa_rotationcorrectly triggers RoA validity checks when the rotation changes. The comment clearly explains why rotation affects acquisition enablement (RoA may go out of FoV) but not acquisition time.
321-328: Verify intent: rotation value of 0.0 is falsy.The condition
elif self._tab_data_model.roa_rotation:checks both that the VA exists and that its value is truthy. Since0.0is falsy in Python, this branch is skipped when rotation is exactly zero. This is likely intentional (no rotation means no bounds expansion), but consider making it explicit for clarity:elif self._tab_data_model.roa_rotation is not None and self._tab_data_model.roa_rotation.value != 0:Alternatively, if the check should always run when
roa_rotationis present (even at 0), access.valueexplicitly:elif self._tab_data_model.roa_rotation is not None:
302-342: LGTM! Clean refactoring with centralized acquisition enablement.The
can_acquireflag effectively consolidates the acquisition button enablement logic. The if-elif chain correctly prioritizes error conditions, and the estimated time computation only happens when no blocking conditions exist.src/odemis/gui/test/comp_overlay_test.py (3)
783-795: LGTM! Well-structured test helper for spectrum streams.The
_create_spectrum_stream()helper creates a properly configuredStaticSpectrumStreamwith relevant metadata includingMD_ROTATION,MD_PIXEL_SIZE, andMD_POS. This enables proper testing of the rotation-aware overlay behavior.
737-749: LGTM! Good test coverage for rotation behavior.This test case properly verifies that the
RepetitionSelectOverlaycorrectly applies rotation when computing the physical selection. The test sets a rotation value, computes the expected rotated corners, and validates the overlay returns matching coordinates.
816-816: LGTM! Tests correctly use the newset_stream()API.The test properly uses
set_stream()with the spectrum stream helper, validating the new streaming-based data binding API for overlays.src/odemis/gui/comp/overlay/rectangle.py (5)
89-91: LGTM! Simplified constructor signature.Removing the
centerparameter from the constructor simplifies the API. The mixin initialization now takes only thecolourparameter, which is cleaner.
108-142: LGTM! Improved label visibility.Setting
flip=Falseensures labels maintain consistent orientation, and the black background(0, 0, 0)improves readability against varied canvas backgrounds. These are good UI improvements for rectangle overlays.
422-466: Improved side label positioning with rotation awareness.The rewritten
draw_side_labelsmethod usesmath.distfor proper distance calculation and perpendicular label shifts for better positioning. The corner detection algorithm is robust for rotated rectangles.One consideration: for significant rotations, the "bottom" and "right" edges are determined by physical coordinate ordering, which may differ from visual orientation. This is acceptable since the labels still correctly display the side dimensions.
468-471: LGTM! Cleaner rotation label formatting.Using
round()produces integer-like display values (e.g., "45°" instead of "45.0°"), improving label readability.
354-358: Thestart_creation()method is properly defined and correctly available. It exists inRectangleEditingMixin(base.py, line 1207) and is semantically appropriate for initializing a new rectangle with point-based coordinates and rotation settings. The method call at line 358 is valid.src/odemis/gui/comp/overlay/repetition_select.py (6)
25-42: LGTM!The imports and class inheritance changes are well-structured. The switch from
WorldSelectOverlaytoWorldOverlay + RectangleEditingMixinis a clean approach for adding rotation support while reusing the existing mixin's rectangle editing functionality.
53-108: LGTM!The constructor correctly initializes both base classes (
WorldOverlayandRectangleEditingMixin) with proper parameter passing. The physical coordinate points (p_point1-p_point4) are well-placed for maintaining synchronization between view and physical coordinate systems. The side labels initialization for displaying rectangle dimensions is clean.
167-200: LGTM!The physical selection getter/setter methods handle the Y-axis inversion between view and physical coordinates correctly. The corner reordering in
get_physical_sel()(lines 171-174) is necessary because physical Y increases upward while view Y increases downward. Theset_physical_sel()properly normalizes the rectangle and recalculates edges after setting.
229-238: Acknowledged: TODO for rotation-aware ROI clipping.The comment correctly documents the limitation where rotated ROAs near 90°/270° might not be properly clipped to the scanner FoV. The workaround via the GUI controller's bounds check (mentioned in line 236-238) is reasonable for now.
563-576: Good defensive check for degenerate rectangles.The check at line 565-566 for
len(all_points) < 4handles the edge case where physical points might overlap. However, note that thesetcomparison relies onVecbeing hashable and having proper__eq__. SinceVecextendstuple, this should work correctly.
602-636: LGTM!The
draw()method is well-structured with appropriate early returns for undefined/degenerate rectangles. The performance optimization to skip repetition drawing during edit/create modes (line 625) is a good UX improvement. The conditional rendering of edit knobs and labels based onactive.valueis correct.src/odemis/gui/comp/overlay/base.py (8)
61-74: LGTM!Clean utility function with proper type hints and docstring. The early return for empty points list is a good defensive check.
318-319: LGTM!The
__truediv__method completes the arithmetic operators forVec. Division by zero will raise a standardZeroDivisionError, which is appropriate behavior.
1156-1162: LGTM!The docstring clearly explains the
can_rotateparameter semantics - users can't rotate whenFalse, but programmatic rotation is still allowed. This is a good design decision for flexibility.
1207-1254: LGTM!The selection lifecycle methods are well-implemented. Setting
rotation = 0instart_creation()ensures new rectangles start axis-aligned. The_normalize_rectangle()call instop_selection()maintains consistent point ordering after editing.
1350-1361: Nice UX feature: Ctrl-snap to 45° increments.The snap-to-45-degrees functionality when Ctrl is held provides precise rotation control. The implementation correctly recalculates all points after snapping.
1562-1580: LGTM!The cursor direction calculation correctly determines whether the edge is more horizontal (NS cursor) or vertical (EW cursor) based on the delta comparison. The ValueError for invalid indices is appropriate defensive coding.
1613-1670: LGTM!The event handlers comprehensively cover all interaction modes (create, drag, rotate, edit). The warning for unhandled hover types (line 1635) aids debugging. The comment explaining the
left_draggingcheck for double-click edge cases is helpful.
1458-1467: LGTM!The conditional addition of
HOVER_ROTATIONedges based oncan_rotatecorrectly prevents rotation interaction when disabled while still allowing the rotation point to be calculated for other purposes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pieleric
force-pushed
the
feat-sparc-gui-support-rotated-roa
branch
2 times, most recently
from
4372fdd to
02ac003
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/odemis/gui/comp/overlay/base.py`:
- Around line 1338-1344: The rotation delta calculation can jump by ±2π when
angles cross the 0/2π boundary; in the block that computes prev_rot =
self._get_point_angle(self.v_point1), knob_pos =
Vec(self.cnvs.clip_to_viewport(self.drag_v_end_pos)), new_rot =
self._get_point_angle(knob_pos) and diff_angle = new_rot - prev_rot before
calling self.v_point1.rotate(diff_angle, self.v_center), normalize diff_angle
into the principal range (e.g. (-π, π]) to remove 2π jumps (use math.pi and
modulo/adjustment) and then pass the normalized diff_angle to rotate so
rotations are continuous across the angle wrap boundary.
- Around line 2000-2007: Add a docstring to set_stream describing that it sets
self._stream and extracts data properties and that the Stream must contain raw
data (accessing stream.raw[0] will fail if empty); then validate the incoming
Stream by checking that stream.raw is non-empty (e.g., if not stream.raw: raise
ValueError("Stream.raw must contain at least one raw element")) before accessing
stream.raw[0], and only then assign self._stream and compute
self._data_resolution, self._phys_pxs and self._rotation; reference symbols:
set_stream, Stream, self._stream, self._data_resolution, self._phys_pxs,
self._rotation and downstream callers like is_over_pixel_data /
view_to_data_pixel so reviewers know this precondition is required.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 1038-1049: The handler _on_roa_rotation is missing a return type
annotation; update its signature (def _on_roa_rotation(self, angle: float)) to
include the explicit return type -> None so it follows project standards,
leaving the body and docstring unchanged and ensuring any callers remain
compatible.
♻️ Duplicate comments (2)
src/odemis/util/__init__.py (1)
48-49: DuplicatedUNDEFINED_ROIconstant.This constant is also defined in
odemis.acq.stream._base.py. Consider importing from the canonical source to avoid maintenance drift. The comment acknowledges this is fromodemis.acq.stream, but a direct import would be cleaner.src/odemis/gui/comp/overlay/base.py (1)
2056-2062: Use per-axis pixel size to support non-square pixels.
Line 2058 uses the X pixel size for both width and height.🐛 Proposed fix
- b_pixel_size = (self._phys_pxs[0] * scale, self._phys_pxs[0] * scale) + b_pixel_size = (self._phys_pxs[0] * scale, self._phys_pxs[1] * scale)
🧹 Nitpick comments (6)
src/odemis/util/test/util_test.py (3)
576-591: Helper method_rotate_pointis missing a return type hint.As per coding guidelines, all functions should have type hints for parameters and return types. The
_assert_corners_equalmethod also usesassertinstead ofself.assert*methods, which won't report proper test failures if assertions are disabled.🔧 Suggested improvements
- def _rotate_point(self, pt: Tuple, center: Tuple, angle: float): + def _rotate_point(self, pt: Tuple[float, float], center: Tuple[float, float], angle: float) -> Tuple[float, float]: x, y = pt cx, cy = center x -= cx y -= cy cos_a = math.cos(angle) sin_a = math.sin(angle) x_new = x * cos_a - y * sin_a y_new = x * sin_a + y * cos_a return (x_new + cx, y_new + cy) - def _assert_corners_equal(self, expected: List[Tuple], actual:List[Tuple]): - assert len(expected) == len(actual) - for e, a in zip(expected, actual): + def _assert_corners_equal(self, expected: List[Tuple[float, float]], actual: List[Tuple[float, float]]) -> None: + self.assertEqual(len(expected), len(actual)) + for e, a in zip(expected, actual, strict=True): self.assertAlmostEqual(e[0], a[0]) self.assertAlmostEqual(e[1], a[1])
661-662: Addstrict=Truetozip()for safer iteration.When iterating over two lists that should have equal lengths, using
strict=Trueensures aValueErroris raised if the lengths differ, catching bugs early.🔧 Suggested fix
- for v_expected, v_actual in zip(corners, recon): + for v_expected, v_actual in zip(corners, recon, strict=True): testing.assert_tuple_almost_equal(v_expected, v_actual)The same applies to lines 678 and 698.
701-704: Unused variableres_angleshould be prefixed with underscore.The static analysis correctly identifies that
res_angleis never used. Prefix it with an underscore to indicate it's intentionally ignored.🔧 Suggested fix
def test_undefined_roi(self): corners = [(2, 3)] * 4 - res_rect, res_angle = util.separate_rect_rotation(corners) + res_rect, _res_angle = util.separate_rect_rotation(corners) self.assertEqual(res_rect, UNDEFINED_ROI)src/odemis/gui/test/comp_overlay_test.py (1)
638-639: Consider addingstrict=Truetozip()for test robustness.Adding
strict=Truewould catch any unexpected length mismatches betweencornersandcorners_back, making tests fail faster with clearer error messages.🔧 Suggested fix
- for o, b in zip(corners, corners_back): + for o, b in zip(corners, corners_back, strict=True): testing.assert_tuple_almost_equal(o, b)The same applies to similar
zip()calls at lines 721, 734, and 747.src/odemis/gui/comp/overlay/repetition_select.py (1)
501-601: Code duplication withRectangleOverlaynoted for future refactoring.The
_draw_edit_knobs(line 502) and_draw_side_labels(line 556) methods contain similar logic toRectangleOverlay.draw_edgesandRectangleOverlay.draw_side_labels. The TODO comments acknowledge this. Consider extracting shared drawing utilities in a future refactor.src/odemis/gui/comp/overlay/base.py (1)
312-319: Add explicit return type hints for Vec arithmetic overloads.
This keeps the typing contract consistent with the new parameter hints.♻️ Proposed update
- def __mul__(self, c: float): + def __mul__(self, c: float) -> "Vec": return Vec(x * c for x in self) - def __rmul__(self, c: float): + def __rmul__(self, c: float) -> "Vec": return Vec(c * x for x in self) - def __truediv__(self, c): + def __truediv__(self, c: float) -> "Vec": return Vec(x / c for x in self)As per coding guidelines, please add return type hints consistently.
pieleric
force-pushed
the
feat-sparc-gui-support-rotated-roa
branch
from
02ac003 to
67e13b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/odemis/gui/comp/overlay/base.py`:
- Around line 312-319: Update the numeric operator methods to include full type
annotations: annotate __truediv__'s parameter as float (def __truediv__(self, c:
float)) and add explicit return type hints -> Vec to __mul__, __rmul__, and
__truediv__. Keep the existing generator expressions but ensure the signatures
are: def __mul__(self, c: float) -> Vec, def __rmul__(self, c: float) -> Vec,
def __truediv__(self, c: float) -> Vec so callers and static type checkers can
infer Vec results.
In `@src/odemis/gui/comp/overlay/rectangle.py`:
- Around line 89-92: The RectangleOverlay.__init__ currently lacks type hints
and a docstring; update its signature to add proper type hints for parameters
(e.g., cnvs: CanvasType, colour: str = gui.SELECTION_COLOUR,
show_selection_points: bool = True) and add a concise reST-style __init__
docstring describing the parameters and purpose (one-line summary + :param: /
:type: entries) while preserving the existing calls to
EditableShape.__init__(self, cnvs) and RectangleEditingMixin.__init__(self,
colour); ensure the docstring sits immediately under the def and follows the
project’s reST formatting conventions.
In `@src/odemis/gui/comp/overlay/repetition_select.py`:
- Around line 53-69: Update the __init__ signature to use typed parameters and
an explicit return type (-> None); e.g. annotate cnvs, roa, scanner, rotation
and colour with the appropriate types used in the module (e.g. Canvas type for
cnvs, Optional[...] for roa/scanner/rotation, and the colour type matching
gui.SELECTION_COLOUR) and add "-> None" to the signature of def __init__(...),
and add any necessary typing imports (Optional, typing aliases) at the top of
the file so WorldOverlay.__init__ and RectangleEditingMixin.__init__ calls
remain unchanged.
In `@src/odemis/util/test/util_test.py`:
- Around line 701-703: In test_single_point replace the unused res_angle
assignment with a throwaway variable to satisfy linter: call
util.separate_rect_rotation(corners) and assign its second return to _ (e.g.,
res_rect, _ ) instead of res_angle; update the assignment in the
test_single_point function that invokes util.separate_rect_rotation so only
res_rect is used.
- Around line 587-592: The zip calls must be made strict to catch length
mismatches: update the loop in _assert_corners_equal (where it iterates for e, a
in zip(expected, actual)) to use zip(..., strict=True), and likewise update the
three other occurrences that iterate with for v_expected, v_actual in
zip(corners, recon) to use zip(corners, recon, strict=True); ensure all four zip
calls include strict=True so mismatched lengths raise immediately.
♻️ Duplicate comments (6)
src/odemis/util/__init__.py (1)
48-48: Avoid duplicatingUNDEFINED_ROIconstant.Defining it here risks drift from the canonical value in
odemis.acq.stream. Prefer importing the shared constant instead.♻️ Proposed change
-from . import weak - -UNDEFINED_ROI = (0,0,0,0) # From odemis.acq.stream +from . import weak +from odemis.acq.stream import UNDEFINED_ROIsrc/odemis/acq/stream/_base.py (1)
1264-1266: Fix docstring parameter name mismatch.Line 1266 mentions
check_boundary, but the parameter ischeck_bbox.🛠️ Proposed fix
- It returns None if it's outside of the image and check_boundary is True + It returns None if it's outside of the image and check_bbox is Truesrc/odemis/gui/cont/stream_bar.py (1)
1038-1049: Add return type annotation to_on_roa_rotation.The handler lacks an explicit return type.
As per coding guidelines, add-> None.🛠️ Proposed fix
- def _on_roa_rotation(self, angle: float): + def _on_roa_rotation(self, angle: float) -> None:src/odemis/gui/comp/overlay/base.py (2)
1338-1344: Normalize rotation delta to avoid ±2π jumps.
Crossing the 0/2π boundary can rotate the rectangle the long way around. Normalizediff_angleto the shortest signed angle.🐛 Suggested fix
- diff_angle = new_rot - prev_rot + # Normalize to the shortest signed angle (-pi, pi] + diff_angle = (new_rot - prev_rot + math.pi) % (2 * math.pi) - math.pi
2000-2007: Guard empty streams and add a docstring.
stream.raw[0]will raiseIndexErrorif the stream has no data. Also, this method needs a docstring per the Python guidelines.✅ Suggested update
- def set_stream(self, stream: Stream) -> None: - self._stream = stream - raw = stream.raw[0] + def set_stream(self, stream: Stream) -> None: + """Set the active stream and derive data properties.""" + if not stream.raw: + raise ValueError("Stream.raw must contain at least one element") + self._stream = stream + raw = stream.raw[0] self._data_resolution = Vec(raw.shape[-1:-3:-1]) # CTZYX -> XY md = stream._find_metadata(raw.metadata) pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6))[0:2] self._phys_pxs = pxs self._rotation = md.get(model.MD_ROTATION, 0.0)src/odemis/gui/comp/miccanvas.py (1)
246-251: Guardtab_data.roa_rotationto avoid AttributeError.
Some GUIData variants exposeTOOL_ROAwithoutroa_rotation. Use a safe fallback when constructing the overlay.✅ Suggested fix
- self.roa_overlay = RepetitionSelectOverlay(self, self._roa, tab_data.fovComp, - tab_data.roa_rotation) + rotation = getattr(tab_data, "roa_rotation", None) + self.roa_overlay = RepetitionSelectOverlay(self, self._roa, tab_data.fovComp, rotation)
🧹 Nitpick comments (3)
src/odemis/util/test/util_test.py (1)
575-588: Add return type annotations and brief docstrings for new helpers/tests.The new helper methods and test case classes don’t include return type hints or class docstrings.
As per coding guidelines, please add them for the new additions.🛠️ Suggested edits (example)
-class RotateRectTestCase(unittest.TestCase): - def _rotate_point(self, pt: Tuple, center: Tuple, angle: float): +class RotateRectTestCase(unittest.TestCase): + """Tests for rotate_rect().""" + def _rotate_point( + self, + pt: Tuple[float, float], + center: Tuple[float, float], + angle: float, + ) -> Tuple[float, float]: + """Rotate a point around a center by a given angle.""" x, y = pt cx, cy = center x -= cx y -= cy cos_a = math.cos(angle) sin_a = math.sin(angle) x_new = x * cos_a - y * sin_a y_new = x * sin_a + y * cos_a return (x_new + cx, y_new + cy) - def _assert_corners_equal(self, expected: List[Tuple], actual:List[Tuple]): + def _assert_corners_equal( + self, + expected: List[Tuple[float, float]], + actual: List[Tuple[float, float]], + ) -> None: + """Assert that two corner lists are equal within tolerance.""" assert len(expected) == len(actual) for e, a in zip(expected, actual): self.assertAlmostEqual(e[0], a[0]) self.assertAlmostEqual(e[1], a[1]) -class SeparateRectRotationTestCase(unittest.TestCase): +class SeparateRectRotationTestCase(unittest.TestCase): + """Tests for separate_rect_rotation()."""Also applies to: 630-631
src/odemis/gui/test/comp_overlay_test.py (2)
638-639: Preferzip(..., strict=True)to avoid silent truncation.
This ensures the test fails if the corner list lengths ever diverge.♻️ Proposed change
-for o, b in zip(corners, corners_back): +for o, b in zip(corners, corners_back, strict=True): testing.assert_tuple_almost_equal(o, b)
721-723: Usezip(..., strict=True)for all corner comparisons.
Avoids hiding mismatched corner counts during rotation tests.♻️ Proposed change
-for o, b in zip(ebeam_corners, roi_back): +for o, b in zip(ebeam_corners, roi_back, strict=True): assert_tuple_almost_equal(o, b, msg="ebeam FoV (%s) != ROI (%s)" % (ebeam_corners, roi_back)) - for o, b in zip(ebeam_corners, roi_back): + for o, b in zip(ebeam_corners, roi_back, strict=True): assert_tuple_almost_equal(o, b, msg="ebeam FoV (%s) != ROI (%s)" % (ebeam_corners, roi_back)) - for o, b in zip(ebeam_corners, roi_back): + for o, b in zip(ebeam_corners, roi_back, strict=True): assert_tuple_almost_equal(o, b, msg="ebeam FoV (%s) != ROI (%s)" % (ebeam_corners, roi_back))Also applies to: 734-735, 747-748
tepals
left a comment
tepals
left a comment
There was a problem hiding this comment.
one small comment, rest looks good
pieleric
force-pushed
the
feat-sparc-gui-support-rotated-roa
branch
from
67e13b3 to
9d3f6c8
Compare
Introduce a function to compute the positions of the 4 corners of a rectangle after rotation, and its inverse function. These two functions will be used by the GUI to support the RoI rotation.
If the e-beam scanner supports vector scanning, it's now possible to acquire with a rotation. So add a "rotation knob" next to the RoA selector to allow rotating it. In order to do that, we drop the original RepetiionSelectOverlay, and provide a new one based on the FASTEM RectangleEditingMixin.
Update code for displaying FASTEM rectangle to match code for the SPARC, to display the size of the sides and the angle during rotation.
Now that SPARC acquisition supports rotation, the pixel selection overlay and spectrum line selection overlay must support rotation too.
pieleric
force-pushed
the
feat-sparc-gui-support-rotated-roa
branch
from
9d3f6c8 to
5d352c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/odemis/gui/comp/overlay/world_select.py`:
- Around line 39-40: The TODO comment referencing merging SelectionMixin,
WorldSelectOverlay, LineSelectOverlay and changing FastEMROCOverlay to inherit
from RepetitionSelectOverlay must be tracked in your issue tracker: create one
or more tickets (e.g., "merge-selection-mixins" and
"change-fastemroc-inheritance") and update the TODO line in
src/odemis/gui/comp/overlay/world_select.py to include the issue IDs or brief
ownership and ETA; specifically modify the comment mentioning SelectionMixin,
WorldSelectOverlay, LineSelectOverlay, FastEMROCOverlay, RepetitionSelectOverlay
and EDIT_MODE_POINT to append the created ticket references (or a single
umbrella ticket) so the work is traceable.
🧹 Nitpick comments (1)
src/odemis/gui/comp/overlay/rectangle.py (1)
429-437: Corner-finding logic may behave unexpectedly for certain rotations.The algorithm finds corners by coordinate extremes, which works well for axis-aligned or slightly rotated rectangles. However, for rotations near 45°, the "bottom-left" and "bottom-right" definitions based on
(x, y)and(-x, y)comparisons may not produce intuitive results.This is likely acceptable for the side-length labeling use case, but consider adding a comment explaining the intent.
📝 Suggested documentation
# Find the 3 corners we need to compute the side lengths and angles + # This heuristic finds corners by coordinate extremes in physical space. + # For rotated rectangles, the "bottom-left" corner is the one with minimum (x, y). all_points = set(points.keys()) if len(all_points) < 4: return # Cannot compute side lengths if points are not unique
| # TODO: merge SelectionMixin and WorldSelectOverlay and LineSelectOverlay into one class (with just EDIT_MODE_POINT) | ||
| # Change FastEMROCOverlay to inherit from RepetitionSelectOverlay. |
There was a problem hiding this comment.
Please track these TODOs with an issue/ticket.
Untracked TODOs tend to be forgotten; consider adding a reference (issue ID) or brief ownership/context.
🤖 Prompt for AI Agents
In `@src/odemis/gui/comp/overlay/world_select.py` around lines 39 - 40, The TODO
comment referencing merging SelectionMixin, WorldSelectOverlay,
LineSelectOverlay and changing FastEMROCOverlay to inherit from
RepetitionSelectOverlay must be tracked in your issue tracker: create one or
more tickets (e.g., "merge-selection-mixins" and "change-fastemroc-inheritance")
and update the TODO line in src/odemis/gui/comp/overlay/world_select.py to
include the issue IDs or brief ownership and ETA; specifically modify the
comment mentioning SelectionMixin, WorldSelectOverlay, LineSelectOverlay,
FastEMROCOverlay, RepetitionSelectOverlay and EDIT_MODE_POINT to append the
created ticket references (or a single umbrella ticket) so the work is
traceable.
3 participants
After extending the SPARC acquisition to support rotated RoA, this extends the GUI to allow rotating the RoA from the GUI.
In practice, it changes mainly two parts: