Allow rotating ortho view #8614
Conversation
📝 WalkthroughWalkthroughThis change implements free rotation in orthogonal viewports by refactoring rotation handling throughout the codebase. It updates state management, rendering logic, mouse interaction mapping, shaders, UI components, and disables certain tools and actions when the viewport is rotated. The update also exposes rotation controls in the UI and adapts measurement, annotation, and context menu logic accordingly. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ho-view-rotation-v2
…alculate correct global position in shader
There was a problem hiding this comment.
Apparently completely unused. Thus, I deleted it
| // TODOM: Maybe do not make this snap to voxel -> new issue | ||
| const newPos = V3.floor(calculateGlobalPos(state, pos, this.initialPlane).rounded); |
There was a problem hiding this comment.
During active rotation the measurement snapping to voxel is pretty strange
| // convert nm to voxel | ||
| const voxelPerNMVector = getVoxelPerUnit(Store.getState().dataset.dataSource.scale); | ||
| V3.scale(voxelPerNMVector, value, this.planeShift); | ||
| this.clippingDistance = value; |
There was a problem hiding this comment.
Previously, this value was scaled by the voxel per unit scale and then unscaled (or something like this) in the shader again. Now this value is kept directly in the scene controller to avoid too many number conversions.
| function _getAreaMeasurementToolInfo(isFlycamRotated: boolean) { | ||
| return { | ||
| [AnnotationTool.AREA_MEASUREMENT.id]: isFlycamRotated | ||
| ? { | ||
| isDisabled: true, | ||
| explanation: rotationActiveDisabledExplanation, | ||
| } | ||
| : NOT_DISABLED_INFO, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I disabled the area measurement tool during rotated state for now as getting the calculation correct seemed a little more complicated and I did not want to implement this in the first iteration. I imagined that this tool would not be that important in a rotated state for now.
| // As the order in which the angles are applied is zyx (see flycam_reducer), | ||
| // this order must be followed when this euler angle is applied to 2d computations. | ||
| function _getRotationInRadian(flycam: Flycam, invertZ: boolean = true): Vector3 { | ||
| // Somehow z rotation is inverted but the others are not. |
There was a problem hiding this comment.
Not really sure why z is the one inverted axis, but when I invert the others this break again, especially the bucket picker and the ortho xy plane is no longer consistent to oblique mode plane if this is not applied
| export const setLastMeasuredPositionAction = (position: Vector3) => | ||
| export const setLastMeasuredAndViewportPositionAction = ( | ||
| lastMeasuredPosition: Vector3, | ||
| viewportPosition: Point2, |
There was a problem hiding this comment.
I optimized displaying the distance / area measurement tool tip a little. Previously lastMeasuredPosition was transformed back into the space of the plane to get the location where to show the tooltip. Now this position is directly stored in the store to avoid the back and forth calculation. This makes the code in the tooltip component also easier
There was a problem hiding this comment.
My try to implement a rotation aware ortho prefretcher. Should be linked to the follow up issue and then deleted
| rotation = Utils.point3ToVector3(someTracing.editRotation); | ||
| } | ||
| let rotation = datasetConfiguration.rotation; | ||
| //TODOM: Now even in ortho mode the default rotation is set. Discuss whether thats what we actually want. |
There was a problem hiding this comment.
Jup, thats something we should discuss: Should the default rotation of a dataset configured in the ds settings be applied every time the dataset is viewed / a new annotation is opened (if nothing else is encoded in the URL)
| @@ -481,7 +483,7 @@ void main() { | |||
| // Remember the original z position, since it can subtly diverge in the | |||
| // following calculations due to floating point inaccuracies. This can | |||
| // result in artifacts, such as the crosshair disappearing. | |||
| float originalZ = gl_Position.z; | |||
| /*float originalZ = gl_Position.z; | |||
There was a problem hiding this comment.
TODO: Reenable shader optimizations
| copyRotationToClipboard = async () => { | ||
| const rotation = V3.round(getRotation(this.props.flycam)).join(", "); | ||
| await navigator.clipboard.writeText(rotation); | ||
| Toast.success("Rotation copied to clipboard"); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I removed this feature as it likely was pretty much unused and reusing this button to make a rotation reset seemed far more useful to me
| const { | ||
| left: viewportLeft, | ||
| top: viewportTop, | ||
| width: viewportWidth, | ||
| height: viewportHeight, | ||
| } = getInputCatcherRect(state, orthoView); | ||
| const tooltipPosition = calculateMaybePlaneScreenPos(state, position, orthoView); | ||
|
|
||
| if (tooltipPosition == null) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is now obsolete due to storeing the measurement tool tip viewport pos directly in the store. No conversion via calculateMaybePlaneScreenPos needed anymore
| state: State = { | ||
| visible: false, | ||
| }; | ||
| const AbstractTreeTab: React.FC<EmptyObject> = () => { |
There was a problem hiding this comment.
Made this a functional component as I had to edit this file anyway
…ho-view-rotation-v2
| setPosition = (x: number, y: number, z: number) => { | ||
| // @ts-expect-error ts-migrate(2339) FIXME: Property 'setGlobalPosition' does not exist on typ... Remove this comment to see the full error message | ||
| this.meshes.mainPlane.material.setGlobalPosition(x, y, z); | ||
| }; | ||
|
|
There was a problem hiding this comment.
If I remember correctly, this was never called anywhere
There was a problem hiding this comment.
I made this a functional component (using AI assistance)
…reusing them. - And some more clean up
|
@philippotto I'd say this PR is finally ready for the first review iteration. What do you think? Do you think you have time to review this? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
frontend/javascripts/viewer/view/action-bar/dataset_rotation_popover_view.tsx (2)
13-16: Consider using CSS-in-JS or theme constants for warning colors.The hardcoded color values should ideally come from a design system or theme constants to ensure consistency across the application.
-export const warningColors: React.CSSProperties = { - color: "rgb(255, 155, 85)", - borderColor: "rgb(241, 122, 39)", -}; +// Consider moving to theme constants +export const warningColors: React.CSSProperties = { + color: "var(--warning-color, rgb(255, 155, 85))", + borderColor: "var(--warning-border-color, rgb(241, 122, 39))", +};
25-83: Excellent implementation with minor suggestions.The PopoverContent component is well-structured with consistent patterns for each rotation axis. Each slider has proper bounds (0-360 degrees) and includes reset functionality.
Minor suggestions:
- Consider extracting the repeated slider pattern into a reusable component
- Add
aria-labelattributes for better accessibility// Consider extracting a reusable RotationSlider component: +interface RotationSliderProps { + label: string; + value: number; + onChange: (value: number) => void; + onReset: () => void; +} + +const RotationSlider: React.FC<RotationSliderProps> = ({ label, value, onChange, onReset }) => ( + <div> + <NumberSliderSetting + label={label} + min={0} + max={360} + step={1} + value={value} + onChange={onChange} + spans={[3, 13, 4, 4]} + postComponent={ + <Button + type="text" + icon={<RollbackOutlined />} + onClick={onReset} + aria-label={`Reset ${label} rotation`} + /> + } + /> + </div> +);frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
898-899: TODO: Address measurement snapping issue during rotation.The comment indicates that measurement snapping to voxel behaves strangely during active rotation. This should be addressed to ensure consistent user experience.
Would you like me to create an issue to track this measurement snapping problem during rotation?
frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_tab.tsx (1)
41-53: Consider combining the useEffect hooks.Both
useEffecthooks have the same dependency (drawTree) and could potentially be combined into a single effect for clarity.- useEffect(() => { - window.addEventListener("resize", drawTree, false); - drawTree(); - - return () => { - window.removeEventListener("resize", drawTree, false); - }; - }, [drawTree]); - - useEffect(() => { - drawTree(); - }, [drawTree]); + useEffect(() => { + window.addEventListener("resize", drawTree, false); + drawTree(); + + return () => { + window.removeEventListener("resize", drawTree, false); + }; + }, [drawTree]);frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts (1)
308-320: Fix typo and clarify the TODO comment.The comment contains a typo ("TODOM" should be "TODO"). Additionally, please clarify what needs to be addressed regarding the difference with OrthoBaseRotations.
- // TODOM: Why is this different to the OrthoBaseRotations??? + // TODO: Why is this different to the OrthoBaseRotations???
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
frontend/javascripts/libs/mjs.ts(1 hunks)frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/test/controller/url_manager.spec.ts(1 hunks)frontend/javascripts/test/fixtures/volumetracing_object.ts(2 hunks)frontend/javascripts/test/reducers/flycam_reducer.spec.ts(7 hunks)frontend/javascripts/test/shaders/shader_syntax.spec.ts(6 hunks)frontend/javascripts/types/schemas/url_state.types.ts(0 hunks)frontend/javascripts/viewer/api/api_latest.ts(6 hunks)frontend/javascripts/viewer/constants.ts(2 hunks)frontend/javascripts/viewer/controller/camera_controller.ts(4 hunks)frontend/javascripts/viewer/controller/combinations/bounding_box_handlers.ts(7 hunks)frontend/javascripts/viewer/controller/combinations/move_handlers.ts(4 hunks)frontend/javascripts/viewer/controller/combinations/segmentation_handlers.ts(2 hunks)frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts(20 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(9 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(2 hunks)frontend/javascripts/viewer/controller/scene_controller.ts(11 hunks)frontend/javascripts/viewer/controller/url_manager.ts(3 hunks)frontend/javascripts/viewer/controller/viewmodes/arbitrary_controller.tsx(3 hunks)frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx(5 hunks)frontend/javascripts/viewer/default_state.ts(2 hunks)frontend/javascripts/viewer/geometries/arbitrary_plane.ts(0 hunks)frontend/javascripts/viewer/geometries/helper_geometries.ts(1 hunks)frontend/javascripts/viewer/geometries/materials/plane_material_factory.ts(10 hunks)frontend/javascripts/viewer/geometries/plane.ts(7 hunks)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts(11 hunks)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts(3 hunks)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts(5 hunks)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx(1 hunks)frontend/javascripts/viewer/model/actions/ui_actions.ts(3 hunks)frontend/javascripts/viewer/model/helpers/nml_helpers.ts(2 hunks)frontend/javascripts/viewer/model/reducers/flycam_reducer.ts(7 hunks)frontend/javascripts/viewer/model/reducers/settings_reducer.ts(1 hunks)frontend/javascripts/viewer/model/reducers/ui_reducer.ts(1 hunks)frontend/javascripts/viewer/model/sagas/prefetch_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts(3 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(3 hunks)frontend/javascripts/viewer/model_initialization.ts(2 hunks)frontend/javascripts/viewer/shaders/coords.glsl.ts(2 hunks)frontend/javascripts/viewer/shaders/filtering.glsl.ts(1 hunks)frontend/javascripts/viewer/shaders/main_data_shaders.glsl.ts(7 hunks)frontend/javascripts/viewer/shaders/segmentation.glsl.ts(1 hunks)frontend/javascripts/viewer/shaders/texture_access.glsl.ts(1 hunks)frontend/javascripts/viewer/shaders/thin_plate_spline.glsl.ts(1 hunks)frontend/javascripts/viewer/store.ts(3 hunks)frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/dataset_rotation_popover_view.tsx(1 hunks)frontend/javascripts/viewer/view/action-bar/tools/toolbar_view.tsx(3 hunks)frontend/javascripts/viewer/view/action_bar_view.tsx(2 hunks)frontend/javascripts/viewer/view/arbitrary_view.ts(1 hunks)frontend/javascripts/viewer/view/components/setting_input_views.tsx(4 hunks)frontend/javascripts/viewer/view/context_menu.tsx(18 hunks)frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx(4 hunks)frontend/javascripts/viewer/view/plane_view.ts(6 hunks)frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_tab.tsx(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx(2 hunks)frontend/javascripts/viewer/view/statusbar.tsx(4 hunks)
💤 Files with no reviewable changes (2)
- frontend/javascripts/viewer/geometries/arbitrary_plane.ts
- frontend/javascripts/types/schemas/url_state.types.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/javascripts/viewer/geometries/materials/plane_material_factory.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
🧬 Code Graph Analysis (24)
frontend/javascripts/viewer/controller/url_manager.ts (1)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getRotationInDegrees(351-351)
frontend/javascripts/viewer/model/sagas/prefetch_saga.ts (1)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
isRotated(352-352)
frontend/javascripts/viewer/view/action-bar/tools/toolbar_view.tsx (2)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts (1)
getDisabledInfoForTools(423-425)
frontend/javascripts/viewer/model/helpers/nml_helpers.ts (1)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getRotationInDegrees(351-351)
frontend/javascripts/libs/mjs.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/viewer/controller/combinations/segmentation_handlers.ts (2)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
getSegmentIdForPosition(50-81)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
clickSegmentAction(222-234)
frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment.tsx (2)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
setActiveNodeAction(241-253)
frontend/javascripts/viewer/controller/combinations/move_handlers.ts (1)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getActiveMagInfo(710-710)
frontend/javascripts/viewer/controller/viewmodes/arbitrary_controller.tsx (2)
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
setActiveNodeAction(241-253)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getRotationInDegrees(351-351)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (3)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
calculateGlobalPos(261-261)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (5)
setContourTracingModeAction(372-376)startEditingAction(168-173)addToLayerAction(175-179)finishEditingAction(196-199)resetContourAction(350-353)frontend/javascripts/viewer/constants.ts (2)
Point2(27-30)OrthoView(70-70)
frontend/javascripts/viewer/view/action-bar/dataset_rotation_popover_view.tsx (5)
frontend/javascripts/types/globals.d.ts (1)
EmptyObject(31-31)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/view/components/setting_input_views.tsx (1)
NumberSliderSetting(62-134)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
isRotated(352-352)
frontend/javascripts/viewer/controller/combinations/tool_controls.ts (2)
frontend/javascripts/libs/mjs.ts (1)
V3(402-402)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
calculateGlobalPos(261-261)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getRotationInDegrees(351-351)
frontend/javascripts/viewer/controller/camera_controller.ts (2)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getRotationInRadian(350-350)frontend/javascripts/viewer/constants.ts (3)
OrthoViewValuesWithoutTDView(98-102)OrthoBaseRotations(120-125)Vector3(14-14)
frontend/javascripts/viewer/view/components/setting_input_views.tsx (1)
frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)Vector4(15-15)
frontend/javascripts/viewer/controller/combinations/bounding_box_handlers.ts (3)
frontend/javascripts/viewer/model/actions/annotation_actions.ts (1)
addUserBoundingBoxAction(199-207)frontend/javascripts/libs/mjs.ts (1)
V3(402-402)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/viewer/store.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Point2(27-30)
frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (1)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
calculateGlobalPos(261-261)
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (7)
frontend/javascripts/types/globals.d.ts (1)
EmptyObject(31-31)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/libs/mjs.ts (1)
V3(402-402)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
getPosition(348-348)frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)frontend/javascripts/libs/vector_input.tsx (1)
Vector3Input(191-195)
frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_tab.tsx (4)
frontend/javascripts/types/globals.d.ts (1)
EmptyObject(31-31)frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_renderer.ts (3)
NodeListItem(26-30)drawTree(45-62)drawTree(141-192)frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (1)
setActiveNodeAction(241-253)
frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts (2)
frontend/javascripts/viewer/model/accessors/skeletontracing_accessor.ts (2)
areGeometriesTransformed(236-242)isSkeletonLayerVisible(244-247)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
isRotated(352-352)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (4)
frontend/javascripts/libs/mjs.ts (1)
Matrix4x4(33-33)frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)OrthoView(70-70)frontend/javascripts/libs/utils.ts (1)
mod(22-24)frontend/javascripts/viewer/store.ts (1)
Flycam(421-428)
frontend/javascripts/viewer/controller/scene_controller.ts (2)
frontend/javascripts/viewer/constants.ts (3)
OrthoBaseRotations(120-125)OrthoViewValuesWithoutTDView(98-102)Vector3(14-14)frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (3)
getPosition(348-348)getRotationInRadian(350-350)getActiveMagIndicesForLayers(379-379)
frontend/javascripts/viewer/shaders/main_data_shaders.glsl.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (138)
frontend/javascripts/viewer/view/components/setting_input_views.tsx (4)
30-30: LGTM! Import properly updated for Vector4 support.The import correctly adds
Vector4type to support the enhancedspansproperty that can now handle 4-element tuples.
57-57: Well-designed prop extensions for 4-column layout support.The changes properly extend the component's capabilities:
spansnow supports both 3 and 4 element configurationspostComponentenables additional UI elements in the 4th columnType safety is maintained with the union type and optional property.
Also applies to: 60-60
90-90: Correct destructuring of new prop.The
postComponentprop is properly destructured fromthis.propsfor use in the render method.
128-130: Solid conditional rendering implementation.The conditional rendering logic correctly:
- Checks for
postComponentexistence- Validates
spansarray has more than 3 elements- Uses the 4th span element for column width
- Renders the
postComponentwithin the new columnThis enables the rotation controls mentioned in the PR to add rollback buttons alongside sliders.
frontend/javascripts/viewer/view/arbitrary_view.ts (1)
45-45: Good addition of view type identification property.The
isOrthoPlaneView = falseproperty correctly identifies this as a non-orthogonal view, which aligns with the broader rotation handling improvements. The boolean type and default value are appropriate forArbitraryView.frontend/javascripts/libs/mjs.ts (1)
391-393: Well-implemented element-wise vector multiplication.The
multiplymethod correctly performs component-wise multiplication of two 3D vectors:
- Takes two
Vector3parameters- Returns a new
Vector3with element-wise products- Follows the established pattern of other V3 utility methods
This utility supports the 3D transformation and scaling operations needed for the rotation functionality.
frontend/javascripts/test/shaders/shader_syntax.spec.ts (1)
50-50: Consistent test updates for new shader uniform.All test scenarios correctly include the new
voxelSizeFactorInverted: [1, 1, 1]property, ensuring shader compilation tests remain valid with the rotation-related shader changes. The default values are appropriate and consistent across all test cases.Also applies to: 102-102, 147-147, 184-184, 229-229, 265-265
frontend/javascripts/viewer/view/action_bar_view.tsx (1)
28-28: LGTM! Consistent component renamingThe import and usage of the component have been consistently updated to reflect the new functionality that includes both position and rotation controls. This aligns well with the PR objective of adding rotation functionality to orthogonal viewports.
Also applies to: 314-314
frontend/javascripts/test/controller/url_manager.spec.ts (1)
212-213: URL format correctly updated to include rotation valuesThe test expectation has been properly updated to include the default rotation values (0,0,180) representing a 180-degree rotation around the z-axis. The explanatory comment helps clarify why these additional values are now expected in the URL format.
frontend/javascripts/viewer/shaders/thin_plate_spline.glsl.ts (1)
77-77:✅ Verification successful
Good shader optimization replacing division with multiplication
The change from division by
voxelSizeFactorto multiplication byvoxelSizeFactorInvertedis a standard performance optimization for shaders. This should be mathematically equivalent while being more performant.Please verify that the
voxelSizeFactorInverteduniform is properly defined and passed to the shader:
🏁 Script executed:
#!/bin/bash # Search for voxelSizeFactorInverted definition and usage in shader-related files rg -A 3 -B 3 "voxelSizeFactorInverted" --type ts --type jsLength of output: 12075
Optimization Approved & Inverse Factor Verified
I’ve confirmed that
voxelSizeFactorInvertedis correctly defined and supplied to your shaders:
- In
plane_material_factory.ts:
const voxelSizeFactorInverted = V3.divide3([1, 1, 1], voxelSizeFactor);
and passed into bothgetMainVertexShaderandgetMainFragmentShader.- In
main_data_shaders.glsl.ts:
Declared asconst vec3 voxelSizeFactorInverted = <%= formatVector3AsVec3(voxelSizeFactorInverted) %>;- Covered by tests in
shader_syntax.spec.ts.No further action required—this optimization is safe to merge.
frontend/javascripts/messages.tsx (1)
162-162:❓ Verification inconclusive
Message key and functionality updated for reset rotation
The change from "copy_rotation" to "reset_rotation" reflects a functional shift in the UI from copying rotation values to resetting them. This aligns with the new rotation functionality being implemented.
Please verify that all references to the old
"tracing.copy_rotation"message key have been updated throughout the codebase:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old message key rg "tracing\.copy_rotation" --type ts --type js --type tsx --type jsxLength of output: 99
#!/bin/bash # Re-run search for any remaining references to the old message key across JS/TS files rg "tracing\.copy_rotation" -g "*.{js,jsx,ts,tsx}"
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old message key across JS/TS files using glob patterns rg -e "tracing\.copy_rotation" --glob '*.js' --glob '*.jsx' --glob '*.ts' --glob '*.tsx'Length of output: 90
#!/bin/bash # Search for any remaining references to the old message key in code rg "tracing.copy_rotation" # Search for any occurrences in translation or config JSON files rg -e "copy_rotation" --glob '*.json'
Confirm removal of old
tracing.copy_rotationreferencesPlease double-check that the message key was fully renamed from
tracing.copy_rotationtotracing.reset_rotationand that there are no lingering references in code or translation files. For example:• Run the following search across your JS/TS and JSON files:
rg -e "tracing.copy_rotation" -g "*.{js,jsx,ts,tsx,json}"If any occurrences remain, update or remove them to prevent missing-key errors at runtime.
frontend/javascripts/viewer/model/reducers/ui_reducer.ts (1)
172-176: LGTM! State update correctly handles dual position tracking.The change appropriately updates the measurement tool state to track both the 3D global position (
lastMeasuredPosition) and the 2D viewport position (viewportPosition). This is essential for proper tooltip positioning and measurement functionality when the viewport is rotated.frontend/javascripts/viewer/model/helpers/nml_helpers.ts (2)
19-19: LGTM! Import updated for standardized rotation format.The change from
getRotationtogetRotationInDegreesis part of a coordinated refactor to standardize rotation representation in degrees across the codebase. This improves consistency and human readability of serialized rotation values.
252-252: LGTM! Rotation serialization updated to use degrees.The update to use
getRotationInDegreesensures that NML files contain rotation values in degrees, which is more intuitive and aligns with the broader rotation handling refactor across the application.frontend/javascripts/viewer/controller/url_manager.ts (3)
15-15: LGTM! Import updated for standardized rotation format.The change to
getRotationInDegreesaligns with the broader refactor to use degrees consistently across the codebase for rotation values.
292-294: LGTM! Rotation now always included in URL state.The change to always include rotation (instead of conditionally based on view mode) is correct for the new orthogonal rotation feature. Using
getRotationInDegreeswith 2-decimal precision provides good balance between URL readability and precision.
381-381: LGTM! Rotation object correctly spread into URL state.The spreading of the rotation object ensures it's properly included in the returned URL state, supporting the new always-available rotation functionality.
frontend/javascripts/viewer/shaders/texture_access.glsl.ts (1)
209-209: LGTM! Shader updated for rotation-aware texture sampling.The change to initialize
beSafewithisFlycamRotatedis appropriate for ensuring correct texture sampling when the viewport is rotated. When rotation is active, the shader correctly falls back to safer lookup methods instead of relying on optimizations that may not work correctly with transformed coordinates.frontend/javascripts/viewer/view/action-bar/tools/toolbar_view.tsx (3)
24-24: LGTM! Clean import of disabled tool accessor.The import follows the established pattern and integrates well with the existing codebase structure.
239-240: LGTM! Proper use of selector pattern for tool disabling.The implementation correctly uses the typed
useWkSelectorhook and destructures the specific disabled state for the area measurement tool.
261-267: LGTM! Correct application of disabled state to UI component.The disabled state and explanation are properly applied to the RadioButtonWithTooltip component, providing both functional disabling and user feedback when the tool is unavailable due to rotation.
frontend/javascripts/viewer/model/sagas/prefetch_saga.ts (2)
10-10: LGTM! Appropriate import of rotation state selector.The
isRotatedimport is correctly added and follows the established pattern for accessing flycam state.
48-59: LGTM! Sound logic for rotation-aware prefetching.The implementation correctly gates prefetching based on both layer visibility and rotation state. Disabling prefetching when the view is rotated makes sense as it could invalidate prefetch strategies and assumptions about data layout.
frontend/javascripts/viewer/constants.ts (2)
1-1: LGTM! Appropriate import of THREE.js library.The import is necessary for the Euler rotation definitions and follows standard Three.js usage patterns.
120-125: LGTM! Well-defined base rotations for orthogonal views.The Euler rotation values appear mathematically correct for each orthogonal plane:
- PLANE_XY: 180° X-axis rotation for proper orientation
- PLANE_YZ: 180° X-axis + 90° Y-axis rotation
- PLANE_XZ: -90° X-axis rotation
- TDView: 45° rotations for isometric 3D view
Using mathematical constants (Math.PI) rather than magic numbers is excellent practice.
frontend/javascripts/viewer/model/actions/skeletontracing_actions.tsx (2)
245-245: LGTM! Consistent parameter addition for rotation control.The
suppressRotationparameter follows the established pattern of the existingsuppressAnimationandsuppressCenteringparameters. The default value oftruemaintains backward compatibility.
252-252: LGTM! Proper inclusion in action object.The
suppressRotationparameter is correctly added to the returned action object, maintaining consistency with the other suppress flags.frontend/javascripts/viewer/controller/combinations/segmentation_handlers.ts (1)
23-23: Excellent consistency in position handling for segmentation operations.The changes to use
globalPosition.roundedfor all segmentation-related operations are well-aligned with the discrete nature of voxel-based segmentation data. Using integer coordinates prevents floating-point precision issues when:
- Loading agglomerate skeletons at specific positions
- Getting segment IDs for positions
- Dispatching segment click actions
This maintains consistency with the broader refactor to use rounded positions for UI interactions throughout the viewer.
Also applies to: 81-81, 85-85
frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment.tsx (1)
59-63: Good conditional rotation handling and performance optimization.The refactored click handler properly:
- Uses
useCallbackto prevent unnecessary re-renders when dependencies haven't changed- Conditionally suppresses rotation in orthogonal mode while allowing it in other view modes
- Correctly includes both
viewModeandcomment.nodeIdin the dependency arrayThis aligns well with the PR's goal of enabling controlled rotation behavior across different view modes.
frontend/javascripts/viewer/model_initialization.ts (1)
697-705: Significant behavioral change requires team discussion.This change removes the conditional initialization of rotation, meaning dataset default rotation is now always applied regardless of view mode, including orthogonal mode. This represents a significant shift from previous behavior.
The TODO comment on line 698 and the existing review comment from MichaelBuessemeyer both indicate this needs team discussion. Key considerations:
- User expectations: Users may expect orthogonal views to start without rotation
- Backward compatibility: Existing workflows might be affected
- Default behavior: Should dataset rotation settings override view mode conventions?
Please ensure this behavioral change has been discussed and approved by the team, as it fundamentally changes how rotation is initialized across all view modes.
frontend/javascripts/viewer/default_state.ts (2)
200-200: Clean addition of rotation state to flycam.Adding the
rotation: [0, 0, 0]property to the flycam default state is well-aligned with the broader rotation support changes throughout the codebase. The zero default is appropriate for initialization.
263-263: Appropriate addition for enhanced measurement tool functionality.The
viewportPosition: nullproperty extends the measurement tool state to track viewport-specific positioning, which supports the enhanced measurement features mentioned in the AI summary. The null default is appropriate for initialization.frontend/javascripts/viewer/shaders/segmentation.glsl.ts (1)
350-350: Coordinate transformation order change looks correct for rotation support.The change to apply
floor()toworldCoordUVWbefore thetransDimtransformation (rather than after) aligns with the new rotation handling. This ensures coordinate rounding happens in the original coordinate space before dimensional transformation, which is important for maintaining consistency when the flycam is rotated.frontend/javascripts/test/fixtures/volumetracing_object.ts (2)
5-5: Import addition supports the rotation matrix initialization.The
M4x4import is correctly added to support the new flycam matrix initialization.
109-115: Good baseline rotation setup for consistent test behavior.Adding the default 180-degree Z-axis rotation matrix to the test fixture establishes a consistent baseline that normalizes the calculated flycam rotation to
[0, 0, 0]for orthographic tests. This change supports the broader rotation handling refactoring while ensuring test predictability.The rotation calculation using
M4x4.rotate(Math.PI, [0, 0, 1], M4x4.identity(), [])is mathematically correct for a 180-degree rotation around the Z-axis.frontend/javascripts/viewer/geometries/helper_geometries.ts (1)
75-80: Excellent optimization to prevent duplicate consecutive points.The duplicate point detection logic properly prevents adding redundant vertices to the contour geometry:
- Correctly extracts the last point using
subarray((pointCount - 1) * 3, pointCount * 3)- Uses
V3.equals()for reliable point comparison- Early return prevents unnecessary vertex buffer operations
This optimization improves geometry efficiency and aligns with the broader precision improvements in the rotation handling refactor.
frontend/javascripts/viewer/model/reducers/settings_reducer.ts (1)
181-183: Correct removal of automatic rotation reset for orthogonal mode.The simplification to just update the
viewModewithout resetting rotation is aligned with the PR's goal of enabling rotation in orthogonal views. Removing the automatic rotation reset logic allows rotation state to be preserved when switching view modes, which is now controlled explicitly through dedicated rotation controls and flags rather than being tied to view mode changes.This change supports the new architecture where rotation state management is decoupled from view mode transitions.
frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx (4)
121-121: LGTM: Proper integration of viewMode selector.The addition of the viewMode selector correctly retrieves the current view mode from the Redux state to determine when rotation should be suppressed.
232-233:✅ Verification successful
Verify the parameter order in setActiveNodeAction call.
The logic for suppressing rotation in orthogonal mode is correct. However, please verify that the parameter order matches the expected signature of
setActiveNodeAction.
🏁 Script executed:
#!/bin/bash # Description: Verify the signature of setActiveNodeAction to ensure parameter order is correct # Expected: Function should accept nodeId, and three boolean parameters including suppressRotation ast-grep --pattern $'export function setActiveNodeAction($$$) { $$$ }'Length of output: 73
🏁 Script executed:
#!/bin/bash # Locate and inspect the signature of setActiveNodeAction rg "export function setActiveNodeAction" -n -C5 || \ rg "setActiveNodeAction" -n -C5Length of output: 37122
Confirmed correct parameter order for setActiveNodeAction
The function signature is(nodeId, suppressAnimation = false, suppressCentering = false, suppressRotation = true), and the callsetActiveNodeAction(nodeId, false, false, suppressRotation)matches this order exactly. No changes required.
121-121: LGTM: Clean viewMode selector addition.The addition of the
viewModeselector is properly structured and follows the existing pattern for accessing Redux state.
232-233: LGTM: Proper rotation suppression logic.The conditional suppression of rotation when in orthogonal view mode is correctly implemented. This ensures that navigating to comments doesn't trigger unwanted camera rotations in orthogonal viewports, which aligns with the PR's objective of controlling rotation behavior.
frontend/javascripts/viewer/store.ts (6)
45-45: LGTM: Point2 type import added for measurement tool enhancement.The Point2 import is correctly added to support the new
viewportPositionproperty in measurement tool info.
427-427: LGTM: Rotation property added to Flycam type.The addition of the rotation property enables consistent tracking of flycam rotation state in degrees across the application, which is essential for the rotation feature implementation.
513-517: LGTM: Enhanced measurement tool info with viewport position.The addition of
viewportPosition: Point2 | nulltomeasurementToolInfoenables improved tooltip positioning and measurement tool behavior when the view is rotated.
45-45: LGTM: Necessary type import addition.The
Point2import is correctly added to support the expandedmeasurementToolInfotype definition.
427-427: LGTM: Rotation state tracking in Flycam.Adding the
rotation: Vector3property to theFlycamtype is essential for tracking camera rotation state consistently across the application. This supports the orthogonal view rotation feature.
513-517: LGTM: Enhanced measurement tool state.The expansion of
measurementToolInfoto includeviewportPosition: Point2 | nullprovides better support for UI positioning of measurement tooltips, especially important when views are rotated.frontend/javascripts/viewer/shaders/filtering.glsl.ts (2)
93-97: LGTM: Smart filtering selection based on rotation state.The conditional filtering logic is well-implemented. Using trilinear filtering when the flycam is rotated ensures better visual quality, while maintaining the performance benefit of bilinear filtering when the view is axis-aligned. This adaptive approach optimizes both quality and performance based on the current view state.
93-97: LGTM: Intelligent filtering based on rotation state.The conditional filtering logic is well-implemented. Using trilinear filtering when the flycam is rotated ensures better visual quality for rotated orthogonal views, while preserving the performance benefits of bilinear filtering for axis-aligned views. The conditional structure is clear and follows the existing pattern.
frontend/javascripts/viewer/view/plane_view.ts (4)
59-59: LGTM: Clear view type identification.The addition of
isOrthoPlaneView = trueproperly identifies this as an orthogonal plane view, which helps distinguish it from arbitrary views throughout the rotation feature implementation.
108-108: LGTM: Consistent camelCase naming convention.The renaming of
SceneControllertosceneControllerimproves code consistency by following camelCase conventions for variable names. This change enhances readability without affecting functionality.Also applies to: 115-116, 128-128, 143-144, 234-235, 279-280
59-59: LGTM: Clear view type identification.Adding the
isOrthoPlaneView = trueproperty provides a clear way to distinguish orthogonal plane views from arbitrary views, which is valuable for the rotation feature implementation.
108-108: LGTM: Consistent camelCase naming.The renaming of
SceneControllertosceneControllerthroughout the class improves consistency with JavaScript/TypeScript naming conventions. The variable names now follow proper camelCase formatting.Also applies to: 115-116, 128-128, 143-144, 234-235, 279-280
frontend/javascripts/viewer/controller/combinations/move_handlers.ts (2)
37-63: LGTM! Good performance optimization.Caching the state and using it consistently throughout the function reduces redundant
Store.getState()calls while maintaining the same logic. The direct dispatch ofmovePlaneFlycamOrthoActionin the else branch aligns with the updated action patterns.
96-96: LGTM! Consistent coordinate handling.Using the
.roundedproperty fromcalculateGlobalPosaligns with the broader pattern across the codebase for consistent handling of global positions.frontend/javascripts/viewer/controller/viewmodes/arbitrary_controller.tsx (3)
8-8: LGTM! Import changes align with refactor patterns.The change from namespace import to default import for React is a common pattern. The replacement of
getRotationwithgetRotationInDegreesaligns with the broader refactor to use degrees consistently for rotation handling.Also applies to: 15-19
300-302: LGTM! Updated action signature with explicit rotation control.The
setActiveNodeActioncall now includes the new parameters forsuppressAnimation,suppressCentering, andsuppressRotation. SettingsuppressRotationtofalse(instead of the defaulttrue) means rotation will be preserved when setting the active node, which is appropriate for the arbitrary controller context where rotation state should be maintained.
411-411: LGTM! Consistent rotation handling.The change from
getRotationtogetRotationInDegreesaligns with the broader refactor to use degrees consistently for rotation representation across the codebase.frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
24-27: LGTM! Consistent coordinate precision across all volume handlers.All volume handler functions now consistently use the
.roundedproperty fromcalculateGlobalPosfor position-based operations. This ensures uniform coordinate precision when dispatching actions and aligns with the broader pattern established across the codebase for handling global positions.Also applies to: 31-31, 35-35, 43-48, 146-147
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (2)
20-20: LGTM! Consistent rotation handling in degrees.The import change from
getRotationtogetRotationInDegreesand its usage in thediffVolumeTracingfunction aligns with the broader refactor to use degrees consistently for rotation representation across the codebase.Also applies to: 486-486
614-614: LGTM! Proper handling of optional global position.Using the
.roundedproperty with optional chaining is appropriate sincecalculateMaybeGlobalPoscan returnnull. This ensures consistent coordinate handling while properly handling cases where no valid global position can be calculated.frontend/javascripts/viewer/view/action-bar/dataset_rotation_popover_view.tsx (3)
1-11: LGTM! Clean imports and component structure.The imports are well-organized and all dependencies are properly typed. The component follows React functional component patterns with proper TypeScript typing.
18-23: LGTM! Proper React hooks usage.The component correctly uses
useWkSelectorfor Redux state anduseCallbackfor memoized event handlers. The empty dependency array forhandleChangeRotationis appropriate since it doesn't depend on any props or state.
85-93: Good implementation with proper conditional styling.The main component correctly uses the
isRotatedselector to apply warning styles when the flycam is rotated. The style merging logic is clean and the zIndex addition ensures proper layering.frontend/javascripts/viewer/shaders/coords.glsl.ts (2)
33-33: LGTM! Clean variable introduction.The
positionOffsetUVWvariable properly transforms the position offset using thetransDimfunction, maintaining consistency with other coordinate transformations in the shader.
48-52: Excellent refactoring with improved performance.The coordinate calculation has been simplified and optimized:
- Performance improvement: Replaced division by
voxelSizeFactorUVWwith multiplication byvoxelSizeFactorInvertedUVW, which is faster in GPU operations- Code clarity: The vectorized operation
(worldCoordUVW - positionOffsetUVW) * voxelSizeFactorInvertedUVWis more readable than the previous component-wise operations- Mathematical correctness: The transformation correctly applies the position offset before scaling, which is the proper order of operations
frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts (3)
27-27: LGTM! Consistent rotation accessor usage.The import change from
getRotationtogetRotationInDegreesaligns with the broader refactoring to use degree-based rotation representation throughout the codebase.
110-127: Excellent implementation of rotation suppression logic.The rotation suppression mechanism is well-implemented:
- Proper flag extraction: Correctly checks for
suppressRotationproperty in the action- Conditional logic: Only applies rotation when not suppressed, maintaining backward compatibility
- Consistent handling: Applied to both immediate position setting and animated centering
- Clear intent: The logic clearly separates position and rotation updates
This allows for precise control over when rotation should be preserved vs. updated during node activation.
702-702: LGTM! Consistent with rotation refactoring.The call to
updateSkeletonTracingnow usesgetRotationInDegrees(flycam)instead of the oldgetRotation, maintaining consistency with the degree-based rotation handling throughout the codebase.frontend/javascripts/test/reducers/flycam_reducer.spec.ts (4)
7-7: LGTM! Consistent with rotation refactoring.The import change from
getRotationtogetRotationInDegreesmaintains consistency with the codebase-wide refactoring to degree-based rotation handling.
46-54: Excellent solution for test normalization.The
initialStateWithDefaultRotationprovides a clean way to handle the default rotation state in tests:
- Clear documentation: The comments explain why the 180-degree Z-axis rotation is applied
- Test isolation: Ensures orthographic tests start with normalized rotation [0, 0, 0] instead of [0, 0, 180]
- Mathematical correctness: Uses
M4x4.rotate(Math.PI, [0, 0, 1], ...)to apply the rotation properly- Selective application: Only used where needed (orthographic tests)
85-139: LGTM! Consistent test updates.All rotation-related assertions have been properly updated to use
getRotationInDegrees, maintaining test accuracy while aligning with the new rotation representation.
143-199: LGTM! Proper use of normalized initial state.The orthographic movement tests correctly use
initialStateWithDefaultRotationto ensure consistent baseline behavior. This addresses the rotation normalization issue mentioned in the comments and maintains test reliability.frontend/javascripts/viewer/controller/combinations/tool_controls.ts (6)
52-52: LGTM! Consistent action name update.The import statement correctly updates to the new action name
setLastMeasuredAndViewportPositionAction.
764-764: Consistent use of rounded positions in QuickSelectToolController.The changes correctly use the
.roundedproperty before applyingV3.floor(). This ensures consistent position handling across the tool.Also applies to: 798-798, 817-817
899-902: LGTM! Measurement tools correctly updated.The changes properly:
- Use
.roundedproperty for global position calculation- Pass both the rounded global position and viewport position to the action
- Maintain consistency with other tool controllers
941-950: Consistent position handling in line measurement click handler.The left click handler correctly uses the same pattern as the mouse move handler for position calculation and action dispatching.
1019-1021: LGTM! Area measurement tool updated consistently.The area measurement tool follows the same pattern as the line measurement tool for position handling.
1084-1084: LGTM! Proofread tool uses rounded position correctly.The proofread tool correctly accesses the
.roundedproperty fromcalculateGlobalPosresult.frontend/javascripts/viewer/view/statusbar.tsx (2)
418-438: LGTM! Rotation suppression in orthogonal view mode.The changes correctly implement rotation suppression when activating nodes in orthogonal view mode. This prevents unwanted camera rotation when navigating between nodes, which aligns with the PR's objective of controlled rotation behavior.
577-599: Consistent use of rounded global mouse position.The changes correctly:
- Rename the selector to
globalMousePositionRoundedfor clarity- Access the
.roundedproperty fromcalculateGlobalPosresult- Maintain the same display logic with the renamed variable
This ensures position precision and consistency with other components.
frontend/javascripts/viewer/model/actions/ui_actions.ts (2)
1-1: LGTM! Import statement updated correctly.The
Point2type is properly imported to support the new viewport position parameter.
204-212: Enhanced measurement action with dual position tracking.The refactored action creator now tracks both:
- 3D global position for accurate distance calculations
- 2D viewport position for UI tooltip placement
This improvement aligns with the comment from MichaelBuessemeyer about optimizing tooltip display by directly storing the viewport position.
frontend/javascripts/viewer/controller/camera_controller.ts (2)
69-73: Good performance optimization with cached THREE.js objects.Creating these objects once as instance properties avoids repeated allocations during each
update()call, which can improve performance significantly when the camera updates frequently.
214-214: Type assertion for camera position is appropriate.The type assertion
as Vector3is necessary here sincecameraData.positionmight be typed more broadly. This ensures TypeScript knows the exact type for the spread operation.frontend/javascripts/viewer/controller/viewmodes/plane_controller.tsx (5)
43-47: LGTM: Well-organized imports for rotation functionality.The new imports are properly structured and necessary for the rotation feature implementation.
68-69: LGTM: Appropriate type imports for dimension handling.The imports align with the new rotation logic that needs to map viewport dimensions to rotation axes.
386-403: LGTM: Well-designed rotation control system.The
axisIndexToRotationmapping androtateViewportAwarehelper function provide a clean abstraction for viewport-aware rotation. The logic correctly:
- Maps dimension indices to appropriate rotation actions
- Applies user configuration for rotation speed
- Handles direction inversion properly
- Uses current viewport indices for context-aware rotation
419-424: LGTM: Intuitive keyboard bindings for rotation.The keyboard mappings follow good UX patterns:
shift + arrowsfor pitch/yaw rotationalt + left/rightfor roll rotationThis provides users with intuitive controls for 3D rotation in orthogonal views.
523-530: LGTM: Consistent use of rounded coordinates for segment ID retrieval.The change to use
globalMousePositionRoundedensures consistency with the new global position format and aligns with similar changes throughout the codebase. Using rounded coordinates for segment ID lookup is appropriate as it maps to discrete voxel positions.frontend/javascripts/viewer/shaders/main_data_shaders.glsl.ts (4)
61-61: LGTM: Appropriate shader uniforms for rotation support.The new uniforms
voxelSizeFactorInvertedandisFlycamRotatedare well-named and necessary for the rotation feature. Adding them to theParamstype ensures type safety.Also applies to: 73-73
155-155: LGTM: Performance optimization with pre-inverted voxel size.Using a pre-computed inverted voxel size factor avoids division operations in the shader, which is a good performance optimization for GPU code.
484-488: LGTM: Appropriate optimization disabling during rotation.The early return when
isFlycamRotatedis true is a reasonable approach. The comment clearly explains that optimized vertex positioning at bucket borders doesn't work with rotations. Since this is a uniform condition, it won't cause branching performance issues.
523-523: LGTM: Consistent use of inverted voxel size factors.Replacing division with
voxelSizeFactorUVW.xby multiplication withvoxelSizeFactorInvertedUVW.xis a good performance optimization. The logic remains mathematically equivalent while being more GPU-friendly.Also applies to: 530-530, 542-542
frontend/javascripts/viewer/controller/combinations/bounding_box_handlers.ts (4)
166-167: LGTM: Improved null safety for global position handling.The enhanced null check ensures both
globalPositionandglobalPosition.roundedare defined before proceeding. This defensive programming prevents potential runtime errors.
181-181: LGTM: Consistent use of rounded coordinates.Using
globalPosition.roundedthroughout the bounding box logic ensures consistency with the new global position format and aligns with similar changes across the codebase.Also applies to: 191-191
238-246: LGTM: Robust bounding box creation with proper coordinate handling.The null safety check and consistent use of
globalPosition.roundedfor both min and max coordinates ensures reliable bounding box creation. The vector addition properly creates a 1x1x1 voxel bounding box.
320-320: LGTM: Enhanced null safety and consistent coordinate usage in resizing logic.The changes improve the robustness of bounding box resizing by:
- Adding comprehensive null checks
- Using rounded coordinates consistently
- Maintaining proper edge switching logic when bounds cross over
The refactored
updateBoundsAccordingToEdgefunction signature is cleaner and more explicit about coordinate types.Also applies to: 329-364, 371-374
frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx (4)
42-47: LGTM: Clear separation of global and viewport position state.Using separate selectors for
lastMeasuredGlobalPositionandlastMeasuredViewportPositionprovides better clarity and aligns with the updated measurement tool state structure.
55-55: LGTM: Improved state management with proper selectors.Using
getRotationInDegreesand the viewport rectangle selector provides clean access to the required state. The destructuring of viewport dimensions is clear and readable.Also applies to: 64-69
88-94: LGTM: Comprehensive null safety checks.The updated early return condition properly validates all required state before rendering, including both global and viewport positions. This prevents rendering errors when state is incomplete.
117-128: LGTM: Improved tooltip positioning with viewport coordinates.The new positioning logic using
lastMeasuredViewportPositionwith proper clamping to viewport bounds is more reliable than the previous plane screen position calculation. The clamping ensures the tooltip remains visible within the viewport.frontend/javascripts/viewer/view/right-border-tabs/abstract_tree_tab.tsx (3)
12-18: LGTM! Clean refactor to functional component.The migration from class component to functional component with hooks is well done. Good use of typed props with
EmptyObject.
20-39: Good use of throttled callback with proper dependencies.The
drawTreefunction is correctly memoized withuseCallbackand has the right dependencies. The throttle delay of 1000ms is appropriate for tree rendering performance.
61-64: Good implementation of rotation suppression in orthogonal mode.The logic to suppress rotation when clicking nodes in orthogonal view mode aligns well with the PR's objective of handling rotation in orthogonal viewports.
frontend/javascripts/viewer/geometries/materials/plane_material_factory.ts (5)
163-165: Good renaming for semantic clarity.The rename from
globalPositiontopositionOffsetbetter describes the uniform's purpose as an offset rather than an absolute position.
250-250: Proper integration of rotation state into shader uniforms.Adding the
isFlycamRotateduniform allows shaders to adapt their rendering logic based on the rotation state, which is essential for the PR's rotation functionality.
587-592: Correct implementation of rotation state listener.The listener properly tracks the flycam rotation state and updates the shader uniform. Good use of the
isRotatedselector.
669-669: Consistent use of rounded position values.Using
.roundedproperty aligns with the broader changes in position handling across the codebase.
1092-1092: Smart optimization using inverted voxel size factor.Pre-computing the inverted voxel size factor and passing it to shaders is a good optimization. Multiplication is generally faster than division in GPU shaders.
Also applies to: 1101-1101, 1128-1128, 1138-1138
frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx (3)
34-38: Clean refactor to functional component with proper state selection.Good use of
useWkSelectorto directly select the needed state slices. The component is now more focused and easier to test.
87-91: Good implementation of position copy functionality.The async clipboard API is used correctly with proper success feedback via Toast.
124-124: Excellent separation of concerns for rotation controls.Moving rotation controls to a dedicated
DatasetRotationPopoverButtonViewcomponent improves modularity and makes the code more maintainable.frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (2)
300-318: Good refactoring to use THREE.js for rotation extraction.The use of reusable THREE.js objects and memoization is a good performance optimization.
530-538: Correct conversion from degrees to radians.The values have been properly converted from degrees (90°, 270°) to radians (π/2, 3π/2).
frontend/javascripts/viewer/view/context_menu.tsx (2)
401-414: Good defensive programming with early return.The early return when
isRotatedis true prevents mesh operations when the view is rotated, which is appropriate given the complexity of rotation transformations.
796-798: Excellent UX with informative tooltip.Providing a clear explanation when features are disabled improves user understanding.
frontend/javascripts/viewer/model/reducers/flycam_reducer.ts (3)
45-49: Excellent performance optimization with reusable THREE.js objects.Avoiding object creation during frequent operations like movement and rotation reduces garbage collection pressure.
84-94: Good state consistency between matrix and rotation.Maintaining both the transformation matrix and rotation angles in sync ensures consistent state across the application.
317-323:✅ Verification successful
Verify Euler order consistency across the codebase.
The code uses "ZYX" Euler order. This must match the order used in
flycam_accessor.ts(line 305 comment mentions zyx order).Also applies to: 344-347
🏁 Script executed:
#!/bin/bash # Search for all Euler order specifications to ensure consistency rg -B 2 -A 2 'makeRotationFromEuler|Euler.*order|zyx|ZYX' --type ts # Check if there are any other Euler orders used rg 'XYZ|YXZ|ZXY|XZY|YZX' --type ts | grep -i eulerLength of output: 7813
ZYX Euler order consistency verified
All flycam-related rotations use the "ZYX" order uniformly across controllers (scene_controller.ts, camera_controller.ts, skeleton_handlers.ts), reducers (flycam_reducer.ts), accessors (flycam_accessor.ts, view_mode_accessor.ts), and geometries (plane.ts). No discrepancies found.frontend/javascripts/viewer/controller/scene_controller.ts (1)
82-507: Well-implemented rotation support with performance optimizations!The refactoring from
planeShifttoclippingDistanceimproves code clarity, and the instance properties for rotation objects (lines 105-107) are a good optimization to avoid repeated object allocations. The inverse scaling approach for the plane group (lines 256-264) is a clever solution to prevent shearing artifacts when the root group is scaled.frontend/javascripts/viewer/api/api_latest.ts (2)
287-301: Good addition of rotation suppression control.The default behavior of disabling rotation when in orthogonal view mode is sensible and maintains backward compatibility.
368-382:✅ Verification successful
Ensure consistent position handling across the codebase.
The dual support for
Vector3andGlobalPositiontypes is good, but make sure all callers are updated to use the appropriate type to avoid confusion.Run the following script to verify that all calls to
createNodeare using the correct position format:
🏁 Script executed:
#!/bin/bash # Description: Find all calls to createNode and verify position parameter usage # Search for createNode calls ast-grep --pattern 'createNode($_)' -l ts tsx js jsx # Search for createNode calls with detailed context rg -A 3 'createNode\(' --type ts --type tsxLength of output: 280
The previous search didn’t yield results due to incorrect flags. Let’s rerun a focused grep across
.tsand.tsxfiles:
🏁 Script executed:
#!/bin/bash # Find all createNode calls with context in TypeScript files rg -n "createNode\(" --glob "*.ts" --glob "*.tsx" -C 2Length of output: 4448
Let’s pinpoint all API calls to
createNodeand ensure they use the correct argument type:
🏁 Script executed:
#!/bin/bash # Find all calls to the API’s createNode across TS/TSX files rg -n "api\.tracing\.createNode" --glob "*.ts" --glob "*.tsx" -C2Length of output: 681
All API createNode calls consistently use GlobalPosition
Verified that the only
api.tracing.createNodeinvocation in
frontend/javascripts/viewer/controller/combinations/tool_controls.ts
passes aGlobalPosition(viacalculateGlobalPos). No callers use rawVector3. No further changes needed.frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
98-103: Good performance optimization with object reuse.Avoiding object creation in frequently called methods is a good practice.
121-127:❓ Verification inconclusive
Verify the rotation order and scaling logic.
The rotation order "ZYX" and the multiplication by -1 at line 126 are critical for correct transformations. Please ensure this is consistent with the flycam rotation handling throughout the codebase.
Run the following script to verify rotation order consistency:
🏁 Script executed:
#!/bin/bash # Description: Verify that "ZYX" rotation order is used consistently # Search for Euler rotation order usage rg '"ZYX"' --type ts --type tsx -A 2 -B 2 # Search for makeRotationFromEuler calls to check rotation order ast-grep --pattern 'makeRotationFromEuler($$$)'Length of output: 121
🏁 Script executed:
#!/bin/bash # Refined search for Euler rotation order consistency in .ts/.tsx files # 1. Find all occurrences of the "ZYX" order literal rg -n --glob '*.ts' --glob '*.tsx' -e '"ZYX"' # 2. Inspect all calls to makeRotationFromEuler to see what order is passed rg -n --glob '*.ts' --glob '*.tsx' -e 'makeRotationFromEuler' -A2 -B2 # 3. Check for other Euler instantiations specifying a rotation order rg -n --glob '*.ts' --glob '*.tsx' -e 'new Euler' -A2 -B2Length of output: 6583
Rotation order ‘ZYX’ is already consistent across all flycam Euler rotations
I verified that every call toflycamRotationEuler.set(..., "ZYX")followed bymakeRotationFromEuleruses the same order throughout the codebase.Please confirm the inversion in view_mode_accessor.ts
- File:
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts- Around line 126: the call to
.multiplyScalar(-1)flips the computed plane position. Ensure this sign inversion matches your intended coordinate handedness or plane orientation.frontend/javascripts/viewer/geometries/plane.ts (3)
181-196: Well-structured position handling with proper scaling.The separation of original position and offset, along with correct dataset scale application, makes the positioning logic clear and maintainable.
216-223: Good reactive programming pattern with proper cleanup.The store subscription is properly managed with cleanup in the destroy method.
165-175:❓ Verification inconclusive
Verify rotation order consistency.
The comment mentions "ZYX" order requirement, but the actual rotation order depends on how
rotVecis constructed. Ensure this is documented at the call sites.Run the following script to find all calls to setRotation and verify they use the correct order:
🏁 Script executed:
#!/bin/bash # Description: Find all calls to Plane.setRotation to verify rotation order # Search for setRotation calls on Plane instances rg 'setRotation\(' --type ts --type tsx -B 3 -A 3 | grep -v "setRotationFromMatrix" # Search for Euler angle creation that might be passed to setRotation ast-grep --pattern 'new THREE.Euler($$$)'Length of output: 1931
Rotation Order Documentation Required
I didn’t find any direct calls to
Plane.setRotationin the repo—only multiplenew THREE.Euler(…)instantiations (which default to"XYZ"). Please audit all call sites where you invokesetRotation(rotVec)and ensure that:
- The passed
rotVec.orderis set to"ZYX"(either by constructing withnew THREE.Euler(x, y, z, 'ZYX')or by callingrotVec.reorder('ZYX')).- You add a brief inline note or JSDoc at each call site explaining why ZYX order is required.
Potential spots to review (where Euler instances are created):
frontend/javascripts/viewer/constants.ts(lines 121–124)frontend/javascripts/viewer/model/reducers/flycam_reducer.ts(line 46)frontend/javascripts/viewer/controller/…(scene_controller.ts, camera_controller.ts, skeleton_handlers.ts)frontend/javascripts/viewer/model/accessors/…If you’d rather enforce this within
setRotation, consider adding at the top of the method:rotVec.order = 'ZYX';frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts (10)
14-15: LGTM!The addition of radian-based rotation accessors aligns with the rotation feature implementation.
28-29: LGTM!The GlobalPosition type provides better precision handling with separate rounded and floating coordinates.
103-104: LGTM!Correctly suppresses rotation when selecting nodes in orthogonal plane views.
150-151: LGTM!Properly handles the GlobalPosition type with safe optional chaining for the rounded coordinates.
Also applies to: 166-166
177-181: LGTM!Good performance optimization by pre-allocating THREE.js objects. The warning about async usage is important and well-placed.
264-264: LGTM!Consistent use of GlobalPosition type for better precision handling.
Also applies to: 285-285
295-299: LGTM!Consistent performance optimization with pre-allocated THREE.js objects.
345-345: LGTM!Proper usage of GlobalPosition with rounded coordinates for discrete operations and floating coordinates for transformations and animations.
Also applies to: 353-353, 367-367, 392-392
497-499: LGTM!Consistent implementation of rotation suppression during node navigation in orthogonal view mode.
Also applies to: 511-517, 533-533
540-542: LGTM!Consistent rotation suppression implementation matching the toSubsequentNode function.
Also applies to: 550-556, 572-572
| // Now set rotation for all cameras respecting the base rotation of each camera. | ||
| const gRot = getRotationInRadian(state.flycam); | ||
| this.flycamRotationEuler.set(gRot[0], gRot[1], gRot[2], "ZYX"); | ||
| this.flycamRotationMatrix.makeRotationFromEuler(this.flycamRotationEuler); | ||
| for (const viewport of OrthoViewValuesWithoutTDView) { | ||
| this.baseRotationMatrix.makeRotationFromEuler(OrthoBaseRotations[viewport]); | ||
| this.props.cameras[viewport].setRotationFromMatrix( | ||
| this.totalRotationMatrix | ||
| .identity() | ||
| .multiply(this.flycamRotationMatrix) | ||
| .multiply(this.baseRotationMatrix), | ||
| ); | ||
| this.props.cameras[viewport].updateProjectionMatrix(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-implemented camera rotation logic.
The rotation implementation correctly:
- Retrieves flycam rotation in radians
- Creates rotation matrix with proper Euler order "ZYX"
- Combines flycam rotation with base rotation for each viewport
- Updates projection matrices after rotation changes
The matrix multiplication order (flycam × base) ensures the base orientation is applied first, then the user's rotation.
🌐 Web query:
What is the standard Euler rotation order (XYZ, ZYX, etc.) used in Three.js for camera rotations, and is "ZYX" the correct order for this use case?
💡 Result:
The standard Euler rotation order in Three.js for camera rotations is "XYZ", which is the default order used when not explicitly specified. However, this default order isn't always ideal for camera rotations in certain scenarios.
Camera Rotation Orders in Three.js
When working with camera rotations in Three.js, the choice of Euler rotation order is crucial because it affects how rotations are applied and can lead to different behaviors.
Default Order
The default Euler rotation order in Three.js is "XYZ"[3][7]. This means rotations are applied first around the X-axis, then around the Y-axis, and finally around the Z-axis. While this works for many cases, it can lead to issues when implementing certain camera behaviors.
Recommended Order for Camera Controls
For first-person or pointer-lock style camera controls, "YXZ" is often recommended instead of the default "XYZ"[2]. This is sometimes referred to as the "yaw-pitch-roll" order and helps avoid the gimbal lock problem that can occur with the default order.
camera.rotation.order = "YXZ";This order is particularly helpful when implementing control schemes where:
- Y rotation controls looking left/right (yaw)
- X rotation controls looking up/down (pitch)
With "YXZ" order, camera rotations behave more like what users would expect in a first-person view, preventing the rotation axes from interfering with each other and causing unexpected rotations[2].
Special Use Cases
For scenes with non-standard coordinate systems, such as those with Z-axis as up (instead of the conventional Y-axis up), different rotation orders might be required:
- For Z-up scenes: "ZXY" has been used successfully[6]
- For specialized VR implementations: "ZYX" can be appropriate in certain scenarios[6]
Choosing the Right Order
The choice of rotation order depends on your specific use case:
- For standard camera movement, the default "XYZ" works but may have limitations.
- For first-person camera controls, "YXZ" is generally recommended to avoid gimbal lock.
- For custom coordinate systems or specialized applications, other orders like "ZXY" or "ZYX" may be more appropriate.
The key is understanding that the order determines how rotations are combined and applied sequentially, which significantly impacts the camera's behavior when multiple rotation axes are involved[7].
Citations:
- 1: https://discourse.threejs.org/t/orbitcontrols-converting-camera-rotation-values-to-euler-values/68656
- 2: https://discourse.threejs.org/t/camera-rotation-axis-glitch/49477
- 3: https://discourse.threejs.org/t/rotation-to-basic-0-360-pitch-roll-yaw/37325
- 4: https://observablehq.com/@severo/three-js-camera-orientation
- 5: https://dustinpfister.github.io/2021/04/28/threejs-euler/
- 6: https://discourse.threejs.org/t/vr-scene-with-z-axis-up/1624
- 7: http://wscg.zcu.cz/WSCG2012/!_WSCG2012-Communications-1.pdf
- 8: https://discourse.threejs.org/t/camera-angle-based-on-world-coordination/68675
- 9: https://discourse.threejs.org/t/camera-rotation-y-flipping/24869
- 10: https://discourse.threejs.org/t/problem-with-camera-rotation-coordinates-in-orbitcontrols/60754
Use recommended Euler order “YXZ” for first-person camera controls
The rotation logic is sound, but Three.js’s default Euler order is “XYZ” and for first-person (yaw-pitch) camera controls “YXZ” is generally recommended to avoid gimbal lock. “ZYX” is typically only used in specialized VR or Z-up scenes.
Please update the flycam Euler order:
- File: frontend/javascripts/viewer/controller/camera_controller.ts
Lines 167–180
Change: - this.flycamRotationEuler.set(gRot[0], gRot[1], gRot[2], "ZYX");
- this.flycamRotationEuler.set(gRot[0], gRot[1], gRot[2], "YXZ");
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
// Now set rotation for all cameras respecting the base rotation of each camera.
const gRot = getRotationInRadian(state.flycam);
- this.flycamRotationEuler.set(gRot[0], gRot[1], gRot[2], "ZYX");
+ this.flycamRotationEuler.set(gRot[0], gRot[1], gRot[2], "YXZ");
this.flycamRotationMatrix.makeRotationFromEuler(this.flycamRotationEuler);
for (const viewport of OrthoViewValuesWithoutTDView) {
this.baseRotationMatrix.makeRotationFromEuler(OrthoBaseRotations[viewport]);
this.props.cameras[viewport].setRotationFromMatrix(
this.totalRotationMatrix
.identity()
.multiply(this.flycamRotationMatrix)
.multiply(this.baseRotationMatrix),
);
this.props.cameras[viewport].updateProjectionMatrix();
}
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/camera_controller.ts around lines 167
to 180, the Euler rotation order for the flycam is currently set to "ZYX", which
is not ideal for first-person camera controls. Change the Euler order from "ZYX"
to "YXZ" to follow Three.js best practices for yaw-pitch camera rotation and
avoid gimbal lock issues. Update the flycamRotationEuler.set call accordingly to
use "YXZ" as the rotation order string.
| useEffect(() => { | ||
| if ( | ||
| position != null && | ||
| Math.floor(currentPosition[thirdDim]) !== Math.floor(position[thirdDim]) | ||
| lastMeasuredGlobalPosition != null && | ||
| _.isEqual(rotation, [0, 0, 0]) && // TODOM: Improve this check to remove the tooltip once moved back / forth regardless of what the current rotation is. | ||
| Math.floor(currentPosition[thirdDim]) !== Math.floor(lastMeasuredGlobalPosition[thirdDim]) | ||
| ) { | ||
| dispatch(hideMeasurementTooltipAction()); | ||
| activeGeometry.resetAndHide(); | ||
| } | ||
| }, [currentPosition[thirdDim]]); | ||
| }, [currentPosition[thirdDim], rotation]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the rotation check logic.
The current rotation check _.isEqual(rotation, [0, 0, 0]) is quite restrictive and the TODO comment indicates this needs improvement. Consider implementing a more flexible check that accounts for small rotation tolerances or different rotation states.
- _.isEqual(rotation, [0, 0, 0]) && // TODOM: Improve this check to remove the tooltip once moved back / forth regardless of what the current rotation is.
+ rotation.every(angle => Math.abs(angle) < 0.1) && // Allow small rotation tolerances📝 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.
| useEffect(() => { | |
| if ( | |
| position != null && | |
| Math.floor(currentPosition[thirdDim]) !== Math.floor(position[thirdDim]) | |
| lastMeasuredGlobalPosition != null && | |
| _.isEqual(rotation, [0, 0, 0]) && // TODOM: Improve this check to remove the tooltip once moved back / forth regardless of what the current rotation is. | |
| Math.floor(currentPosition[thirdDim]) !== Math.floor(lastMeasuredGlobalPosition[thirdDim]) | |
| ) { | |
| dispatch(hideMeasurementTooltipAction()); | |
| activeGeometry.resetAndHide(); | |
| } | |
| }, [currentPosition[thirdDim]]); | |
| }, [currentPosition[thirdDim], rotation]); | |
| useEffect(() => { | |
| if ( | |
| lastMeasuredGlobalPosition != null && | |
| rotation.every(angle => Math.abs(angle) < 0.1) && // Allow small rotation tolerances | |
| Math.floor(currentPosition[thirdDim]) !== Math.floor(lastMeasuredGlobalPosition[thirdDim]) | |
| ) { | |
| dispatch(hideMeasurementTooltipAction()); | |
| activeGeometry.resetAndHide(); | |
| } | |
| }, [currentPosition[thirdDim], rotation]); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/distance_measurement_tooltip.tsx around
lines 77 to 86, the rotation check using _.isEqual(rotation, [0, 0, 0]) is too
strict and does not handle small variations or different rotation states. Update
this check to allow a tolerance range for rotation values, for example by
comparing each rotation component to zero within a small epsilon threshold, so
the tooltip hides appropriately even with minor rotation changes.
| const isPositionOutOfBounds = useCallback( | ||
| (position: Vector3) => { | ||
| const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset); | ||
|
|
||
| handleChangeRotation = (rotation: Vector3) => { | ||
| Store.dispatch(setRotationAction(rotation)); | ||
| }; | ||
| const isPositionOutOfBounds = (min: Vector3, max: Vector3) => | ||
| position[0] < min[0] || | ||
| position[1] < min[1] || | ||
| position[2] < min[2] || | ||
| position[0] >= max[0] || | ||
| position[1] >= max[1] || | ||
| position[2] >= max[2]; | ||
|
|
||
| isPositionOutOfBounds = (position: Vector3) => { | ||
| const { dataset, task } = this.props; | ||
| const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset); | ||
| const isOutOfDatasetBounds = isPositionOutOfBounds(datasetMin, datasetMax); | ||
| let isOutOfTaskBounds = false; | ||
|
|
||
| const isPositionOutOfBounds = (min: Vector3, max: Vector3) => | ||
| position[0] < min[0] || | ||
| position[1] < min[1] || | ||
| position[2] < min[2] || | ||
| position[0] >= max[0] || | ||
| position[1] >= max[1] || | ||
| position[2] >= max[2]; | ||
| if (task?.boundingBox) { | ||
| const bbox = task.boundingBox; | ||
| const bboxMax = [ | ||
| bbox.topLeft[0] + bbox.width, | ||
| bbox.topLeft[1] + bbox.height, | ||
| bbox.topLeft[2] + bbox.depth, | ||
| ]; | ||
| // @ts-expect-error ts-migrate(2345) | ||
| isOutOfTaskBounds = isPositionOutOfBounds(bbox.topLeft, bboxMax); | ||
| } | ||
|
|
||
| const isOutOfDatasetBounds = isPositionOutOfBounds(datasetMin, datasetMax); | ||
| let isOutOfTaskBounds = false; | ||
| return { | ||
| isOutOfDatasetBounds, | ||
| isOutOfTaskBounds, | ||
| }; | ||
| }, | ||
| [dataset, task], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the TypeScript error instead of suppressing it.
The @ts-expect-error comment on line 64 can be resolved by properly typing the bboxMax array.
- const bboxMax = [
+ const bboxMax: Vector3 = [
bbox.topLeft[0] + bbox.width,
bbox.topLeft[1] + bbox.height,
bbox.topLeft[2] + bbox.depth,
];
- // @ts-expect-error ts-migrate(2345)
isOutOfTaskBounds = isPositionOutOfBounds(bbox.topLeft, bboxMax);📝 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 isPositionOutOfBounds = useCallback( | |
| (position: Vector3) => { | |
| const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset); | |
| handleChangeRotation = (rotation: Vector3) => { | |
| Store.dispatch(setRotationAction(rotation)); | |
| }; | |
| const isPositionOutOfBounds = (min: Vector3, max: Vector3) => | |
| position[0] < min[0] || | |
| position[1] < min[1] || | |
| position[2] < min[2] || | |
| position[0] >= max[0] || | |
| position[1] >= max[1] || | |
| position[2] >= max[2]; | |
| isPositionOutOfBounds = (position: Vector3) => { | |
| const { dataset, task } = this.props; | |
| const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset); | |
| const isOutOfDatasetBounds = isPositionOutOfBounds(datasetMin, datasetMax); | |
| let isOutOfTaskBounds = false; | |
| const isPositionOutOfBounds = (min: Vector3, max: Vector3) => | |
| position[0] < min[0] || | |
| position[1] < min[1] || | |
| position[2] < min[2] || | |
| position[0] >= max[0] || | |
| position[1] >= max[1] || | |
| position[2] >= max[2]; | |
| if (task?.boundingBox) { | |
| const bbox = task.boundingBox; | |
| const bboxMax = [ | |
| bbox.topLeft[0] + bbox.width, | |
| bbox.topLeft[1] + bbox.height, | |
| bbox.topLeft[2] + bbox.depth, | |
| ]; | |
| // @ts-expect-error ts-migrate(2345) | |
| isOutOfTaskBounds = isPositionOutOfBounds(bbox.topLeft, bboxMax); | |
| } | |
| const isOutOfDatasetBounds = isPositionOutOfBounds(datasetMin, datasetMax); | |
| let isOutOfTaskBounds = false; | |
| return { | |
| isOutOfDatasetBounds, | |
| isOutOfTaskBounds, | |
| }; | |
| }, | |
| [dataset, task], | |
| ); | |
| const isPositionOutOfBounds = useCallback( | |
| (position: Vector3) => { | |
| const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset); | |
| const isPositionOutOfBounds = (min: Vector3, max: Vector3) => | |
| position[0] < min[0] || | |
| position[1] < min[1] || | |
| position[2] < min[2] || | |
| position[0] >= max[0] || | |
| position[1] >= max[1] || | |
| position[2] >= max[2]; | |
| const isOutOfDatasetBounds = isPositionOutOfBounds(datasetMin, datasetMax); | |
| let isOutOfTaskBounds = false; | |
| if (task?.boundingBox) { | |
| const bbox = task.boundingBox; | |
| const bboxMax: Vector3 = [ | |
| bbox.topLeft[0] + bbox.width, | |
| bbox.topLeft[1] + bbox.height, | |
| bbox.topLeft[2] + bbox.depth, | |
| ]; | |
| isOutOfTaskBounds = isPositionOutOfBounds(bbox.topLeft, bboxMax); | |
| } | |
| return { | |
| isOutOfDatasetBounds, | |
| isOutOfTaskBounds, | |
| }; | |
| }, | |
| [dataset, task], | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx between
lines 41 and 73, the TypeScript error caused by the @ts-expect-error comment on
line 64 occurs because bboxMax is not properly typed as a Vector3. Fix this by
explicitly typing bboxMax as a Vector3 array with three numeric elements to
match the expected type, removing the need for the suppression comment.
| function _getRotationInRadianFromMatrix(flycamMatrix: Matrix4x4, invertZ: boolean = true): Vector3 { | ||
| // Somehow z rotation is inverted but the others are not. | ||
| const zInvertFactor = invertZ ? -1 : 1; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider documenting the z-axis inversion behavior more clearly.
The comment indicates uncertainty about why z-axis needs inversion. This workaround should be better documented or investigated to understand the root cause.
🏁 Script executed:
#!/bin/bash
# Search for other references to z-axis inversion or rotation issues
rg -A 5 -B 5 "invert.*z|z.*invert" --type ts
# Look for rotation-related tests that might explain this behavior
fd -e spec.ts -e test.ts | xargs rg -l "rotation.*z|z.*rotation"Length of output: 3132
Document z-axis inversion in flycam_accessor.ts
The current comment (“Somehow z rotation is inverted but the others are not”) indicates uncertainty. Please add a brief explanation of the root cause—e.g. Three.js’s right-handed coordinate system combined with zyx Euler ordering—and link to any existing rationale (such as the inversion in dataset_layer_transformation_accessor.ts).
• File: frontend/javascripts/viewer/model/accessors/flycam_accessor.ts, around lines 306–308
• Update the inline comment to explain why only the z-axis is negated
• (Optional) Reference invertAndTranspose in dataset_layer_transformation_accessor.ts for consistency
Suggested diff:
- // Somehow z rotation is inverted but the others are not.
+ // Invert z-axis rotation to compensate for Three.js’s right-handed coordinate system
+ // when converting from flycamMatrix to screen-space Euler angles.
+ // (See dataset_layer_transformation_accessor.ts invertAndTranspose for a similar inversion rationale.)📝 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.
| function _getRotationInRadianFromMatrix(flycamMatrix: Matrix4x4, invertZ: boolean = true): Vector3 { | |
| // Somehow z rotation is inverted but the others are not. | |
| const zInvertFactor = invertZ ? -1 : 1; | |
| function _getRotationInRadianFromMatrix(flycamMatrix: Matrix4x4, invertZ: boolean = true): Vector3 { | |
| // Invert z-axis rotation to compensate for Three.js’s right-handed coordinate system | |
| // when converting from flycamMatrix to screen-space Euler angles. | |
| // (See dataset_layer_transformation_accessor.ts invertAndTranspose for a similar inversion rationale.) | |
| const zInvertFactor = invertZ ? -1 : 1; |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/accessors/flycam_accessor.ts around lines
306 to 308, the comment about the z-axis rotation inversion is vague and
unclear. Replace the current comment with a clear explanation that the z-axis
inversion is due to Three.js’s right-handed coordinate system combined with the
zyx Euler rotation order. Also, optionally reference the similar inversion logic
in dataset_layer_transformation_accessor.ts's invertAndTranspose function for
consistency and further context.
| const globalPositionForNode = globalPosition | ||
| ? { rounded: globalPosition, floating: globalPosition } | ||
| : undefined; | ||
| const skeletonActions: ItemType[] = | ||
| skeletonTracing != null && globalPosition != null && allowUpdate | ||
| skeletonTracing != null && | ||
| globalPosition != null && | ||
| globalPositionForNode != null && | ||
| allowUpdate |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New GlobalPosition type needs clarification.
The code introduces a new position format with rounded and floating properties, but it's not clear why both are needed or how they differ.
🏁 Script executed:
#!/bin/bash
# Search for GlobalPosition type definition and usage
ast-grep --pattern 'type GlobalPosition = $$$'
# Search for handleCreateNodeFromGlobalPosition to understand the new signature
ast-grep --pattern 'handleCreateNodeFromGlobalPosition($$$)'Length of output: 700
🏁 Script executed:
#!/bin/bash
# Display lines 80-120 from the file where GlobalPosition is defined for context
sed -n '80,120p' frontend/javascripts/viewer/model/accessors/view_mode_accessor.tsLength of output: 2126
🏁 Script executed:
#!/bin/bash
# Search for definition of handleCreateNodeFromGlobalPosition with context
rg -n "handleCreateNodeFromGlobalPosition" -C 5 frontend/javascripts/viewer/controller/combinations/skeleton_handlers.tsLength of output: 747
🏁 Script executed:
#!/bin/bash
# Show the implementation of handleCreateNodeFromGlobalPosition to see usage of rounded vs floating
sed -n '260,340p' frontend/javascripts/viewer/controller/combinations/skeleton_handlers.tsLength of output: 3258
🏁 Script executed:
#!/bin/bash
# Inspect how roundedPosition and floatingPosition are computed in _calculateMaybeGlobalPos
rg -n "roundedPosition =" -C 6 frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts
rg -n "floatingPosition =" -C 6 frontend/javascripts/viewer/model/accessors/view_mode_accessor.tsLength of output: 1798
🏁 Script executed:
#!/bin/bash
# Find calculateGlobalPos usage and definition in context_menu.tsx
rg -n "calculateGlobalPos" -C 5 frontend/javascripts/viewer/view/context_menu.tsxLength of output: 81
#!/bin/bash
# Locate where createSkeletonNode is defined to inspect its handling of GlobalPosition
rg -n "export function createSkeletonNode" -C5 frontend/javascriptsClarify GlobalPosition semantics
Please add inline documentation (or rename fields) to make it clear why we carry both snapped (“rounded”) and precise (“floating”) coordinates through node creation:
• In frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (at the GlobalPosition definition around line 95), annotate each property. For example:
export type GlobalPosition = {
/** World‐space coordinates from the click ray (continuous values). */
floating: Vector3;
/** Voxel‐grid coordinates for skeleton node placement (rounded/floored). */
rounded: Vector3;
};• In frontend/javascripts/viewer/view/context_menu.tsx (around line 1090), update the creation of globalPositionForNode or add a comment explaining that downstream handlers use floating for precise placement and rounded to snap to voxel centers.
This will make it immediately obvious how and why each component is used when invoking handleCreateNodeFromGlobalPosition.
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts around line
95, add inline comments to the GlobalPosition type definition explaining that
'floating' represents precise world-space coordinates with continuous values,
while 'rounded' represents voxel-grid coordinates snapped or floored for
skeleton node placement. In frontend/javascripts/viewer/view/context_menu.tsx
around line 1090, add a comment near the creation of globalPositionForNode
clarifying that downstream handlers use 'floating' for precise placement and
'rounded' for snapping to voxel centers. This will clarify the purpose of both
properties in the new GlobalPosition format.
| const rotationActiveDisabledExplanation = | ||
| "The tools is disabled because you are currently viewing the dataset rotated. Please reset the rotation to [0,0,0] to be able to use this tool."; |
There was a problem hiding this comment.
Fix grammatical error in the explanation message.
The message contains a grammatical error that should be corrected.
-const rotationActiveDisabledExplanation =
- "The tools is disabled because you are currently viewing the dataset rotated. Please reset the rotation to [0,0,0] to be able to use this tool.";
+const rotationActiveDisabledExplanation =
+ "The tool is disabled because you are currently viewing the dataset rotated. Please reset the rotation to [0,0,0] to be able to use this tool.";📝 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 rotationActiveDisabledExplanation = | |
| "The tools is disabled because you are currently viewing the dataset rotated. Please reset the rotation to [0,0,0] to be able to use this tool."; | |
| const rotationActiveDisabledExplanation = | |
| "The tool is disabled because you are currently viewing the dataset rotated. Please reset the rotation to [0,0,0] to be able to use this tool."; |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts at
lines 44 to 45, fix the grammatical error in the explanation message by changing
"The tools is disabled" to "The tool is disabled" to ensure correct subject-verb
agreement.
| switch (planeIdFilled) { | ||
| case OrthoViews.PLANE_XY: { | ||
| position = [ | ||
| Math.round(curGlobalPos[0] - diffX * planeRatio[0]), | ||
| Math.round(curGlobalPos[1] - diffY * planeRatio[1]), | ||
| Math.floor(curGlobalPos[2]), | ||
| roundedPosition = [ | ||
| Math.round(globalFloatingPosition.x), | ||
| Math.round(globalFloatingPosition.y), | ||
| Math.floor(globalFloatingPosition.z), | ||
| ]; | ||
| break; | ||
| } | ||
|
|
||
| case OrthoViews.PLANE_YZ: { | ||
| position = [ | ||
| Math.floor(curGlobalPos[0]), | ||
| Math.round(curGlobalPos[1] - diffY * planeRatio[1]), | ||
| Math.round(curGlobalPos[2] - diffX * planeRatio[2]), | ||
| roundedPosition = [ | ||
| Math.floor(globalFloatingPosition.x), | ||
| Math.round(globalFloatingPosition.y), | ||
| Math.round(globalFloatingPosition.z), | ||
| ]; | ||
| break; | ||
| } | ||
|
|
||
| case OrthoViews.PLANE_XZ: { | ||
| position = [ | ||
| Math.round(curGlobalPos[0] - diffX * planeRatio[0]), | ||
| Math.floor(curGlobalPos[1]), | ||
| Math.round(curGlobalPos[2] - diffY * planeRatio[2]), | ||
| roundedPosition = [ | ||
| Math.round(globalFloatingPosition.x), | ||
| Math.floor(globalFloatingPosition.y), | ||
| Math.round(globalFloatingPosition.z), | ||
| ]; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Inconsistent rounding strategy across planes may cause alignment issues.
The rounding logic differs between planes:
- XY plane: round X, round Y, floor Z
- YZ plane: floor X, round Y, round Z
- XZ plane: round X, floor Y, round Z
This asymmetry could lead to misalignment when switching between views or when coordinates are used across different planes.
Consider using a consistent rounding strategy. If the different rounding is intentional to handle the "third dimension" differently, please add a comment explaining the rationale:
+ // The third dimension (perpendicular to the plane) uses floor() to ensure
+ // consistent positioning when switching between planes, while the in-plane
+ // dimensions use round() for symmetric rounding behavior.
switch (planeIdFilled) {📝 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.
| switch (planeIdFilled) { | |
| case OrthoViews.PLANE_XY: { | |
| position = [ | |
| Math.round(curGlobalPos[0] - diffX * planeRatio[0]), | |
| Math.round(curGlobalPos[1] - diffY * planeRatio[1]), | |
| Math.floor(curGlobalPos[2]), | |
| roundedPosition = [ | |
| Math.round(globalFloatingPosition.x), | |
| Math.round(globalFloatingPosition.y), | |
| Math.floor(globalFloatingPosition.z), | |
| ]; | |
| break; | |
| } | |
| case OrthoViews.PLANE_YZ: { | |
| position = [ | |
| Math.floor(curGlobalPos[0]), | |
| Math.round(curGlobalPos[1] - diffY * planeRatio[1]), | |
| Math.round(curGlobalPos[2] - diffX * planeRatio[2]), | |
| roundedPosition = [ | |
| Math.floor(globalFloatingPosition.x), | |
| Math.round(globalFloatingPosition.y), | |
| Math.round(globalFloatingPosition.z), | |
| ]; | |
| break; | |
| } | |
| case OrthoViews.PLANE_XZ: { | |
| position = [ | |
| Math.round(curGlobalPos[0] - diffX * planeRatio[0]), | |
| Math.floor(curGlobalPos[1]), | |
| Math.round(curGlobalPos[2] - diffY * planeRatio[2]), | |
| roundedPosition = [ | |
| Math.round(globalFloatingPosition.x), | |
| Math.floor(globalFloatingPosition.y), | |
| Math.round(globalFloatingPosition.z), | |
| ]; | |
| break; | |
| } | |
| // The third dimension (perpendicular to the plane) uses floor() to ensure | |
| // consistent positioning when switching between planes, while the in-plane | |
| // dimensions use round() for symmetric rounding behavior. | |
| switch (planeIdFilled) { | |
| case OrthoViews.PLANE_XY: { | |
| roundedPosition = [ | |
| Math.round(globalFloatingPosition.x), | |
| Math.round(globalFloatingPosition.y), | |
| Math.floor(globalFloatingPosition.z), | |
| ]; | |
| break; | |
| } | |
| case OrthoViews.PLANE_YZ: { | |
| roundedPosition = [ | |
| Math.floor(globalFloatingPosition.x), | |
| Math.round(globalFloatingPosition.y), | |
| Math.round(globalFloatingPosition.z), | |
| ]; | |
| break; | |
| } | |
| case OrthoViews.PLANE_XZ: { | |
| roundedPosition = [ | |
| Math.round(globalFloatingPosition.x), | |
| Math.floor(globalFloatingPosition.y), | |
| Math.round(globalFloatingPosition.z), | |
| ]; | |
| break; | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts between
lines 131 and 157, the rounding logic for globalFloatingPosition coordinates is
inconsistent across the three planes, which may cause alignment issues. To fix
this, either unify the rounding method for all coordinates across all planes to
ensure consistency or, if the current differences are intentional to handle the
third dimension uniquely, add clear comments explaining the rationale behind
each rounding choice to improve code clarity and maintainability.
| const flycamRotation = getRotationInRadian(state.flycam); | ||
| const isRotated = V3.equals(flycamRotation, [0, 0, 0]); | ||
|
|
||
| flycamRotationMatrix.makeRotationFromEuler(flycamRotationEuler.set(...flycamRotation, "ZYX")); | ||
| const vectorRotated = movementVector.set(...vector).applyMatrix4(flycamRotationMatrix); | ||
|
|
||
| const zoomFactor = state.flycam.zoomStep; | ||
| const scaleFactor = getBaseVoxelFactorsInUnit(state.dataset.dataSource.scale); | ||
|
|
||
| const op = (val: number) => { | ||
| if (useFloat) { | ||
| if (useFloat || isRotated) { |
There was a problem hiding this comment.
Fix the inverted rotation check logic.
The isRotated variable has inverted logic. V3.equals(flycamRotation, [0,0,0]) returns true when there is NO rotation, but the variable name and usage suggest it should be true when rotation IS present.
Apply this diff to fix the logic:
- const isRotated = V3.equals(flycamRotation, [0, 0, 0]);
+ const isRotated = !V3.equals(flycamRotation, [0, 0, 0]);📝 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 flycamRotation = getRotationInRadian(state.flycam); | |
| const isRotated = V3.equals(flycamRotation, [0, 0, 0]); | |
| flycamRotationMatrix.makeRotationFromEuler(flycamRotationEuler.set(...flycamRotation, "ZYX")); | |
| const vectorRotated = movementVector.set(...vector).applyMatrix4(flycamRotationMatrix); | |
| const zoomFactor = state.flycam.zoomStep; | |
| const scaleFactor = getBaseVoxelFactorsInUnit(state.dataset.dataSource.scale); | |
| const op = (val: number) => { | |
| if (useFloat) { | |
| if (useFloat || isRotated) { | |
| const flycamRotation = getRotationInRadian(state.flycam); | |
| const isRotated = !V3.equals(flycamRotation, [0, 0, 0]); | |
| flycamRotationMatrix.makeRotationFromEuler(flycamRotationEuler.set(...flycamRotation, "ZYX")); | |
| const vectorRotated = movementVector.set(...vector).applyMatrix4(flycamRotationMatrix); | |
| const zoomFactor = state.flycam.zoomStep; | |
| const scaleFactor = getBaseVoxelFactorsInUnit(state.dataset.dataSource.scale); | |
| const op = (val: number) => { | |
| if (useFloat || isRotated) { |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/combinations/skeleton_handlers.ts
around lines 204 to 214, the variable isRotated is incorrectly set to true when
there is no rotation because it uses V3.equals(flycamRotation, [0, 0, 0]). To
fix this, invert the logic by setting isRotated to false when the rotation
equals [0, 0, 0] and true otherwise, for example by using
!V3.equals(flycamRotation, [0, 0, 0]). This will align the variable name with
its actual meaning and usage.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
adapt bucket picking (reuse picker for arbitrary mode)The URL zoom state is increasing with every created skeleton node xDdynamicSpaceDirectionsetting while rotation is activeIssues:
(Please delete unneeded items, merge only when none are left open)