Skip to content

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Mar 31, 2025

This is a follow up for #8159. The rotation feature now allows to mirror datasets along axes as well.

Moreover, the rotation settings no longer save a transformation in case it is "nulled" for every setting resulting in an identity transform.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open your favourite dataset in a tab in e.g. view mode.
  • duplicate that tab
  • Open the dataset settings in one of the tabs and configure one axis to be mirrored. then view the dataset again
  • Go to a specific slice and compare the result with the same position viewed in the tab that does not have the newly configured mirrored axis.
  • The data should be rendered mirrored just like configured. Compare some more dataset positions and confirm that this is correct.

Issues:


(Please delete unneeded items, merge only when none are left open)

Copy link
Contributor

coderabbitai bot commented Mar 31, 2025

📝 Walkthrough
## Walkthrough

The changes incorporate a new mirroring functionality into dataset rotation settings. A `Checkbox` component is added to allow users to specify if an axis rotation should be mirrored, and the rotation structure is updated to include both the rotation degree and the mirror flag. Function signatures and type names—renaming `DatasetRotation` to `DatasetRotationAndMirroringSettings`—are modified accordingly. Additionally, a layout adjustment is made in the dataset settings view, and the transformation accessor functions are revised to compute rotations using the new `RotationAndMirroringSettings` type.

## Changes

| File(s)                                                        | Change Summary                                                                                                                                            |
| -------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `frontend/.../dataset_rotation_form_item.tsx`, `frontend/.../dataset_settings_view.tsx` | Updated rotation settings: added a new Checkbox for mirroring, revised the structure to include `rotationInDegrees` and `isMirrored`, renamed `DatasetRotation` to `DatasetRotationAndMirroringSettings`, and modified function signatures accordingly. |
| `frontend/.../dataset_settings_data_tab.tsx`                   | Adjusted layout: increased the column span for the `AxisRotationSettingForDataset` component from `xl={6}` to `xl={12}`.                                   |
| `frontend/.../dataset_layer_transformation_accessor.ts`        | Enhanced rotation transformations: introduced an `axisPositionInMatrix` constant and a new `RotationAndMirroringSettings` type; updated and renamed rotation functions to compute both rotation degrees and the mirror state; added identity transformation check. |

## Assessment against linked issues

| Objective                          | Addressed | Explanation |
| --------------------------------- | --------- | ----------- |
| Add Flip in dataset rotation (#8431) ||             |

## Suggested labels

`usability`

## Suggested reviewers

- philippotto

## Poem

> I’m a rabbit in a code-filled glen,  
> Hopping through changes time and again.  
> Mirrored rotations now dance in the light,  
> With checkboxes and settings shining bright.  
> A playful tweak that makes my heart skip a beat! 🐇  
> ASCII hops and happy codes complete my treat.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between abc92d6 and 30abf6d.

📒 Files selected for processing (2)
  • CHANGELOG.unreleased.md (1 hunks)
  • frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • CHANGELOG.unreleased.md
  • frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
  • GitHub Check: frontend-tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (1)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)

79-96: ⚠️ Potential issue

Bug in mirroring toggle handling.

When the isMirrored parameter is false, it won't update the rotation values because of the condition if (isMirrored). This means toggling mirroring off won't work correctly.

Apply this fix:

-if (isMirrored) {
-  rotationValues.isMirrored = isMirrored;
+if (isMirrored !== undefined) {
+  rotationValues.isMirrored = isMirrored;
}
🧹 Nitpick comments (3)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)

149-161: New checkbox for mirror functionality.

The added checkbox component allows users to toggle mirroring for each axis. The label and formatting look good, but the comment is misleading.

-label={`Mirror ${axis.toUpperCase()} Axis`} /* Whitespace label is needed for correct formatting*/
+label={`Mirror ${axis.toUpperCase()} Axis`}

Consider also adding a tooltip explaining what mirroring does:

<FormItem
  name={["datasetRotation", axis, "isMirrored"]}
  colon={false}
  valuePropName="checked"
-  label={`Mirror ${axis.toUpperCase()} Axis`} /* Whitespace label is needed for correct formatting*/
+  label={`Mirror ${axis.toUpperCase()} Axis`}
+  tooltip="Mirror the dataset across this axis, flipping the image orientation"
>
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (1)

212-213: Default initialization for rotation settings.

The code initializes rotation settings with default values for both rotation degrees and mirroring state.

Consider adding a comment explaining the default values, especially for the isMirrored property:

-const nulledSetting = { rotationInDegrees: 0, isMirrored: false };
+// Initialize with no rotation and no mirroring by default
+const nulledSetting = { rotationInDegrees: 0, isMirrored: false };
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (1)

407-409: Updated comments to indicate mirroring possibility.

The comments have been updated to indicate that the rotation transformations may include mirroring, which is helpful for understanding the transformation pipeline.

Consider adding a brief explanation of what mirroring means in this context:

-// 2. Rotation around x-axis (potentially mirrored)
-// 3. Rotation around y-axis (potentially mirrored)
-// 4. Rotation around z-axis (potentially mirrored)
+// 2. Rotation around x-axis (potentially mirrored - flipping the axis orientation)
+// 3. Rotation around y-axis (potentially mirrored - flipping the axis orientation)
+// 4. Rotation around z-axis (potentially mirrored - flipping the axis orientation)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 975d1dc and 0402de6.

📒 Files selected for processing (4)
  • frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (7 hunks)
  • frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1 hunks)
  • frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (4 hunks)
  • frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (5 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (2)
  • RotationSetting (76-79)
  • getRotationMatrixAroundAxis (123-144)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (2)
frontend/javascripts/types/api_flow_types.ts (2)
  • CoordinateTransformation (74-74)
  • AffineTransformation (64-67)
frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts (1)
  • nestedToFlatMatrix (7-9)
frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (2)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
  • DatasetRotationSettings (170-174)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (2)
  • EXPECTED_TRANSFORMATION_LENGTH (411-411)
  • getRotationSettingsFromTransformationIn90DegreeSteps (82-105)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: circleci_build
🔇 Additional comments (11)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)

304-305: Appropriate UI adjustment for new mirroring functionality.

The column width has been increased from xl={6} to xl={12} to accommodate the new mirroring controls in the AxisRotationSettingForDataset component. This layout change ensures that the rotation settings and mirroring checkboxes have sufficient space.

frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)

4-4: Added checkbox import for new mirroring functionality.

The import of the Checkbox component is required for the newly added mirroring controls.


58-62: Updated rotation values structure to support mirroring.

The rotation values structure now uses the RotationSetting type which includes both rotation degree and mirroring information.


170-174: Type renamed for better clarity.

The type has been renamed from DatasetRotation to DatasetRotationSettings to better represent its purpose, and its structure has been updated to use the new RotationSetting type for each axis.

frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (3)

31-31: Updated function name to reflect its expanded functionality.

The function has been renamed from getRotationFromTransformationIn90DegreeSteps to getRotationSettingsFromTransformationIn90DegreeSteps to reflect that it now returns both rotation degree and mirroring information.


85-85: Updated FormData type to use new DatasetRotationSettings.

The FormData type has been updated to use the renamed and restructured DatasetRotationSettings type.


216-229: Updated to use new rotation settings structure.

The code now correctly initializes rotation settings for each axis using the new getRotationSettingsFromTransformationIn90DegreeSteps function, which returns both rotation degree and mirroring information.

frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (4)

74-74: Added helper constant for axis indexing.

This new constant maps axis names to their corresponding positions in the transformation matrix, making the code more readable and maintainable.


76-79: New type to support mirroring functionality.

The new RotationSetting type nicely encapsulates both the rotation angle and mirroring state, providing a clean way to handle these related properties together.


81-105: Updated rotation extraction function to include mirroring detection.

The function now detects both rotation angle and whether mirroring is applied, returning this information as a RotationSetting object. The implementation correctly identifies mirroring by checking the sign of the diagonal matrix element corresponding to the axis.


123-144: Added mirroring support to rotation matrix generation.

The function now handles mirroring by conditionally applying a scale transformation that flips the axis. The implementation correctly combines rotation and mirroring transformations.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)

4-4: Consider unifying your imports from Ant Design.

Importing directly from "antd" rather than "antd/lib/checkbox/Checkbox" can help maintain consistency across your codebase:

- import Checkbox, { type CheckboxChangeEvent } from "antd/lib/checkbox/Checkbox";
+ import { Checkbox, type CheckboxChangeEvent } from "antd";
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0402de6 and c352a75.

📒 Files selected for processing (2)
  • frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (8 hunks)
  • frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (6 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
  • RotationSetting (76-79)
  • getRotationMatrixAroundAxis (123-144)
  • transformationEqualsAffineIdentityTransform (463-483)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
frontend/javascripts/types/api_flow_types.ts (2)
  • CoordinateTransformation (74-74)
  • AffineTransformation (64-67)
frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts (4)
  • nestedToFlatMatrix (7-9)
  • chainTransforms (89-163)
  • createAffineTransformFromMatrix (29-32)
  • Transform (11-27)
frontend/javascripts/oxalis/constants.ts (1)
  • IdentityTransform (385-389)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: circleci_build
🔇 Additional comments (18)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (11)

9-9: The added import for RotationSetting looks good.


14-14: Importing transformationEqualsAffineIdentityTransform is appropriate for pruning identity transformations.


59-63: Defining a structured rotationValues object enhances clarity.


80-81: Allowing isMirrored as an optional parameter is a solid extension of the API.


110-116: Setting transformations to null when they are effectively identity is a good space-saver.


125-125: Adjusting the Col layout span to 8 is a fine layout change.


127-127: Using ["datasetRotation", axis, "rotationInDegrees"] aligns with your new rotation model.


137-137: Keeping the field name consistent for the InputNumber is correct.


153-153: Changing the layout span for the next column to 4 is consistent with the UI adjustments.


154-166: Adding the isMirrored checkbox integrates well with the refined rotation logic.

Though note that toggling from checked to unchecked depends on the fix suggested in lines 90-96 for properly clearing the flag.


175-178: Renaming and exporting DatasetRotationSettings improves type clarity for all axes.

frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (7)

74-74: Introducing axisPositionInMatrix clarifies index lookups for different axes.


76-79: Defining RotationSetting as a distinct type is a clean approach to storing rotation angles and mirroring.


80-105: getRotationSettingsFromTransformationIn90DegreeSteps neatly extracts angle and mirroring details in 90° increments.


125-137: getRotationMatrixAroundAxis correctly applies both rotation and mirroring.


384-392: Mirroring check relies on vector length, which can miss negative scales.

Use an absolute value approach per axis to accurately detect mirrored transformations:

- return translation.length() === 0 && scale.length() === NON_SCALED_VECTOR.length();
+ return translation.length() === 0 &&
+   Math.abs(scale.x) === 1 && Math.abs(scale.y) === 1 && Math.abs(scale.z) === 1;

407-409: These updated comments help clarify that each axis can be mirrored.


463-483: transformationEqualsAffineIdentityTransform is a thorough check for identity transformations.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (2)

81-81: Update function signature to handle degrees=0 case.

The function signature has been updated to accept an optional isMirrored boolean, but rotationInDegrees is typed as number | undefined which is different from the previous type of just number.

- (rotationInDegrees: number | undefined, isMirrored?: boolean): void => {
+ (rotationInDegrees: number | null | undefined, isMirrored?: boolean): void => {

This would be more consistent with how you handle the InputNumber in lines 146-149 where null is a possible value.


153-166: Consider adding a tooltip to explain the mirror feature.

The mirroring checkbox lacks additional explanation that would help users understand what mirroring does. Consider adding a tooltip similar to what you've done for the rotation slider.

  <FormItem
    name={["datasetRotation", axis, "isMirrored"]}
    colon={false}
    valuePropName="checked"
-   label={`Mirror ${axis.toUpperCase()} Axis`} /* Whitespace label is needed for correct formatting*/
+   label={`Mirror ${axis.toUpperCase()} Axis`}
  >
    <Checkbox
      onChange={(evt: CheckboxChangeEvent) =>
        setMatrixRotationsForAllLayer(undefined, evt.target.checked)
      }
+     title={`Flip the dataset along the ${axis}-axis.`}
    />
  </FormItem>

Or, for consistency with the rotation controls, use the FormItemWithInfo component:

- <FormItem
+ <FormItemWithInfo
    name={["datasetRotation", axis, "isMirrored"]}
    colon={false}
    valuePropName="checked"
    label={`Mirror ${axis.toUpperCase()} Axis`}
+   info={`Flip the dataset along the ${axis}-axis.`}
  >
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c352a75 and a72341b.

📒 Files selected for processing (2)
  • CHANGELOG.unreleased.md (1 hunks)
  • frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
  • RotationSetting (76-79)
  • getRotationMatrixAroundAxis (123-144)
  • transformationEqualsAffineIdentityTransform (463-483)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)

94-96: The isMirrored toggle issue has been properly fixed.

The code now correctly handles toggling off the isMirrored flag by checking if it's explicitly defined rather than truthy.


110-113: Good optimization for identity transforms.

Setting transformations to null when they equal the identity transform is a smart optimization that avoids storing unnecessary data.


175-179: Good type renaming for clarity.

Renaming DatasetRotation to DatasetRotationSettings better reflects the purpose of this type, especially now that it encapsulates both rotation degrees and mirroring flags.

CHANGELOG.unreleased.md (1)

16-16: LGTM! Clear and concise changelog entry.

The changelog entry correctly documents the new mirroring feature and includes the PR reference.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)

146-149: Consider handling undefined values in the InputNumber onChange handler.

While the handler correctly checks for null values, it might be worth explicitly handling undefined values as well for completeness.

-            onChange={(value: number | null) =>
-              // InputNumber might be called with null, so we need to check for that.
-              value != null && setMatrixRotationsForAllLayer(value)
+            onChange={(value: number | null) =>
+              // InputNumber might be called with null or undefined, so we need to check for that.
+              value != null && setMatrixRotationsForAllLayer(value)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a72341b and a7dbbf7.

📒 Files selected for processing (1)
  • frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
  • RotationSetting (76-79)
  • getRotationMatrixAroundAxis (123-144)
  • transformationEqualsAffineIdentityTransform (463-483)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: circleci_build
🔇 Additional comments (7)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (7)

4-4: Good addition of required imports for the new mirroring functionality.

The necessary imports for Checkbox component, RotationSetting type, and the transformationEqualsAffineIdentityTransform function have been correctly added.

Also applies to: 9-9, 14-14


59-63: Well-structured implementation of the RotationSetting type.

The rotation values structure has been properly updated to use the RotationSetting type for each axis, which supports the new mirroring functionality.


80-81: Good function signature update for mirroring support.

The setMatrixRotationsForAllLayer function signature has been appropriately updated to support setting rotation degrees and mirroring independently.


90-97: Robust implementation for handling rotation and mirroring settings.

The implementation correctly handles both rotation degrees and mirroring flags, with proper undefined checks to ensure values are only updated when explicitly set. This properly addresses the previous issues with toggling off mirroring and setting rotation to 0 degrees.


110-113: Excellent optimization for identity transformations.

Setting coordinateTransformations to null when they equal the identity transform is a good optimization that prevents storing unnecessary identity transformations.


125-166: Well-designed UI layout for the mirroring functionality.

The layout has been effectively reorganized to accommodate the new mirroring checkbox, with appropriate column spans and form item configurations. The connection between the UI and the underlying data model is well implemented.


175-179: Good type renaming for clarity.

Renaming DatasetRotation to DatasetRotationSettings and updating its structure to use RotationSetting for each axis improves code clarity and better reflects the expanded functionality.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good overall 👍 only left a few comments :)

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your throughout review 🙏

Please have a look at my comments below

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)

59-63: Ensure resilience to missing form data.
The code looks good, but consider guarding against undefined or partially absent fields from the form.

frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (1)

384-392: Allowing rotation with optional mirroring.
isOnlyRotatedOrMirrored correctly checks zero translation and ±1 scale. Small floating-point variations may arise, but likely acceptable.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7dbbf7 and abc92d6.

📒 Files selected for processing (4)
  • CHANGELOG.unreleased.md (1 hunks)
  • frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (8 hunks)
  • frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx (4 hunks)
  • frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • CHANGELOG.unreleased.md
  • frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx
🧰 Additional context used
🧠 Learnings (1)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-02T13:29:57.427Z
Learning: The function `isRotationAndMirrorMaybeOnly` in the dataset_layer_transformation_accessor.ts is intentionally designed to allow mirroring transformations (negative scale values). It uses the length comparison (`scale.length() === NON_SCALED_VECTOR.length()`) rather than component equality to permit mirrored axes while ensuring the overall scale magnitude remains the same.
🧬 Code Definitions (2)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
  • RotationAndMirroringSettings (76-79)
  • getRotationMatrixAroundAxis (123-144)
  • transformationEqualsAffineIdentityTransform (466-485)
frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (3)
frontend/javascripts/types/api_flow_types.ts (2)
  • CoordinateTransformation (88-88)
  • AffineTransformation (78-81)
frontend/javascripts/oxalis/model/helpers/transformation_helpers.ts (4)
  • nestedToFlatMatrix (7-9)
  • chainTransforms (89-163)
  • createAffineTransformFromMatrix (29-32)
  • Transform (11-27)
frontend/javascripts/oxalis/constants.ts (1)
  • IdentityTransform (385-389)
🔇 Additional comments (22)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (12)

4-4: Import from antd Checkbox library is correct.
No issues identified here.


9-9: Properly importing the new type.
Including RotationAndMirroringSettings aligns with the updated transformations.


14-14: Importing the identity transformation check.
This import cleanly references the helper for skipping identity transforms.


80-81: Extended setter signature for mirroring.
Allowing isMirrored to be optional is consistent with partial updates.


90-98: Partial updates preserve toggling off.
This conditional assignment correctly updates only the provided properties, preventing stale values.


112-115: Verify setting transformations to null for the identity case.
Nulling coordinate transformations if they are effectively no-ops is valid, but confirm it won't break downstream consumers expecting an array.


118-118: Assigning updated transformations.
Straightforward approach to store the new transformations.


127-127: Layout adjustment.
Changing the column span is purely a UI concern and seems fine.


129-129: Linking rotation controls to the form.
Binding rotation values to "datasetRotation" is consistent.


139-139: Consistent name property usage.
Coordinates the same "rotationInDegrees" field for the numeric input.


155-168: Mirroring checkbox added.
This correctly updates isMirrored upon user toggling. Good integration with setMatrixRotationsForAllLayer.


177-180: Introducing a combined type for rotation and mirroring.
Renaming to DatasetRotationAndMirroringSettings is clear and descriptive.

frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts (10)

74-74: Matrix index mapping for axes.
This helper object provides clarity in accessing matrix entries per axis.


76-79: Definition of rotation and mirroring settings.
Straightforward type combining angle and boolean mirror flag.


82-85: Renamed function to reflect multiple properties.
getRotationSettingsFromTransformationIn90DegreeSteps clarifies that mirroring is also inferred.


87-87: Fallback for non-affine transformations.
Returning { rotationInDegrees: 0, isMirrored: false } is suitable for ignoring non-affine transforms.


90-90: Mirroring detection via the diagonal component.
This negative check on the axis diagonal is a concise solution for a flipped axis sign.


104-104: Protected return structure.
Safely returns a valid angle and the new mirror state.


123-123: Updated function signature for rotation and mirroring.
This ensures the matrix can handle both transformations.


128-137: Combining rotation and scale for mirroring.
Applying a negative scale when isMirrored is true is a well-established approach.


410-412: Descriptive axis-level documentation.
Renaming to reflect potential mirroring for each axis is clear.


466-485: Identity transformation detection.
The check ensures each transformation is affine and then reduces them to confirm identity. This is consistent and readable.

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Apr 16, 2025

@philippotto 🐱

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, this slipped through! code looks good and works well, too :)

@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) April 22, 2025 08:14
@MichaelBuessemeyer MichaelBuessemeyer merged commit 1a9ee86 into master Apr 22, 2025
5 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the mirror-datasets branch April 22, 2025 08:25
@coderabbitai coderabbitai bot mentioned this pull request Jul 21, 2025
22 tasks
@coderabbitai coderabbitai bot mentioned this pull request Sep 17, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Flip in dataset rotation
2 participants