Voxel data pipette tool #8989
Voxel data pipette tool #8989
Conversation
📝 WalkthroughWalkthroughIntroduces a new "Voxel Pipette" tool that replaces the "Pick Cell" tool, enabling users to view intensity values for all visible layers via tooltip while supporting shift-click segment activation. Updates include tool controllers, UI components, test assertions, API changes, and supporting accessor functions for mouse position and segment ID queries. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR involves substantial structural changes across multiple domains: tool controller refactoring with new parameter signatures, new public APIs for position and segment queries, introduction of a new UI component, and widespread test updates. While changes are cohesive and follow a clear pattern (PICK_CELL → VOXEL_PIPETTE), the variety of affected subsystems and density of logic modifications (new tooltip positioning, measurement state handling, layer-space transformations) warrant careful review. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 |
|
|
||
| {status === "loaded" && <WkContextMenu />} | ||
|
|
||
| {status === "loaded" && distanceMeasurementTooltipPosition != null && ( |
There was a problem hiding this comment.
moving was necessary to get the positioning relative to the canvas (otherwise, the navbar height caused problems).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/statusbar.tsx (1)
386-395: Boolean null-check bug hides SegmentInfo logic.hasVisibleSegmentation is a boolean. Checking
== nullis always false; SegmentInfo will render even without a visible segmentation.- if (hasVisibleSegmentation == null) { + if (!hasVisibleSegmentation) { return null; }
🧹 Nitpick comments (9)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
427-443: New function looks good; consider refactoring to reduce duplication.The implementation correctly mirrors
getCurrentMagby returning the mag index instead of the mag vector. The logic is sound and documentation is clear.Since
getCurrentMagIndexandgetCurrentMag(lines 414-425) share nearly identical logic, consider extracting the common portion into a helper function to improve maintainability:function _getExistingMagIndex( state: WebknossosState, layerName: string, ): number | null | undefined { const magInfo = getMagInfo(getLayerByName(state.dataset, layerName).mags); const magIndex = getActiveMagIndexForLayer(state, layerName); return magInfo.getIndexOrClosestHigherIndex(magIndex); } export function getCurrentMag( state: WebknossosState, layerName: string, ): Vector3 | null | undefined { const existingMagIndex = _getExistingMagIndex(state, layerName); if (existingMagIndex == null) { return null; } return getMagInfo(getLayerByName(state.dataset, layerName).mags).getMagByIndex(existingMagIndex); } export function getCurrentMagIndex( state: WebknossosState, layerName: string, ): number | null | undefined { const existingMagIndex = _getExistingMagIndex(state, layerName); if (existingMagIndex == null) { return null; } return existingMagIndex; }frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
26-26: Use rounded mouse position to reduce churn and stabilize hovered IDs.Floating coordinates trigger expensive lookups on every pixel move within a voxel. Prefer rounded global mouse position for hovered segment ID updates.
Apply:
- import { getGlobalMousePositionFloating } from "viewer/model/accessors/view_mode_accessor"; + import { getGlobalMousePosition } from "viewer/model/accessors/view_mode_accessor";- const globalMousePosition = yield* select(getGlobalMousePositionFloating); + const globalMousePosition = yield* select(getGlobalMousePosition);Also applies to: 616-625
frontend/javascripts/viewer/view/action-bar/tools/tool_buttons.tsx (1)
263-281: Mention Shift+Click to activate a segment in the pipette’s description.Improves discoverability and aligns with tool behavior.
- description="Inspect a voxel by showing the data values per layer in a tooltip. Clicking on a voxel will pin the tooltip to make the values selectable with the mouse cursor." + description="Inspect voxel values per layer in a tooltip. Click to pin the tooltip for selection; Shift+Click activates the segment ID."frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
96-136: Memoization key is too narrow; results can be stale across state changes.getSegmentIdInfoForPosition is memoized only by globalPos, but output also depends on:
- Visible segmentation layer
- Mapping state (name/lock, HDF5 availability)
- additionalCoordinates
- Rendered zoom step
This can return stale IDs when these change without mouse movement.
Options:
- Include state-derived keys in the cache key (e.g., layerName, mappingVersion, additionalCoordinates.join(","), zoomStep) or
- Compare by voxel index (e.g., floor layer-space position) to reduce thrash, or
- Remove memoization (let upper layers debounce if needed).
Also consider de-duplicating logic between _getSegmentIdForPosition and _getSegmentIdInfoForPosition to avoid divergence.
frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx (3)
33-50: Prevent event bubbling on copy; support numeric values.Stop click propagation and stringify numeric values for the clipboard.
-function VoxelValueEntry({ +function VoxelValueEntry({ layerName, value, category, -}: { layerName: string; value: string; category: "color" | "segmentation" }) { +}: { layerName: string; value: string | number; category: "color" | "segmentation" }) { @@ - <Tooltip title="Copy to clipboard"> + <Tooltip title="Copy to clipboard"> <CopyOutlined - onClick={() => { - copyToClipboad(value); + onClick={(e) => { + e.stopPropagation(); + void copyToClipboad(String(value)); }} /> </Tooltip>
175-177: Widen value type to include numbers.API values are typically numeric; widen the map type.
- const voxelValuesByLayer: Record<string, ["color" | "segmentation", string]> | null = + const voxelValuesByLayer: Record<string, ["color" | "segmentation", string | number]> | null = layerNamesWithDataValue != null ? Object.fromEntries(layerNamesWithDataValue) : null;
104-118: Hide tooltip → also end measuring.When auto-hiding because the point left the plane, also clear measuring to keep state consistent.
- dispatch(hideMeasurementTooltipAction()); + dispatch(hideMeasurementTooltipAction()); + dispatch(setIsMeasuringAction(false));frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
622-625: Also reset measuring on tool deselect.Keep measurement state consistent when switching tools.
static onToolDeselected() { Store.dispatch(hideMeasurementTooltipAction()); + Store.dispatch(setIsMeasuringAction(false)); }
636-641: Label suggestion: clarify toggle action.Optional: When pinned, show “Unpin Tooltip” instead of “Pin Tooltip” to reflect the second click.
I can wire this by reading lastMeasuredPosition in the descriptor builder if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
docs/ui/toolbar.md(2 hunks)docs/volume_annotation/tools.md(0 hunks)frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts(1 hunks)frontend/javascripts/test/sagas/annotation_tool_disabled_info.spec.ts(2 hunks)frontend/javascripts/test/sagas/annotation_tool_saga.spec.ts(4 hunks)frontend/javascripts/viewer/api/api_latest.ts(3 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(12 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(2 hunks)frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx(5 hunks)frontend/javascripts/viewer/model.ts(3 hunks)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/tool_accessor.ts(6 hunks)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts(4 hunks)frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts(3 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(4 hunks)frontend/javascripts/viewer/view/action-bar/tools/tool_buttons.tsx(2 hunks)frontend/javascripts/viewer/view/context_menu.tsx(1 hunks)frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx(3 hunks)frontend/javascripts/viewer/view/input_catcher.tsx(1 hunks)frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx(5 hunks)frontend/javascripts/viewer/view/statusbar.tsx(4 hunks)frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx(1 hunks)frontend/stylesheets/trace_view/_tracing_view.less(1 hunks)unreleased_changes/8989.md(1 hunks)
💤 Files with no reviewable changes (1)
- docs/volume_annotation/tools.md
🧰 Additional context used
🧬 Code graph analysis (18)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (2)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(575-595)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
getMagInfo(40-40)getLayerByName(147-166)
frontend/javascripts/viewer/model.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getActiveMagIndexForLayer(405-407)
frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (2)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
VoxelPipetteToolController(590-643)
frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx (11)
frontend/javascripts/admin/voxelytics/utils.ts (1)
copyToClipboad(62-65)frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)OrthoView(66-66)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (4)
calculateInViewportPos(371-371)getGlobalMousePositionFloating(373-375)calculateMaybePlaneScreenPos(370-370)getInputCatcherRect(38-44)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (3)
getPosition(365-365)getRotationInRadian(367-367)getCurrentMagIndex(432-443)frontend/javascripts/viewer/model/scaleinfo.ts (1)
getBaseVoxelFactorsInUnit(26-33)frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
hideMeasurementTooltipAction(200-203)frontend/javascripts/viewer/model.ts (2)
getColorLayers(85-90)getVisibleSegmentationLayer(121-135)frontend/javascripts/libs/react_helpers.tsx (1)
useFetch(35-52)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
globalToLayerTransformedPosition(487-506)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getReadableNameForLayerName(176-188)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (2)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
getGlobalMousePositionFloating(373-375)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
getSegmentIdInfoForPosition(134-136)
frontend/javascripts/test/sagas/annotation_tool_saga.spec.ts (2)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
VoxelPipetteToolController(590-643)FillCellToolController(644-675)frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)
frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts (1)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (1)
frontend/javascripts/viewer/view/action-bar/tools/tool_buttons.tsx (1)
VoxelPipetteTool(263-281)
frontend/javascripts/test/sagas/annotation_tool_disabled_info.spec.ts (1)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)
frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (3)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (3)
AnnotationTool(91-105)AnnotationTool(109-109)MeasurementTools(155-158)frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx (1)
VoxelValueTooltip(74-216)frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx (1)
DistanceMeasurementTooltip(61-175)
frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts (1)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)
frontend/javascripts/viewer/view/statusbar.tsx (3)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
getGlobalMousePosition(372-372)
frontend/javascripts/viewer/view/action-bar/tools/tool_buttons.tsx (3)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts (1)
getDisabledInfoForTools(422-424)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
frontend/javascripts/viewer/store.ts (1)
WebknossosState(575-595)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (4)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (2)
AnnotationTool(91-105)AnnotationTool(109-109)frontend/javascripts/viewer/constants.ts (2)
OrthoView(66-66)Point2(27-30)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
calculateGlobalPos(368-368)frontend/javascripts/viewer/model/actions/ui_actions.ts (3)
setLastMeasuredPositionAction(204-208)hideMeasurementTooltipAction(200-203)setIsMeasuringAction(209-213)
frontend/javascripts/viewer/api/api_latest.ts (3)
frontend/javascripts/viewer/model/accessors/tool_accessor.ts (1)
AnnotationToolId(11-11)frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)AdditionalCoordinate(2-2)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (3)
getLayerByName(147-166)getMagInfo(40-40)getMappingInfoOrNull(675-683)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (4)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
globalToLayerTransformedPosition(487-506)frontend/javascripts/viewer/model.ts (1)
getVisibleSegmentationLayer(121-135)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
setActiveCellAction(206-218)
⏰ 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). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (35)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
403-403: LGTM! Documentation clarification.The wording updates improve consistency in the documentation.
Also applies to: 410-410
docs/ui/toolbar.md (1)
128-130: LGTM! Clear documentation for the new tool.The documentation clearly explains the voxel pipette functionality including hover behavior, tooltip pinning, and shift-click segment activation.
frontend/stylesheets/trace_view/_tracing_view.less (1)
176-196: LGTM! Tooltip styling updated for new orientation.The tooltip arrow repositioning from bottom-left to top-right makes sense for a pipette tool that follows the cursor. The CSS changes correctly implement the new arrow orientation.
frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts (2)
131-137: LGTM! Clear explanation of rounding behavior.The comment clarifies why rounding is used for XY dimensions but floor for Z, explaining that floor is used for Z to match what's rendered in the viewport.
967-978: Voxel coordinate flooring ingetDataValueis safe for all callers.The addition of
Math.flooris a necessary and correct safeguard. Analysis shows:
- Callers using pre-rounded coordinates (e.g.,
plane_controller.tsxwith.rounded): Flooring is idempotent and harmless.- Callers explicitly flooring before calling (e.g.,
voxel_pipette_tooltip.tsx): Their explicit flooring becomes redundant but remains valid.- Callers passing potentially float coordinates (e.g.,
merger_mode.tswith matrix-transformed positions): The flooring now provides essential protection.The semantics are correct—voxels are discrete cells and require integer indices. All examined callers are compatible with this change.
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
294-320: LGTM! Well-structured mouse position accessors.The new
_getGlobalMousePositionand_getGlobalMousePositionFloatingfunctions provide both rounded and floating-point global positions, which is useful for different use cases (rounded for node placement, floating for smooth animations).
241-241: Removal of navbarHeight was intentional and part of tooltip positioning fix.The git history shows commit
eb984903("fix tooltip positioning", Oct 23 14:56) deliberately removednavbarHeightfrom the_calculateMaybePlaneScreenPoscalculation. The developer changedMath.round(point[1] + height / 2 + top + navbarHeight)toMath.round(point[1] + height / 2 + top)as a fix. SincegetInputCatcherRectreturns pre-calculated viewport-relative coordinates from state, the navbar offset adjustment was unnecessary and was causing incorrect positioning. This change is verified as correct through the commit message and git history.frontend/javascripts/viewer/api/api_latest.ts (3)
1520-1521: LGTM! Documentation updated for new tool name.The documentation correctly references VOXEL_PIPETTE as one of the available annotation tools.
Also applies to: 1528-1529
1865-1903: LGTM! Well-implemented mapping support.The new
respectMappingparameter provides a clean way to optionally apply segment mappings during data retrieval. The implementation correctly:
- Fetches state once for efficiency
- Uses
getMappingInfoOrNullto safely resolve the mapping- Checks for
MappingStatusEnum.ENABLEDbefore applying the mapping- Passes the resolved mapping (or null) to the cube
1908-1923: LGTM! Parameter naming clarifies coordinate space.Renaming
positiontopositionInLayerSpacemakes it clear that these positions should already be in layer space, which helps prevent coordinate space confusion.frontend/javascripts/viewer/model.ts (1)
167-203: LGTM! Consistent parameter naming across model methods.The renaming of
positiontopositionInLayerSpaceingetCurrentlyRenderedZoomStepAtPositionandgetUltimatelyRenderedZoomStepAtPositionis consistent with the API layer changes and clarifies the expected coordinate space.frontend/javascripts/viewer/view/input_catcher.tsx (1)
104-104: LGTM! Tool rename in cursor mapping.The cursor mapping correctly updates PICK_CELL to VOXEL_PIPETTE while keeping the appropriate eye-dropper icon.
unreleased_changes/8989.md (1)
1-2: LGTM! Clear changelog entry.The changelog accurately describes the generalization from "Segment Picker" to "Voxel Pipette" and notes that the shift-click behavior preserves backward compatibility.
frontend/javascripts/viewer/view/context_menu.tsx (1)
428-428: LGTM! Tool reference correctly updated.The condition properly uses the renamed
VOXEL_PIPETTEtool to gate the "select-cell" action visibility in the context menu, maintaining consistent behavior with the tool rename.frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts (1)
195-195: LGTM! Test expectation correctly updated.The test now expects
VOXEL_PIPETTEafter cycling pastFILL_CELL, which aligns with the tool rename and maintains the correct tool sequence.frontend/javascripts/test/sagas/annotation_tool_disabled_info.spec.ts (2)
9-9: LGTM! Zoom sensitivity correctly configured.The
VOXEL_PIPETTEtool is appropriately excluded from zoom-sensitive tools, allowing users to inspect voxel values at any zoom level, which is the expected behavior for a read-only inspection tool.
155-155: LGTM! Rotation behavior correctly configured.The
VOXEL_PIPETTEtool is correctly included intoolsDisregardingRotation, allowing voxel inspection even when the dataset is rotated. This is appropriate for a non-editing inspection tool.frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts (2)
94-94: LGTM! Always-enabled configuration is appropriate.The
VOXEL_PIPETTEtool is correctly added toALWAYS_ENABLED_TOOL_INFOS, which is appropriate for a read-only inspection tool that should always be available regardless of annotation state.
307-307: LGTM! Volume-enabled state correctly configured.The
VOXEL_PIPETTEis mapped toNOT_DISABLED_INFOwhen volume annotation is enabled, ensuring the inspection tool remains available alongside volume editing tools.frontend/javascripts/viewer/view/layouting/tracing_layout_view.tsx (2)
398-403: LGTM! Tool-based tooltip rendering improves separation of concerns.The change from position-based to tool-based tooltip rendering is well-structured:
VoxelValueTooltipshows voxel data whenVOXEL_PIPETTEis activeDistanceMeasurementTooltipshows measurements when measurement tools are activeThis provides better modularity and clearer component responsibilities.
446-446: LGTM! State selector correctly updated.Adding
activeToolto the component props enables the tool-based tooltip rendering logic above.frontend/javascripts/test/sagas/annotation_tool_saga.spec.ts (2)
41-41: LGTM! Controller reference correctly updated.The import and usage of
VoxelPipetteToolControllercorrectly replaces the previousPickCellToolControllerreference, maintaining consistency across the test suite.Also applies to: 59-59
101-101: LGTM! Test expectations correctly updated.The test assertions now verify
VoxelPipetteToolController.onToolDeselectedis called at the appropriate points in the tool cycling sequence, ensuring the tool lifecycle saga works correctly with the renamed controller.Also applies to: 142-145
frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx (3)
17-17: LGTM! Simplified logic with parent-controlled visibility.The removal of the
MeasurementToolscheck is correct since the parent component (tracing_layout_view.tsxlines 401-403) now controls when this tooltip is rendered. This improves separation of concerns and makes the component's responsibilities clearer.Also applies to: 122-122
146-159: LGTM! Dynamic offset improves tooltip UX.The
OFFSETlogic correctly adapts based on interaction state:
8pxwhen measuring (tooltip follows cursor) provides clearance from the cursor0pxwhen pinned (tooltip is fixed) allows precise positioningThe updated positioning calculations properly incorporate the dynamic offset to keep the tooltip within viewport bounds.
164-164: LGTM! Consistent styling class.The className change from
"measurement-tooltip"to"cursor-tooltip"aligns with the voxel pipette tooltip component, ensuring consistent styling across tooltip types.frontend/javascripts/viewer/model/accessors/tool_accessor.ts (4)
21-21: LGTM! Voxel Pipette tool properly defined.The
VoxelPipetteToolclass is correctly implemented:
- Added to
_AnnotationToolHelperenum- Extends
AbstractAnnotationToolwith appropriate id and readableName- Registered in the
AnnotationToolpublic APIThe implementation follows the established pattern for other annotation tools.
Also applies to: 64-67, 99-99
128-141: LGTM! Tool correctly categorized in toolkits.The
VOXEL_PIPETTEtool is appropriately included in:
VOLUME_TOOLS- since it operates on volumetric dataREAD_ONLY_TOOLS- correctly categorized as non-editingSPLIT_SEGMENTS- useful for inspection during segmentation workflowsThese categorizations align with the tool's read-only inspection purpose.
146-150: LGTM! VolumeTools composition correctly updated.Excluding both
MOVEandVOXEL_PIPETTEfromVolumeToolsis correct, as this collection represents editing-specific volume tools. The Voxel Pipette is a read-only inspection tool and should not be included in editing tool groups.
205-205: LGTM! Shortcut behavior consistently implemented.The
adaptActiveToolToShortcutsfunction correctly mapsShiftkey presses toVOXEL_PIPETTEat lines 205 and 222, maintaining consistency with the existing behavior where Shift switches to a picker/inspection tool. This provides intuitive keyboard shortcuts aligned with the brush and trace tool workflows.Also applies to: 222-222
frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (1)
24-25: Voxel pipette wiring looks consistent.Controller import, control aggregation, and the ‘p’ keybinding integration are correct. No issues spotted.
Also applies to: 356-357, 379-399
frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx (2)
144-156: Handle null/undefined magIndex before calling getDataValue.getCurrentMagIndex can return null; clarify API expectations and guard.
Apply a safe fallback or skip layer on null:
- const magIndex = getCurrentMagIndex(Store.getState(), layer.name); + const magIndex = getCurrentMagIndex(Store.getState(), layer.name); + if (magIndex == null) { + return [getReadableNameForLayerName(dataset, annotation, layer.name), [layer.category, "n/a"] as const]; + } @@ - magIndex, - additionalCoordinates, + magIndex, + additionalCoordinates || undefined, true,Please confirm whether api.data.getDataValue accepts null; if yes, adjust accordingly.
160-164: Keying by “readable name” can collide.Two layers may resolve to the same readable name; prefer the unique layer.name as key and render the readable label separately.
I can adjust to use layer.name as key and pass the readable name for display if you confirm collisions are possible in your datasets.
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
593-601: Guard against TDView in pipette.Pipette logic can’t resolve a plane in 3D; ignore clicks there to avoid flicker (tooltip immediately hidden by the view-side guard).
- leftClick: (position: Point2, plane: OrthoView, event: MouseEvent) => { + leftClick: (position: Point2, plane: OrthoView, event: MouseEvent) => { + if (plane === OrthoViews.TDView) { + return; + } if (event.shiftKey) {
593-601: Alt-move fallback is good.Mouse move delegates to MoveHandlers when alt is pressed; matches other tools.
| if (event.shiftKey) { | ||
| VolumeHandlers.handlePickCell(position); | ||
| return; | ||
| } | ||
|
|
||
| const state = Store.getState(); | ||
| const lastMeasuredGlobalPosition = | ||
| state.uiInformation.measurementToolInfo.lastMeasuredPosition; | ||
|
|
||
| if (lastMeasuredGlobalPosition == null) { | ||
| const globalPosition = calculateGlobalPos(state, position, plane).floating; | ||
| Store.dispatch(setLastMeasuredPositionAction(globalPosition)); | ||
| } else { | ||
| Store.dispatch(hideMeasurementTooltipAction()); | ||
| } | ||
| Store.dispatch(setIsMeasuringAction(true)); |
There was a problem hiding this comment.
Left click toggle leaves measuring=true after hiding.
When unpinning (hide), set measuring=false; otherwise the UI stays in “measuring” state.
if (lastMeasuredGlobalPosition == null) {
const globalPosition = calculateGlobalPos(state, position, plane).floating;
Store.dispatch(setLastMeasuredPositionAction(globalPosition));
} else {
Store.dispatch(hideMeasurementTooltipAction());
+ Store.dispatch(setIsMeasuringAction(false));
+ return;
}
- Store.dispatch(setIsMeasuringAction(true));
+ Store.dispatch(setIsMeasuringAction(true));📝 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.
| if (event.shiftKey) { | |
| VolumeHandlers.handlePickCell(position); | |
| return; | |
| } | |
| const state = Store.getState(); | |
| const lastMeasuredGlobalPosition = | |
| state.uiInformation.measurementToolInfo.lastMeasuredPosition; | |
| if (lastMeasuredGlobalPosition == null) { | |
| const globalPosition = calculateGlobalPos(state, position, plane).floating; | |
| Store.dispatch(setLastMeasuredPositionAction(globalPosition)); | |
| } else { | |
| Store.dispatch(hideMeasurementTooltipAction()); | |
| } | |
| Store.dispatch(setIsMeasuringAction(true)); | |
| if (lastMeasuredGlobalPosition == null) { | |
| const globalPosition = calculateGlobalPos(state, position, plane).floating; | |
| Store.dispatch(setLastMeasuredPositionAction(globalPosition)); | |
| } else { | |
| Store.dispatch(hideMeasurementTooltipAction()); | |
| Store.dispatch(setIsMeasuringAction(false)); | |
| return; | |
| } | |
| Store.dispatch(setIsMeasuringAction(true)); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/combinations/tool_controls.ts around
lines 602 to 617, the left-click branch that hides (unpins) the measurement
tooltip does not clear the measuring flag so the UI remains in measuring=true;
when lastMeasuredGlobalPosition is null you should continue to set
measuring=true after storing the computed globalPosition, but in the else branch
where you dispatch hideMeasurementTooltipAction() also dispatch
setIsMeasuringAction(false) to turn off measuring; implement that single
additional dispatch in the hide branch so measuring is cleared when unpinning.
There was a problem hiding this comment.
I'd say the AI is correct here by solely reading the changes
There was a problem hiding this comment.
@MichaelBuessemeyer Thanks for confirming! You're absolutely right—the current implementation leaves isMeasuring=true even after hiding the tooltip, which is incorrect.
The fix is straightforward: when unpinning (the else branch where lastMeasuredGlobalPosition already exists), we should dispatch setIsMeasuringAction(false) to properly clear the measuring state. The suggested code change correctly handles both cases:
- First click (pin): Set position → set measuring=true
- Second click (unpin): Hide tooltip → set measuring=false → early return
The early return in the else branch is also good practice to make the control flow explicit and avoid the subsequent setIsMeasuringAction(true) call.
Feel free to apply the suggested fix! 🐰
| const posInLayerSpace = globalToLayerTransformedPosition( | ||
| globalPos, | ||
| visibleSegmentationLayer.name, | ||
| "segmentation", | ||
| Store.getState(), | ||
| ); | ||
|
|
||
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | ||
| if (visibleSegmentationLayer == null) { | ||
| const segmentId = getSegmentIdForPosition(posInLayerSpace); | ||
|
|
||
| if (segmentId === 0) { | ||
| return; | ||
| } | ||
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); | ||
|
|
There was a problem hiding this comment.
Double transform bug: posInLayerSpace passed to a function that transforms again.
getSegmentIdForPosition expects global coordinates and performs its own global→layer transform. You pass posInLayerSpace, causing an extra transform and incorrect IDs when layer transforms exist.
Use the layer-space position directly with the cube (and the provided additionalCoordinates), or call the new helper with the global position.
Apply one of:
Option A (use cube directly):
- const segmentId = getSegmentIdForPosition(posInLayerSpace);
+ const zoomStep = api.data.getRenderedZoomStepAtPosition(
+ visibleSegmentationLayer.name,
+ posInLayerSpace,
+ );
+ const segmentId = visibleSegmentationLayer.cube.getMappedDataValue(
+ posInLayerSpace,
+ additionalCoordinates,
+ zoomStep,
+ );Option B (use new helper with global pos):
- const segmentId = getSegmentIdForPosition(posInLayerSpace);
+ const { mapped: segmentId } = getSegmentIdInfoForPosition(globalPos);📝 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.
| const posInLayerSpace = globalToLayerTransformedPosition( | |
| globalPos, | |
| visibleSegmentationLayer.name, | |
| "segmentation", | |
| Store.getState(), | |
| ); | |
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | |
| if (visibleSegmentationLayer == null) { | |
| const segmentId = getSegmentIdForPosition(posInLayerSpace); | |
| if (segmentId === 0) { | |
| return; | |
| } | |
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); | |
| const posInLayerSpace = globalToLayerTransformedPosition( | |
| globalPos, | |
| visibleSegmentationLayer.name, | |
| "segmentation", | |
| Store.getState(), | |
| ); | |
| const zoomStep = api.data.getRenderedZoomStepAtPosition( | |
| visibleSegmentationLayer.name, | |
| posInLayerSpace, | |
| ); | |
| const segmentId = visibleSegmentationLayer.cube.getMappedDataValue( | |
| posInLayerSpace, | |
| additionalCoordinates, | |
| zoomStep, | |
| ); | |
| if (segmentId === 0) { | |
| return; | |
| } | |
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); |
| const posInLayerSpace = globalToLayerTransformedPosition( | |
| globalPos, | |
| visibleSegmentationLayer.name, | |
| "segmentation", | |
| Store.getState(), | |
| ); | |
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | |
| if (visibleSegmentationLayer == null) { | |
| const segmentId = getSegmentIdForPosition(posInLayerSpace); | |
| if (segmentId === 0) { | |
| return; | |
| } | |
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); | |
| const posInLayerSpace = globalToLayerTransformedPosition( | |
| globalPos, | |
| visibleSegmentationLayer.name, | |
| "segmentation", | |
| Store.getState(), | |
| ); | |
| const { mapped: segmentId } = getSegmentIdInfoForPosition(globalPos); | |
| if (segmentId === 0) { | |
| return; | |
| } | |
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/combinations/volume_handlers.ts around
lines 182-195, you're passing posInLayerSpace into getSegmentIdForPosition which
expects global coordinates and re-applies a global→layer transform, causing a
double transform and wrong segment IDs; fix by either A) using the layer-space
position directly with the cube-based lookup (call the cube API that accepts
layer-space coords and pass posInLayerSpace plus additionalCoordinates) or B)
keep the existing getSegmentIdForPosition but pass the original globalPos (and
then dispatch setActiveCellAction with the returned segmentId and globalPos);
pick one option and remove the extra transform so the coordinate space matches
the lookup function.
There was a problem hiding this comment.
seems like the ai noticed the same thing here
There was a problem hiding this comment.
@MichaelBuessemeyer Yes, that's a genuine bug! The double transformation will cause incorrect segment IDs when layer transformations are active.
I'd recommend Option B (using getSegmentIdInfoForPosition(globalPos)) since it's cleaner and reuses the new helper that was added in this PR specifically for this purpose. It also returns both mapped and unmapped IDs in one call, which could be useful if needed later.
The fix is straightforward—just pass the original globalPos to the helper instead of the already-transformed posInLayerSpace to getSegmentIdForPosition.
| const layerNamesWithDataValue = useFetch( | ||
| async () => { | ||
| if (positionToPick == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return Promise.all( | ||
| layers.map(async (layer) => { | ||
| // getCurrentMagIndex depends on the current state, but we don't | ||
| // want to refetch these data values here every time the state changes. | ||
| // This is not ideal, but the downsides should be negligible (e.g., when | ||
| // zooming, the data value won't be read again with the changed mag). | ||
| const magIndex = getCurrentMagIndex(Store.getState(), layer.name); | ||
| const positionInLayer = globalToLayerTransformedPosition( | ||
| positionToPick, | ||
| layer.name, | ||
| layer.category, | ||
| Store.getState(), | ||
| ); | ||
|
|
||
| const dataValue = await api.data.getDataValue( | ||
| layer.name, | ||
| positionInLayer.map((el) => Math.floor(el)) as Vector3, | ||
| magIndex, | ||
| additionalCoordinates, | ||
| true, | ||
| ); | ||
|
|
||
| return [ | ||
| getReadableNameForLayerName(dataset, annotation, layer.name), | ||
| [layer.category, dataValue], | ||
| ]; | ||
| }), | ||
| ); | ||
| }, | ||
| [], | ||
| [positionToPick], | ||
| ); |
There was a problem hiding this comment.
Avoid hammering the API on every mouse move; restore proper “Loading…” state.
- Fetch runs on every pixel change for each visible layer; add throttling/debouncing or a short polling interval, and drop stale responses. Also make defaultValue null so the UI can show “Loading…”.
Minimal changes to restore loading state and type:
- const layerNamesWithDataValue = useFetch(
+ const layerNamesWithDataValue = useFetch<
+ Array<[string, ["color" | "segmentation", string | number]]> | null
+ >(
async () => {
if (positionToPick == null) {
return null;
}
@@
- [],
+ null,
- [positionToPick],
+ [positionToPick], // consider a throttled key, e.g., positionToPick rounded or debounced
);Follow-up (perf): debounce positionToPick (e.g., to 30–60 ms) or fetch only when pinned, or batch per-layer requests if backend supports it. I can draft this if you decide the strategy.
📝 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.
| const layerNamesWithDataValue = useFetch( | |
| async () => { | |
| if (positionToPick == null) { | |
| return null; | |
| } | |
| return Promise.all( | |
| layers.map(async (layer) => { | |
| // getCurrentMagIndex depends on the current state, but we don't | |
| // want to refetch these data values here every time the state changes. | |
| // This is not ideal, but the downsides should be negligible (e.g., when | |
| // zooming, the data value won't be read again with the changed mag). | |
| const magIndex = getCurrentMagIndex(Store.getState(), layer.name); | |
| const positionInLayer = globalToLayerTransformedPosition( | |
| positionToPick, | |
| layer.name, | |
| layer.category, | |
| Store.getState(), | |
| ); | |
| const dataValue = await api.data.getDataValue( | |
| layer.name, | |
| positionInLayer.map((el) => Math.floor(el)) as Vector3, | |
| magIndex, | |
| additionalCoordinates, | |
| true, | |
| ); | |
| return [ | |
| getReadableNameForLayerName(dataset, annotation, layer.name), | |
| [layer.category, dataValue], | |
| ]; | |
| }), | |
| ); | |
| }, | |
| [], | |
| [positionToPick], | |
| ); | |
| const layerNamesWithDataValue = useFetch< | |
| Array<[string, ["color" | "segmentation", string | number]]> | null | |
| >( | |
| async () => { | |
| if (positionToPick == null) { | |
| return null; | |
| } | |
| return Promise.all( | |
| layers.map(async (layer) => { | |
| // getCurrentMagIndex depends on the current state, but we don't | |
| // want to refetch these data values here every time the state changes. | |
| // This is not ideal, but the downsides should be negligible (e.g., when | |
| // zooming, the data value won't be read again with the changed mag). | |
| const magIndex = getCurrentMagIndex(Store.getState(), layer.name); | |
| const positionInLayer = globalToLayerTransformedPosition( | |
| positionToPick, | |
| layer.name, | |
| layer.category, | |
| Store.getState(), | |
| ); | |
| const dataValue = await api.data.getDataValue( | |
| layer.name, | |
| positionInLayer.map((el) => Math.floor(el)) as Vector3, | |
| magIndex, | |
| additionalCoordinates, | |
| true, | |
| ); | |
| return [ | |
| getReadableNameForLayerName(dataset, annotation, layer.name), | |
| [layer.category, dataValue], | |
| ]; | |
| }), | |
| ); | |
| }, | |
| null, | |
| [positionToPick], // consider a throttled key, e.g., positionToPick rounded or debounced | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx around lines
132–169, the current fetch triggers on every mouse-move pixel and returns
immediate values so the UI never shows a “Loading…” state; change the useFetch
default value to null so the UI can render “Loading…”, debounce positionToPick
input (30–60ms) before calling the fetch to reduce request frequency, and ignore
stale responses by stamping each fetch with an incrementing requestId (or use an
AbortController) and only set results when the requestId matches the latest;
keep changes minimal: set defaultValue null, wrap the call trigger in a short
debounce, and add requestId/abort logic inside the async fetch to drop stale
responses.
| const tooltipWidth = tooltipRef.current?.offsetWidth ?? 0; | ||
| // Position tooltip just below and to the left of the cursor | ||
| const left = clamp( | ||
| viewportLeft - tooltipWidth + OFFSET, // min | ||
| tooltipPosition[0] - tooltipWidth - OFFSET, // desired position (left of cursor, small offset) | ||
| viewportLeft + viewportWidth - tooltipWidth - OFFSET, // max (stay in viewport) | ||
| ); | ||
| const top = clamp( | ||
| viewportTop, // min | ||
| tooltipPosition[1] + OFFSET, // just below cursor | ||
| viewportTop + viewportHeight - OFFSET, // max | ||
| ); |
There was a problem hiding this comment.
Fix tooltip clamping (can overflow viewport and ignores height).
Left min should be viewportLeft (not viewportLeft - tooltipWidth) and bottom clamp must subtract tooltip height.
Apply:
- const tooltipWidth = tooltipRef.current?.offsetWidth ?? 0;
+ const tooltipWidth = tooltipRef.current?.offsetWidth ?? 0;
+ const tooltipHeight = tooltipRef.current?.offsetHeight ?? 0;
// Position tooltip just below and to the left of the cursor
const left = clamp(
- viewportLeft - tooltipWidth + OFFSET, // min
- tooltipPosition[0] - tooltipWidth - OFFSET, // desired position (left of cursor, small offset)
- viewportLeft + viewportWidth - tooltipWidth - OFFSET, // max (stay in viewport)
+ viewportLeft + OFFSET, // min (stay in viewport)
+ tooltipPosition[0] - tooltipWidth - OFFSET, // desired (left of cursor)
+ viewportLeft + viewportWidth - tooltipWidth - OFFSET, // max (stay in viewport)
);
const top = clamp(
- viewportTop, // min
- tooltipPosition[1] + OFFSET, // just below cursor
- viewportTop + viewportHeight - OFFSET, // max
+ viewportTop + OFFSET, // min
+ tooltipPosition[1] + OFFSET, // just below cursor
+ viewportTop + viewportHeight - tooltipHeight - OFFSET, // max
);📝 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.
| const tooltipWidth = tooltipRef.current?.offsetWidth ?? 0; | |
| // Position tooltip just below and to the left of the cursor | |
| const left = clamp( | |
| viewportLeft - tooltipWidth + OFFSET, // min | |
| tooltipPosition[0] - tooltipWidth - OFFSET, // desired position (left of cursor, small offset) | |
| viewportLeft + viewportWidth - tooltipWidth - OFFSET, // max (stay in viewport) | |
| ); | |
| const top = clamp( | |
| viewportTop, // min | |
| tooltipPosition[1] + OFFSET, // just below cursor | |
| viewportTop + viewportHeight - OFFSET, // max | |
| ); | |
| const tooltipWidth = tooltipRef.current?.offsetWidth ?? 0; | |
| const tooltipHeight = tooltipRef.current?.offsetHeight ?? 0; | |
| // Position tooltip just below and to the left of the cursor | |
| const left = clamp( | |
| viewportLeft + OFFSET, // min (stay in viewport) | |
| tooltipPosition[0] - tooltipWidth - OFFSET, // desired (left of cursor) | |
| viewportLeft + viewportWidth - tooltipWidth - OFFSET, // max (stay in viewport) | |
| ); | |
| const top = clamp( | |
| viewportTop + OFFSET, // min | |
| tooltipPosition[1] + OFFSET, // just below cursor | |
| viewportTop + viewportHeight - tooltipHeight - OFFSET, // max | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/voxel_pipette_tooltip.tsx around lines
181-192, the tooltip clamping is wrong: change the left clamp minimum from
viewportLeft - tooltipWidth to viewportLeft, and compute tooltipHeight (e.g.
tooltipRef.current?.offsetHeight ?? 0) and use it to set the top clamp maximum
to viewportTop + viewportHeight - tooltipHeight - OFFSET so the tooltip cannot
overflow the bottom of the viewport; keep the desired positions (left:
tooltipPosition[0] - tooltipWidth - OFFSET, top: tooltipPosition[1] + OFFSET)
and the existing max for left (viewportLeft + viewportWidth - tooltipWidth -
OFFSET).
| if (event.shiftKey) { | ||
| VolumeHandlers.handlePickCell(position); | ||
| return; | ||
| } | ||
|
|
||
| const state = Store.getState(); | ||
| const lastMeasuredGlobalPosition = | ||
| state.uiInformation.measurementToolInfo.lastMeasuredPosition; | ||
|
|
||
| if (lastMeasuredGlobalPosition == null) { | ||
| const globalPosition = calculateGlobalPos(state, position, plane).floating; | ||
| Store.dispatch(setLastMeasuredPositionAction(globalPosition)); | ||
| } else { | ||
| Store.dispatch(hideMeasurementTooltipAction()); | ||
| } | ||
| Store.dispatch(setIsMeasuringAction(true)); |
There was a problem hiding this comment.
I'd say the AI is correct here by solely reading the changes
| } | ||
|
|
||
| static onToolDeselected() { | ||
| Store.dispatch(hideMeasurementTooltipAction()); |
There was a problem hiding this comment.
Maybe also add Store.dispatch(setIsMeasuringAction(false));?
| return { | ||
| leftClick: "Pick Segment", | ||
| leftClick: | ||
| _activeToolWithoutModifiers === AnnotationTool.VOXEL_PIPETTE && !shiftKey |
There was a problem hiding this comment.
Without checking the code too deeply:
Isn't it ensured that once this function is called AnnotationTool.VOXEL_PIPETTE is always the active tool? Thus, this check might not be needed 🤔
| const posInLayerSpace = globalToLayerTransformedPosition( | ||
| globalPos, | ||
| visibleSegmentationLayer.name, | ||
| "segmentation", | ||
| Store.getState(), | ||
| ); | ||
|
|
||
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | ||
| if (visibleSegmentationLayer == null) { | ||
| const segmentId = getSegmentIdForPosition(posInLayerSpace); |
There was a problem hiding this comment.
Am I missing something or is the globalToLayerTransformedPosition applied twice to globalPos. This seems incorrect to me.
The first call of globalToLayerTransformedPosition is here and the second one is within getSegmentIdForPosition.
There was a problem hiding this comment.
Ok somehow this seems to work. But still. It seems like you are applying the transformations twice 🤔
There was a problem hiding this comment.
Ok some more looking at the code & testing showed that this is indeed broken.
- The "double transformation applying" of
handlePickCellFromGlobalPositiondoes break the new voxel inspection because it is not used by the tool. Only when picking the segment id via shift + click. handlePickCellFromGlobalPositionis only used byhandlePickCellwhich is used by other tools as well. All these tools are broken currently when using this function. e.g. the Move Tool:
- Open an annotation of a dataset with transformations configured (e.g. a rotation via the dataset settings).
- Render the active volume layer without transformations.
- Use the new voxel pipette tool to activate a segment (shift + click).
- Turn on some transformation for the volume layer (e.g. rotation) and do the same again for the same position / cell.
- The now copied cell has the wrong id.
There was a problem hiding this comment.
This can likely be fixed by applying the volume layers transformations only once I think
| const posInLayerSpace = globalToLayerTransformedPosition( | ||
| globalPos, | ||
| visibleSegmentationLayer.name, | ||
| "segmentation", | ||
| Store.getState(), | ||
| ); | ||
|
|
||
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | ||
| if (visibleSegmentationLayer == null) { | ||
| const segmentId = getSegmentIdForPosition(posInLayerSpace); | ||
|
|
||
| if (segmentId === 0) { | ||
| return; | ||
| } | ||
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); | ||
|
|
There was a problem hiding this comment.
seems like the ai noticed the same thing here
| return Math.abs(posInViewport[thirdDim]) < 1; | ||
| } | ||
|
|
||
| export default function VoxelValueTooltip() { |
There was a problem hiding this comment.
VoxelValueTooltip and DistanceMeasurementTooltip do have quite a lot in common. Can we maybe combine both and make specialized "sup functional components" via inheritance?
| return; | ||
| } | ||
| Store.dispatch(setActiveCellAction(segmentId, globalPos, additionalCoordinates)); | ||
| const posInLayerSpace = globalToLayerTransformedPosition( |
There was a problem hiding this comment.
handlePickCellFromGlobalPosition no longer needs to be exported. No other file seems to import it
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)