Revert "Allow permanent dataset layer rotation in dataset settings" #8353
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the dataset transformation and rotation handling in the frontend codebase. The modifications involve restructuring how transformations are managed, removing specific rotation-related components, and consolidating transformation-related functions into a single module. The changes primarily affect how dataset layers are transformed, with a focus on simplifying the import structure and reducing complexity in transformation logic. Changes
Possibly related PRs
Suggested Labels
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: 1
🧹 Nitpick comments (7)
frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (3)
718-750: Clarify handling of multiple or unsupported transforms in_getOriginalTransformsForLayerOrNull.This function logs an error and returns null if multiple transforms are present or if the transform type is unsupported. Consider:
- Throwing an exception instead of silently returning null, if multiple transforms are truly not supported.
- Logging additional context (e.g., dataset ID, layer name) to aid diagnostics.
789-799: Memoization approach inmemoizeWithThreeKeys.Manually managing a MultiKeyMap can be efficient, but watch out for memory usage if it’s storing many large keys. Ensure that the intended key space remains small and that the function is cleared appropriately if needed.
875-878: Refine dataset transform check inhasDatasetTransforms.The function quickly checks if any layer has transforms. Consider expanding the logging if transforms exist but fail to apply.
frontend/javascripts/oxalis/model_initialization.ts (1)
848-849: Validate necessity of deep cloning
Deep cloning the original dataset settings helps avoid unintended mutations. However, if performance concerns arise, consider whether a shallow clone would suffice—particularly if nested objects withinoriginalDatasetSettingsdo not need to be deeply copied.frontend/javascripts/types/api_flow_types.ts (1)
63-71: Redefined CoordinateTransformation type
Switching from a nested matrix to a typed array ofVector4clarifies dimensional expectations for affine transformations. Verify that all references toCoordinateTransformationin your codebase handle the new union type, including potential migrations from the removedNestedMatrix4.frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (2)
1236-1236: Consider passing a full skeleton layer object for clarity.
Currently, an inline object with{ category: "skeleton" }is used, which may cause confusion if you need to expand skeleton properties in the future.
766-767: Check lock/unlock icon consistency.
Hovering shows a different icon, which can be intuitive if used intentionally, but consider confirming that the icon states align with the actual locked/unlocked transitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
CHANGELOG.unreleased.md(0 hunks)frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(0 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(0 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx(0 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx(1 hunks)frontend/javascripts/libs/mjs.ts(0 hunks)frontend/javascripts/oxalis/api/api_latest.ts(1 hunks)frontend/javascripts/oxalis/constants.ts(0 hunks)frontend/javascripts/oxalis/controller/combinations/volume_handlers.ts(2 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(1 hunks)frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts(3 hunks)frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts(0 hunks)frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts(3 hunks)frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts(3 hunks)frontend/javascripts/oxalis/model/accessors/tool_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts(0 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(1 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/context_menu.tsx(0 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(0 hunks)frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts(0 hunks)
💤 Files with no reviewable changes (11)
- CHANGELOG.unreleased.md
- frontend/javascripts/libs/mjs.ts
- frontend/javascripts/oxalis/constants.ts
- frontend/javascripts/types/schemas/dataset_view_configuration.schema.ts
- frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
- frontend/javascripts/types/globals.d.ts
- frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts
- frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx
- frontend/javascripts/oxalis/view/context_menu.tsx
- frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts
- frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx
✅ Files skipped from review due to trivial changes (11)
- frontend/javascripts/oxalis/model/sagas/quick_select_heuristic_saga.ts
- frontend/javascripts/oxalis/geometries/materials/node_shader.ts
- frontend/javascripts/oxalis/model/helpers/nml_helpers.ts
- frontend/javascripts/oxalis/api/api_latest.ts
- frontend/javascripts/admin/dataset/composition_wizard/04_configure_new_dataset.tsx
- frontend/javascripts/oxalis/model/accessors/tool_accessor.ts
- frontend/javascripts/oxalis/merger_mode.ts
- frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts
- frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx
- frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts
- frontend/javascripts/oxalis/store.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (37)
frontend/javascripts/test/reducers/flycam_reducer.spec.ts (1)
39-39: Ensure correct usage ofM4x4.identity.
IfM4x4.identityis a function that returns a new identity matrix, we should call it with parentheses to avoid referencing (and potentially mutating) a single static object. Please verify that referencingM4x4.identitydirectly is intended.Run the following script to confirm whether
M4x4.identityis a static property or a function within the codebase:frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts (3)
2-2: Removed unused imports is a good cleanup step.
SinceV3is no longer used, eliminating the redundant import fromlibs/mjshelps minimize clutter.
23-24: Confirm the correctness of the new import references.
Shifting transformation-related imports todataset_accessorappears consistent with broader changes. Ensure that the logic ingetTransformsForLayerandinvertAndTransposealigns with the new structure (especially if the previousdataset_layer_transformation_accessorcode is removed).
197-197: Clarify the usage ofM4x4.identityvs.M4x4.identity().
This substitution suggests thatM4x4.identityis now a property rather than a function. Verify that it delivers a fresh identity matrix copy each time, rather than a single shared object.frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (13)
3-3: Confirm correct usage of new imports for transformations.This import of M4x4, Matrix4x4, and V3 appears consistent with the rest of the file and seems well-structured for handling transformations and vector operations.
9-15: Ensure consistent usage of newly imported constants and types.The import statements for IdentityTransform, LongUnitToShortUnitMap, and type Vector3, Vector4, and ViewMode are well-structured. Make sure the usage throughout the file adheres to these validated types and constants.
32-32: New import for APISkeletonLayer is clear.No issues found. The addition is straightforward and well aligned with the rest of the type imports from the same module.
39-46: Centralize transformation helper imports.These transformation helper imports are properly co-located. Consider verifying that all transformation logic is consistently defined and consumed from this module to reduce duplication.
752-788: Check for edge cases around native rendering in_getTransformsForLayerOrNull.This function inverts transforms when another layer is “native.” Verify:
- Behavior when
nativelyRenderedLayerNamedoes not exist in the dataset.- Potential risk of returning null transforms if the native layer is invalid.
801-811: Fallback toIdentityTransformenhances robustness.Returning the IdentityTransform when no transform is found protects against null references. Implementation is concise and consistent.
813-834: Skeleton layer handling in_getTransformsForSkeletonLayerOrNull.This approach aligns skeleton transforms with the logic in
_getOriginalTransformsForLayerOrNull. Confirm that future skeleton-specific transforms (e.g., partial transforms) won’t trigger unexpected null returns.
836-847: Memoization forgetTransformsForSkeletonLayerOrNull.Implementation is consistent with the pattern above. Confirm that any dynamic changes to skeleton transforms are handled properly if the memoized result becomes stale.
848-860: Constructing transforms per layer in_getTransformsPerLayer.The iteration that assigns transforms for each layer is straightforward. Make sure performance is acceptable on datasets with large layer counts.
862-863: Memoized export forgetTransformsPerLayer.No issues. This is consistent with prior memoization usage.
864-873: Inline transform usage ingetInverseSegmentationTransformer.Returning a specialized transform function is flexible. Verify that the unscaled transform usage is always correct for segmentation.
880-887: Flattened-to-nested matrix conversion withflatToNestedMatrix.Straightforward partitioning of the 16-element array into 4x4 vectors. Implementation is concise and appears correct.
889-899: Preserve GPU compatibility ininvertAndTranspose.The memoized function satisfies common transformation usage for rendering. Confirm that all relevant usage sites respect the difference between math libraries that expect row-major vs. column-major ordering.
frontend/javascripts/oxalis/controller/combinations/volume_handlers.ts (2)
59-62: Validate direct usage ofglobalPosingetSegmentIdForPosition.The function no longer applies layer-specific transforms to the incoming position. Confirm that skipping transforms is intentional and that it won’t break segmentation logic for layers with custom transforms.
84-94: Confirm correct async bucket loading ingetSegmentIdForPositionAsync.
- Ensuring the newly retrieved zoom step and data are suitable for untransformed
globalPos.- Validate queries on multi-transformed datasets or skeleton layers.
frontend/javascripts/oxalis/geometries/materials/edge_shader.ts (1)
6-6: Updated import ofgetTransformsForSkeletonLayer.The function is now centralized in
dataset_accessor, which simplifies code organization. Make sure the new import path references the same transform logic to avoid unexpected discrepancies.frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts (3)
7-7: Function signature matches newly introduced Vector4-based structure.
Using a typed 4-element array of Vector4 for flattening improves clarity by exposing each row vector. This approach seems maintainable and consistent with the rest of the transform-related refactoring.
29-31: Align affine transform creation with the new matrix flattening logic.
The refactored function properly leveragesnestedToFlatMatrixfor consistency. Confirm no downline code still depends on the old nested matrix format.✅ Verification successful
Matrix transformation system is properly aligned ✅
The implementation correctly usesnestedToFlatMatrixand is part of a well-structured, type-safe transformation system. All matrix format conversions are consistent throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure no calls are left that pass a NestedMatrix4 to createAffineTransformFromMatrix rg 'createAffineTransformFromMatrix\s*\(' -A 2Length of output: 731
Script:
#!/bin/bash # Check implementation and context echo "=== Implementation of createAffineTransformFromMatrix ===" ast-grep --pattern 'export function createAffineTransformFromMatrix($_) { $$$ }' echo -e "\n=== Context of usage in dataset_accessor.ts ===" rg -B 5 -A 5 'createAffineTransformFromMatrix' frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts echo -e "\n=== Related matrix transformation utilities ===" rg 'nestedToFlatMatrix|flatToNestedMatrix' -A 2Length of output: 3740
5-5: Ensure references to Vector4 are consistently updated across codebase.
ReplacingNestedMatrix4with[Vector4, Vector4, Vector4, Vector4]is logical as it consolidates matrix handling under a single approach. Verify all references and type exports are updated to avoid errors in files that previously relied onNestedMatrix4.✅ Verification successful
Vector4 type usage is consistent across codebase
The migration fromNestedMatrix4to[Vector4, Vector4, Vector4, Vector4]is complete and consistent. All matrix-related operations use the new type pattern uniformly, with no remaining references to the old type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Script to find any references to 'NestedMatrix4' across the project and ensure they are removed or replaced properly rg 'NestedMatrix4' -A 3 -B 3Length of output: 28
Script:
#!/bin/bash # Search for Vector4 type references and matrix-related types echo "=== Vector4 References ===" rg "Vector4" -t typescript -A 2 -B 2 echo -e "\n=== Matrix-Related Type Definitions ===" ast-grep --pattern 'type $_ = [Vector4, Vector4, Vector4, Vector4]' echo -e "\n=== Type Exports with Vector4 ===" rg "export.*Vector4" -t typescriptLength of output: 454
Script:
#!/bin/bash # Search for Vector4 references in frontend directory echo "=== Vector4 References ===" rg "Vector4" ./frontend -A 2 -B 2 echo -e "\n=== Constants File Check ===" fd -e ts -e tsx constants.ts -x cat {} echo -e "\n=== Matrix Type Patterns ===" rg "\[Vector4,\s*Vector4,\s*Vector4,\s*Vector4\]" ./frontendLength of output: 59518
frontend/javascripts/oxalis/model/sagas/dataset_saga.ts (1)
18-19: ReinstatingisLayerVisibleusage.
Re-addingisLayerVisiblefromdataset_accessorclarifies the layer visibility check. Confirm that usage is correct in the offsets where it’s called to ensure we don’t replicate any previous logic issues.frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts (4)
29-30: Importing newly consolidated transformation logic.
Replacing older transformation imports withgetTransformsForSkeletonLayerandgetTransformsForSkeletonLayerOrNullis consistent with your refactoring strategy. Ensure old calls aren’t left in the code.
221-221: Use ofgetTransformsForSkeletonLayerOrNull.
This succinctly checks for skeleton-layer transformations. Double-check edge cases where the layer might not exist or be invalid.
234-235: Explicit assignment ofnativelyRenderedLayerName.
Storing the name in a local variable helps keep code readable and consistent with usage in the following lines. Just ensure no concurrency issues arise from referencing updated states.
242-243: Reuse local variable fornativelyRenderedLayerName.
Similar logic to lines 234-235. Maintaining consistent usage avoids confusion. Ensure any changes tostate.datasetConfigurationare properly accounted for before reusing.frontend/javascripts/oxalis/model.ts (1)
227-228: Directly retrievingadditionalCoordinatesfor segment ID lookup.
By cutting out extraneous coordinate transformations, the approach is simplified. Watch for potential mismatch if other code still expects transformed coordinates.frontend/javascripts/oxalis/model_initialization.ts (1)
845-845: Preserving the original dataset settings when annotation is absent
This return statement effectively reverts to using the unmodified dataset settings in scenarios without an annotation. This is consistent with the goal of removing rotation-specific logic.frontend/javascripts/types/api_flow_types.ts (2)
10-10: New Vector4 import aligns with 4×4 matrix representation
The addition ofVector4supports the new affine matrix structure. Ensure all code referencing 4×4 transformations consistently usesVector4to maintain type safety.
97-98: Simplified APISkeletonLayer
Removing thenameproperty fromAPISkeletonLayermakes its interface more concise. If any references toAPISkeletonLayer.nameremain, ensure they are also updated.frontend/javascripts/oxalis/controller/scene_controller.ts (1)
35-35: Refactored import for getTransformsForLayerOrNull
ImportinggetTransformsForLayerOrNullfromdataset_accessorrather than the deprecateddataset_layer_transformation_accessorindicates proper alignment with new transformation handling. Confirm that all references to the old module have been removed.frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (4)
57-59: Imports look good.
The usage ofgetWidestMagsandhasDatasetTransformsis valid and consistent with the rest of the code.
60-60: Import usage confirmed.
getMaxZoomValueForMagandgetPositionare used correctly and help keep the code structured.
84-87: No issues with the transformation helpers import.
The logic forinvertTransformandtransformPointUnscaledappears consistent and properly integrated.
1186-1186: LGTM on destructuring showSkeletons.
This ensures clarity in referencing the skeleton's display state.
| const position = [matrix[12], matrix[13], matrix[14]]; | ||
| // Reset rotation | ||
| const newMatrix = rotateOnAxis(M4x4.scale(scale, M4x4.identity(), []), Math.PI, [0, 0, 1]); | ||
| const newMatrix = rotateOnAxis(M4x4.scale(scale, M4x4.identity, []), Math.PI, [0, 0, 1]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent unintended mutation of the identity matrix.
If M4x4.identity is not returning a fresh matrix instance on each reference, scaling or rotating it might unintentionally affect other parts of the code reusing the same matrix object. Consider calling it as a function (if it indeed is one) or cloning, to ensure a unique identity matrix is obtained:
-const newMatrix = rotateOnAxis(M4x4.scale(scale, M4x4.identity, []), Math.PI, [0, 0, 1]);
+const newMatrix = rotateOnAxis(M4x4.scale(scale, M4x4.identity(), []), Math.PI, [0, 0, 1]);Committable suggestion skipped: line range outside the PR's diff.
MichaelBuessemeyer
deleted the
revert-8159-allow-perma-dataset-rotation
branch
Reverts #8159
Seems like this PR caused error/bug: https://scm.slack.com/archives/C5AKLAV0B/p1737746802362229