Allow permanent dataset layer rotation in dataset settings #8159
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing dataset layer transformations and user interaction features. Key changes include the addition of new components for managing dataset rotation, improved error handling, and the restructuring of import statements for better organization. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
…ow-perma-dataset-rotation
…ow-perma-dataset-rotation
… according to affine matrix coordTransform
- and some code clean up
…ow-perma-dataset-rotation
- also always translate by dataset bounding box and not by layer bounding box for consistent rotation results
…ow-perma-dataset-rotation
…a single one. - in case layers have the same transformation, the automatic inverse of the natively rendered layer (applied to all other layers) will cancel out the layers transformation - and fixing the code according to the logic of only saving one native layer
…t datasetconfig when nativelyRenderedLayerName is not present in current view / dataset
…ow-perma-dataset-rotation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (19)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
153-177: Avoid unnecessary re-renders by memoizing Tooltip contentThe JSX returned inside the
Tooltipcomponent creates a new anonymous function on each render. To prevent unnecessary re-renders and optimize performance, consider memoizing the tooltip content or extracting it into a separate component.frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx (3)
27-28: Remove unused import 'getReadableURLPart'The
getReadableURLPartfunction is imported but not used in the file. Removing unused imports can improve code readability and prevent potential maintenance issues.Apply this diff to remove the unused import:
import _ from "lodash"; import messages from "messages"; -import { getReadableURLPart } from "oxalis/model/accessors/dataset_accessor"; import { flatToNestedMatrix } from "oxalis/model/accessors/dataset_layer_rotation_accessor";
Line range hint
119-123: Use dependency array in useEffectOnlyOnceThe
handleTransformImportfunction is defined outside theuseEffectOnlyOnce, but it's not included in its dependency array. AlthoughuseEffectOnlyOnceimplies it runs only once, adding dependencies ensures correct behavior if the hook's implementation changes.
Line range hint
174-178: Improve error message clarityThe error message in the exception
"Cannot create dataset without being logged in."could be more user-friendly by rephrasing it.Consider rephrasing the error message:
-throw new Error("Cannot create dataset without being logged in."); +throw new Error("User must be logged in to create a dataset.");frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts (1)
366-412: Simplify 'doAllLayersHaveTheSameRotation' functionThe function can be simplified for better readability and maintainability. Consider refactoring nested conditions and loops.
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (3)
Line range hint
55-58: Remove unused function 'doesSupportVolumeWithFallback'The function
doesSupportVolumeWithFallbackis defined but not used anywhere in the codebase. Removing unused code improves maintainability.
Line range hint
264-269: Avoid directly modifying function argumentsIn
getDatasetExtentAsProduct, the function takesextentas an argument and might be modifying it if not careful. Ensure that the function does not have side effects.
Line range hint
379-384: Handle potential null values in 'getMappingInfoOrNull'The function
getMappingInfoOrNullaccessesactiveMappingInfos[layerName]without checking iflayerNameis inactiveMappingInfos. This could lead to undefined behavior iflayerNameis not present.Apply this diff to add a check:
export function getMappingInfoOrNull( activeMappingInfos: Record<string, ActiveMappingInfo>, layerName: string | null | undefined, ): ActiveMappingInfo | null { - if (layerName != null && activeMappingInfos[layerName]) { + if (layerName != null && layerName in activeMappingInfos) { return activeMappingInfos[layerName]; } return null; }frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (2)
247-254: Clarify condition logic in 'TransformationIcon'The complex condition in
isDisabledcould be difficult to read. Consider simplifying or adding comments to clarify the logic.
797-798: Replace 'LockOutlined' and 'UnlockOutlined' iconsIn Ant Design v4, the
LockOutlinedandUnlockOutlinedicons are deprecated. Consider replacing them with the updated icons from@ant-design/icons.frontend/javascripts/types/globals.d.ts (1)
19-21: LGTM! Consider documenting type usageThe
Mutable<T>type is well-defined using TypeScript's mapped type feature. This utility type will be helpful for working with dataset rotation settings where immutable types need to be temporarily mutable.Consider adding a brief JSDoc comment explaining when to use this type, as removing readonly modifiers should be done judiciously. Example:
/** * Removes readonly modifiers from all properties of type T. * Use sparingly, primarily for initialization of immutable data structures. */ export type Mutable<T> = { -readonly [K in keyof T]: T[K]; };frontend/javascripts/oxalis/geometries/materials/edge_shader.ts (2)
Line range hint
47-52: Add error handling for invalid layer namesThe transformation matrix calculation assumes the layer exists but doesn't handle invalid
nativelyRenderedLayerNamevalues.Add error handling:
this.uniforms["transform"] = { value: M4x4.transpose( - getTransformsForSkeletonLayer(dataset, nativelyRenderedLayerName).affineMatrix, + (() => { + try { + return getTransformsForSkeletonLayer(dataset, nativelyRenderedLayerName).affineMatrix; + } catch (error) { + console.error("Failed to get transforms:", error); + return M4x4.identity(); // Fallback to identity matrix + } + })(), ), };
Line range hint
71-89: Consider debouncing transform updatesThe store listener for transformation changes could trigger frequent material updates. Consider debouncing these updates for better performance.
listenToStoreProperty( (storeState) => getTransformsForSkeletonLayer( storeState.dataset, storeState.datasetConfiguration.nativelyRenderedLayerName, ), - (skeletonTransforms) => { + _.debounce((skeletonTransforms) => { const transforms = skeletonTransforms; const { affineMatrix } = transforms; // ... rest of the handler - }, + }, 16), // Debounce to ~60fps true, );frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts (1)
28-33: Consider adding input validation for BoundingBoxObject dimensions.While the implementation is correct, it might be good to add validation to ensure that width, height, and depth are non-negative values to prevent invalid bounding boxes.
static fromBoundBoxObject(boundingBox: BoundingBoxObject): BoundingBox { + if (boundingBox.width < 0 || boundingBox.height < 0 || boundingBox.depth < 0) { + throw new Error('BoundingBoxObject dimensions must be non-negative'); + } return new BoundingBox({ min: boundingBox.topLeft, max: V3.add(boundingBox.topLeft, [boundingBox.width, boundingBox.height, boundingBox.depth]), }); }frontend/javascripts/libs/mjs.ts (1)
223-226: Consider adding type safety to the identity matrix.The implementation is correct, but consider adding type checking to ensure the returned identity matrix matches the expected
Matrix4x4type.identity(): Matrix4x4 { - return BareM4x4.identity; + const identityMatrix = BareM4x4.identity; + if (!(identityMatrix instanceof Float32Array) && !Array.isArray(identityMatrix)) { + throw new Error('Invalid identity matrix type'); + } + return identityMatrix; }frontend/javascripts/oxalis/constants.ts (1)
18-19: LGTM! Consider enhancing the type documentation.The
NestedMatrix4type addition is well-structured and appropriate for handling affine transformations.Consider enhancing the documentation to include:
- Usage examples
- Relationship with affine transformations
- Expected matrix structure (e.g., rotation, translation components)
-export type NestedMatrix4 = [Vector4, Vector4, Vector4, Vector4]; // Represents a row major matrix. +/** + * Represents a 4x4 row-major matrix used for affine transformations. + * Structure: + * [ + * [m11, m12, m13, m14], // First row: rotation + scale + * [m21, m22, m23, m24], // Second row: rotation + scale + * [m31, m32, m33, m34], // Third row: rotation + scale + * [m41, m42, m43, m44] // Fourth row: translation + perspective + * ] + */ +export type NestedMatrix4 = [Vector4, Vector4, Vector4, Vector4];frontend/javascripts/oxalis/model_initialization.ts (2)
Line range hint
481-508: Consider adding error handling for the coordinate transformation logic.While the conditional logic for applying transformations is sound, consider adding error handling for edge cases:
- When
originalLayersis empty- When the first layer's transformations are undefined
- When fallback layer's transformations are invalid
const allLayersSameRotation = doAllLayersHaveTheSameRotation(originalLayers); +if (originalLayers.length === 0) { + return {}; +} let coordinateTransformsMaybe = {}; if (allLayersSameRotation) { + if (!originalLayers[0]?.coordinateTransformations) { + console.warn('First layer transformations undefined, using identity matrix'); + return {}; + } coordinateTransformsMaybe = { coordinateTransformations: originalLayers?.[0].coordinateTransformations, }; } else if (fallbackLayer?.coordinateTransformations) { + try { coordinateTransformsMaybe = { coordinateTransformations: fallbackLayer.coordinateTransformations, }; + } catch (err) { + console.error('Failed to apply fallback layer transformations:', err); + return {}; + } }
856-881: Simplify the nativelyRenderedLayerName validation logic.The current nested conditions can be simplified for better readability while maintaining the same functionality.
- if (originalDatasetSettings.nativelyRenderedLayerName) { - const isNativelyRenderedNamePresent = - dataset.dataSource.dataLayers.some( - (layer) => - layer.name === originalDatasetSettings.nativelyRenderedLayerName || - (layer.category === "segmentation" && - layer.fallbackLayer === originalDatasetSettings.nativelyRenderedLayerName), - ) || - annotation?.annotationLayers.some( - (layer) => layer.name === originalDatasetSettings.nativelyRenderedLayerName, - ); - if (!isNativelyRenderedNamePresent) { - initialDatasetSettings.nativelyRenderedLayerName = null; - } - } + const { nativelyRenderedLayerName } = originalDatasetSettings; + if (!nativelyRenderedLayerName) { + return initialDatasetSettings; + } + + const isLayerInDataset = dataset.dataSource.dataLayers.some( + (layer) => + layer.name === nativelyRenderedLayerName || + (layer.category === "segmentation" && layer.fallbackLayer === nativelyRenderedLayerName) + ); + + const isLayerInAnnotation = annotation?.annotationLayers.some( + (layer) => layer.name === nativelyRenderedLayerName + ); + + if (!isLayerInDataset && !isLayerInAnnotation) { + initialDatasetSettings.nativelyRenderedLayerName = null; + }frontend/javascripts/test/reducers/flycam_reducer.spec.ts (1)
39-39: Consider adding tests for dataset layer rotation interactionsGiven that this PR introduces dataset layer rotation features, consider adding test cases that verify:
- Flycam behavior when interacting with rotated dataset layers
- Transformation matrix calculations with layer rotation applied
- Edge cases when switching between rotated and non-rotated views
Would you like me to help draft these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(3 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(3 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx(1 hunks)frontend/javascripts/libs/mjs.ts(1 hunks)frontend/javascripts/oxalis/api/api_latest.ts(1 hunks)frontend/javascripts/oxalis/constants.ts(1 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts(1 hunks)frontend/javascripts/oxalis/geometries/materials/edge_shader.ts(1 hunks)frontend/javascripts/oxalis/geometries/materials/node_shader.ts(1 hunks)frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts(2 hunks)frontend/javascripts/oxalis/merger_mode.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts(2 hunks)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts(3 hunks)frontend/javascripts/oxalis/model/accessors/tool_accessor.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/nml_helpers.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts(2 hunks)frontend/javascripts/oxalis/model/reducers/flycam_reducer.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/dataset_saga.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/quick_select_heuristic_saga.ts(1 hunks)frontend/javascripts/oxalis/model_initialization.ts(5 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(7 hunks)frontend/javascripts/test/reducers/flycam_reducer.spec.ts(1 hunks)frontend/javascripts/types/api_flow_types.ts(3 hunks)frontend/javascripts/types/globals.d.ts(1 hunks)frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- frontend/javascripts/oxalis/model/helpers/nml_helpers.ts
- frontend/javascripts/oxalis/store.ts
- frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx
- frontend/javascripts/oxalis/api/api_latest.ts
🔇 Additional comments (30)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (2)
72-105:
Add 'dataLayers' and 'datasetBoundingBox' to useCallback dependency array
The setMatrixRotationsForAllLayer function uses dataLayers and datasetBoundingBox obtained from form.getFieldValue and getDatasetBoundingBoxFromLayers(dataLayers). To ensure the callback updates correctly when these values change, include them in the dependency array.
Apply this diff to update the dependencies:
}, [
axis,
form,
+ dataLayers,
+ datasetBoundingBox,
]);Likely invalid or redundant comment.
47-70:
Include 'rotationValues' in useEffect dependency array
The rotationValues variable is derived from form.getFieldValue(["datasetRotation"]) and is used within the useEffect hook. To ensure that the effect runs whenever rotationValues change, consider including rotationValues in the dependency array.
Apply this diff to include rotationValues in the dependency array:
}, [
datasetBoundingBox,
dataLayers,
form,
+ rotationValues,
]);Likely invalid or redundant comment.
frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx (1)
Line range hint 223-225: Check for empty 'layersWithTransforms' before proceeding
After the withTransforms function, ensure that layersWithTransforms is not empty before proceeding to prevent potential runtime errors.
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (1)
Line range hint 1185-1235: Ensure consistent rendering of 'TransformationIcon' in 'getSkeletonLayer'
When rendering the TransformationIcon within the getSkeletonLayer method, verify that the layer prop is correctly passed and that the skeleton layer supports transformations if applicable.
frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts (1)
152-154:
Add validation constraints for nativelyRenderedLayerName
The schema allows any string value for nativelyRenderedLayerName without validation. This could lead to runtime errors if invalid layer names are provided.
Consider adding these constraints:
nativelyRenderedLayerName: {
type: "string",
+ minLength: 1,
+ pattern: "^[a-zA-Z0-9_-]+$"
},Let's verify the layer name usage:
frontend/javascripts/oxalis/geometries/materials/edge_shader.ts (1)
14-14: Verify transformation consistency after module reorganization
The getTransformsForSkeletonLayer import has been moved to a new module. While the change improves code organization, we need to ensure transformation calculations remain consistent.
Let's verify the transformation logic:
✅ Verification successful
Based on the search results, I can now generate the final response:
Import change for getTransformsForSkeletonLayer is safe and consistent
The function is used consistently across the codebase:
- In edge_shader.ts and node_shader.ts for shader transformations
- In nml_helpers.ts for NML transformations
- In skeletontracing_accessor.ts for node position transformations
All usages correctly handle the transformation matrix and follow the same pattern of accessing the dataset and natively rendered layer name from the state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of getTransformsForSkeletonLayer to ensure consistent behavior
rg "getTransformsForSkeletonLayer" -A 5
# Look for related transformation functions
ast-grep --pattern 'function getTransformsFor$_($_, $_) {
$$$
}'
Length of output: 7571
frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts (1)
5-5: LGTM: Type imports and function signature changes enhance type safety.
The changes improve type safety by introducing the NestedMatrix4 type and updating function signatures accordingly.
Also applies to: 7-9
frontend/javascripts/oxalis/model/sagas/dataset_saga.ts (1)
15-18: LGTM: Import restructuring improves code organization.
Moving transformation-related functions to a dedicated accessor module enhances code organization and maintainability.
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts (1)
8-8: LGTM: New type import.
The import of BoundingBoxObject type is appropriate for the new functionality.
frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts (2)
26-30: LGTM: Import changes align with new transformation module
The imports have been updated to use the new transformation helpers and layer rotation accessor, improving code organization.
221-224: Verify transformation changes across the codebase
The refactoring of transformation logic looks correct, but we should verify its usage across the codebase.
Also applies to: 234-236, 241-243
✅ Verification successful
Let me gather more information about the transformation functions to ensure completeness of the verification.
Transformation functions are correctly used across the codebase
The verification shows that:
getTransformsForLayerWithoutTransformationConfigOrNullis used consistently inskeletontracing_accessor.tsanddataset_layer_rotation_accessor.tsgetTransformsForSkeletonLayeris used in shader-related files and NML helpers- All transformation function calls follow the same parameter pattern (dataset and nativelyRenderedLayerName)
- No references to deprecated transformation functions were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to old transformation functions
rg -l "getTransformsForSkeletonLayerOrNull"
# Verify all transformation function calls
ast-grep --pattern 'getTransformsForSkeletonLayer($_, $_)'
Length of output: 1980
Script:
#!/bin/bash
# Check for all transformation-related function calls
ast-grep --pattern 'getTransformsForLayerWithoutTransformationConfigOrNull($_, $_)'
# Search for any other transformation-related functions
rg -l "getTransformsFor.*Layer"
Length of output: 2028
frontend/javascripts/oxalis/model/reducers/flycam_reducer.ts (1)
105-105: LGTM: Fixed matrix initialization
The change from M4x4.identity to M4x4.identity() ensures a new matrix instance is created, preventing potential shared reference issues.
frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts (1)
33-36: LGTM! Clean refactoring of transformation functions.
The relocation of transformation-related functions to a dedicated accessor module improves code organization and maintainability.
frontend/javascripts/oxalis/geometries/materials/node_shader.ts (1)
17-17: Verify skeleton layer transformations.
The import change aligns with the refactoring, but given the PR's mention of skeleton layer compatibility with transformations, we should verify that all skeleton layer transformations work correctly.
✅ Verification successful
Let me gather more information about the transformation usage to better understand the impact.
Based on the search results, I can now generate the final response:
Import change for skeleton layer transformations is correctly implemented
The getTransformsForSkeletonLayer function is properly used across the codebase:
- Used in shader materials (node_shader.ts, edge_shader.ts) for rendering transformations
- Integrated with NML helpers and skeleton tracing accessors
- Proper error handling exists for transformed skeleton layers in proofreading functionality
The import change aligns with the existing usage patterns and maintains the expected transformation behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for skeleton layer transformation usage and potential issues.
# Search for skeleton layer transformation tests
rg -A 5 "test.*skeleton.*transform|test.*transform.*skeleton"
# Search for skeleton layer transformation error handling
rg -A 5 "catch.*skeleton.*transform|error.*skeleton.*transform"
# Search for skeleton layer transformation edge cases
ast-grep --pattern $'getTransformsForSkeletonLayer($_, $_) {
$$$
if ($cond) {
$$$
}
$$$
}'
Length of output: 1419
Script:
#!/bin/bash
# Search for usages of getTransformsForSkeletonLayer
rg -A 5 "getTransformsForSkeletonLayer"
# Search for skeleton layer transformation related code
ast-grep --pattern 'class $_ {
$$$
transform($_, $_) {
$$$
}
$$$
}'
# Check for any other transformation related imports
rg "import.*transform.*dataset_layer_rotation"
Length of output: 7633
frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (1)
10-11: LGTM! Verify refactoring impact.
The import changes appropriately reflect the reorganization of transformation-related functions into a dedicated accessor module, which aligns with the PR's objective of supporting dataset layer rotations.
✅ Verification successful
Import changes are correctly refactored across the codebase
The verification shows that the transformation-related functions have been consistently moved to dataset_layer_rotation_accessor. There are no remaining imports from the old location in dataset_accessor, and the new import pattern is used consistently across 11 different files in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all transformation-related imports are consistently updated across the codebase
# Check for any remaining old imports
rg "import.*getTransformsPerLayer.*dataset_accessor"
# Verify the new import pattern is used consistently
rg "import.*dataset_layer_rotation_accessor"
Length of output: 1818
frontend/javascripts/oxalis/merger_mode.ts (1)
10-11: LGTM! Verify transformation handling in merger mode.
The import changes ensure proper handling of layer transformations in merger mode, maintaining correct coordinate mapping through inverse transformations.
✅ Verification successful
LGTM! Transformation handling in merger mode is correctly implemented.
The code shows proper usage of the imported transformation functions:
getInverseSegmentationTransformeris used in two places (lines 203 and 370) to correctly transform node positions before looking up segment IDs- The transformation flow maintains coordinate mapping integrity:
- Node positions are first transformed to rendered coordinates
- Inverse segmentation transform is applied to get the correct lookup position
- Segment IDs are retrieved using the transformed coordinates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that merger mode correctly handles transformed layers
# Check for proper transformation handling in merger mode
rg -A 5 "getInverseSegmentationTransformer.*merger_mode"
# Verify no direct access to transformation functions from dataset_accessor
rg "import.*getTransformsPerLayer.*dataset_accessor.*merger_mode"
Length of output: 126
Script:
#!/bin/bash
# Let's search for usage of the imported functions in merger_mode.ts
ast-grep --pattern 'getInverseSegmentationTransformer($$$)'
# Also check for any usage of getVisibleSegmentationLayer
ast-grep --pattern 'getVisibleSegmentationLayer($$$)'
# Let's see the full content of merger_mode.ts to understand the context better
cat frontend/javascripts/oxalis/merger_mode.ts
Length of output: 20557
frontend/javascripts/oxalis/controller/scene_controller.ts (3)
49-49: LGTM: Clean import addition
The import of getTransformsForLayerOrNull is well-placed and follows the module structure.
Line range hint 408-411: LGTM: Improved null check for dataLayers
The null check for dataLayers has been improved to prevent potential runtime errors when filtering layer names.
Line range hint 203-225: LGTM: Well-implemented transformation logic
The transformation application logic is well-structured:
- Correctly retrieves transformations for each layer
- Properly applies the affine transformation matrix using Three.js Matrix4
- Maintains the layer visibility state
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
36-36: LGTM: Clean import addition
The import of AxisRotationSettingForDataset is well-placed and follows the module structure.
271-276: LGTM: Well-structured UI layout
The rotation settings UI is well-integrated:
- Follows Ant Design's Row/Col pattern for responsive layout
- Appropriate placement within the form structure
- Clear separation of concerns with dedicated column for rotation settings
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (3)
46-50: LGTM: Clean type imports and declarations
The imports and type declarations are well-organized and properly documented.
84-84: LGTM: Clean type extension
The FormData type is appropriately extended with the optional datasetRotation field.
203-225: LGTM: Well-implemented rotation settings initialization
The rotation settings initialization logic is robust:
- Properly checks for consistent rotation across layers
- Handles missing transformations gracefully with default values
- Correctly extracts rotation values from transformation matrices
- Clear comments explaining the transformation sequence
frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts (1)
196-196: LGTM! Proper initialization of identity matrix.
The change correctly ensures that M4x4.identity() is called as a function rather than accessing it as a property.
frontend/javascripts/oxalis/model/sagas/quick_select_heuristic_saga.ts (1)
61-61: LGTM! Improved module organization.
The import has been correctly updated to use the new dedicated dataset_layer_rotation_accessor module, which better organizes transformation-related functions.
frontend/javascripts/types/api_flow_types.ts (2)
63-73: Well-structured type definitions for transformations.
The separation of transformation types and explicit documentation of matrix storage order (row-major) improves type safety and prevents potential matrix operation errors.
99-100: Clear documentation for APISkeletonLayer name property.
The comment effectively explains that the name should be the skeleton tracing ID to ensure uniqueness.
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts (1)
53-57: Good modularization of transformation-related functions.
Moving transformation functions to a dedicated accessor module improves code organization and maintainability.
frontend/javascripts/test/reducers/flycam_reducer.spec.ts (1)
39-39: Good change: Ensures proper matrix initialization
The change from M4x4.identity to M4x4.identity() ensures each test gets a fresh identity matrix instance, preventing potential mutation issues between tests.
| <Row gutter={24}> | ||
| <Col span={16}> | ||
| <FormItemWithInfo | ||
| name={["datasetRotation", axis]} | ||
| label={`${axis.toUpperCase()} Axis Rotation`} | ||
| info={`Change the datasets rotation around the ${axis}-axis.`} | ||
| colon={false} | ||
| > | ||
| <Slider min={0} max={270} step={90} onChange={setMatrixRotationsForAllLayer} /> | ||
| </FormItemWithInfo> | ||
| </Col> | ||
| <Col span={8} style={{ marginRight: -12 }}> | ||
| <FormItem | ||
| name={["datasetRotation", axis]} | ||
| colon={false} | ||
| label=" " /* Whitespace label is needed for correct formatting*/ | ||
| > | ||
| <InputNumber | ||
| min={0} | ||
| max={270} | ||
| step={90} | ||
| precision={0} | ||
| onChange={(value: number | null) => value && setMatrixRotationsForAllLayer(value)} | ||
| /> | ||
| </FormItem> | ||
| </Col> | ||
| </Row> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Handle null 'value' in InputNumber onChange handler
In the InputNumber component, the onChange handler checks value && setMatrixRotationsForAllLayer(value), which will ignore 0 since 0 is a falsy value. Use a more explicit null check to ensure that zero values are handled correctly.
Apply this diff to fix the condition:
<InputNumber
min={0}
max={270}
step={90}
precision={0}
- onChange={(value: number | null) => value && setMatrixRotationsForAllLayer(value)}
+ onChange={(value: number | null) => {
+ if (value !== null) {
+ setMatrixRotationsForAllLayer(value);
+ }
+ }}
/>📝 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.
| <Row gutter={24}> | |
| <Col span={16}> | |
| <FormItemWithInfo | |
| name={["datasetRotation", axis]} | |
| label={`${axis.toUpperCase()} Axis Rotation`} | |
| info={`Change the datasets rotation around the ${axis}-axis.`} | |
| colon={false} | |
| > | |
| <Slider min={0} max={270} step={90} onChange={setMatrixRotationsForAllLayer} /> | |
| </FormItemWithInfo> | |
| </Col> | |
| <Col span={8} style={{ marginRight: -12 }}> | |
| <FormItem | |
| name={["datasetRotation", axis]} | |
| colon={false} | |
| label=" " /* Whitespace label is needed for correct formatting*/ | |
| > | |
| <InputNumber | |
| min={0} | |
| max={270} | |
| step={90} | |
| precision={0} | |
| onChange={(value: number | null) => value && setMatrixRotationsForAllLayer(value)} | |
| /> | |
| </FormItem> | |
| </Col> | |
| </Row> | |
| ); | |
| }; | |
| <Row gutter={24}> | |
| <Col span={16}> | |
| <FormItemWithInfo | |
| name={["datasetRotation", axis]} | |
| label={`${axis.toUpperCase()} Axis Rotation`} | |
| info={`Change the datasets rotation around the ${axis}-axis.`} | |
| colon={false} | |
| > | |
| <Slider min={0} max={270} step={90} onChange={setMatrixRotationsForAllLayer} /> | |
| </FormItemWithInfo> | |
| </Col> | |
| <Col span={8} style={{ marginRight: -12 }}> | |
| <FormItem | |
| name={["datasetRotation", axis]} | |
| colon={false} | |
| label=" " /* Whitespace label is needed for correct formatting*/ | |
| > | |
| <InputNumber | |
| min={0} | |
| max={270} | |
| step={90} | |
| precision={0} | |
| onChange={(value: number | null) => { | |
| if (value !== null) { | |
| setMatrixRotationsForAllLayer(value); | |
| } | |
| }} | |
| /> | |
| </FormItem> | |
| </Col> | |
| </Row> | |
| ); | |
| }; |
| const matrix = transformation ? transformation.matrix : IDENTITY_MATRIX; | ||
| const cosineLocation = cosineLocationOfRotationInMatrix[axis]; | ||
| const sinusLocation = sinusLocationOfRotationInMatrix[axis]; | ||
| const sinOfAngle = matrix[sinusLocation[0]][sinusLocation[1]]; | ||
| const cosOfAngle = matrix[cosineLocation[0]][cosineLocation[1]]; | ||
| const rotation = | ||
| Math.abs(cosOfAngle) > 1e-6 // Avoid division by zero | ||
| ? Math.atan2(sinOfAngle, cosOfAngle) | ||
| : sinOfAngle > 0 | ||
| ? Math.PI / 2 | ||
| : -Math.PI / 2; | ||
| const rotationInDegrees = rotation * (180 / Math.PI); | ||
| // Round to multiple of 90 degrees and keep the result positive. | ||
| const roundedRotation = mod(Math.round((rotationInDegrees + 360) / 90) * 90, 360); | ||
| return roundedRotation; | ||
| } |
There was a problem hiding this comment.
Handle division by zero in 'getRotationFromTransformation'
The function getRotationFromTransformation includes a check to avoid division by zero when cosOfAngle is close to zero. However, the condition Math.abs(cosOfAngle) > 1e-6 might not be sufficient. Consider using a stricter threshold or handling the edge case more robustly.
frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
@philippotto This PR is ready for the first review round. To you have time to take on this task or should I ask daniel? Because I think you are already packed with the Unified Annotation Versioning but as you built the coordinateTransforms & multi modality feature you might be a little more suited for the review 🤔
| return null; | ||
| } | ||
|
|
||
| // The skeleton layer needs transformed just like the other layers. Thus, we simply use the first usable layer. | ||
| // Filtering for a layer that might actually have transforms prevents an infinite loop | ||
| // between cyclic calls of _getTransformsForLayerWithoutTransformationConfigOrNull and getTransformsForLayerOrNull. | ||
| const usableReferenceLayer = layers.find( | ||
| (layer) => | ||
| layer.category === "color" || (layer.category === "segmentation" && layer.fallbackLayer), | ||
| ); | ||
| const someLayersTransformsMaybe = usableReferenceLayer | ||
| ? getTransformsForLayerOrNull(dataset, usableReferenceLayer, nativelyRenderedLayerName) | ||
| : null; | ||
| return someLayersTransformsMaybe; |
There was a problem hiding this comment.
The filtering here avoids a potential infinite loop in the following scenario:
In the scenario the user has an annotation with no color layers and just two volume layers with fallback segmentation. For this DS a rotation is defined.
Now in case the user detaches the first volume layer from its segmentation layer, the layer has no transforms anymore. => This _getTransformsForLayerWithoutTransformationConfigOrNull function will be called for this layer without the fallback segmentation and thus no transformation.
=> If we would not filter for a suitable layer and instead take e.g. the first layer via dataset.dataSource.layers[0] we would get the same layer again and then call getTransformsForLayerOrNull which then automatically calls _getTransformsForLayerWithoutTransformationConfigOrNull again, because the layer fallback segmentation. And thus would have a cyclic call in this scenario.
Therefore, I think the filter should avoid a potential infinite loop.
frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Show resolved
Hide resolved
| let coordinateTransformsMaybe = {}; | ||
| if (doAllLayersHaveTheSameRotation) { | ||
| coordinateTransformsMaybe = { | ||
| coordinateTransformations: originalLayers?.[0].coordinateTransformations, | ||
| }; | ||
| } else if (fallbackLayer?.coordinateTransformations) { | ||
| coordinateTransformsMaybe = { | ||
| coordinateTransformations: fallbackLayer.coordinateTransformations, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Here transfer the coordinateTransforms from the fallback layer to its volume annotation layer in order to not loose these settings
Yes, I can review it :) |
| @@ -94,8 +96,8 @@ export type APISegmentationLayer = APIDataLayerBase & { | |||
| export type APIDataLayer = APIColorLayer | APISegmentationLayer; | |||
|
|
|||
| // Only used in rare cases to generalize over actual data layers and | |||
| // a skeleton layer. | |||
| export type APISkeletonLayer = { category: "skeleton" }; | |||
| // a skeleton layer. The name should be the skeleton tracing id to very likely ensure it is unique. | |||
There was a problem hiding this comment.
why only very likely? because a dataset layer name could be equal to the skeleton id? but then the category wouldn't be right, would it?
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
Show resolved
Hide resolved
| // so that the user will still look at the same data location. | ||
| // Get transform of layer. null is passed as nativelyRenderedLayerName to | ||
| // get the layers transform even in case the is currently rendered natively. | ||
| const layersTransforms = getTransformsForLayer(state.dataset, layer, null); |
There was a problem hiding this comment.
on master, there is _getOriginalTransformsForLayerOrNull. can that be reused? I think, I like it better to call a method which uses "original transforms" (or a similar term) compared to passing null for the last arg.
| const transformWhichWillBeApplied = !isRenderedNatively | ||
| ? invertTransform(layersTransforms) | ||
| : layersTransforms; | ||
|
|
||
| const currentPosition = getPosition(state.flycam); | ||
| const currentTransforms = getTransformsForLayer( | ||
| state.dataset, | ||
| layer, | ||
| state.datasetConfiguration.nativelyRenderedLayerName, | ||
| ); | ||
| const invertedTransform = invertTransform(currentTransforms); | ||
| const newPosition = transformPointUnscaled(invertedTransform)(currentPosition); | ||
| const newPosition = transformPointUnscaled(transformWhichWillBeApplied)(currentPosition); |
There was a problem hiding this comment.
I'm a bit confused. Here's my understanding of the old and the new behavior (I might be wrong, of course).
Setup:
- DS has two layers. L1 has no transforms. L2 has a translation transform that moves the entire layer by 1000vx in X (let's call that T).
- I open the DS and select that L2 is rendered natively. Therefore, L1 will be rendered with an -1000vx offset in X (T_inv).
- the current position is at 0,0,0
- Now I select that L1 should be rendered natively ->
layerwill refer to L1
Old behavior:
- currentTransforms = T_inv
- invertedTransforms = T
- newPosition = 1000,0,0
New behavior:
- layersTransforms = identity (because L1 has no transforms in DS)
- transformWhichWillBeApplied = identity
- newPosition = 0,0,0
the new behavior seems wrong to me? am I missing something?
| } | ||
|
|
||
| export function isIdentityTransform(transform: Transform) { | ||
| return transform.type === "affine" && _.isEqual(transform.affineMatrix, Identity4x4); |
There was a problem hiding this comment.
we don't need to use Number.EPSILON when comparing affineMatrix to the identity matrix?
frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts
Show resolved
Hide resolved
| /* This function checks if all layers have the same transformation settings that represent | ||
| * a translation to the dataset center and a rotation around each axis and a translation back. | ||
| * All together this makes 5 affine transformation matrices. */ | ||
| function _doAllLayersHaveTheSameRotation(dataLayers: Array<APIDataLayer>): boolean { |
There was a problem hiding this comment.
maybe we can discuss this in person. I don't understand how this fits into the bigger picture yet.
…ow-perma-dataset-rotation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (1)
257-273: Consider improving variable names and documentationWhile the transformation logic is correct, readability could be improved:
- Variable
layersTransformscould be renamed tooriginalLayerTransformto better reflect its purpose- Consider adding a comment explaining why the transform needs to be inverted when not rendered natively
- const layersTransforms = - getOriginalTransformsForLayerOrNull(state.dataset, layer) || IdentityTransform; + // Get the layer's original transformation matrix, defaulting to identity if none exists + const originalLayerTransform = + getOriginalTransformsForLayerOrNull(state.dataset, layer) || IdentityTransform; - const transformWhichWillBeApplied = !isRenderedNatively - ? invertTransform(layersTransforms) - : layersTransforms; + // When switching from transformed to native rendering, we need to apply the inverse transform + // to maintain the correct view position + const transformWhichWillBeApplied = !isRenderedNatively + ? invertTransform(originalLayerTransform) + : originalLayerTransform;CHANGELOG.unreleased.md (1)
14-14: Consider enhancing the changelog entry with more details.The changelog entry could be more informative by mentioning that rotations are limited to 90-degree steps and any specific limitations or requirements (e.g., all layers must have the same rotation).
-Added the possibility to configure a rotation for a dataset which can be toggled off and on when viewing and annotating data. [#8159](https://github.com/scalableminds/webknossos/pull/8159) +Added the possibility to configure dataset rotation in 90-degree steps around each axis, which can be toggled off and on when viewing and annotating data. Note: All layers must have the same rotation settings. [#8159](https://github.com/scalableminds/webknossos/pull/8159)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)
47-70: Consider adding useCallback for the useEffect dependency.The useEffect hook depends on the form instance which could lead to unnecessary re-renders. Consider memoizing the form operations.
+const updateTransformations = useCallback(( + datasetBoundingBox: BoundingBox, + dataLayers: APIDataLayer[], + form: FormInstance, +) => { + const rotationValues = form.getFieldValue(["datasetRotation"]); + const transformations = [ + fromCenterToOrigin(datasetBoundingBox), + getRotationMatrixAroundAxis("x", rotationValues["x"]), + getRotationMatrixAroundAxis("y", rotationValues["y"]), + getRotationMatrixAroundAxis("z", rotationValues["z"]), + fromOriginToCenter(datasetBoundingBox), + ]; + const dataLayersWithUpdatedTransforms = dataLayers.map((layer) => ({ + ...layer, + coordinateTransformations: transformations, + })); + form.setFieldValue(["dataSource", "dataLayers"], dataLayersWithUpdatedTransforms); +}, []); useEffect(() => { if ( datasetBoundingBox == null || dataLayers[0].coordinateTransformations?.length !== 5 || !form ) { return; } - const rotationValues = form.getFieldValue(["datasetRotation"]); - const transformations = [ - fromCenterToOrigin(datasetBoundingBox), - getRotationMatrixAroundAxis("x", rotationValues["x"]), - getRotationMatrixAroundAxis("y", rotationValues["y"]), - getRotationMatrixAroundAxis("z", rotationValues["z"]), - fromOriginToCenter(datasetBoundingBox), - ]; - const dataLayersWithUpdatedTransforms = dataLayers.map((layer) => ({ - ...layer, - coordinateTransformations: transformations, - })); - form.setFieldValue(["dataSource", "dataLayers"], dataLayersWithUpdatedTransforms); + updateTransformations(datasetBoundingBox, dataLayers, form); }, [datasetBoundingBox, dataLayers, form, updateTransformations]);
159-171: Consider extracting tooltip content to a constant.The tooltip content is quite long and could be reused. Consider extracting it to a constant for better maintainability.
+const ROTATION_REQUIREMENTS_TOOLTIP = ( + <div> + Each layers transformations must be equal and each layer needs exactly 5 affine + transformation with the following schema: + <ul> + <li>Translation to the origin</li> + <li>Rotation around the x-axis</li> + <li>Rotation around the y-axis</li> + <li>Rotation around the z-axis</li> + <li>Translation back to the original position</li> + </ul> + To easily enable this setting, delete all coordinateTransformations of all layers in the + advanced tab, save and reload the dataset settings. + </div> +); <Tooltip - title={ - <div> - Each layers transformations must be equal and each layer needs exactly 5 affine - transformation with the following schema: - <ul> - <li>Translation to the origin</li> - <li>Rotation around the x-axis</li> - <li>Rotation around the y-axis</li> - <li>Rotation around the z-axis</li> - <li>Translation back to the original position</li> - </ul> - To easily enable this setting, delete all coordinateTransformations of all layers in the - advanced tab, save and reload the dataset settings. - </div> - } + title={ROTATION_REQUIREMENTS_TOOLTIP}
174-177: Improve the error message clarity.The current error message could be more user-friendly by providing clearer instructions.
-Setting a dataset's rotation is only supported when all layers have the same rotation -transformation. <InfoCircleOutlined /> +Dataset rotation requires consistent transformation settings across all layers. Please ensure all layers +have matching rotation configurations before proceeding. <InfoCircleOutlined />frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
203-225: Consider architectural implications of rotation implementationBased on the PR objectives, there are several important architectural considerations:
- The interface being "under design" suggests we should make the rotation implementation flexible for future UI changes
- The transformation sequence (translation to origin → rotate → translate back) correctly handles rotation around dataset's center
- Using fixed 90-degree steps mitigates the non-deterministic nature of Euler angles, but this constraint should be documented
Consider:
- Adding an interface abstraction layer to decouple rotation logic from UI
- Documenting the 90-degree step constraint in the API documentation
- Adding unit tests to verify rotation behavior around dataset's center
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(3 hunks)frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts(3 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/oxalis/store.ts
- frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
- frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts
🔇 Additional comments (10)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (5)
87-87: LGTM: New imports support rotation functionality
The added imports from dataset layer rotation accessor and transformation helpers modules appropriately support the new rotation functionality requirements.
Also applies to: 127-138
228-230: LGTM: Robust validation for transformation toggles
The implementation correctly prevents invalid transformation states by:
- Checking if all layers have the same rotation
- Preventing toggling on layers that cannot have transformations (skeleton layers and segmentation layers without fallback)
- Preventing toggling when it would affect other layers' coordinate systems
Also applies to: 234-234, 247-254
295-302: LGTM: Clear visual feedback for interaction states
The styling changes appropriately communicate:
- Interactive state through cursor styles
- Disabled state through opacity
308-312: LGTM: Clear user feedback for transformation states
The tooltip text and image source changes effectively communicate:
- Whether the layer is shown natively or with transformations
- The type of transformation being applied
- Whether the transformation can be toggled
Also applies to: 316-319
1189-1189: LGTM: Proper skeleton layer identification
Added tracingId to skeleton layer object to ensure proper transformation handling.
Also applies to: 1239-1239
frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts (2)
83-87:
Enhance division by zero handling.
The current threshold for avoiding division by zero might not be sufficient in all cases. Consider using a more robust approach.
- Math.abs(cosOfAngle) > 1e-6 // Avoid division by zero
+ Math.abs(cosOfAngle) > Number.EPSILON * 100 // Use a more robust threshold for floating-point comparisonLikely invalid or redundant comment.
335-337:
Optimize memoization strategy.
The current memoization using _.memoize might lead to memory leaks as it doesn't have a cache size limit. Consider using a more robust memoization approach.
-export const invertAndTranspose = _.memoize((mat: Matrix4x4) => {
- return M4x4.transpose(M4x4.inverse(mat));
-});
+const memoizedInvertAndTranspose = new Map<string, Matrix4x4>();
+const MAX_CACHE_SIZE = 1000;
+
+export const invertAndTranspose = (mat: Matrix4x4) => {
+ const key = JSON.stringify(mat);
+ if (!memoizedInvertAndTranspose.has(key)) {
+ if (memoizedInvertAndTranspose.size >= MAX_CACHE_SIZE) {
+ // Clear the oldest entry if cache is full
+ const firstKey = memoizedInvertAndTranspose.keys().next().value;
+ memoizedInvertAndTranspose.delete(firstKey);
+ }
+ memoizedInvertAndTranspose.set(key, M4x4.transpose(M4x4.inverse(mat)));
+ }
+ return memoizedInvertAndTranspose.get(key)!;
+};Likely invalid or redundant comment.
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (3)
46-50: LGTM! Well-organized imports with good separation of concerns.
The rotation-related functionality is properly modularized in a dedicated accessor module, following good architectural practices.
84-84: Verify the DatasetRotation type definition
The optional datasetRotation field is correctly added. Let's verify the type definition to ensure it properly constrains rotation values.
✅ Verification successful
DatasetRotation type is well-defined with proper constraints
The type definition is found in dataset_rotation_form_item.tsx and correctly defines the expected rotation properties:
- x: number
- y: number
- z: number
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the DatasetRotation type definition and its usage
# Expected: Type should define x, y, z rotation properties with appropriate constraints
ast-grep --pattern 'type DatasetRotation = {
$$$
}'
# Also check for any validation logic
rg -A 5 'DatasetRotation.*validation'
Length of output: 539
203-225: 🛠️ Refactor suggestion
Improve rotation initialization robustness and documentation
The rotation initialization logic has several areas for improvement:
- The magic number
5in the transformation length check needs explanation - Missing validation for rotation values
- The transformation sequence (origin translation → rotations → position restoration) needs better documentation
- No error handling for malformed transformation data
Consider these improvements:
// Retrieve the initial dataset rotation settings from the data source config.
if (doAllLayersHaveTheSameRotation(dataSource.dataLayers)) {
const firstLayerTransformations = dataSource.dataLayers[0].coordinateTransformations;
+ // Expected transformation sequence:
+ // 1. Translation to origin
+ // 2-4. Rotations around x, y, z axes
+ // 5. Translation back to original position
+ const EXPECTED_TRANSFORMATION_COUNT = 5;
let initialDatasetRotationSettings: DatasetRotation;
- if (!firstLayerTransformations || firstLayerTransformations.length !== 5) {
+ if (!firstLayerTransformations || firstLayerTransformations.length !== EXPECTED_TRANSFORMATION_COUNT) {
initialDatasetRotationSettings = {
x: 0,
y: 0,
z: 0,
};
} else {
+ try {
initialDatasetRotationSettings = {
- // First transformation is a translation to the coordinate system origin.
x: getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[1], "x"),
y: getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[2], "y"),
z: getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[3], "z"),
- // Fifth transformation is a translation back to the original position.
};
+ } catch (error) {
+ console.error("Failed to parse rotation transformations:", error);
+ initialDatasetRotationSettings = { x: 0, y: 0, z: 0 };
+ }
}Let's verify the rotation value constraints:
| function _doAllLayersHaveTheSameRotation(dataLayers: Array<APIDataLayer>): boolean { | ||
| const firstDataLayerTransformations = dataLayers[0]?.coordinateTransformations; | ||
| if (firstDataLayerTransformations == null || firstDataLayerTransformations.length === 0) { | ||
| // No transformations in all layers compatible with setting a rotation for the whole dataset. | ||
| return dataLayers.every( | ||
| (layer) => | ||
| layer.coordinateTransformations == null || layer.coordinateTransformations.length === 0, | ||
| ); | ||
| } | ||
| // There should be a translation to the origin, one transformation for each axis and one translation back. => A total of 5 affine transformations. | ||
| if ( | ||
| dataLayers.some((layer) => layer.coordinateTransformations?.length !== 5) || | ||
| dataLayers.some((layer) => | ||
| layer.coordinateTransformations?.some((transformation) => transformation.type !== "affine"), | ||
| ) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( | ||
| !isTranslationOnly(firstDataLayerTransformations[0] as AffineTransformation) || | ||
| !isRotationOnly(firstDataLayerTransformations[1] as AffineTransformation) || | ||
| !isRotationOnly(firstDataLayerTransformations[2] as AffineTransformation) || | ||
| !isRotationOnly(firstDataLayerTransformations[3] as AffineTransformation) || | ||
| !isTranslationOnly(firstDataLayerTransformations[4] as AffineTransformation) | ||
| ) { | ||
| return false; | ||
| } | ||
| for (let i = 1; i < dataLayers.length; i++) { | ||
| const transformations = dataLayers[i].coordinateTransformations; | ||
| if ( | ||
| transformations == null || | ||
| // Not checking matrix 0 and 4 for equality as these are transformations depending on the layer's bounding box. | ||
| // The bounding box can be different for each layer. | ||
| !_.isEqual(transformations[1], firstDataLayerTransformations[1]) || | ||
| !_.isEqual(transformations[2], firstDataLayerTransformations[2]) || | ||
| !_.isEqual(transformations[3], firstDataLayerTransformations[3]) | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the complex validation function.
The _doAllLayersHaveTheSameRotation function is quite complex and could benefit from being broken down into smaller, more focused functions.
+function hasValidTransformationCount(dataLayers: Array<APIDataLayer>): boolean {
+ return !dataLayers.some((layer) => layer.coordinateTransformations?.length !== 5);
+}
+function hasOnlyAffineTransformations(dataLayers: Array<APIDataLayer>): boolean {
+ return !dataLayers.some((layer) =>
+ layer.coordinateTransformations?.some((transformation) => transformation.type !== "affine")
+ );
+}
+function hasValidTransformationPattern(transformations: CoordinateTransformation[]): boolean {
+ return (
+ isTranslationOnly(transformations[0] as AffineTransformation) &&
+ isRotationOnly(transformations[1] as AffineTransformation) &&
+ isRotationOnly(transformations[2] as AffineTransformation) &&
+ isRotationOnly(transformations[3] as AffineTransformation) &&
+ isTranslationOnly(transformations[4] as AffineTransformation)
+ );
+}
function _doAllLayersHaveTheSameRotation(dataLayers: Array<APIDataLayer>): boolean {
const firstDataLayerTransformations = dataLayers[0]?.coordinateTransformations;
if (firstDataLayerTransformations == null || firstDataLayerTransformations.length === 0) {
return dataLayers.every(
(layer) =>
layer.coordinateTransformations == null || layer.coordinateTransformations.length === 0,
);
}
- if (
- dataLayers.some((layer) => layer.coordinateTransformations?.length !== 5) ||
- dataLayers.some((layer) =>
- layer.coordinateTransformations?.some((transformation) => transformation.type !== "affine"),
- )
- ) {
+ if (!hasValidTransformationCount(dataLayers) || !hasOnlyAffineTransformations(dataLayers)) {
return false;
}
- if (
- !isTranslationOnly(firstDataLayerTransformations[0] as AffineTransformation) ||
- !isRotationOnly(firstDataLayerTransformations[1] as AffineTransformation) ||
- !isRotationOnly(firstDataLayerTransformations[2] as AffineTransformation) ||
- !isRotationOnly(firstDataLayerTransformations[3] as AffineTransformation) ||
- !isTranslationOnly(firstDataLayerTransformations[4] as AffineTransformation)
- ) {
+ if (!hasValidTransformationPattern(firstDataLayerTransformations)) {
return false;
}Committable suggestion skipped: line range outside the PR's diff.
This PR adds new settings to the dataset settings for each layer. An interface (still in design process) enables the user to set a affine transformation matrix by letting the user define angles for each axis around which the dataset should be rotated.
Steps to test:
nativelyRenderedLayerNamecannot be set to one of these layers. Moreover, the transforms cannot be toggled on on such layers as they do not have transformations.nativelyRenderedLayerNamesettings.nativelyRenderedLayerNameset to the tracingId of the volume layer of the first annotation. This does not exist in the newly opened other annotation. => The annotation should still open up (but with all layers transformed), as thenativelyRenderedLayerNamesetting should be automatically disregarded as the layer does not exist in this annotation.TODOs / Problems:
nativelyRenderedLayerNamein model init if this layer does not exist. (Might happen when switching annotations)Problems:
Issues:
(Please delete unneeded items, merge only when none are left open)