Readd fixed version of "Allow permanent dataset layer rotation in dataset settings"" #8355
Conversation
…tings (#…" This reverts commit e20c218.
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive feature for configuring dataset rotations. The changes span multiple frontend files, creating a new system that allows users to rotate dataset layers around x, y, and z axes. A new module Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
frontend/javascripts/types/globals.d.ts (1)
19-21: Add JSDoc documentation for the Mutable type utility.The type implementation looks correct, but it would benefit from documentation explaining its purpose and usage guidelines.
+/** + * Makes all properties of type T mutable by removing readonly modifiers. + * Use with caution as it bypasses readonly protection. + * @template T - The type to make mutable + * @example + * type ReadonlyUser = { readonly name: string }; + * type MutableUser = Mutable<ReadonlyUser>; // { name: string } + */ export type Mutable<T> = { -readonly [K in keyof T]: T[K]; };frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts (1)
153-155: New schema property for nativelyRenderedLayerName.
This addition provides a dedicated location in view configuration for specifying the layer to render natively.
However, note that the default value indefaultDatasetViewConfigurationWithoutNullisnull, while the schema strictly requires a string. If you intend to allownull, consider adjusting the schema to:-nativelyRenderedLayerName: { - type: "string", -}, +nativelyRenderedLayerName: { + type: ["string", "null"], +},frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)
25-36: Consider memoizing dataset bounding box computation.The
getDatasetBoundingBoxFromLayersfunction performs iterations and object creation. Since it's used in multiple places, consider memoizing its results to avoid unnecessary recomputation.-function getDatasetBoundingBoxFromLayers(layers: APIDataLayer[]): BoundingBox | undefined { +const getDatasetBoundingBoxFromLayers = memoizeOne((layers: APIDataLayer[]): BoundingBox | undefined => { if (!layers || layers.length === 0) { return undefined; } let datasetBoundingBox = BoundingBox.fromBoundBoxObject(layers[0].boundingBox); for (let i = 1; i < layers.length; i++) { datasetBoundingBox = datasetBoundingBox.extend( BoundingBox.fromBoundBoxObject(layers[i].boundingBox), ); } return datasetBoundingBox; -} +});
116-116: Consider adding aria-label for accessibility.The slider control should have an aria-label for better accessibility.
- <Slider min={0} max={270} step={90} onChange={setMatrixRotationsForAllLayer} /> + <Slider + min={0} + max={270} + step={90} + onChange={setMatrixRotationsForAllLayer} + aria-label={`Rotate dataset around ${axis} axis`} + />
48-71: Consider debouncing the transformation updates.The effect that updates transformations runs on every bounding box change. Consider debouncing these updates to prevent excessive re-renders, especially when multiple changes occur in quick succession.
+import { debounce } from 'lodash'; +const debouncedUpdateTransformations = debounce(( + datasetBoundingBox: BoundingBox, + dataLayers: APIDataLayer[], + form: FormInstance, +) => { + if ( + datasetBoundingBox == null || + dataLayers[0].coordinateTransformations?.length !== EXPECTED_TRANSFORMATION_LENGTH || + !form + ) { + return; + } + // ... rest of the transformation update logic +}, 250); useEffect(() => { - if ( - datasetBoundingBox == null || - dataLayers[0].coordinateTransformations?.length !== EXPECTED_TRANSFORMATION_LENGTH || - !form - ) { - return; - } - // ... rest of the effect + if (form) { + debouncedUpdateTransformations(datasetBoundingBox, dataLayers, form); + } return () => { + debouncedUpdateTransformations.cancel(); }; }, [datasetBoundingBox, dataLayers, form]);frontend/javascripts/oxalis/model.ts (1)
229-237: LGTM! Improved coordinate transformation in getHoveredCellId.The use of
globalToLayerTransformedPositioncorrectly transforms the mouse position from global to layer space before retrieving the data value. The code is now more accurate in handling coordinate transformations.Consider extracting the layer category "segmentation" as a constant since it's used as a magic string:
+const SEGMENTATION_LAYER_CATEGORY = "segmentation"; + const posInLayerSpace = globalToLayerTransformedPosition( pos, segmentationLayer.name, - "segmentation", + SEGMENTATION_LAYER_CATEGORY, state, );frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)
204-229: Consider enhancing the rotation initialization logic.While the current implementation handles basic cases, consider adding:
- Validation for rotation values to ensure they are multiples of 90 degrees
- Error handling for malformed transformation arrays
- Logging when falling back to default values
if (doAllLayersHaveTheSameRotation(dataSource.dataLayers)) { const firstLayerTransformations = dataSource.dataLayers[0].coordinateTransformations; let initialDatasetRotationSettings: DatasetRotation; if ( - !firstLayerTransformations || - firstLayerTransformations.length !== EXPECTED_TRANSFORMATION_LENGTH + !Array.isArray(firstLayerTransformations) || + firstLayerTransformations.length !== EXPECTED_TRANSFORMATION_LENGTH || + !firstLayerTransformations.every(transform => transform != null) ) { + console.warn('Invalid transformation array, falling back to default rotation values'); initialDatasetRotationSettings = { x: 0, y: 0, z: 0, }; } else { + const validateRotation = (value: number) => Math.round(value / 90) * 90; initialDatasetRotationSettings = { - x: getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[1], "x"), - y: getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[2], "y"), - z: getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[3], "z"), + x: validateRotation(getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[1], "x")), + y: validateRotation(getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[2], "y")), + z: validateRotation(getRotationFromTransformationIn90DegreeSteps(firstLayerTransformations[3], "z")), }; } form.setFieldsValue({ datasetRotation: initialDatasetRotationSettings, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
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_data_tab.tsx(2 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(4 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/combinations/volume_handlers.ts(3 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.ts(2 hunks)frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts(4 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/context_menu.tsx(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 (8)
- frontend/javascripts/oxalis/controller/scene_controller.ts
- frontend/javascripts/oxalis/model/sagas/quick_select_heuristic_saga.ts
- frontend/javascripts/oxalis/model/helpers/nml_helpers.ts
- frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx
- frontend/javascripts/oxalis/merger_mode.ts
- frontend/javascripts/oxalis/geometries/materials/node_shader.ts
- frontend/javascripts/oxalis/model/sagas/dataset_saga.ts
- frontend/javascripts/oxalis/api/api_latest.ts
👮 Files not reviewed due to content moderation or server errors (4)
- frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts
- frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
- frontend/javascripts/oxalis/view/context_menu.tsx
- CHANGELOG.unreleased.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (44)
frontend/javascripts/types/globals.d.ts (1)
19-21: Verify usage patterns of the Mutable type utility.The type implementation is correct, but let's verify its usage patterns to ensure type safety is maintained.
✅ Verification successful
The Mutable type utility is being used correctly and safely.
The codebase follows a consistent pattern of using readonly types by default and explicitly marking mutable versions where needed. This approach helps prevent accidental mutations while allowing controlled mutability when required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of Mutable type to verify type safety # Search for imports and usages echo "Searching for Mutable type imports and usages..." rg "Mutable.*" -A 5 # Search for readonly properties that might be affected echo "Searching for readonly properties that might be affected..." rg "readonly.*" -A 5Length of output: 96329
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (16)
1-33: Imports appear well-organized.
All the imported modules (e.g.,lodash,three, etc.) are referenced in subsequent code, and there are no obvious unused imports.
34-63: Constants and indexing mappings look correct.
TheIDENTITY_MATRIX,IDENTITY_TRANSFORM, and the sin/cos index lookups for rotation axes are neatly defined. The approach is consistent across the codebase, and these constants are easy to maintain.
65-72: Good matrix reshaping utility.
flatToNestedMatrixcorrectly slices a 16-element array into a 4x4 nested matrix. Straightforward and efficient approach.
74-98: Rotation extraction in 90-degree steps is well-structured.
The function properly calculates angles, avoids division by zero, and neatly rounds to multiples of 90. Keep in mind that non-orthonormal matrices may produce unexpected angles, but this is likely intended.
100-114: Center-origin translations are consistent.
UsingTHREE.Matrix4and transposing to adjust row-major vs. column-major is handled cleanly. This accurately shifts the bounding box center to (and from) the origin.
115-127: Euler rotation and zero-value normalization.
getRotationMatrixAroundAxisleveragesTHREE.Eulerproperly and clamps near-zero values. This helps avoid floating-point drift in rotation matrices.
129-151: Cache behavior with multi-key memoization.
The custommemoizeWithThreeKeysandmemoizeWithTwoKeysare clear and concise. Watch memory usage in scenarios with a large key domain, but otherwise good for performance.
153-192: Layer transform creation and config checks.
_getOriginalTransformsForLayerOrNullcorrectly distinguishes between affine and thin-plate-spline transformations, defaulting to identity on invalid.isLayerWithoutTransformationConfigSupportis a helpful convenience check.
Implementation is straightforward.
194-240: Layer transform retrieval and identity checks.
_getTransformsForLayerOrNullneatly handles the natively rendered layer logic and merges inverses when needed.getTransformsForLayerprovides a consistent default (identity) if no transform is found.isIdentityTransformis a succinct utility.
246-301: Fallback logic for layers lacking config support.
_getTransformsForLayerThatDoesNotSupportTransformationConfigOrNullcarefully checks if all layers share a consistent rotation or a natively rendered layer.- The final fallback of returning the inverse transformation of the native layer is logical.
getTransformsForSkeletonLayerreuses the same approach.
303-317: Comprehensive per-layer transforms.
_getTransformsPerLayeriterates over layers and aggregates transforms in a single object, simplifying usage upstream. No issues.
319-333: Inverse segmentation transformer & dataset transform checks.
getInverseSegmentationTransformerandhasDatasetTransformsdo exactly what their names imply, with clear logic.
342-373: Matrix inversion & decomposition checks.
invertAndTransposeappropriately inverts and transposes a 4x4 matrix for GPU usage.isTranslationOnlyandisRotationOnlyrely onTHREE.Matrix4.decompose, then compare the result. Straightforward approach to check transformation nature.
375-435: Dataset-level rotation consistency checks.
hasValidTransformationCount,hasOnlyAffineTransformations, andhasValidTransformationPatternensure the transformation array structure is as expected._doAllLayersHaveTheSameRotationimplements the logic for verifying that all layers follow this rotation pattern.
437-438: Memoized rotation check export.
ExportingdoAllLayersHaveTheSameRotationwith a memoized version ensures repeated calls remain performant.
439-458: Global-to-layer position transformation.
globalToLayerTransformedPositionensures correct coordinate conversion by inverting the layer transform. Straightforward and readable.frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (1)
3-8: Import updates look consistent.
The addition ofV3from"libs/mjs"and typed imports from"oxalis/constants"align with usage in the file. No immediate issues or redundancies noted.frontend/javascripts/oxalis/controller/combinations/volume_handlers.ts (1)
55-60: LGTM! Consistent coordinate transformation handling.The changes correctly transform global coordinates to layer space before accessing segment data. This ensures proper handling of rotated datasets.
Also applies to: 86-91
frontend/javascripts/oxalis/geometries/materials/edge_shader.ts (1)
6-6: LGTM! Import path updated.The import path change aligns with the centralization of transformation-related functions.
frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts (1)
5-5: LGTM! Enhanced type safety for matrix operations.The switch to
NestedMatrix4type provides better type guarantees while maintaining the same functionality.Also applies to: 7-7, 29-29
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
73-106: Verify the rotation matrix computation.The rotation matrix computation and application look correct, but please verify:
- The order of rotations (x, y, z) is consistent with the expected behavior
- The conversion from degrees to radians is accurate
- The matrix multiplication order preserves the intended transformation
✅ Verification successful
The rotation matrix computation and application are correctly implemented ✓
The implementation is mathematically sound and follows best practices:
- Proper transformation sequence: translate to origin → rotate → translate back
- Correct rotation order (x:1, y:2, z:3) maintained through AXIS_TO_TRANSFORM_INDEX
- Accurate degree-to-radian conversion using Math.PI/180
- Robust matrix operations using THREE.js with proper handling of floating-point precision
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other rotation matrix implementations to verify consistency rg -A 5 "getRotationMatrix|rotateMatrix|rotation.*matrix" --type tsLength of output: 4747
Script:
#!/bin/bash # Get the implementation of getRotationMatrixAroundAxis and related constants ast-grep --pattern 'export function getRotationMatrixAroundAxis($_) { $$$ }' # Find AXIS_TO_TRANSFORM_INDEX definition rg "AXIS_TO_TRANSFORM_INDEX.*=.*{" -A 5 --type ts # Look for any matrix multiplication or transformation order related code rg "EXPECTED_TRANSFORMATION_LENGTH|matrix.*multiplication|transform.*order" --type tsLength of output: 1302
Script:
#!/bin/bash # Find AXIS_TO_TRANSFORM_INDEX definition with fixed regex rg "AXIS_TO_TRANSFORM_INDEX" -B 2 -A 2 --type ts # Get the implementation details from the transformation accessor cat frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts # Look for documentation about transformation sequence rg -l "transformation.*sequence|matrix.*order" --type ts --type mdLength of output: 18404
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts (1)
28-33: LGTM! The implementation is correct and efficient.The function correctly creates a
BoundingBoxinstance by:
- Using
topLeftas the minimum coordinates- Calculating maximum coordinates by adding dimensions to
topLeftfrontend/javascripts/test/reducers/flycam_reducer.spec.ts (1)
39-39: LGTM! Correctly updated to invokeM4x4.identityas a function.The change aligns with the codebase-wide update to invoke
M4x4.identityas a function instead of accessing it as a property.frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts (3)
28-30: LGTM! Imports are correctly updated.The imports are correctly updated to use the new transformation accessor functions from the
dataset_layer_transformation_accessormodule.
221-224: LGTM! Function correctly uses the new transformation accessor.The function maintains its behavior while using the new transformation accessor function.
234-235: LGTM! Code readability improved using object destructuring.The changes improve code readability by using object destructuring to extract
nativelyRenderedLayerNamewithout altering functionality.Also applies to: 241-242
frontend/javascripts/oxalis/model/reducers/flycam_reducer.ts (1)
105-105: LGTM! Correctly updated to invokeM4x4.identityas a function.The change aligns with the codebase-wide update to invoke
M4x4.identityas a function instead of accessing it as a property.frontend/javascripts/libs/mjs.ts (1)
223-226: LGTM! Clean implementation of the identity matrix method.The addition of the
identity()method toM4x4is well-implemented, providing a straightforward way to obtain the identity matrix for transformation operations.frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts (1)
29-32: LGTM! Good refactoring of transformation-related imports.The relocation of transformation-related functions to a dedicated accessor module improves code organization and maintainability.
frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx (1)
31-32: LGTM! Consistent refactoring of transformation-related imports.The relocation of
flatToNestedMatrixto the dedicated transformation accessor module maintains consistency with the codebase-wide refactoring.frontend/javascripts/oxalis/store.ts (1)
339-346: LGTM! Documentation improvements enhance clarity.The updated comments provide better clarity about:
- How layers with matching transformations are handled
- Special cases for skeleton layers and volume layers without fallback
frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts (2)
261-289: LGTM! Well-documented transformation calculation.The implementation correctly:
- Calculates transformation differences
- Determines position changes
- Computes scale changes using reference coordinates
Note: The scale change calculation only considers XY dimensions. Verify if Z-axis scaling should also be considered for your use case.
202-202: LGTM! Fixed method call syntax.Corrected the
M4x4.identityinvocation by adding parentheses.frontend/javascripts/oxalis/model_initialization.ts (2)
481-481: LGTM! Enhanced layer transformation handling.The implementation correctly:
- Checks if all layers share the same rotation
- Assigns coordinate transformations based on rotation consistency
- Falls back to the fallback layer's transformations when rotations differ
Also applies to: 499-508
860-876: LGTM! Improved validation of nativelyRenderedLayerName.The implementation now properly validates that the nativelyRenderedLayerName corresponds to an existing layer in the dataset or annotation layers. If not found, it's correctly set to null.
frontend/javascripts/types/api_flow_types.ts (2)
63-73: LGTM! Improved type safety for transformations.The split into separate types for affine and thin plate spline transformations:
- Enhances type safety
- Improves code clarity
- Makes the transformation types more maintainable
99-100: LGTM! Enhanced APISkeletonLayer type.Added name property to ensure uniqueness by using the skeleton tracing ID.
frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (1)
9-9: LGTM! Import refactoring improves code organization.The relocation of
getTransformsPerLayerto a dedicated transformation accessor module enhances code organization by centralizing transformation-related functionality.Also applies to: 21-21
frontend/javascripts/oxalis/constants.ts (1)
18-19: LGTM! Well-defined matrix type with clear documentation.The
NestedMatrix4type is properly defined as a tuple of fourVector4elements, and the comment clearly indicates it represents a row-major matrix.frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (2)
37-37: LGTM! Clean import of the rotation settings component.The import statement is properly placed with other component imports.
271-276: LGTM! Well-structured layout for rotation settings.The rotation settings are cleanly integrated using Ant Design's grid system, with appropriate column spans and spacing.
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (2)
28-32: LGTM! Well-organized imports for transformation functionality.The imports are properly grouped and specific to the transformation-related functionality being added.
85-85: LGTM! Clear type definition update.The FormData type is properly extended with an optional datasetRotation field.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (6)
74-127: Consider enhancing documentation for mathematical concepts.While the code is well-implemented, the mathematical concepts (e.g., rotation matrices, Euler angles) could benefit from more detailed documentation explaining:
- The choice of rotation order
- The handling of gimbal lock
- The rationale for 90-degree step rounding
129-151: Consider adding cache management.The memoization implementation could benefit from:
- A maximum cache size limit
- A cache clearing mechanism
- A least-recently-used (LRU) eviction policy
This would prevent potential memory leaks in long-running sessions.
function memoizeWithThreeKeys<A, B, C, T>(fn: (a: A, b: B, c: C) => T) { const map = new MultiKeyMap<A | B | C, T, [A, B, C]>(); + const MAX_CACHE_SIZE = 1000; return (a: A, b: B, c: C): T => { let res = map.get([a, b, c]); if (res === undefined) { res = fn(a, b, c); + if (map.size >= MAX_CACHE_SIZE) { + // Clear half of the cache when limit is reached + const keys = Array.from(map.keys()).slice(0, MAX_CACHE_SIZE / 2); + keys.forEach(key => map.delete([key])); + } map.set([a, b, c], res); } return res; }; }
175-178: Enhance error handling with more descriptive messages.The error message for unsupported transformation types could be more informative by including:
- The actual type encountered
- The layer name/ID
- Possible resolution steps
console.error( - "Data layer has defined a coordinate transform that is not affine or thin_plate_spline. This is currently not supported and ignored", + `Layer "${layer.name}" has unsupported coordinate transform type "${type}". Only "affine" and "thin_plate_spline" are supported. The transform will be ignored. Please update the layer configuration.`, );
194-229: Consider optimizing transform chain calculations.The transform chain calculations in
_getTransformsForLayerOrNullcould be optimized by:
- Pre-computing and caching inverse transforms
- Avoiding unnecessary chain operations when possible
- Adding early returns for common cases
303-315: Consider batch processing optimization for multiple layers.The current implementation processes layers sequentially. For datasets with many layers, consider:
- Processing layers in batches
- Parallelizing transform calculations where possible
- Adding progress tracking for large datasets
393-402: Consider extracting validation patterns into a configuration object.The transformation pattern validation could be more maintainable by:
- Extracting the validation rules into a configuration object
- Making the validation order configurable
- Adding support for custom validation patterns
+const TRANSFORMATION_PATTERN = { + steps: [ + { index: 0, type: 'translation', validate: isTranslationOnly }, + { index: 1, type: 'rotation', validate: isRotationOnly }, + { index: 2, type: 'rotation', validate: isRotationOnly }, + { index: 3, type: 'rotation', validate: isRotationOnly }, + { index: 4, type: 'translation', validate: isTranslationOnly }, + ], +}; function hasValidTransformationPattern(transformations: CoordinateTransformation[]): boolean { return ( transformations.length === EXPECTED_TRANSFORMATION_LENGTH && - 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) + TRANSFORMATION_PATTERN.steps.every(step => + step.validate(transformations[step.index] as AffineTransformation) + ) ); }frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (5)
25-36: Consider optimizing the bounding box calculation.The function could be optimized to use a single pass through the layers array instead of checking length separately.
Here's an optimized version:
function getDatasetBoundingBoxFromLayers(layers: APIDataLayer[]): BoundingBox | undefined { - if (!layers || layers.length === 0) { + if (!Array.isArray(layers) || layers.length === 0) { return undefined; } - let datasetBoundingBox = BoundingBox.fromBoundBoxObject(layers[0].boundingBox); - for (let i = 1; i < layers.length; i++) { - datasetBoundingBox = datasetBoundingBox.extend( - BoundingBox.fromBoundBoxObject(layers[i].boundingBox), - ); - } + return layers.reduce((acc, layer) => { + const layerBox = BoundingBox.fromBoundBoxObject(layer.boundingBox); + return acc ? acc.extend(layerBox) : layerBox; + }, undefined as BoundingBox | undefined); - return datasetBoundingBox; }
48-71: Consider optimizing the useEffect dependencies.The effect dependency on
dataLayerscould cause unnecessary rerenders since it's an array reference. Consider using a stable value derived from dataLayers.useEffect(() => { if ( datasetBoundingBox == null || - dataLayers[0].coordinateTransformations?.length !== EXPECTED_TRANSFORMATION_LENGTH || + !dataLayers?.[0]?.coordinateTransformations?.length !== EXPECTED_TRANSFORMATION_LENGTH || !form ) { return; } // ... rest of the effect -}, [datasetBoundingBox, dataLayers, form]); +}, [datasetBoundingBox, form]);
116-116: Extract magic numbers into named constants.The Slider component uses magic numbers (0, 270, 90) which should be extracted into named constants for better maintainability.
+const ROTATION_CONFIG = { + MIN_DEGREES: 0, + MAX_DEGREES: 270, + STEP_DEGREES: 90, +} as const; + -<Slider min={0} max={270} step={90} onChange={setMatrixRotationsForAllLayer} /> +<Slider + min={ROTATION_CONFIG.MIN_DEGREES} + max={ROTATION_CONFIG.MAX_DEGREES} + step={ROTATION_CONFIG.STEP_DEGREES} + onChange={setMatrixRotationsForAllLayer} +/>
161-184: Enhance accessibility and user guidance.The tooltip content could be more accessible and user-friendly.
<Tooltip + role="tooltip" + aria-label="Rotation requirements" title={ <div> - Each layers transformations must be equal and each layer needs exactly 5 affine - transformation with the following schema: + To enable dataset rotation, all layers must have identical transformations following this structure: <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. + Quick Fix: To enable rotation, + 1. Go to the advanced tab + 2. Remove all coordinate transformations from each layer + 3. Save and reload the dataset settings </div> } >
1-194: Consider extracting rotation logic into a custom hook.The rotation logic is complex and could benefit from being extracted into a custom hook (e.g.,
useDatasetRotation). This would:
- Improve testability
- Make the component logic more reusable
- Separate concerns between UI and rotation calculations
Example structure:
function useDatasetRotation(form: FormInstance | undefined) { // Extract rotation logic here return { setRotation, updateTransformations, isRotationSupported, // ... other rotation-related utilities }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (1)
1-72: LGTM! Well-structured constants and utility functions.The code demonstrates good practices with proper type definitions, clear constants, and focused utility functions.
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)
1-24: LGTM! Well-structured imports and type definitions.The imports are organized logically, and the type definitions are clear and properly typed.
145-149: LGTM! Well-defined type for dataset rotation.The DatasetRotation type is clear and properly typed for x, y, z rotation values.
73-106: Add error handling for rotation calculations.The setMatrixRotationsForAllLayer function should handle potential errors in rotation calculations and invalid form states.
Consider adding validation and error handling:
const setMatrixRotationsForAllLayer = useCallback( (rotationInDegrees: number): void => { if (!form) { return; } + if (typeof rotationInDegrees !== 'number' || isNaN(rotationInDegrees)) { + console.error('Invalid rotation value:', rotationInDegrees); + return; + } const dataLayers: APIDataLayer[] = form.getFieldValue(["dataSource", "dataLayers"]); + if (!Array.isArray(dataLayers) || dataLayers.length === 0) { + console.error('No data layers found'); + return; + } // ... rest of the function }, [axis, form], );
There was a problem hiding this comment.
This PR should readd and fix the dataset rotation feature.
Please only check commit 3e6a780. This should include the bug fix
| // form -> dataSource -> dataLayers can be undefined in case of the add remote dataset form which is initially empty. | ||
| const dataLayers: APIDataLayer[] | undefined = form?.getFieldValue(["dataSource", "dataLayers"]); | ||
| const isRotationOnly = useMemo( | ||
| () => (dataLayers ? doAllLayersHaveTheSameRotation(dataLayers) : false), | ||
| [dataLayers], | ||
| ); |
There was a problem hiding this comment.
This should be the acutal fix
| if (dataLayers.length === 0) { | ||
| // The dataset does not have any layers. Therefore no layers can be rotated. | ||
| return false; | ||
| } | ||
| const firstDataLayerTransformations = dataLayers[0].coordinateTransformations; |
There was a problem hiding this comment.
And here I noticed while thinking about this bug, that the previous code would return true for _doAllLayersHaveTheSameRotation when it was called with an empty array of data layers. This seemed wrong to me on a conceptual level as one cannot rotate something that has no data. Feel free to disagree :)
…ert-8353-revert-8159-allow-perma-dataset-rotation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/javascripts/types/api_flow_types.ts (1)
64-74: Well-structured transformation types with room for documentation.The transformation types are well-designed, supporting both affine and non-linear transformations. The row-major order specification for the matrix is crucial for correct usage.
Consider adding JSDoc comments to document:
- The mathematical properties of each transformation type
- Example usage scenarios
- The expected format and constraints of the transformation matrices
- The meaning of source/target correspondences in thin plate spline transformations
+/** + * Represents an affine transformation using a 4x4 matrix. + * Supports operations like rotation, scaling, translation, and shearing. + */ export type AffineTransformation = { type: "affine"; matrix: NestedMatrix4; // Stored in row major order. }; +/** + * Represents a non-linear transformation using thin plate spline. + * Useful for smooth interpolation between corresponding points. + */ export type ThinPlateSplineTransformation = { type: "thin_plate_spline"; correspondences: { source: Vector3[]; target: Vector3[] }; };frontend/javascripts/oxalis/model_initialization.ts (1)
924-945: Consider adding a null check for annotation layers.The implementation correctly handles edge cases, but the optional chaining in the
isNativelyRenderedNamePresentcheck might not be sufficient. Ifannotationis defined butannotationLayersis undefined, the check could fail silently.Consider this safer implementation:
const isNativelyRenderedNamePresent = dataset.dataSource.dataLayers.some( (layer) => layer.name === originalDatasetSettings.nativelyRenderedLayerName || (layer.category === "segmentation" && layer.fallbackLayer === originalDatasetSettings.nativelyRenderedLayerName), ) || - annotation?.annotationLayers.some( + (annotation?.annotationLayers ?? []).some( (layer) => layer.name === originalDatasetSettings.nativelyRenderedLayerName, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/api/api_latest.ts(1 hunks)frontend/javascripts/oxalis/merger_mode.ts(1 hunks)frontend/javascripts/oxalis/model.ts(2 hunks)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts(3 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/dataset_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/types/api_flow_types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.unreleased.md
- frontend/javascripts/oxalis/model/sagas/dataset_saga.ts
- frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts
- frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts
- frontend/javascripts/oxalis/model.ts
- frontend/javascripts/oxalis/store.ts
- frontend/javascripts/oxalis/merger_mode.ts
- frontend/javascripts/oxalis/api/api_latest.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (9)
frontend/javascripts/types/api_flow_types.ts (2)
6-6: LGTM!The
NestedMatrix4type import is correctly placed with related geometric types.
100-101: LGTM!Good documentation clarifying that the
nameshould be the skeleton tracing ID for uniqueness.frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (5)
52-65: LGTM! Well-organized imports for transformation functionality.The new imports are properly organized and clearly indicate the separation of concerns between dataset transformations and flycam (camera) operations.
202-206: LGTM! Clear state management for transformation support.The new state variables provide a clear way to track transformation capabilities and configuration status.
224-229: LGTM! Clear documentation of transformation constraints.The comments effectively explain the constraints around toggling transformations for different layer types.
256-264: LGTM! Well-structured styling with proper accessibility.The style object is well-organized and properly handles disabled states with cursor and opacity changes.
318-319: LGTM! Clear bounding box coordinate display.The bounding box coordinates are properly displayed in the tooltip.
frontend/javascripts/oxalis/model_initialization.ts (2)
109-110: LGTM: New imports are correctly added.The imports for
Mutabletype anddoAllLayersHaveTheSameRotationfunction are properly added and align with their usage in the code.
545-545: LGTM: Robust handling of coordinate transformations.The implementation correctly handles coordinate transformations by:
- Checking rotation consistency across layers
- Using the first layer's transformations if all layers have the same rotation
- Falling back to the fallback layer's transformations if available
Let's verify the rotation consistency check:
Also applies to: 563-572, 594-594
✅ Verification successful
✓ Verified: Rotation consistency check is correctly implemented
The
doAllLayersHaveTheSameRotationfunction performs thorough validation of layer rotations by:
- Validating transformation existence and count
- Ensuring only affine transformations are present
- Verifying the transformation pattern across layers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of doAllLayersHaveTheSameRotation # Test: Search for the implementation to ensure it correctly compares rotations ast-grep --pattern 'export function doAllLayersHaveTheSameRotation($$$) { $$$ }'Length of output: 84
Script:
#!/bin/bash # Search for any occurrence of the function name or rotation-related code rg -l "doAllLayersHaveTheSameRotation|rotation" frontend/javascripts/oxalis/model_initialization.ts # Show the context around any rotation-related code rg -B 5 -A 10 "rotation" frontend/javascripts/oxalis/model_initialization.tsLength of output: 2416
Script:
#!/bin/bash # Search for the function in the entire codebase rg -g '*.ts' -g '*.js' -B 2 -A 10 "function doAllLayersHaveTheSameRotation|function.*SameRotation|checkRotation" # Search for files that might contain layer rotation comparisons fd -e ts -e js | xargs rg -l "layer.*rotation|rotation.*layer"Length of output: 2008
Script:
#!/bin/bash # Get the complete function implementation rg -B 2 -A 20 "function _doAllLayersHaveTheSameRotation" frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.tsLength of output: 1153
| isRenderedNatively | ||
| ? `This layer is shown natively (i.e., without any transformations).${isDisabled ? "" : " Click to render this layer with its configured transforms."}` | ||
| : `This layer is rendered with ${ | ||
| typeToLabel[transform.type] | ||
| } transformation. Click to render this layer without any transforms.` | ||
| : "This layer is shown natively (i.e., without any transformations)." | ||
| } transformation.${isDisabled ? "" : " Click to render this layer without any transforms."}` |
There was a problem hiding this comment.
Improve tooltip text construction to avoid potential undefined access.
The tooltip text construction could throw an error if transform.type is undefined when accessing typeToLabel[transform.type].
Apply this diff to add a null check:
- ? `This layer is shown natively (i.e., without any transformations).${isDisabled ? "" : " Click to render this layer with its configured transforms."}`
- : `This layer is rendered with ${
- typeToLabel[transform.type]
- } transformation.${isDisabled ? "" : " Click to render this layer without any transforms."}`
+ ? `This layer is shown natively (i.e., without any transformations).${
+ isDisabled ? "" : " Click to render this layer with its configured transforms."
+ }`
+ : `This layer is rendered with ${
+ transform?.type ? typeToLabel[transform.type] : "an unknown"
+ } transformation.${
+ isDisabled ? "" : " Click to render this layer without any transforms."
+ }`📝 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.
| isRenderedNatively | |
| ? `This layer is shown natively (i.e., without any transformations).${isDisabled ? "" : " Click to render this layer with its configured transforms."}` | |
| : `This layer is rendered with ${ | |
| typeToLabel[transform.type] | |
| } transformation. Click to render this layer without any transforms.` | |
| : "This layer is shown natively (i.e., without any transformations)." | |
| } transformation.${isDisabled ? "" : " Click to render this layer without any transforms."}` | |
| isRenderedNatively | |
| ? `This layer is shown natively (i.e., without any transformations).${ | |
| isDisabled ? "" : " Click to render this layer with its configured transforms." | |
| }` | |
| : `This layer is rendered with ${ | |
| transform?.type ? typeToLabel[transform.type] : "an unknown" | |
| } transformation.${ | |
| isDisabled ? "" : " Click to render this layer without any transforms." | |
| }` |
MichaelBuessemeyer
deleted the
revert-8353-revert-8159-allow-perma-dataset-rotation
branch
Reverts #8353
And re adds: #8159
Related Slack canvas: https://scm.slack.com/docs/T02A8MN9K/F083DBRCFC6
--> 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)