Allow permanent dataset layer rotation in dataset settings #8159
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to dataset layer transformations across multiple frontend files. The modifications focus on enhancing dataset rotation capabilities, reorganizing transformation-related functions, and improving the user interface for layer transformations. Key changes include creating a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
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?
There was a problem hiding this comment.
All yes, but the layer name is used to save which layer is currently rendered natively. This is not supported for the skeleton layer as the skeleton layer has no rotation settings.
But in case a volume layer is currently rendered natively and has the same name as the skeleton layer's tracing id, this would lead to unwanted behaviour in _getTransformsForLayerOrNull. The skeleton layer would be treated as the layer rendered natively and thus would not have transforms instead of transforming into the coordinate system of the volume layer
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.
There was a problem hiding this comment.
Yeah, awesome, thanks for the suggestion. The advantage of using getTransformsForLayer was that it could handle all kinda of layers. Also layers that do not support configuring a transformation (isLayerWithoutTransformationConfigSupport). But this should not happen as in that case the button is disabled. Therefore, I the code now uses getOriginalTransformsForLayerOrNull as suggested by you :)
| 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?
There was a problem hiding this comment.
In getRotationMatrixAroundAxis (which generates the problematic matrixes with very small values close to 0) I replace all very small values (Math.abs(value) < Number.EPSILON) to 0 to avoid the bug overall. This makes the saved matrix in the datasource-properties.json also easier to read.
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
Outdated
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.
…scussion & refactoring
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)
261-274: Consider refactoring transformation logicThe transformation logic is complex and could benefit from:
- Breaking down into smaller, focused functions
- Adding comments explaining the mathematical transformations
- Adding error handling for edge cases
Consider refactoring into a dedicated transformation handler:
+const calculateNewPosition = ( + currentPosition: Vector3, + layersTransforms: Transform, + isRenderedNatively: boolean +): Vector3 => { + const transformWhichWillBeApplied = !isRenderedNatively + ? invertTransform(layersTransforms) + : layersTransforms; + return transformPointUnscaled(transformWhichWillBeApplied)(currentPosition); +}; const toggleLayerTransforms = () => { if (layer.category === "skeleton") { return; } const state = Store.getState(); const layersTransforms = getOriginalTransformsForLayerOrNull(state.dataset, layer) || IdentityTransform; - const transformWhichWillBeApplied = !isRenderedNatively - ? invertTransform(layersTransforms) - : layersTransforms; - const newPosition = transformPointUnscaled(transformWhichWillBeApplied)(currentPosition); + const currentPosition = getPosition(state.flycam); + const newPosition = calculateNewPosition( + currentPosition, + layersTransforms, + isRenderedNatively + );frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts (5)
28-33: Consider moving the type assertion to the declarationThe type assertion can be more effectively placed at the declaration level for better type inference.
-const IDENTITY_MATRIX = [ +const IDENTITY_MATRIX: NestedMatrix4 = [ [1, 0, 0, 0], [0, 1, 0, 0], [0, 0, 1, 0], [0, 0, 0, 1], -] as NestedMatrix4; +];
83-84: Extract magic number to a named constantThe threshold
1e-6should be extracted to a named constant for better maintainability and clarity.+const COSINE_ANGLE_THRESHOLD = 1e-6; + const rotation = - Math.abs(cosOfAngle) > 1e-6 // Avoid division by zero + Math.abs(cosOfAngle) > COSINE_ANGLE_THRESHOLD // Avoid division by zero ? Math.atan2(sinOfAngle, cosOfAngle)
123-145: Add explicit return type annotations to memoization functionsAdding explicit return type annotations would improve type safety and code clarity.
-function memoizeWithThreeKeys<A, B, C, T>(fn: (a: A, b: B, c: C) => T) { +function memoizeWithThreeKeys<A, B, C, T>(fn: (a: A, b: B, c: C) => T): (a: A, b: B, c: C) => T { -function memoizeWithTwoKeys<A, B, T>(fn: (a: A, b: B) => T) { +function memoizeWithTwoKeys<A, B, T>(fn: (a: A, b: B) => T): (a: A, b: B) => T {
169-172: Enhance error logging with more contextThe error message could be more helpful by including the actual transform type that was encountered.
console.error( - "Data layer has defined a coordinate transform that is not affine or thin_plate_spline. This is currently not supported and ignored", + `Data layer has defined a coordinate transform of type "${type}" that is not supported (only affine or thin_plate_spline are supported). This transform will be ignored.`, );
336-338: Consider using a more appropriate memoization strategy for matrix operationsThe current use of
_.memoizemight lead to memory leaks as it doesn't have a cache size limit and uses reference equality. Consider using a custom memoization strategy with:
- A fixed cache size
- Deep equality comparison for matrices
- Cache eviction policy
-export const invertAndTranspose = _.memoize((mat: Matrix4x4) => { +const matrixCache = new LRUCache<string, Matrix4x4>({ max: 100 }); +export const invertAndTranspose = (mat: Matrix4x4) => { + const key = JSON.stringify(mat); + let result = matrixCache.get(key); + if (!result) { + result = M4x4.transpose(M4x4.inverse(mat)); + matrixCache.set(key, result); + } + return result; -}); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/model/accessors/dataset_layer_rotation_accessor.ts(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(9 hunks)
🔇 Additional comments (5)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (5)
127-137: LGTM: New imports for transformation handling
The added imports for transformation-related functionality align well with the PR's objective of enabling dataset layer rotation.
226-228: LGTM: Improved transformation state management
The addition of hasLayerTransformsConfigured and the updated native rendering check improve the handling of layer transformations.
Also applies to: 233-233
246-255: LGTM: Comprehensive disabled state handling
The disabled state logic properly handles various cases:
- Layers without transforms
- Skeleton layers
- Volume tracing layers without fallback
299-306: LGTM: Improved UI feedback for transformations
The UI changes provide better visual feedback:
- Clear cursor states for disabled/enabled transformations
- Informative tooltips explaining the transformation state
Also applies to: 312-316
1243-1243: LGTM: Proper skeleton layer transformation handling
The skeleton layer now correctly integrates with the transformation system while maintaining its special handling requirements.
| // TODOM: Fix behaviour is broken!!! | ||
|
|
There was a problem hiding this comment.
Address TODO comments
There are two TODO comments indicating broken behavior that need to be addressed:
- "Fix behaviour is broken!!!"
- "Make the identification what transformation will be applied to keep the position an own function"
Would you like help implementing these fixes? I can help:
- Debug and fix the broken behavior
- Refactor the transformation logic into a separate function
- fix & refactor positional change when toggling natively rendered layer - only toggling natively rendered layer name when layer is rendered natively and has not transforms configured
…ow-perma-dataset-rotation
|
@philippotto my newest changes should implement discussed changes and also fix the new position calculation after toggling which layer is rendered natively. Please have another look at my changes 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
frontend/javascripts/oxalis/model/sagas/quick_select_heuristic_saga.ts (1)
Line range hint
632-701: Consider optimizing flood fill for large datasets.The flood fill implementation uses a simple queue-based approach. For large datasets, consider:
- Using a more efficient data structure for the queue
- Implementing batch processing for better memory usage
- Adding progress tracking for long-running operations
function fillHolesInPlace(arr: ndarray.NdArray<Uint8Array>) { + // Add size check for large arrays + const BATCH_SIZE = 1000; + const isLargeArray = arr.size > 1000000; + const visitedField = ndarray(new Uint8Array(arr.size), arr.shape); - const queue: Vector2[] = [[0, 0]]; + // Use a more efficient queue implementation for large arrays + const queue = isLargeArray + ? new FastQueue<Vector2>() + : [[0, 0]];frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
75-99: Consider a clearer approach to partial rotations.The logic in getRotationFromTransformationIn90DegreeSteps() (lines 75-99) assumes only a single-axis rotation in 90-degree increments. If additional transformations (e.g., combined rotations around multiple axes) are added in the future, this approach might become confusing or less robust. Consider handling multi-axis rotations or validating that only one axis is rotated for clarity and maintainability.
154-182: Graceful handling of other transformation types._getOriginalTransformsForLayerOrNull() (lines 154-182) logs an error when encountering unsupported transformations, yet always returns an IdentityTransform for that transformation. While this is acceptable error handling, consider either surfacing the error to the user in a user-friendly manner or preserving partial transformations that are valid when a single transform fails. This helps in debugging and user transparency.
195-230: Inversion logic for natively rendered layers.In _getTransformsForLayerOrNull() (lines 195-230), the function applies the inverse of the nativelyRenderedLayerName’s transforms to the current layer. If the transformations remain consistent across layers, this should be fine. However, if future changes allow more complex or mismatched transformations, this logic might lead to unexpected results. Consider documenting the assumptions or adding checks.
CHANGELOG.unreleased.md (1)
14-14: Add missing comma after “dataset”.In the phrase “possibility to configure a rotation for a dataset which can be toggled off and on…”, consider adding a comma after “dataset” to improve readability.
- Added the possibility to configure a rotation for a dataset which can be toggled off and on when viewing... + Added the possibility to configure a rotation for a dataset, which can be toggled off and on when viewing...🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...ssibility to configure a rotation for a dataset which can be toggled off and on when vi...(AI_HYDRA_LEO_MISSING_COMMA)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (2)
57-63: Revisit repetitive structure in transformations array.Here (lines 57-63), you repeatedly build the array of five transformations (translate → rotation x → rotation y → rotation z → translate). If this pattern is used across multiple places, consider extracting a utility function to avoid duplicated logic.
129-131: Explicit null check for onChange.You are correctly checking (value != null) in the onChange handler so that zero is handled. However, a short inline comment explaining the rationale might help future maintainers understand why the check is != null instead of a simple truthiness check.
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (4)
222-224: Consider memoizing the selector for better performanceThe selector for
hasLayerTransformsConfiguredcould benefit from memoization to prevent unnecessary recalculations.Consider using
createSelectorfrom Redux Toolkit:+import { createSelector } from '@reduxjs/toolkit'; + +const selectHasLayerTransformsConfigured = createSelector( + [(state: OxalisState) => state.dataset, (_, layer) => layer], + (dataset, layer) => getTransformsForLayerOrNull(dataset, layer, null) != null +); + const hasLayerTransformsConfigured = useSelector( - (state: OxalisState) => getTransformsForLayerOrNull(state.dataset, layer, null) != null, + (state: OxalisState) => selectHasLayerTransformsConfigured(state, layer) );
242-245: Add accessibility attributes to disabled elementsThe code correctly handles disabled state for layers without transforms, but could be more accessible.
Add ARIA attributes to improve accessibility:
const isDisabled = isRenderedNatively && !hasLayerTransformsConfigured; + const ariaLabel = isDisabled + ? "Layer transformation controls (disabled - no transforms available)" + : "Layer transformation controls";
273-281: Consider using CSS-in-JS or styled-componentsThe inline styles could be moved to a styled component or CSS-in-JS solution for better maintainability.
Consider using styled-components:
+import styled from 'styled-components'; + +const TransformationIconWrapper = styled.img<{ isDisabled: boolean }>` + width: 14px; + height: 14px; + margin-bottom: 4px; + margin-right: 5px; + cursor: ${props => props.isDisabled ? 'not-allowed' : 'pointer'}; + opacity: ${props => props.isDisabled ? '0.5' : '1.0'}; +`; + - const style = { - width: 14, - height: 14, - marginBottom: 4, - marginRight: 5, - ...(isDisabled - ? { cursor: "not-allowed", opacity: "0.5" } - : { cursor: "pointer", opacity: "1.0" }), - };
1218-1218: Consider type safety for skeleton layer transformationThe skeleton layer transformation could benefit from stronger typing.
Add type safety for the skeleton layer:
+type SkeletonLayer = { + category: 'skeleton'; + name: string; +}; + -<TransformationIcon layer={{ category: "skeleton", name: tracingId }} /> +<TransformationIcon layer={{ category: "skeleton", name: tracingId } as SkeletonLayer} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
CHANGELOG.unreleased.md(1 hunks)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_view.tsx(3 hunks)frontend/javascripts/oxalis/api/api_latest.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_layer_transformation_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/layer_rendering_manager.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/nml_helpers.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/view/left-border-tabs/layer_settings_tab.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- frontend/javascripts/oxalis/model/helpers/nml_helpers.ts
- frontend/javascripts/oxalis/controller/scene_controller.ts
- frontend/javascripts/oxalis/merger_mode.ts
- frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts
- frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts
- frontend/javascripts/oxalis/geometries/materials/node_shader.ts
- frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts
- frontend/javascripts/oxalis/model/sagas/dataset_saga.ts
- frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx
- frontend/javascripts/oxalis/api/api_latest.ts
- frontend/javascripts/oxalis/geometries/materials/edge_shader.ts
- frontend/javascripts/oxalis/model/accessors/tool_accessor.ts
👮 Files not reviewed due to content moderation or server errors (1)
- frontend/javascripts/oxalis/model_initialization.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...ssibility to configure a rotation for a dataset which can be toggled off and on when vi...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (6)
frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts (2)
38-38: LGTM! Clean import of transformation functions.
The import of invertAndTranspose and getTransformsForLayer from the new dataset_layer_transformation_accessor module aligns with the PR's goal of centralizing transformation-related functions.
196-196:
Critical fix: Ensure new matrix instance creation.
The change from M4x4.identity to M4x4.identity() is crucial as it ensures a new identity matrix instance is created rather than potentially sharing a reference. This prevents potential mutation-related bugs in matrix transformations.
Let's verify the usage of M4x4.identity() across the codebase to ensure consistency:
frontend/javascripts/oxalis/model/sagas/quick_select_heuristic_saga.ts (2)
61-61: LGTM: Import change aligns with transformation reorganization.
The relocation of getTransformsForLayer to a dedicated transformation accessor module improves code organization.
Line range hint 144-153: Verify transform comparison with rotated layers.
The transform comparison is crucial for the new rotation feature. While the implementation looks correct, we should verify that it handles all rotation scenarios correctly.
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (1)
130-140: Memoization map usage note.
memoizeWithThreeKeys() (lines 130-140) uses MultiKeyMap for caching. Consider verifying if the key parameters may be large or change frequently, impacting memory. If so, a cache invalidation policy or an upper-bound size check may help avoid unbounded growth.
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
203-225: Ensure consistent rotation defaults.
When setting initialDatasetRotationSettings (lines 203-225), you assume five transformations exist or use [0,0,0] by default. This is consistent with your “translate-rotate-rotate-rotate-translate” approach. If transformation definitions expand or a user manually modifies them, ensure that unexpected transformations do not silently get ignored.
✅ Verification successful
Let me gather more information about the transformation setup by checking the related code.
Let me check one more aspect to understand how the transformations are applied and validated.
Based on the gathered information, I can now provide the final response:
The five-transformation setup is a well-defined pattern in the codebase
The code's assumption of five transformations (translate-rotate-rotate-rotate-translate) is a deliberate design choice, consistently implemented across the codebase. The validation through doAllLayersHaveTheSameRotation ensures this pattern is maintained. When the pattern isn't present, the code safely defaults to [0,0,0] rotation, which is the correct fallback behavior. The transformation definitions are unlikely to silently get ignored as:
- The database schema enforces the structure (
dataset_layer_coordinateTransformationstable) - The codebase consistently handles transformations through well-defined interfaces
- The rotation extraction is specifically designed for 90-degree steps around main axes
- Layer transformations are validated before being applied
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are references to other transformation setups that might conflict.
rg 'coordinateTransformations'
Length of output: 14754
Script:
#!/bin/bash
# Check the implementation of getRotationFromTransformationIn90DegreeSteps and related functions
ast-grep --pattern 'function getRotationFromTransformationIn90DegreeSteps'
# Check for any other rotation-related transformations
rg -A 5 'rotation.*transform|transform.*rotation'
Length of output: 3910
Script:
#!/bin/bash
# Check for transformation validation or application logic
ast-grep --pattern 'function doAllLayersHaveTheSameRotation'
# Check for any other transformation validation
rg -A 5 'validateTransform|checkTransform|applyTransform'
Length of output: 5810
| 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 (!hasValidTransformationCount(dataLayers) || !hasOnlyAffineTransformations(dataLayers)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!hasValidTransformationPattern(firstDataLayerTransformations)) { | ||
| return false; | ||
| } | ||
| for (let i = 1; i < dataLayers.length; i++) { | ||
| const transformations = dataLayers[i].coordinateTransformations; | ||
| if ( | ||
| transformations == null || | ||
| !_.isEqual(transformations[0], firstDataLayerTransformations[0]) || | ||
| !_.isEqual(transformations[1], firstDataLayerTransformations[1]) || | ||
| !_.isEqual(transformations[2], firstDataLayerTransformations[2]) || | ||
| !_.isEqual(transformations[3], firstDataLayerTransformations[3]) || | ||
| !_.isEqual(transformations[4], firstDataLayerTransformations[4]) | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rotation consistency checks might be brittle.
_doAllLayersHaveTheSameRotation() (lines 396-427) strictly expects five transform steps (translate → rotate x → rotate y → rotate z → translate). This is clear for the current design, but might not scale well if transformations become more varied (e.g., scale, shear). Consider making the check more flexible or storing transform metadata if you foresee further extension of transformation configs.
| // Get getOriginalTransformsForLayerOrNull is not used to handle layers that do not support configuring a transformation. | ||
| const nextNativelyRenderedLayerName = isRenderedNatively ? null : layer.name; | ||
| const activeTransformation = getTransformsForLayer( | ||
| state.dataset, | ||
| layer, | ||
| state.datasetConfiguration.nativelyRenderedLayerName, | ||
| ); | ||
| const invertedTransform = invertTransform(currentTransforms); | ||
| const newPosition = transformPointUnscaled(invertedTransform)(currentPosition); | ||
|
|
||
| // Also transform a reference coordinate to determine how the scaling | ||
| // changed. Then, adapt the zoom accordingly. | ||
| const referenceOffset: Vector3 = [10, 10, 10]; | ||
| const secondPosition = V3.add(currentPosition, referenceOffset, [0, 0, 0]); | ||
| const newSecondPosition = transformPointUnscaled(invertedTransform)(secondPosition); | ||
|
|
||
| const scaleChange = _.mean( | ||
| // Only consider XY for now to determine the zoom change (by slicing from 0 to 2) | ||
| V3.abs(V3.divide3(V3.sub(newPosition, newSecondPosition), referenceOffset)).slice(0, 2), | ||
| const nextTransform = getTransformsForLayer( | ||
| state.dataset, | ||
| layer, | ||
| nextNativelyRenderedLayerName, | ||
| ); | ||
| const { scaleChange, newPosition } = getNewPositionAndZoomChangeFromTransformationChange( | ||
| activeTransformation, | ||
| nextTransform, | ||
| state, |
There was a problem hiding this comment.
Improve error handling in transformation logic
The transformation logic should handle potential errors when retrieving transforms.
Add error handling and validation:
const toggleLayerTransforms = () => {
const state = Store.getState();
+ try {
const nextNativelyRenderedLayerName = isRenderedNatively ? null : layer.name;
const activeTransformation = getTransformsForLayer(
state.dataset,
layer,
state.datasetConfiguration.nativelyRenderedLayerName,
);
+ if (!activeTransformation) {
+ throw new Error('Failed to get active transformation');
+ }
const nextTransform = getTransformsForLayer(
state.dataset,
layer,
nextNativelyRenderedLayerName,
);
+ if (!nextTransform) {
+ throw new Error('Failed to get next transformation');
+ }
+ } catch (error) {
+ console.error('Error toggling layer transforms:', error);
+ Toast.error('Failed to toggle layer transformation');
+ return;
+ }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:
[ ] Design a visualization of the result. For some first ideas look at the issue #7334--> deferred / not plannednativelyRenderedLayerNamein 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)