Refactor labelling code in preparation to support transformed layers #9023
Conversation
📝 WalkthroughWalkthroughExtracts dataset rotation construction into an exported helper; renames volume-tracing concepts from VolumeLayer/ADD_TO_LAYER to SectionLabeler/ADD_TO_CONTOUR_LIST across actions, reducers, sagas, helpers and tests; shifts segmentation edits to layer-space via a new helper; tightens SceneController visibility and removes Drawing smoothing API. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
philippotto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(3 hunks)frontend/javascripts/libs/drawing.ts(4 hunks)frontend/javascripts/test/global_mocks.ts(1 hunks)frontend/javascripts/test/model/transformed_section_labeler.spec.ts(1 hunks)frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts(4 hunks)frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts(3 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(7 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(6 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts(8 hunks)frontend/javascripts/viewer/constants.ts(1 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(2 hunks)frontend/javascripts/viewer/controller/scene_controller.ts(4 hunks)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts(1 hunks)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts(3 hunks)frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts(1 hunks)frontend/javascripts/viewer/model/helpers/action_logger_middleware.ts(1 hunks)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(2 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/helpers.ts(6 hunks)frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(8 hunks)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts(15 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
Applied to files:
frontend/javascripts/test/model/transformed_section_labeler.spec.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/constants.tsfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-04-01T09:45:17.527Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
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.
Applied to files:
frontend/javascripts/test/model/transformed_section_labeler.spec.tsfrontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsxfrontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.tsfrontend/javascripts/viewer/model/helpers/transformation_helpers.ts
📚 Learning: 2025-08-14T14:12:03.293Z
Learnt from: hotzenklotz
Repo: scalableminds/webknossos PR: 8849
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/components/collapsible_split_merger_evaluation_settings.tsx:39-51
Timestamp: 2025-08-14T14:12:03.293Z
Learning: Since Ant Design version 5.18, Form.Item components support the layout prop (layout="horizontal", layout="vertical", etc.), allowing individual form items to override the parent Form's layout setting. This is a newer API addition that provides more granular control over form item layouts.
Applied to files:
frontend/javascripts/test/global_mocks.tsfrontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx
📚 Learning: 2025-08-14T14:12:03.293Z
Learnt from: hotzenklotz
Repo: scalableminds/webknossos PR: 8849
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/components/collapsible_split_merger_evaluation_settings.tsx:39-51
Timestamp: 2025-08-14T14:12:03.293Z
Learning: Since Ant Design version 5.18, Form.Item components support the layout prop (layout="horizontal", layout="vertical", layout="inline"), allowing individual form items to override the parent Form's layout setting. This enables mixed layouts within a single form and was added as a new API feature in v5.18 according to the official changelog.
Applied to files:
frontend/javascripts/test/global_mocks.tsfrontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.tsfrontend/javascripts/test/reducers/volumetracing_reducer.spec.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.tsfrontend/javascripts/viewer/constants.tsfrontend/javascripts/viewer/model/actions/volumetracing_actions.tsfrontend/javascripts/viewer/controller/combinations/volume_handlers.tsfrontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.tsfrontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.tsfrontend/javascripts/viewer/model/sagas/volumetracing_saga.tsxfrontend/javascripts/viewer/model/sagas/volume/helpers.tsfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/test/reducers/volumetracing_reducer.spec.tsfrontend/javascripts/viewer/model/helpers/action_logger_middleware.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.tsfrontend/javascripts/viewer/model/actions/volumetracing_actions.tsfrontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/test/reducers/volumetracing_reducer.spec.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Applied to files:
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.tsfrontend/javascripts/viewer/model/bucket_data_handling/data_cube.tsfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-07-22T12:56:47.507Z
Learnt from: daniel-wer
Repo: scalableminds/webknossos PR: 8787
File: frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts:132-136
Timestamp: 2025-07-22T12:56:47.507Z
Learning: Three.js Vector3 has explicitly defined x, y, and z properties that can be safely accessed via dynamic string indexing when using union types like "x" | "y" | "z". The pattern `vector[axis] = value` where axis is "x" | "y" | "z" does not cause TypeScript strict mode errors because TypeScript can infer the property access is valid.
Applied to files:
frontend/javascripts/viewer/constants.tsfrontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts
📚 Learning: 2024-11-25T14:38:49.345Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.tsfrontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2025-05-15T19:44:16.110Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8492
File: frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts:671-674
Timestamp: 2025-05-15T19:44:16.110Z
Learning: Visibility updates for user bounding boxes are intentionally handled separately from other property updates, with dedicated update actions for visibility changes.
Applied to files:
frontend/javascripts/viewer/controller/scene_controller.ts
🧬 Code graph analysis (18)
frontend/javascripts/test/model/transformed_section_labeler.spec.ts (5)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
mapTransformedPlane(592-636)frontend/javascripts/viewer/constants.ts (2)
OrthoView(66-66)IdentityTransform(387-391)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (1)
Transform(11-27)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
combineCoordinateTransformations(191-212)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
getRotationalTransformation(225-240)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (4)
addToContourListAction(180-184)finishEditingAction(201-204)setActiveCellAction(206-218)startEditingAction(173-178)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (2)
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (4)
RotationAndMirroringSettings(76-79)fromCenterToOrigin(107-113)getRotationMatrixAroundAxis(123-144)fromOriginToCenter(115-121)frontend/javascripts/types/api_types.ts (1)
AffineTransformation(81-84)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts (2)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (5)
addToContourListAction(180-184)finishEditingAction(201-204)setActiveCellAction(206-218)startEditingAction(173-178)setContourTracingModeAction(377-381)frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
setToolAction(111-115)
frontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.ts (1)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (1)
createSectionLabeler(282-301)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (6)
frontend/javascripts/viewer/constants.ts (3)
Point2(27-30)OrthoView(66-66)Vector3(14-14)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (1)
calculateGlobalPos(338-338)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (5)
setContourTracingModeAction(377-381)startEditingAction(173-178)addToContourListAction(180-184)finishEditingAction(201-204)resetContourAction(355-358)frontend/javascripts/viewer/store.ts (1)
WebknossosState(575-595)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
transformPointUnscaled(165-172)invertTransform(71-87)
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (2)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
Transform(11-27)createThinPlateSplineTransform(53-69)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3Indices(50-50)
frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts (1)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
addToContourListAction(180-184)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (1)
addToContourListReducer(113-127)
frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
finishSectionLabeler(362-387)
frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts (1)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (1)
createSectionLabeler(282-301)
frontend/javascripts/viewer/controller/scene_controller.ts (3)
frontend/javascripts/viewer/constants.ts (2)
OrthoViewMap(69-69)OrthoViewWithoutTDMap(70-70)frontend/javascripts/viewer/geometries/helper_geometries.ts (3)
ContourGeometry(31-172)QuickSelectGeometry(181-340)LineMeasurementGeometry(343-459)frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
SegmentMeshController(80-707)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (3)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (3)
createSectionLabeler(282-301)labelWithVoxelBuffer2D(174-280)BooleanBox(44-46)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (4)
startEditingAction(173-178)finishEditingAction(201-204)addToContourListAction(180-184)registerLabelPointAction(349-353)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
TransformedSectionLabeler(654-736)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (4)
frontend/javascripts/viewer/store.ts (1)
VolumeTracing(192-213)frontend/javascripts/viewer/constants.ts (2)
OrthoView(66-66)Vector3(14-14)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
TransformedSectionLabeler(654-736)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (5)
frontend/javascripts/viewer/constants.ts (5)
Vector2(13-13)Vector3(14-14)OrthoView(66-66)Vector3Indices(50-50)Vector2Indices(49-49)frontend/javascripts/viewer/model/scaleinfo.ts (1)
getBaseVoxelFactorsInUnit(26-33)frontend/javascripts/viewer/model/helpers/position_converter.ts (1)
zoomedPositionToGlobalPosition(45-47)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (3)
Transform(11-27)transformPointUnscaled(165-172)invertTransform(71-87)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
invertAndTranspose(372-374)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (25)
frontend/javascripts/test/global_mocks.ts (1)
143-145: LGTM!The Typography mock follows the same pattern as other antd component mocks and appropriately ensures that tests referencing
Typography.Textwon't encounter resolution errors.frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts (2)
14-14: Import organization aligns with transformation refactor.The reordering of
IdentityTransformand addition ofgetTransformsPerLayerare consistent with the PR's transformation-related refactoring. Both imports are used in thegetDisabledVolumeInfofunction to check for segmentation layer transformations.Also applies to: 22-22
336-340: Need to see the full implementation of_getTransformsPerLayerto confirm the behavior:Let me look at the actual implementation directly in the repository:
Based on the script output and my analysis, I can now verify the concern. Let me examine the structure of the implementation more carefully by looking at what information we have:
From the earlier script output, I can see:
- Line 337:
const transformsPerLayer: Record<string, Transform> = {};- initializes empty map- The function iterates through
dataset.dataSource.dataLayers- Only entries with transforms would be added to this map
- Missing entries return
undefinedThe key insight is that if one of the operands is null or undefined, the other must also be null or undefined to return true when using strict equality. Therefore,
undefined !== IdentityTransformwill always betrue, which means untransformed layers (returningundefinedfrom the map lookup) will be incorrectly treated as "transformed."However, I need to determine if this is the intended behavior or a bug. Let me check the learnings to see if there's prior context:
The learnings mention that in
isRotationAndMirrorMaybeOnly, the function is "intentionally designed" to use length comparison rather than component equality. This suggests designers are careful about transform semantics. Let me generate the final response based on the evidence:Verify behavior of
getTransformsPerLayerfor layers without explicit transforms.The code at lines 336-340 accesses
getTransformsPerLayer(...)[segmentationTracingLayer.tracingId]without a fallback for missing keys. If a layer has no explicit transform, the map access returnsundefined, making the comparisonundefined !== IdentityTransformevaluate totrue. This would incorrectly mark untransformed layers as "transformed," disabling volume tools for layers that should support editing.Confirm whether layers without transforms should:
- Be absent from the returned map (current behavior, causes the issue), or
- Have an explicit
IdentityTransformentry, or- Use a nullish coalescing operator to default missing entries to
IdentityTransform.If missing entries should be treated as untransformed, apply a fix:
const isSegmentationTracingTransformed = segmentationTracingLayer != null && - getTransformsPerLayer(state.dataset, state.datasetConfiguration.nativelyRenderedLayerName)[ - segmentationTracingLayer.tracingId - ] !== IdentityTransform; + (getTransformsPerLayer(state.dataset, state.datasetConfiguration.nativelyRenderedLayerName)[ + segmentationTracingLayer.tracingId + ] ?? IdentityTransform) !== IdentityTransform;frontend/javascripts/libs/drawing.ts (1)
7-7: Good refactor: narrowing the public API surface.Making
drawHorizontalLine2d,addNextLine, andpaintBorderprivate correctly encapsulates internal drawing implementation details. Verification confirms these methods are called only internally within the Drawing class and have no external callers elsewhere in the codebase.frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (3)
6-6: LGTM: Import addition is necessary for the new function.The
AffineTransformationtype import is correctly added to support the return type ofgetRotationalTransformation.
67-67: LGTM: Clean refactor improves code organization.Replacing the inline transformation array construction with the extracted
getRotationalTransformationfunction improves code reusability and maintainability.
225-240: LGTM: Well-structured utility function for rotational transformations.This extracted function correctly implements the standard transformation sequence (center-to-origin → rotations → origin-to-center) and improves code organization. The export makes it available for reuse across the codebase, which aligns well with the PR's goal of supporting transformed layers.
frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (1)
169-171: LGTM - Stylistic refactor.The return statement is now inside the else block rather than after it. No functional change.
frontend/javascripts/test/model/transformed_section_labeler.spec.ts (1)
1-244: Well-structured test suite for plane mapping transformations.The tests cover identity, 90-degree rotations, and complex affine transformations. The skipped test at line 175 is acknowledged in the PR description as a known issue to be addressed.
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
187-212: LGTM - Extracted reusable transformation combination logic.The new
combineCoordinateTransformationsfunction parameterizes the scale factor, making it reusable for testing (as seen in transformed_section_labeler.spec.ts) and enabling more flexible transformation handling.frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
20-20: LGTM - Typo correction.Fixed misspelling:
Vector3Indicies→Vector3Indices, aligning with the corrected constant name in constants.ts.Also applies to: 247-250
frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts (1)
37-40: LGTM - Updated import path.Import path updated from
volumetracing/volumelayertovolumetracing/section_labelingto align with the broader refactoring from VolumeLayer to SectionLabeler abstractions.frontend/javascripts/viewer/model/sagas/volume/helpers.ts (3)
198-203: No action required — methods exist and are correctly implemented.Verification confirms that both
getTopLeft3DCoord()andgetBottomRight3DCoord()methods exist on the VoxelBuffer2D class (infrontend/javascripts/viewer/model/volumetracing/section_labeling.tsat lines 62–63) and are correctly imported and called in helpers.ts.
229-229: Verified: getValueFromGlobal method exists on VoxelBuffer2D.The method is properly defined in
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts:83with signaturegetValueFromGlobal(globalX: number, globalY: number): number, and is correctly imported and used in the helpers.ts file.
282-300: Verify whetherTransformedSectionLabeleroptimizes identity transforms or whether unnecessary overhead should be avoided.The refactored
createSectionLabelernow always returnsTransformedSectionLabeler, even when the underlying transform is identity (as returned bygetTransformsForLayeron line 268 of dataset_layer_transformation_accessor.ts).The issue:
transformPointUnscaledperforms type checking and applies transformations without detecting identity—even for theIdentityTransformcase, matrix operations are executed. Additionally,TransformedSectionLabeleralways wraps a baseSectionLabeler, adding delegation overhead.For layers without transforms, consider either:
- Early-returning a plain
SectionLabelerfromcreateSectionLabelerwhen transform is identity, or- Optimizing
transformPointUnscaledto detect and short-circuit identity transforms before performing matrix operations.frontend/javascripts/viewer/constants.ts (1)
49-53: ✓ LGTM - Typo correction verified and complete.Confirmed: All
VectorNIndiciesnames (N=2-6) have been corrected toVectorNIndices. No remaining usages of the old misspelled names found anywhere in the codebase. The numeric values and export structure remain unchanged.frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts (1)
11-11: LGTM! Consistent action rename.The import and dispatch calls have been correctly updated from
addToLayerActiontoaddToContourListAction, aligning with the broader refactoring in this PR.Also applies to: 63-63, 77-77
frontend/javascripts/viewer/model/helpers/action_logger_middleware.ts (1)
14-14: LGTM! Action blacklist updated correctly.The action type string has been properly updated from
"ADD_TO_LAYER"to"ADD_TO_CONTOUR_LIST"to match the renamed action.frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
24-24: LGTM! Comprehensive test updates.All occurrences of
addToLayerActionhave been correctly updated toaddToContourListActionthroughout the integration tests, maintaining test consistency with the renamed action.Also applies to: 72-72, 77-77, 132-132, 186-186, 236-236, 282-282
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (1)
113-127: LGTM! Reducer function renamed consistently.The function has been renamed from
addToLayerReducertoaddToContourListReducerwhile maintaining the same signature and implementation, aligning with the action rename throughout the codebase.frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts (1)
253-253: LGTM! Test variables renamed consistently.All test helper variables and function calls have been updated from
addToLayerActionFntoaddToContourListActionFn, maintaining test consistency with the action rename.Also applies to: 265-267, 277-277, 281-283, 299-299, 301-303
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
40-40: LGTM! Reducer updated consistently.The import, action case, and reducer function call have all been correctly updated to use the new
ADD_TO_CONTOUR_LISTaction andaddToContourListReducerfunction, maintaining correct reducer flow.Also applies to: 595-597
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts (1)
20-20: LGTM! Extensive test suite updated correctly.All dispatch calls throughout this comprehensive integration test suite have been correctly updated to use
addToContourListAction, ensuring test consistency with the renamed action.Also applies to: 65-65, 155-155, 319-319, 392-392, 467-468, 472-473, 528-529, 533-534, 539-540, 606-607, 610-611, 614-615
frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts (1)
63-63: LGTM! VolumeLayer to SectionLabeler refactor applied correctly.The refactoring from
createVolumeLayertocreateSectionLabelerhas been properly implemented:
- Import updated correctly
- Function call updated with same argument structure
- Variable renamed from
volumeLayertosectionLabeler- Method calls (
createVoxelBuffer2D,globalCoordToMag2DFloat) remain compatibleThis change aligns with the broader PR objective of refactoring to support transformed layers.
Also applies to: 515-527
frontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.ts (1)
377-385: Potential coordinate scaling inconsistency in voxel buffer positioning.On line 380,
targetOffsetWis added directly without scaling:V3.add(relevantBoxMag1.min, transpose([0, 0, targetOffsetW]))However, on line 375, the same
targetOffsetWis scaled bylabeledMag[thirdDim]:relevantBoxMag1.min[thirdDim] + labeledMag[thirdDim] * targetOffsetWAnd on line 335, slice offsets are also scaled by the mag:
transpose([0, 0, -directionFactor * interpolationDepth * labeledMag[thirdDim]])Since
targetOffsetWrepresents a slice index andrelevantBoxMag1.minis in global (mag1) coordinates, the offset should be scaled when converting to global coordinates. For consistency and correctness (especially with TransformedSectionLabeler that may depend on all 3D coordinates), line 380 should likely be:- sectionLabeler.globalCoordToMag2DFloat( - V3.add(relevantBoxMag1.min, transpose([0, 0, targetOffsetW])), - ), + sectionLabeler.globalCoordToMag2DFloat( + V3.add(relevantBoxMag1.min, transpose([0, 0, targetOffsetW * labeledMag[thirdDim]])), + ),Run the following script to check if
globalCoordToMag2DFloatuses all three dimensions or only the first two:
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
Outdated
Show resolved
Hide resolved
| globalCoordToMag2DFloat(position: Vector3): Vector2 { | ||
| return this.base.globalCoordToMag2DFloat(position); | ||
| } |
There was a problem hiding this comment.
Transform positions before querying the base labeler.
globalCoordToMag2DFloat is still delegating the untouched coordinate to the base SectionLabeler, so rotated viewers report the wrong (u, v). Transform the point first.
- return this.base.globalCoordToMag2DFloat(position);
+ const transformed = this.applyTransform(position);
+ return this.base.globalCoordToMag2DFloat(transformed);📝 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.
| globalCoordToMag2DFloat(position: Vector3): Vector2 { | |
| return this.base.globalCoordToMag2DFloat(position); | |
| } | |
| globalCoordToMag2DFloat(position: Vector3): Vector2 { | |
| const transformed = this.applyTransform(position); | |
| return this.base.globalCoordToMag2DFloat(transformed); | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/volumetracing/section_labeling.ts around
lines 716-718, the method currently forwards the untransformed world coordinate
to the base labeler causing incorrect (u,v) for rotated viewers; before calling
this.base.globalCoordToMag2DFloat, convert the input position into the base
labeler’s coordinate system by applying the section/view transform (the existing
transform helper on this — e.g. the per-section transform or localize function
used elsewhere in this class) and pass the transformed point to the base method
so the returned 2D coordinates reflect viewer rotation.
| getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { | ||
| let scale = this.adaptScaleFn( | ||
| getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), | ||
| ); | ||
|
|
||
| return this.base.getCircleVoxelBuffer2D(position, scale); | ||
| } |
There was a problem hiding this comment.
Apply the transform before computing the circle buffer.
Right now the circle buffer is created in the original coordinate system, so on transformed planes the brush lands at the wrong place. Transform the input first.
- return this.base.getCircleVoxelBuffer2D(position, scale);
+ const transformed = this.applyTransform(position);
+ return this.base.getCircleVoxelBuffer2D(transformed, scale);📝 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.
| getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { | |
| let scale = this.adaptScaleFn( | |
| getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), | |
| ); | |
| return this.base.getCircleVoxelBuffer2D(position, scale); | |
| } | |
| getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { | |
| let scale = this.adaptScaleFn( | |
| getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), | |
| ); | |
| const transformed = this.applyTransform(position); | |
| return this.base.getCircleVoxelBuffer2D(transformed, scale); | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/volumetracing/section_labeling.ts around
lines 720-726, the code currently passes the original world position into
getCircleVoxelBuffer2D causing misalignment on transformed planes; instead,
apply the section/plane transform to the input position first (i.e. compute
transformedPosition by applying the existing transform/matrix for this section —
if no helper exists add one that multiplies the Vector3 by the section's
transform matrix), then call
this.base.getCircleVoxelBuffer2D(transformedPosition, scale) (keep the scale
computation as-is and ensure the transformedPosition is in the coordinate space
expected by getCircleVoxelBuffer2D).
| function getFast3DCoordinateFn( | ||
| plane: OrthoView, | ||
| thirdDimensionValue: number, | ||
| _isSwapped: boolean, | ||
| ): (coordX: number, coordY: number, out: Vector3 | Float32Array) => void { | ||
| switch (plane) { | ||
| case OrthoViews.PLANE_XY: | ||
| return (coordX, coordY, out) => { | ||
| out[0] = coordX; | ||
| out[1] = coordY; | ||
| out[2] = thirdDimensionValue; | ||
| }; | ||
|
|
||
| case OrthoViews.PLANE_YZ: | ||
| return (coordX, coordY, out) => { | ||
| out[0] = thirdDimensionValue; | ||
| out[1] = coordY; | ||
| out[2] = coordX; | ||
| }; | ||
|
|
||
| case OrthoViews.PLANE_XZ: | ||
| return (coordX, coordY, out) => { | ||
| out[0] = coordX; | ||
| out[1] = thirdDimensionValue; | ||
| out[2] = coordY; | ||
| }; | ||
|
|
||
| default: { | ||
| throw new Error("Unknown plane id"); | ||
| } | ||
| } | ||
| let [u, v, w] = Dimensions.getIndices(plane); | ||
| return (coordX, coordY, out) => { | ||
| out[u] = coordX; | ||
| out[v] = coordY; | ||
| out[w] = thirdDimensionValue; | ||
| }; |
There was a problem hiding this comment.
Respect the swapped orientation when mapping back to 3D.
_isSwapped is effectively ignored, so whenever we detect a swap the generated 3D coordinates are still written with the original axis order, corrupting the geometry. Handle the swap explicitly.
- let [u, v, w] = Dimensions.getIndices(plane);
- return (coordX, coordY, out) => {
- out[u] = coordX;
- out[v] = coordY;
- out[w] = thirdDimensionValue;
- };
+ const [u, v, w] = Dimensions.getIndices(plane);
+ const isSwapped = _isSwapped;
+ return (coordX, coordY, out) => {
+ if (isSwapped) {
+ out[u] = coordY;
+ out[v] = coordX;
+ } else {
+ out[u] = coordX;
+ out[v] = coordY;
+ }
+ out[w] = thirdDimensionValue;
+ };📝 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.
| function getFast3DCoordinateFn( | |
| plane: OrthoView, | |
| thirdDimensionValue: number, | |
| _isSwapped: boolean, | |
| ): (coordX: number, coordY: number, out: Vector3 | Float32Array) => void { | |
| switch (plane) { | |
| case OrthoViews.PLANE_XY: | |
| return (coordX, coordY, out) => { | |
| out[0] = coordX; | |
| out[1] = coordY; | |
| out[2] = thirdDimensionValue; | |
| }; | |
| case OrthoViews.PLANE_YZ: | |
| return (coordX, coordY, out) => { | |
| out[0] = thirdDimensionValue; | |
| out[1] = coordY; | |
| out[2] = coordX; | |
| }; | |
| case OrthoViews.PLANE_XZ: | |
| return (coordX, coordY, out) => { | |
| out[0] = coordX; | |
| out[1] = thirdDimensionValue; | |
| out[2] = coordY; | |
| }; | |
| default: { | |
| throw new Error("Unknown plane id"); | |
| } | |
| } | |
| let [u, v, w] = Dimensions.getIndices(plane); | |
| return (coordX, coordY, out) => { | |
| out[u] = coordX; | |
| out[v] = coordY; | |
| out[w] = thirdDimensionValue; | |
| }; | |
| function getFast3DCoordinateFn( | |
| plane: OrthoView, | |
| thirdDimensionValue: number, | |
| _isSwapped: boolean, | |
| ): (coordX: number, coordY: number, out: Vector3 | Float32Array) => void { | |
| const [u, v, w] = Dimensions.getIndices(plane); | |
| const isSwapped = _isSwapped; | |
| return (coordX, coordY, out) => { | |
| if (isSwapped) { | |
| out[u] = coordY; | |
| out[v] = coordX; | |
| } else { | |
| out[u] = coordX; | |
| out[v] = coordY; | |
| } | |
| out[w] = thirdDimensionValue; | |
| }; | |
| } |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/volumetracing/section_labeling.ts around
lines 738 to 748, the getFast3DCoordinateFn ignores the _isSwapped flag so
returned coordinates write axes in the original order and corrupt geometry when
swapped; update the function to respect _isSwapped by swapping the u and v
indices (or swapping coordX/coordY mapping) before returning the closure so that
when _isSwapped is true the closure writes coordX to the v index and coordY to
the u index (ensuring out[u]/out[v]/out[w] assignments reflect the swapped
orientation).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (4)
94-136: Consider updating test description to reflect new terminology.The test name "should create a volume layer" still uses the old terminology. Consider renaming it to "should create a section labeler" to align with the refactored API.
Apply this diff to update the test description:
- it("should create a volume layer (saga test)", () => { + it("should create a section labeler (saga test)", () => {
138-192: Consider updating test description to reflect new terminology.The test name "should add values to volume layer" still uses the old terminology. Consider renaming it to "should add values to section labeler" to align with the refactored API.
Apply this diff to update the test description:
- it("should add values to volume layer (saga test)", () => { + it("should add values to section labeler (saga test)", () => {
194-259: Consider updating test description and comment to reflect new terminology.Both the test name and the comment on line 242 still use the old terminology ("volume layer" and "finishLayer"). Consider updating them to align with the refactored API.
Apply this diff to update the test description and comment:
- it("should finish a volume layer (saga test)", () => { + it("should finish a section labeler (saga test)", () => { // ... (lines omitted for brevity) - // Validate that finishLayer was called + // Validate that finishSectionLabeler was called
261-328: Consider updating test description and comment to reflect new terminology.Both the test name and the comment on line 314 still use the old terminology ("volume layer" and "finishLayer"). Consider updating them to align with the refactored API.
Apply this diff to update the test description and comment:
- it("should finish a volume layer in delete mode (saga test)", () => { + it("should finish a section labeler in delete mode (saga test)", () => { // ... (lines omitted for brevity) - // Validate that finishLayer was called + // Validate that finishSectionLabeler was called
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(9 hunks)frontend/javascripts/viewer/model/sagas/volume/helpers.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/viewer/model/sagas/volume/helpers.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
🧬 Code graph analysis (1)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (3)
frontend/javascripts/test/fixtures/dataset_server_object.ts (1)
sampleTracingLayer(63-67)frontend/javascripts/test/helpers/sagaHelpers.ts (1)
execCall(21-26)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
finishSectionLabeler(362-387)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (4)
1-1: LGTM: Import updates align with API refactoring.The imports correctly reflect the refactored API, including the new
SectionLabelerclass, the renamedfinishSectionLabelerfunction, and the test fixtures needed for the updated mock setup.Also applies to: 17-17, 22-22, 27-28
46-46: LGTM: Action rename reflects API changes.The rename from
addToLayerActiontoaddToContourListActionFncorrectly reflects the broader terminology shift from "layer" to "contour list" throughout the codebase.
49-56: LGTM: Mock dataset setup is correct.The
mockedDatasetsetup usingimmutability-helpercorrectly provides the necessary test fixture for the saga, replacing the data layers withsampleTracingLayerfor testing purposes.
330-418: LGTM: Remaining tests are correctly structured.The tests for busy state handling and mapping locking are correctly implemented and don't require terminology updates since they focus on different aspects of the saga behavior.
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Thanks for all this hard refactoring work 👍
Honestly, to me the whole mapTransformedPlane stuff is a little unclear. Maybe we could call about this so I can understand it better :)
frontend/javascripts/test/model/transformed_section_labeler.spec.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
Outdated
Show resolved
Hide resolved
| [OrthoViews.PLANE_XZ]: new THREE.Vector3(0, 1, 0), | ||
| }; | ||
|
|
||
| export function mapTransformedPlane( |
There was a problem hiding this comment.
I do not 100% understand what this function is doing exactly. Is it trying to detect the best plane representing the result of a transformed plane? So in case the xy plane is rotated by 90° in some way this function detects that the xy viewport now behaves like the yz viewport and would show data that would without transformation be shown in this viewport? In this case yz plane is returned?
There was a problem hiding this comment.
yes, your assumption is correct 👍
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
46-69: Consider clearer naming for the coordinate transformation helper.While the function logic is correct, the naming could be improved:
Parameter name:
globalPosRoundedis misleading since line 186 passesglobalPos.floating(not rounded). Consider renaming to simplyglobalPos.Function name:
getUntransformedSegmentationPositionis ambiguous—"untransformed" might suggest identity transformation, but it actually means "in layer space" (transformed from global space). ConsiderglobalToLayerSegmentationPositionorgetSegmentationPositionInLayerSpacefor clarity.The comment on lines 51-52 is helpful and correctly describes the conversion from world to layer space.
-export function getUntransformedSegmentationPosition( +export function globalToLayerSegmentationPosition( state: WebknossosState, - globalPosRounded: Vector3, + globalPos: Vector3, ) { /* * Converts the given position from world space to layer space. */ const { nativelyRenderedLayerName } = state.datasetConfiguration; const maybeLayer = Model.getVisibleSegmentationLayer(); if (maybeLayer == null) { throw new Error("Segmentation layer does not exist"); } const layer = getLayerByName(state.dataset, maybeLayer.name); const segmentationTransforms = getTransformsForLayer( state.dataset, layer, nativelyRenderedLayerName, ); const layerPos = transformPointUnscaled(invertTransform(segmentationTransforms))( - globalPosRounded, + globalPos, ); return layerPos; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(3 hunks)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts(4 hunks)frontend/javascripts/viewer/store.ts(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/viewer/store.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Applied to files:
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsxfrontend/javascripts/viewer/controller/combinations/volume_handlers.ts
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/actions/volumetracing_actions.tsfrontend/javascripts/viewer/controller/combinations/volume_handlers.ts
🧬 Code graph analysis (3)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (2)
frontend/javascripts/viewer/constants.ts (2)
Vector3(14-14)AdditionalCoordinate(2-2)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
setActiveCellAction(206-218)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (5)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
calculateGlobalPos(338-338)PositionWithRounding(95-95)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (3)
startEditingAction(173-178)addToContourListAction(180-184)setActiveCellAction(206-218)frontend/javascripts/viewer/store.ts (1)
WebknossosState(575-595)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
transformPointUnscaled(165-172)invertTransform(71-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (6)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (1)
256-262: LGTM! Parameter rename improves clarity.The rename from
somePositiontosomePositionInLayerSpacemakes the coordinate space explicit, which is helpful given the transformation-related refactoring in this PR. The dispatch call correctly passes the layer-space position tosetActiveCellAction.frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (2)
17-17: LGTM! Type definitions updated consistently.The type rename from
AddToLayerActiontoAddToContourListActionand the corresponding union update align with the PR's terminology refactoring from "add to layer" → "add to contour list".Also applies to: 83-83
173-184: LGTM! Action renamed and coordinate space clarified.The rename from
addToLayerActiontoaddToContourListAction(with action type "ADD_TO_CONTOUR_LIST") is consistent with the PR's terminology updates. The added "// in layer space" comments on lines 176 and 208 are valuable documentation for the transformation refactoring.Also applies to: 206-218
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (3)
36-44: LGTM! Draw flow correctly uses layer-space coordinates.The refactored
handleDrawStartproperly converts the global position to layer space viagetUntransformedSegmentationPositionbefore dispatchingstartEditingActionandaddToContourListAction, which is consistent with these actions expecting layer-space positions.
71-78: LGTM! Erase and move flows correctly handle coordinate transformations.Both
handleEraseStartandhandleMoveForDrawOrErasefollow the same pattern ashandleDrawStart, properly converting global positions to layer space before dispatching actions. This is consistent with the PR's transformation refactoring.Also applies to: 79-84
89-94: Pick cell flow looks correct now; minor question about precision.The coordinate transformation flow appears correct and the double-inversion issue mentioned in past review comments has been resolved:
handlePickCellobtains the global position viacalculateGlobalPos(line 91)- Passes
globalPos(in global space) tohandlePickCellFromGlobalPosition(line 93)- The handler uses
globalPos.roundedfor segment ID lookup (line 180), which internally converts to layer space- Uses
globalPos.floatingfor the position passed tosetActiveCellAction(line 186), converting to layer spaceMinor question: Is the use of
globalPos.floating(line 186) vsglobalPos.rounded(line 180) forsetActiveCellActionintentional? Using floating-point precision for storing the segment'ssomePositionmight provide sub-voxel accuracy, but it's worth confirming this is the desired behavior.Also applies to: 176-204
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
frontend/javascripts/libs/drawing.ts (3)
7-22: LGTM! Good refactoring to reduce public API surface.Making
drawHorizontalLine2dprivate is appropriate since it's an internal helper method for the other drawing operations.
87-129: LGTM! Good refactoring to reduce public API surface.Making
addNextLineprivate is appropriate since it's an internal helper for the scanline flood-fill algorithm and not meant for external use.
131-155: LGTM! Good refactoring to reduce public API surface.Making
paintBorderprivate is appropriate since it's an internal helper for the rotated rectangle rasterization algorithm.frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (2)
265-273: Simplify the action destructuring pattern.The current pattern creates an object just to immediately destructure it. Consider simplifying:
- const { addToContourListAction, finishEditingAction } = { - addToContourListAction: currentAction.type === "ADD_TO_CONTOUR_LIST" ? currentAction : null, - finishEditingAction: currentAction.type === "FINISH_EDITING" ? currentAction : null, - }; - if (finishEditingAction) break; - - if (!addToContourListAction || addToContourListAction.type !== "ADD_TO_CONTOUR_LIST") { - throw new Error("Unexpected action. Satisfy typescript."); - } + if (currentAction.type === "FINISH_EDITING") { + break; + } + + if (currentAction.type !== "ADD_TO_CONTOUR_LIST") { + throw new Error("Unexpected action. Satisfy typescript."); + } + + const addToContourListAction = currentAction;This eliminates the intermediate object creation and the redundant type check at Line 271.
363-388: LGTM! Function refactoring correctly delegates to the labeler.The renamed
finishSectionLabelerproperly uses the labeler's methods to obtain the filling buffer, plane, and centroid. The centroid retrieval viagetUnzoomedCentroid()correctly applies inverse transformation when usingTransformedSectionLabeler.Minor: Remove unnecessary null check.
At Line 371, the
sectionLabeler == nullcheck is unnecessary since the parameter type doesn't include null/undefined:- if (sectionLabeler == null || sectionLabeler.isEmpty()) { + if (sectionLabeler.isEmpty()) {frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (2)
46-69: Rename parameter for clarity.The parameter
globalPosRoundedis misleading because this function is called with both rounded positions (lines 39, 74, 82) and floating-point positions (line 233). Since the function simply transforms the input coordinate from world space to layer space regardless of rounding, consider renaming it toglobalPosorworldPosfor clarity.Apply this diff:
export function getUntransformedSegmentationPosition( state: WebknossosState, - globalPosRounded: Vector3, + globalPos: Vector3, ) { /* * Converts the given position from world space to layer space. */ const { nativelyRenderedLayerName } = state.datasetConfiguration; const maybeLayer = Model.getVisibleSegmentationLayer(); if (maybeLayer == null) { throw new Error("Segmentation layer does not exist"); } const layer = getLayerByName(state.dataset, maybeLayer.name); const segmentationTransforms = getTransformsForLayer( state.dataset, layer, nativelyRenderedLayerName, ); const layerPos = transformPointUnscaled(invertTransform(segmentationTransforms))( - globalPosRounded, + globalPos, ); return layerPos; }
219-247: Consider consolidatingStore.getState()calls.The function calls
Store.getState()twice (lines 228 and 232). Consider retrieving the state once at the beginning to improve efficiency.Apply this diff:
export function handlePickCellFromGlobalPosition( globalPos: PositionWithRounding, additionalCoordinates: AdditionalCoordinate[], ) { + const state = Store.getState(); const segmentId = getSegmentIdForPosition(globalPos.rounded); if (segmentId === 0) { return; } - const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); + const visibleSegmentationLayer = getVisibleSegmentationLayer(state); if (visibleSegmentationLayer == null) { return; } - const state = Store.getState(); const layerPos = getUntransformedSegmentationPosition(state, globalPos.floating); Store.dispatch(setActiveCellAction(segmentId, layerPos, additionalCoordinates)); Store.dispatch( updateSegmentAction( segmentId, { isVisible: true, }, visibleSegmentationLayer.name, undefined, true, ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/javascripts/libs/drawing.ts(4 hunks)frontend/javascripts/test/model/transformed_section_labeler.spec.ts(1 hunks)frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts(4 hunks)frontend/javascripts/viewer/constants.ts(1 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(3 hunks)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts(1 hunks)frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(8 hunks)frontend/javascripts/viewer/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts
- frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts
- frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts
- frontend/javascripts/viewer/constants.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-25T14:38:49.345Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8229
File: frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:348-354
Timestamp: 2024-11-25T14:38:49.345Z
Learning: For the `getDatasetExtentAsProduct` function in `dataset_accessor.ts`, input validation for negative or zero dimensions is not necessary.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2025-07-22T12:56:47.507Z
Learnt from: daniel-wer
Repo: scalableminds/webknossos PR: 8787
File: frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts:132-136
Timestamp: 2025-07-22T12:56:47.507Z
Learning: Three.js Vector3 has explicitly defined x, y, and z properties that can be safely accessed via dynamic string indexing when using union types like "x" | "y" | "z". The pattern `vector[axis] = value` where axis is "x" | "y" | "z" does not cause TypeScript strict mode errors because TypeScript can infer the property access is valid.
Applied to files:
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsxfrontend/javascripts/viewer/controller/combinations/volume_handlers.ts
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
Applied to files:
frontend/javascripts/test/model/transformed_section_labeler.spec.ts
📚 Learning: 2025-04-01T09:45:17.527Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
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.
Applied to files:
frontend/javascripts/test/model/transformed_section_labeler.spec.ts
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
🧬 Code graph analysis (4)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3Indices(50-50)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (4)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (3)
createSectionLabeler(282-302)labelWithVoxelBuffer2D(174-280)BooleanBox(44-46)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (4)
startEditingAction(173-178)finishEditingAction(201-204)addToContourListAction(180-184)registerLabelPointAction(349-353)frontend/javascripts/viewer/model/accessors/tool_accessor.ts (4)
isBrushTool(170-172)AnnotationTool(91-105)AnnotationTool(109-109)isVolumeDrawingTool(162-169)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
TransformedSectionLabeler(633-715)
frontend/javascripts/test/model/transformed_section_labeler.spec.ts (5)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
mapTransformedPlane(587-631)frontend/javascripts/viewer/constants.ts (2)
OrthoView(66-66)IdentityTransform(387-391)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (1)
Transform(11-27)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
combineCoordinateTransformations(191-212)frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
getRotationalTransformation(225-240)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (6)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
calculateGlobalPos(368-368)PositionWithRounding(95-95)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (6)
setContourTracingModeAction(377-381)startEditingAction(173-178)addToContourListAction(180-184)finishEditingAction(201-204)resetContourAction(355-358)setActiveCellAction(206-218)frontend/javascripts/viewer/store.ts (1)
WebknossosState(576-596)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
getLayerByName(147-166)getVisibleSegmentationLayer(430-440)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
transformPointUnscaled(165-172)invertTransform(71-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (8)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (4)
5-5: LGTM! Import changes align with the refactoring.The imports correctly bring in the new SectionLabeler/TransformedSectionLabeler types and the createSectionLabeler helper function to support the refactored labeling approach.
Also applies to: 82-83, 90-90
239-245: LGTM! Correct usage of createSectionLabeler.The refactored call correctly instantiates a SectionLabeler or TransformedSectionLabeler based on the transform configuration. The third dimension extractor lambda appropriately captures the value from the start position.
251-255: LGTM! Labeler method calls correctly delegate transformation logic.The calls to
getCircleVoxelBuffer2D,getRectangleVoxelBuffer2D,updateArea, andgetPlanecorrectly pass positions to the labeler, which handles coordinate transformation internally when usingTransformedSectionLabeler. This maintains proper separation of concerns.Also applies to: 291-291, 295-323
665-688: LGTM! Contour geometry maintenance updated for renamed action.The function now correctly listens for
ADD_TO_CONTOUR_LISTinstead ofADD_TO_LAYER, consistent with the action rename across the codebase.frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
20-20: LGTM! Typo fix applied consistently.The spelling correction from
Vector3IndiciestoVector3Indicesis applied consistently in both the import and usage. No functional changes.Also applies to: 247-247
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (2)
6-34: LGTM! Imports align with transform-aware refactoring.The new imports for transformation helpers, layer accessors, and position types are appropriate for converting the volume handling code to work with transformed layer coordinates.
36-44: LGTM! Drawing and erasing handlers correctly transform to layer space.Both
handleDrawStartandhandleEraseStartnow correctly:
- Calculate the global position from the viewport coordinates
- Transform it to layer space via
getUntransformedSegmentationPosition- Dispatch actions with layer-space coordinates
This aligns with the PR's objective to support transformed layers.
Also applies to: 71-78
frontend/javascripts/test/model/transformed_section_labeler.spec.ts (1)
16-172: Thorough transform coverage pays offNice work locking down canonical, rotated, and chained affine cases (including adaptScale ordering) for
TransformedSectionLabeler; this gives strong confidence in the refactor.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
🧬 Code graph analysis (1)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (8)
frontend/javascripts/viewer/constants.ts (4)
Point2(27-30)OrthoView(66-66)Vector3(14-14)AdditionalCoordinate(2-2)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
calculateGlobalPos(368-368)PositionWithRounding(95-95)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (6)
setContourTracingModeAction(377-381)startEditingAction(173-178)addToContourListAction(180-184)finishEditingAction(201-204)resetContourAction(355-358)setActiveCellAction(206-218)frontend/javascripts/viewer/store.ts (1)
WebknossosState(576-596)frontend/javascripts/viewer/model.ts (2)
getLayerByName(217-223)getVisibleSegmentationLayer(121-135)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
getLayerByName(147-166)getVisibleSegmentationLayer(430-440)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
transformPointUnscaled(165-172)invertTransform(71-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (4)
6-34: LGTM! Imports properly support transformation logic.The new imports correctly bring in the layer transformation utilities, position types, and renamed action creators needed for the refactoring.
36-44: LGTM! Draw flow correctly transforms to layer space.The function properly computes layer-space coordinates from the global position before dispatching editing actions, which is consistent with the documented action contracts.
46-69: LGTM! Transformation helper correctly inverts layer transforms.This new exported function properly converts world-space positions to layer-space by inverting the segmentation layer's transforms. The error thrown when the segmentation layer is missing is appropriate for these user-initiated handlers.
71-84: LGTM! Erase and move handlers correctly apply transformations.Both functions follow the established pattern of converting screen positions to global, then to layer space before dispatching actions.
| export function handlePickCellFromGlobalPosition( | ||
| globalPos: PositionWithRounding, | ||
| additionalCoordinates: AdditionalCoordinate[], | ||
| ) { | ||
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| const posInLayerSpace = globalToLayerTransformedPosition( | ||
| globalPos, | ||
| visibleSegmentationLayer.name, | ||
| "segmentation", | ||
| Store.getState(), | ||
| ); | ||
|
|
||
| const segmentId = getSegmentIdForPosition(globalPos); | ||
| const segmentId = getSegmentIdForPosition(globalPos.rounded); | ||
|
|
||
| if (segmentId === 0) { | ||
| return; | ||
| } | ||
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); | ||
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | ||
| if (visibleSegmentationLayer == null) { | ||
| return; | ||
| } | ||
| const state = Store.getState(); | ||
| const layerPos = getUntransformedSegmentationPosition(state, globalPos.floating); | ||
| Store.dispatch(setActiveCellAction(segmentId, layerPos, additionalCoordinates)); |
There was a problem hiding this comment.
Consider using globalPos.rounded consistently for both segment ID and position.
Lines 223 and 233 use different precision levels:
- Line 223:
globalPos.roundeddetermines which segment was clicked - Line 233:
globalPos.floatingcomputes the layer-space position to store
With non-linear transforms (thin plate spline), the rounded and floating global positions could map to different voxels in layer space after transformation. This means the stored somePosition might not correspond to the actual segment identified by segmentId, which could cause confusion or bugs downstream.
Consider using globalPos.rounded on line 233 for consistency:
- const layerPos = getUntransformedSegmentationPosition(state, globalPos.floating);
+ const layerPos = getUntransformedSegmentationPosition(state, globalPos.rounded);Alternatively, if floating-point precision is important for the stored position, add a comment explaining why the precision difference is intentional and safe.
📝 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.
| export function handlePickCellFromGlobalPosition( | |
| globalPos: PositionWithRounding, | |
| additionalCoordinates: AdditionalCoordinate[], | |
| ) { | |
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | |
| if (visibleSegmentationLayer == null) { | |
| return; | |
| } | |
| const posInLayerSpace = globalToLayerTransformedPosition( | |
| globalPos, | |
| visibleSegmentationLayer.name, | |
| "segmentation", | |
| Store.getState(), | |
| ); | |
| const segmentId = getSegmentIdForPosition(globalPos); | |
| const segmentId = getSegmentIdForPosition(globalPos.rounded); | |
| if (segmentId === 0) { | |
| return; | |
| } | |
| Store.dispatch(setActiveCellAction(segmentId, posInLayerSpace, additionalCoordinates)); | |
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | |
| if (visibleSegmentationLayer == null) { | |
| return; | |
| } | |
| const state = Store.getState(); | |
| const layerPos = getUntransformedSegmentationPosition(state, globalPos.floating); | |
| Store.dispatch(setActiveCellAction(segmentId, layerPos, additionalCoordinates)); | |
| export function handlePickCellFromGlobalPosition( | |
| globalPos: PositionWithRounding, | |
| additionalCoordinates: AdditionalCoordinate[], | |
| ) { | |
| const segmentId = getSegmentIdForPosition(globalPos.rounded); | |
| if (segmentId === 0) { | |
| return; | |
| } | |
| const visibleSegmentationLayer = getVisibleSegmentationLayer(Store.getState()); | |
| if (visibleSegmentationLayer == null) { | |
| return; | |
| } | |
| const state = Store.getState(); | |
| const layerPos = getUntransformedSegmentationPosition(state, globalPos.rounded); | |
| Store.dispatch(setActiveCellAction(segmentId, layerPos, additionalCoordinates)); |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/controller/combinations/volume_handlers.ts around
lines 219-234 the code uses globalPos.rounded to pick the segmentId but
globalPos.floating to compute layerPos, which can map to different voxels under
non-linear transforms; change the call to getUntransformedSegmentationPosition
to use globalPos.rounded (or otherwise ensure the same precision is used for
both segmentId and stored position), verify the argument type matches (convert
if necessary), and update or add a brief inline comment/tests if you
intentionally keep floating precision explaining why it is safe.
4821048 to
8f73d72
Compare
…DCoord and getBottomRight3DCoord methods
…tput for layer pos to status bar
philippotto
force-pushed
the
rotated-annotation
branch
from
8f73d72 to
0fb28a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (2)
633-705: Apply transforms in TransformedSectionLabeler before delegating to base for 2D coords and circles
TransformedSectionLabelerexposes the same external “global coordinate” API asSectionLabelerbut only some methods apply the transform:
updateAreaandgetRectangleVoxelBuffer2Dcorrectly callthis.applyTransform(...)before delegating.globalCoordToMag2DFloatandgetCircleVoxelBuffer2Dcurrently forward the untransformedpositionto the base labeler, which expects coordinates in the transformed plane.On rotated/mapped layers this will yield incorrect (u, v) coordinates and misaligned brush circles, even though the rest of the pipeline (plane selection, voxel buffer labeling) is transform-aware. This is a functional correctness issue for transformed views.
A minimal fix that keeps identity-transforms working is:
- globalCoordToMag2DFloat(position: Vector3): Vector2 { - return this.base.globalCoordToMag2DFloat(position); - } + globalCoordToMag2DFloat(position: Vector3): Vector2 { + const transformed = this.applyTransform(position); + return this.base.globalCoordToMag2DFloat(transformed); + } … - getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { - let scale = this.adaptScaleFn( - getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), - ); - - return this.base.getCircleVoxelBuffer2D(position, scale); - } + getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { + const transformed = this.applyTransform(position); + const scale = this.adaptScaleFn( + getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), + ); + return this.base.getCircleVoxelBuffer2D(transformed, scale); + }This makes
TransformedSectionLabelerconsistently treat its arguments as untransformed global positions, with all geometry handled in the transformed base space.
560-631: Scale adaptation in mapTransformedPlane uses originalPlane instead of mapped plane
mapTransformedPlanecomputesbestView(the mapped plane) from the transformed normal, butadaptScaleFnstill callsDimensions.transDim(scale, originalPlane)and applies the swap based on the pre-mappingswappedflag. For transforms that change the canonical plane (e.g. XY → YZ), this mixes the wrong axes and is very likely why some transformed layers appear “squished” despite correct plane detection.Aligning the scale mapping with the mapped plane and only swapping when we stayed in the same plane would fix this:
- const swapped = isAlmostZero(basis.u.dot(u2)); + const swapped = isAlmostZero(basis.u.dot(u2)); … - const adaptScaleFn = (scale: Vector3): Vector2 => { - const transposed = Dimensions.transDim(scale, originalPlane); - if (swapped) { - return [transposed[1], transposed[0]]; - } else { - return [transposed[0], transposed[1]]; - } - }; - - return [bestView, bestView === originalPlane && swapped, adaptScaleFn]; + const adaptScaleFn = (scale: Vector3): Vector2 => { + const transposed = Dimensions.transDim(scale, bestView); + if (bestView === originalPlane && swapped) { + return [transposed[1], transposed[0]]; + } + return [transposed[0], transposed[1]]; + }; + + return [bestView, bestView === originalPlane && swapped, adaptScaleFn];This keeps the
isSwappedflag semantics while making the scale consistent with the plane actually used by the baseSectionLabeler.
🧹 Nitpick comments (3)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (1)
225-240: Approve extraction; consider relocating to transformation accessor module.The function correctly extracts the transformation construction logic and reduces duplication. However, since this is now an exported utility function, consider moving it to
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.tsalongside the related transformation helpers (fromCenterToOrigin,fromOriginToCenter,getRotationMatrixAroundAxis) for better organization and discoverability.If you relocate the function, apply a change like this:
In
dataset_layer_transformation_accessor.ts, add:export function getRotationalTransformation( datasetBoundingBox: BoundingBox, rotationValues: { x: RotationAndMirroringSettings; y: RotationAndMirroringSettings; z: RotationAndMirroringSettings; }, ): AffineTransformation[] { return [ fromCenterToOrigin(datasetBoundingBox), getRotationMatrixAroundAxis("x", rotationValues["x"]), getRotationMatrixAroundAxis("y", rotationValues["y"]), getRotationMatrixAroundAxis("z", rotationValues["z"]), fromOriginToCenter(datasetBoundingBox), ]; }Then import it in this file instead:
import { AXIS_TO_TRANSFORM_INDEX, EXPECTED_TRANSFORMATION_LENGTH, IDENTITY_TRANSFORM, type RotationAndMirroringSettings, doAllLayersHaveTheSameRotation, fromCenterToOrigin, fromOriginToCenter, getRotationMatrixAroundAxis, + getRotationalTransformation, transformationEqualsAffineIdentityTransform, } from "viewer/model/accessors/dataset_layer_transformation_accessor";frontend/javascripts/viewer/model/sagas/volume/helpers.ts (1)
282-302: createSectionLabeler wiring is coherent with transformed-layer design
createSectionLabelercorrectly pullsdatasetanddatasetConfigurationfrom the store, resolves the segmentation layer viagetLayerByName, and derives transforms withgetTransformsForLayerbefore constructing aTransformedSectionLabeler. Always returning the transformed variant (with identity transforms as a no-op) simplifies saga code and is a reasonable design choice.If you later need to avoid the overhead for untransformed datasets, you could branch on an identity transform and return a plain
SectionLabeler, but that’s an optimisation, not a blocker.frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
172-201: SectionLabeler’s new plane/3D-coordinate plumbing is coherentThe introduction of
plane,thirdDimensionValue, andfast3DCoordinateFunctiontogether withget2DCoordinate,getUnzoomedCentroid, andcreateVoxelBuffer2Dprovides a clean way to map between 2D slice coordinates and 3D positions for any orthogonal plane. The fast coordinate function and use ofVector2Indices/Vector3Indicesmake the code less error-prone across XY/YZ/XZ. No issues here beyond the transform-specific concerns already called out.If
isSwappedstays unused insideSectionLabeler, consider documenting its intended future use or removing it to avoid confusion.Also applies to: 365-367, 516-553, 717-727
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx(3 hunks)frontend/javascripts/libs/drawing.ts(4 hunks)frontend/javascripts/test/global_mocks.ts(1 hunks)frontend/javascripts/test/model/transformed_section_labeler.spec.ts(1 hunks)frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts(4 hunks)frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts(3 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(9 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts(6 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts(8 hunks)frontend/javascripts/viewer/constants.ts(1 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(3 hunks)frontend/javascripts/viewer/controller/scene_controller.ts(4 hunks)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts(2 hunks)frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts(1 hunks)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts(3 hunks)frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts(1 hunks)frontend/javascripts/viewer/model/helpers/action_logger_middleware.ts(1 hunks)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts(1 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(2 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volume/helpers.ts(6 hunks)frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(8 hunks)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts(15 hunks)frontend/javascripts/viewer/store.ts(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/javascripts/viewer/model/helpers/action_logger_middleware.ts
- frontend/javascripts/viewer/model/helpers/transformation_helpers.ts
- frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts
- frontend/javascripts/test/reducers/volumetracing_reducer.spec.ts
- frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_2.spec.ts
- frontend/javascripts/viewer/model/accessors/dataset_accessor.ts
- frontend/javascripts/viewer/model/accessors/disabled_tool_accessor.ts
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/viewer/constants.ts
- frontend/javascripts/viewer/model/actions/volumetracing_actions.ts
- frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
- frontend/javascripts/test/model/transformed_section_labeler.spec.ts
- frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts
- frontend/javascripts/libs/drawing.ts
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Applied to files:
frontend/javascripts/viewer/model/bucket_data_handling/data_cube.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.tsfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.tsfrontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.tsfrontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.tsfrontend/javascripts/viewer/controller/combinations/volume_handlers.tsfrontend/javascripts/viewer/model/sagas/volume/helpers.tsfrontend/javascripts/viewer/model/sagas/volumetracing_saga.tsxfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
📚 Learning: 2025-08-14T14:12:03.293Z
Learnt from: hotzenklotz
Repo: scalableminds/webknossos PR: 8849
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/components/collapsible_split_merger_evaluation_settings.tsx:39-51
Timestamp: 2025-08-14T14:12:03.293Z
Learning: Since Ant Design version 5.18, Form.Item components support the layout prop (layout="horizontal", layout="vertical", etc.), allowing individual form items to override the parent Form's layout setting. This is a newer API addition that provides more granular control over form item layouts.
Applied to files:
frontend/javascripts/test/global_mocks.tsfrontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx
📚 Learning: 2025-08-14T14:12:03.293Z
Learnt from: hotzenklotz
Repo: scalableminds/webknossos PR: 8849
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/components/collapsible_split_merger_evaluation_settings.tsx:39-51
Timestamp: 2025-08-14T14:12:03.293Z
Learning: Since Ant Design version 5.18, Form.Item components support the layout prop (layout="horizontal", layout="vertical", layout="inline"), allowing individual form items to override the parent Form's layout setting. This enables mixed layouts within a single form and was added as a new API feature in v5.18 according to the official changelog.
Applied to files:
frontend/javascripts/test/global_mocks.tsfrontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts
📚 Learning: 2025-04-01T09:45:17.527Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
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.
Applied to files:
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsxfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts
📚 Learning: 2025-05-15T19:44:16.110Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8492
File: frontend/javascripts/viewer/model/sagas/skeletontracing_saga.ts:671-674
Timestamp: 2025-05-15T19:44:16.110Z
Learning: Visibility updates for user bounding boxes are intentionally handled separately from other property updates, with dedicated update actions for visibility changes.
Applied to files:
frontend/javascripts/viewer/controller/scene_controller.ts
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/viewer/model/sagas/volume/helpers.ts
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
🧬 Code graph analysis (11)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (4)
addToContourListAction(184-188)finishEditingAction(205-208)setActiveCellAction(210-222)startEditingAction(177-182)
frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts (1)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (1)
createSectionLabeler(282-302)
frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts (1)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
addToContourListAction(184-188)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (3)
frontend/javascripts/test/fixtures/dataset_server_object.ts (1)
sampleTracingLayer(63-67)frontend/javascripts/test/helpers/sagaHelpers.ts (1)
execCall(21-26)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
finishSectionLabeler(364-389)
frontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.ts (1)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (1)
createSectionLabeler(282-302)
frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (2)
frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (4)
RotationAndMirroringSettings(76-79)fromCenterToOrigin(107-113)getRotationMatrixAroundAxis(123-144)fromOriginToCenter(115-121)frontend/javascripts/types/api_types.ts (1)
AffineTransformation(81-84)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (5)
frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
calculateGlobalPos(368-368)PositionWithRounding(95-95)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
getLayerByName(147-166)getVisibleSegmentationLayer(430-440)frontend/javascripts/viewer/model.ts (2)
getLayerByName(215-221)getVisibleSegmentationLayer(119-133)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
transformPointUnscaled(165-172)invertTransform(71-87)
frontend/javascripts/viewer/controller/scene_controller.ts (3)
frontend/javascripts/viewer/constants.ts (2)
OrthoViewMap(69-69)OrthoViewWithoutTDMap(70-70)frontend/javascripts/viewer/geometries/helper_geometries.ts (3)
ContourGeometry(31-172)QuickSelectGeometry(181-340)LineMeasurementGeometry(343-459)frontend/javascripts/viewer/controller/segment_mesh_controller.ts (1)
SegmentMeshController(80-707)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (4)
frontend/javascripts/viewer/store.ts (1)
VolumeTracing(193-219)frontend/javascripts/viewer/constants.ts (2)
OrthoView(66-66)Vector3(14-14)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
TransformedSectionLabeler(633-715)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (4)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (3)
createSectionLabeler(282-302)labelWithVoxelBuffer2D(174-280)BooleanBox(44-46)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (4)
startEditingAction(177-182)finishEditingAction(205-208)addToContourListAction(184-188)registerLabelPointAction(363-367)frontend/javascripts/viewer/model/accessors/tool_accessor.ts (4)
isBrushTool(170-172)AnnotationTool(91-105)AnnotationTool(109-109)isVolumeDrawingTool(162-169)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
TransformedSectionLabeler(633-715)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (5)
frontend/javascripts/viewer/constants.ts (5)
Vector2(13-13)Vector3(14-14)OrthoView(66-66)Vector3Indices(50-50)Vector2Indices(49-49)frontend/javascripts/viewer/model/scaleinfo.ts (1)
getBaseVoxelFactorsInUnit(26-33)frontend/javascripts/viewer/model/helpers/position_converter.ts (1)
zoomedPositionToGlobalPosition(45-47)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (3)
Transform(11-27)transformPointUnscaled(165-172)invertTransform(71-87)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
invertAndTranspose(372-374)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (19)
frontend/javascripts/test/global_mocks.ts (1)
143-145: LGTM!The addition of the
Typography.Textmock follows the same pattern as other antd component mocks in this file. The empty object placeholder is consistent with the existing mocking strategy.frontend/javascripts/dashboard/dataset/dataset_rotation_form_item.tsx (2)
6-6: LGTM: Import added for new function signature.The
AffineTransformationtype import is correctly added to support the return type of the newgetRotationalTransformationfunction.
67-67: LGTM: Good refactor to use extracted helper.Replacing the inline transformation construction with the extracted
getRotationalTransformationfunction reduces duplication and improves maintainability.frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
46-69: LGTM! Clean world-to-layer coordinate conversion.The new helper function properly handles the transformation pipeline: retrieves the segmentation layer, obtains the appropriate transform (considering the natively rendered layer), and applies the inverse transform to convert from world space to layer space. Error handling for missing layers is also in place.
frontend/javascripts/viewer/controller/scene_controller.ts (1)
570-608: LGTM! Transform coverage extended to annotation tools.The renamed method now correctly applies transforms to the
annotationToolsGeometryGroupin addition to user bounding boxes and meshes. This ensures that annotation-specific geometries (contour, quick select, etc.) are properly transformed, which aligns with the PR's goal of supporting transformed layers.frontend/javascripts/viewer/model/bucket_data_handling/data_cube.ts (1)
37-40: LGTM! Import path update aligns with refactoring.The import path change from
volumelayertosection_labelingis consistent with the broader migration to the new labeling abstractions introduced in this PR.frontend/javascripts/viewer/model/sagas/volume/volume_interpolation_saga.ts (1)
370-386: LGTM! Consistent migration to SectionLabeler API.The switch from
createVolumeLayertocreateSectionLabeleris properly implemented with matching updates to variable names and method calls (createVoxelBuffer2D,globalCoordToMag2DFloat). The logic remains unchanged, reflecting a clean API migration.frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts (1)
11-11: LGTM! Action rename applied consistently.The test helper correctly uses the renamed
addToContourListActionin both dispatch calls, aligning with the broader action API refactor fromADD_TO_LAYERtoADD_TO_CONTOUR_LIST.Also applies to: 63-63, 76-76
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga_integration_1.spec.ts (1)
24-24: LGTM! Integration tests updated consistently.All test cases correctly use the renamed
addToContourListActionthroughout the file, maintaining consistency with the broader action API refactor.Also applies to: 72-72, 77-77, 132-132, 186-186, 236-236, 282-282
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (1)
131-148: LGTM! Reducer renamed to match action refactor.The function rename from
addToLayerReducertoaddToContourListReduceraligns with the broader migration fromADD_TO_LAYERtoADD_TO_CONTOUR_LIST. The implementation logic remains unchanged, reflecting a clean naming update.frontend/javascripts/viewer/model/sagas/volume/quick_select/quick_select_heuristic_saga.ts (1)
515-527: LGTM! Quick select updated to use SectionLabeler.The migration from
createVolumeLayertocreateSectionLabeleris implemented consistently with the same pattern used involume_interpolation_saga.ts. Variable names and method calls are properly updated to use the newSectionLabelerAPI.frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (4)
17-55: Saga wiring and imports look consistent with new SectionLabeler flowThe updated imports (including
finishSectionLabeler,SectionLabeler,sampleTracingLayer, anddefaultVolumeState) and theaddToContourListActionFnalias line up with the refactored saga API and renamed action. No issues from a typing or wiring perspective.
49-56: createSectionLabeler saga driving and mocked dataset are soundManually advancing the
createSectionLabelergenerator and feeding inmockedDatasetand a minimal{ nativelyRenderedLayerName: undefined }config matches the saga’sselect-based contract and ensuresgetLayerByNamecan resolvevolumeTracingId. The finallabeller.getPlane()assertion is an appropriate sanity check for the plane mapping.Also applies to: 129-136
174-192: SectionLabeler usage in “add values” test matches the new constructorInstantiating
SectionLabelerwith(tracingId, OrthoViews.PLANE_XY, 10, [1, 1, 1], false)matches the new signature, and theADD_TO_CONTOUR_LISTevents driveupdateAreaas expected, making theminCoordandmaxCoordassertions meaningful. This provides good coverage for the new bounding-box logic.
230-328: finishSectionLabeler expectations correctly reflect the new APIBoth “finish volume layer” tests now assert a
call(finishSectionLabeler, …)with the right arguments (SectionLabeler instance, tool, contour mode, overwrite mode, zoom step, andwroteVoxelsBox). This keeps the spec aligned with the refactored saga semantics.frontend/javascripts/viewer/model/sagas/volume/helpers.ts (1)
12-18: VoxelBuffer2D integration in labelWithVoxelBuffer2D looks correctThe switch to
voxelBuffer.getTopLeft3DCoord()/getBottomRight3DCoord()andgetValueFromGlobal(globalA, globalB)is consistent with the new VoxelBuffer2D contract. The third-dimension increment before constructing theBoundingBoxpreserves the inclusive 2D area while making the 3D box non-empty, and the usage ofdimensionIndicesmatches the plane-dependent layout.Also applies to: 27-31, 174-259
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (3)
5-7: editVolumeLayerAsync correctly adopts SectionLabeler / contour-list semanticsSwitching to
createSectionLabelerwith agetThirdDimValuecallback, routing brush labeling throughcurrentSectionLabeler.getCircleVoxelBuffer2D(...)andcurrentSectionLabeler.getPlane(), and listening on["ADD_TO_CONTOUR_LIST", "FINISH_EDITING"]aligns the saga with the new section-labeling abstraction and action naming without changing the high-level control flow. The destructuring ofaddToContourListAction/finishEditingActionand the position usage look consistent.Also applies to: 83-92, 159-259
261-335: finishSectionLabeler encapsulates finishing logic cleanlyThe new
finishSectionLabelergenerator correctly guards against null/empty labelers, callslabelWithVoxelBuffer2Donly for volume drawing tools, and usessectionLabeler.getPlane()andgetFillingVoxelBuffer2D(activeTool)to drive finishing on the proper transformed plane. EmittingregisterLabelPointAction(sectionLabeler.getUnzoomedCentroid())preserves previous behaviour in a cleaner, reusable helper.Also applies to: 364-389
671-693: Contour geometry maintenance correctly tracks ADD_TO_CONTOUR_LISTListening for
["ADD_TO_CONTOUR_LIST", "RESET_CONTOUR"]and rebuilding the contour fromvolumeTracing.contourListkeeps scene geometry in sync with the renamed action and the reducer semantics. The conditional on active trace tool and active segmentation tracing remains intact.
| readonly map: Uint8Array; | ||
| readonly width: number; | ||
| readonly height: number; | ||
| readonly minCoord2d: Vector2; | ||
| readonly getFast3DCoordinate: (arg0: number, arg1: number, arg2: Vector3 | Float32Array) => void; | ||
|
|
||
| static empty(): VoxelBuffer2D { | ||
| return new VoxelBuffer2D( | ||
| new Uint8Array(0), | ||
| 0, | ||
| 0, | ||
| [0, 0], | ||
| () => [0, 0, 0], | ||
| () => {}, | ||
| ); | ||
| return new VoxelBuffer2D(new Uint8Array(0), 0, 0, [0, 0], () => {}); | ||
| } |
There was a problem hiding this comment.
Fix VoxelBuffer2D.print iteration bounds
The new VoxelBuffer2D shape and helpers are fine, but print() iterates y only up to this.width, which is wrong for non-square buffers and will skip or misread rows.
- print(): void {
- const lines = [];
- for (let y = 0; y < this.width; y++) {
- const line = [];
- for (let x = 0; x < this.width; x++) {
- line.push(this.getValue(x, y));
- }
- lines.push(line);
- }
- console.log("VoxelBuffer content:", lines.join("\n"));
- }
+ print(): void {
+ const lines = [];
+ for (let y = 0; y < this.height; y++) {
+ const line = [];
+ for (let x = 0; x < this.width; x++) {
+ line.push(this.getValue(x, y));
+ }
+ lines.push(line);
+ }
+ console.log("VoxelBuffer content:", lines.join("\n"));
+ }This is only a debugging helper, so it’s a minor correctness issue.
Also applies to: 71-82, 93-103
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/volumetracing/section_labeling.ts around
lines 34-42 (and similarly at 71-82 and 93-103), the VoxelBuffer2D.print helper
incorrectly iterates the y loop up to this.width; change the outer loop to
iterate y from 0 to this.height and keep the inner loop x from 0 to this.width
so rows are printed correctly for non-square buffers, and update any similar
loops in the other mentioned ranges to use this.height for row iteration and
this.width for column iteration.
|
@MichaelBuessemeyer let me know if we should have a chat about this PR again :) |
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Yeah a little talk about this PR again would be great. Understanding the rotations / transforms and when layer space and when global space is used is a little hard. Maybe comments & variable renaming would make things clearer. E.g. is a section labeler in global or in layer space 🤔? What about a LabeledVoxelsMap?
Besides this I left a few mini suggestions / comment and thanks :D
| expect(mapTransformedPlane("PLANE_XZ", IdentityTransform)).toEqual(["PLANE_XZ", false, [0, 2]]); | ||
| }); | ||
|
|
||
| it("Rotation by 90deg around X should be handled correctly", async () => { |
There was a problem hiding this comment.
Why does this description not start with a [L4]?
There was a problem hiding this comment.
because in this test, I created transforms that don't have anything to do with L4. in the other tests, I specifically added transforms that work for the L4 dataset so that I could check in the browser, that everything works as expected (and then could hardcode the expected values into the tests).
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts
Outdated
Show resolved
Hide resolved
| ({ | ||
| type: "START_EDITING", | ||
| position, | ||
| position, // in layer space |
There was a problem hiding this comment.
nice, good hint 🎉
Or maybe even better: rename the variable to something like "positionInLayerSpace". Is much longer but also directly self explanatory when just reading the variable.
Might be also good for the one below
There was a problem hiding this comment.
The var renaming could maybe also be useful for the somePosition in setActiveCellAction -> somePositionInLayerSpace or so
There was a problem hiding this comment.
moreover, I thought you wanted to renaming the somePosition to anchorPosition. Thus, maybe also apply this renaming here as well 🤔? But only if thats the PR where you are doing this renaming
There was a problem hiding this comment.
And maybe look out for other actions 🙈. Sorry for so many suggestion ...
There is also clickSegmentAction with a position. And maybe even more in this file
There was a problem hiding this comment.
I improved the naming for some of the actions. somePosition -> anchorPosition happens in another pr (#9045) which is why I want to avoid renaming things here again.
| ) | ||
| ] | ||
| ) { | ||
| if (voxelBuffer.getValueFromGlobal(globalA, globalB)) { |
There was a problem hiding this comment.
The comment above claims that these coordinates are global. Are they really? Shouldn't the LabeledVoxelMap be in layer space and thus the iteration variable of the for loops as well?
There was a problem hiding this comment.
in the past, we often used "global coordinates" as the opposite of "bucket coordinates". however, with the advent of layer transforms, some of the old "global coordinates" now mean "layer space". I tried to clean this up in the spots that caught my eye, but there are probably still places where it's not really precise.
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(9 hunks)frontend/javascripts/viewer/constants.ts(1 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(8 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
- frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
Applied to files:
frontend/javascripts/viewer/constants.ts
📚 Learning: 2025-07-22T12:56:47.507Z
Learnt from: daniel-wer
Repo: scalableminds/webknossos PR: 8787
File: frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts:132-136
Timestamp: 2025-07-22T12:56:47.507Z
Learning: Three.js Vector3 has explicitly defined x, y, and z properties that can be safely accessed via dynamic string indexing when using union types like "x" | "y" | "z". The pattern `vector[axis] = value` where axis is "x" | "y" | "z" does not cause TypeScript strict mode errors because TypeScript can infer the property access is valid.
Applied to files:
frontend/javascripts/viewer/constants.ts
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
🧬 Code graph analysis (1)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (2)
frontend/javascripts/test/fixtures/dataset_server_object.ts (1)
sampleTracingLayer(63-67)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
finishSectionLabeler(367-392)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (7)
frontend/javascripts/viewer/constants.ts (1)
50-54: Spelling fix on Vector*Indices exports looks goodRenaming
Vector[2-6]Indicies→Vector[2-6]Indiceswhile keeping the contents identical is a pure naming cleanup and improves readability. Since these are exported constants, just ensure all imports have been updated (which the PR summary indicates you already did).frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (6)
1-1: LGTM!The import changes correctly reflect the refactoring from "VolumeLayer" to "SectionLabeler" terminology and add necessary fixtures for the updated test flow.
Also applies to: 17-17, 22-22, 27-28
138-192: Note the inconsistency in SectionLabeler instantiation approach.Test 1 (lines 94-136) validates the saga-based SectionLabeler creation flow with a nested saga, while this test and subsequent tests (lines 194-259, 261-326) directly instantiate
SectionLabelervia its constructor. This inconsistency might indicate:
- Test 1 is specifically validating the creation saga, while others are unit testing the labeler behavior directly (intentional)
- Tests 2-4 weren't fully updated to use the new saga-based creation flow (unintentional)
Given the PR notes mention "not all tests currently succeed," please confirm this mixed approach is intentional.
190-191: Verify the expected minCoord and maxCoord values.The test asserts that after adding positions
[1,2,3],[2,3,4], and[3,4,5], the section labeler's bounding box isminCoord: [-1,0,1]andmaxCoord: [5,6,7]. This represents a 2-unit expansion in each direction.Please confirm these expected values are still correct after the refactoring, as the expansion logic might be affected by the changes to
SectionLabeler.
242-258: LGTM!The
finishSectionLabelercalls correctly match the function signature from the relevant code snippet, passing all required parameters (sectionLabeler, activeTool, contourTracingMode, overwriteMode, labeledZoomStep, wroteVoxelsBox).Also applies to: 312-325
174-180: TheisSwappedparameter controls coordinate system swapping during plane transformation.The fifth boolean parameter is
isSwapped, which indicates whether the plane's coordinates are swapped when applying coordinate transformations. In production code, this value is determined bymapTransformedPlane()based on the actual transform applied. For these direct unit tests ofSectionLabelerwithout coordinate transformation,falseis the correct value—it indicates the plane operates in standard (non-swapped) coordinates.All three test instances (lines 174–180, 230–236, 297–303) consistently use
false, which is appropriate since they test the baseSectionLabelerin isolation.
49-55: The mockedDataset setup is complete and sufficient.The
update()function from immutability-helper preserves all properties fromdefaultVolumeState.datasetexcept those explicitly modified. SincegetLayerByName()only accessesdataSource.dataLayers(viagetDataLayers()), andmockedDatasetupdates this with thesampleTracingLayer, all required properties are present. The test at line 131 successfully usesmockedDatasetto create a SectionLabeler, confirming the setup works as intended.Likely an incorrect or invalid review comment.
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
89-94: Align segment picking and storedsomePositionto use the same rounded global position.
handlePickCellFromGlobalPositioncurrently:
- Uses
globalPos.roundedto look up thesegmentIdviagetSegmentIdForPosition- But uses
globalPos.floatingto computelayerPosviagetUntransformedSegmentationPosition, which then becomessomePosition.With non-linear transforms (TPS),
globalPos.roundedandglobalPos.floatingcan map to different voxels in layer space, sosomePositionmight not correspond to the voxel from which the segmentId was actually picked.To keep the picked segment and stored position consistent, consider using the same rounded global coordinates for both:
- const layerPos = getUntransformedSegmentationPosition(state, globalPos.floating); + const layerPos = getUntransformedSegmentationPosition(state, globalPos.rounded);If you intentionally want higher-precision storage, a short comment explaining why this mismatch is safe would also help future readers.
Also applies to: 219-235
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
49-56: Tighten DatasetConfiguration mock and strengthen labeller assertionsThe create-section-labeler saga test looks good conceptually, but two details are worth adjusting:
mockedDatasetis fine, but the config passed tocreateSectionLabelerSagauses{ nativelyRenderedLayerName: undefined }. TheDatasetConfigurationshape defines this asstring | null, so usingnullinstead ofundefinedwould better match the real type and other tests.- You currently only assert
labeller.getPlane() === OrthoViews.PLANE_XY. Given you construct the labeller viacreateSectionLabeler, you could additionally assert that it’s initialized for the expected tracing (volumeTracing.tracingId) and active mag (e.g. by checking specific properties or behavior) to catch miswired transforms early.Also applies to: 94-136
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (2)
74-106: FixVoxelBuffer2D.printto iterate rows by height, not width
linearizeIndexstores values asx * this.height + y, butprint()loops bothxandyup tothis.width. For non-square buffers this will either skip rows or read incorrect entries.You can fix the iteration as follows:
- print(): void { - const lines = []; - for (let y = 0; y < this.width; y++) { - const line = []; - for (let x = 0; x < this.width; x++) { - line.push(this.getValue(x, y)); - } - lines.push(line); - } - console.log("VoxelBuffer content:", lines.join("\n")); - } + print(): void { + const lines = []; + for (let y = 0; y < this.height; y++) { + const line = []; + for (let x = 0; x < this.width; x++) { + line.push(this.getValue(x, y)); + } + lines.push(line); + } + console.log("VoxelBuffer content:", lines.join("\n")); + }This only affects a debugging helper but keeps it accurate.
702-712: Apply the transform inTransformedSectionLabelerbefore delegating to the base labeler
TransformedSectionLabelercorrectly transforms coordinates forupdateArea, rectangles, and centroids, but:
globalCoordToMag2DFloatforwards the untransformed position to the base labeler.getCircleVoxelBuffer2Dforwards the untransformed center point as well.Given that
baseoperates in the transformed plane, these two methods should also work in that coordinate system; otherwise, 2D coordinates and brush masks will be computed for the wrong location when a non-identity transform is active.A minimal fix would be:
- globalCoordToMag2DFloat(position: Vector3): Vector2 { - return this.base.globalCoordToMag2DFloat(position); - } + globalCoordToMag2DFloat(position: Vector3): Vector2 { + const transformed = this.applyTransform(position); + return this.base.globalCoordToMag2DFloat(transformed); + } - getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { - let scale = this.adaptScaleFn( - getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), - ); - - return this.base.getCircleVoxelBuffer2D(position, scale); - } + getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { + const scale = this.adaptScaleFn( + getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), + ); + const transformed = this.applyTransform(position); + return this.base.getCircleVoxelBuffer2D(transformed, scale); + }This keeps identity transforms unchanged while making transformed views consistent with how
updateAreaandgetRectangleVoxelBuffer2Dalready behave.
🧹 Nitpick comments (2)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
248-251: Use the passed-instateinhandleFloodFillinstead ofStore.getState().
handleFloodFilltakesstate: WebknossosStatebut still callscalculateGlobalPos(Store.getState(), screenPos). For a cleaner, deterministic API (and easier testing), it would be better to rely solely on the passed-in state:-export function handleFloodFill(state: WebknossosState, screenPos: Point2, plane: OrthoView) { - const globalPosRounded = calculateGlobalPos(Store.getState(), screenPos).rounded; +export function handleFloodFill(state: WebknossosState, screenPos: Point2, plane: OrthoView) { + const globalPosRounded = calculateGlobalPos(state, screenPos).rounded; handleFloodFillFromGlobalPosition(state, globalPosRounded, plane); }This keeps the world→layer mapping and the dataset/transforms in
handleFloodFillFromGlobalPositionon the same state snapshot.frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
599-643: Check whether scale adaptation should use the mapped plane instead of the original
mapTransformedPlanecomputesbestViewfrom the transformed normal butadaptScaleFnstill callsDimensions.transDim(scale, originalPlane)and decides on swapping based on the original basis. WhenbestView !== originalPlanethis may yield scale vectors that don’t match the plane actually used by theSectionLabeler, which could contribute to the remaining “squished” appearance mentioned in the PR description.If you’re still seeing anisotropic brushes on transformed views, consider driving
adaptScaleFnoffbestView(and only applying the swap when you truly stay in the same plane) so the 2D scale matches the effective viewing plane.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/javascripts/test/model/transformed_section_labeler.spec.ts(1 hunks)frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts(12 hunks)frontend/javascripts/viewer/controller/combinations/tool_controls.ts(2 hunks)frontend/javascripts/viewer/controller/combinations/volume_handlers.ts(4 hunks)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts(4 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts(2 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/floodfill_saga.tsx(1 hunks)frontend/javascripts/viewer/model/sagas/volume/helpers.ts(6 hunks)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx(8 hunks)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts(15 hunks)frontend/javascripts/viewer/store.ts(2 hunks)frontend/javascripts/viewer/view/context_menu.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/store.ts
- frontend/javascripts/test/model/transformed_section_labeler.spec.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/volume/floodfill_saga.tsxfrontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/volumetracing/section_labeling.tsfrontend/javascripts/viewer/model/actions/volumetracing_actions.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.tsfrontend/javascripts/viewer/controller/combinations/volume_handlers.tsfrontend/javascripts/viewer/model/sagas/volume/helpers.tsfrontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Applied to files:
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-05-30T12:36:09.930Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8542
File: frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx:0-0
Timestamp: 2025-05-30T12:36:09.930Z
Learning: The WebKnossos frontend follows Redux immutability principles where reducers act immutably and create new state objects when changes occur. This makes reference equality checks (like `prevVolumeTracing.segments !== volumeTracing.segments`) reliable and efficient for detecting actual state changes, allowing memoization to work correctly.
Applied to files:
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.tsfrontend/javascripts/viewer/model/reducers/volumetracing_reducer.tsfrontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.
Applied to files:
frontend/javascripts/viewer/view/context_menu.tsxfrontend/javascripts/viewer/controller/combinations/volume_handlers.ts
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-04-01T09:45:17.527Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
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.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts
🧬 Code graph analysis (9)
frontend/javascripts/viewer/model/sagas/volume/floodfill_saga.tsx (1)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
floodFillAction(190-200)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (2)
frontend/javascripts/viewer/store.ts (1)
VolumeTracing(193-219)frontend/javascripts/viewer/constants.ts (1)
Vector3(15-15)
frontend/javascripts/viewer/view/context_menu.tsx (1)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (1)
handleFloodFillFromGlobalPosition(252-259)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (1)
addToContourListReducer(131-148)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (6)
frontend/javascripts/viewer/constants.ts (5)
Vector2(14-14)Vector3(15-15)OrthoView(67-67)Vector3Indices(51-51)Vector2Indices(50-50)frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
getVolumeTracingById(69-82)frontend/javascripts/viewer/model/scaleinfo.ts (1)
getBaseVoxelFactorsInUnit(26-33)frontend/javascripts/viewer/model/helpers/position_converter.ts (1)
zoomedPositionToGlobalPosition(45-47)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (1)
Transform(11-27)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
invertAndTranspose(372-374)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)
frontend/javascripts/viewer/constants.ts (2)
Vector3(15-15)OrthoView(67-67)
frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (3)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (2)
finishEditingAction(209-212)startEditingAction(177-182)frontend/javascripts/test/fixtures/dataset_server_object.ts (1)
sampleTracingLayer(63-67)frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
finishSectionLabeler(367-392)
frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (8)
frontend/javascripts/viewer/constants.ts (4)
Point2(28-31)OrthoView(67-67)Vector3(15-15)AdditionalCoordinate(3-3)frontend/javascripts/viewer/model/accessors/view_mode_accessor.ts (2)
calculateGlobalPos(368-368)PositionWithRounding(95-95)frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (5)
startEditingAction(177-182)addToContourListAction(184-188)finishEditingAction(209-212)setActiveCellAction(214-226)floodFillAction(190-200)frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
getLayerByName(147-166)getVisibleSegmentationLayer(430-440)frontend/javascripts/viewer/model.ts (2)
getLayerByName(215-221)getVisibleSegmentationLayer(119-133)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)frontend/javascripts/viewer/model/helpers/transformation_helpers.ts (2)
transformPointUnscaled(165-172)invertTransform(71-87)frontend/javascripts/types/api_types.ts (1)
AdditionalCoordinate(33-33)
frontend/javascripts/viewer/model/sagas/volume/helpers.ts (4)
frontend/javascripts/viewer/store.ts (1)
VolumeTracing(193-219)frontend/javascripts/viewer/constants.ts (2)
OrthoView(67-67)Vector3(15-15)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
TransformedSectionLabeler(645-722)frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts (1)
getTransformsForLayer(262-270)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (15)
frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (3)
17-18: RenamingAddToLayer→AddToContourListin action types is consistent and clear.The new
AddToContourListActionalias and its inclusion inVolumeTracingActioncleanly reflect the renamed semantics without changing behavior.Also applies to: 82-87
177-201: Layer-spacepositionInLayerSpaceparameters align actions with the new coordinate conventions.Using
positionInLayerSpace(with the inline “in layer space” hint) forSTART_EDITING,ADD_TO_CONTOUR_LIST, andFLOOD_FILLmatches the new world→layer conversion in the controllers/sagas and makes the coordinate space explicit.
418-425:dispatchFloodfillAsynccorrectly forwards layer-space coordinates.The async helper now clearly expects a layer-space
positionInLayerSpaceand simply wrapsfloodFillActionwith a completion callback; no hidden transforms or behavior changes introduced.frontend/javascripts/viewer/controller/combinations/volume_handlers.ts (2)
36-44: Centralized world→layer conversion viagetUntransformedSegmentationPositionlooks correct.Using
calculateGlobalPos(...).roundedfollowed bygetUntransformedSegmentationPositionfor draw/erase/move and flood fill ensures all volumetric tools now operate in layer space, aligning with the newpositionInLayerSpacepayloads and the transformation helpers.Also applies to: 46-69, 71-84, 252-259
675-696: Contour geometry listener correctly updated toADD_TO_CONTOUR_LIST.Switching the contour-maintenance loop to listen for
"ADD_TO_CONTOUR_LIST"matches the renamed action and keeps the scene contour meshes in sync with the new contour list semantics.frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
5-5: SectionLabeler-based editing flow andfinishSectionLabelerextraction look correct and consistent.
startEditingAction.positionInLayerSpaceis now written intosegment.somePositionand used as the anchor forcreateSectionLabeler, so all subsequent voxel buffers operate in layer space.- The ADD_TO_CONTOUR_LIST channel uses
positionInLayerSpaceeverywhere (lastPosition,updateArea, rectangle/circle voxel buffers), and relies oncurrentSectionLabeler.getPlane()instead of the viewport, which matches the new SectionLabeler API.finishSectionLabelercleanly encapsulates the fill step (viagetFillingVoxelBuffer2D) and centroid registration (getUnzoomedCentroid), and supports bothSectionLabelerandTransformedSectionLabelerwithout duplicating logic.Overall, the saga wiring for drawing/erasing/contouring after the refactor is internally consistent and matches the new layer-space semantics.
Also applies to: 83-85, 91-91, 231-241, 243-249, 255-255, 260-260, 264-266, 270-276, 286-327, 331-338, 367-392
frontend/javascripts/viewer/view/context_menu.tsx (1)
1459-1463: Flood-fill menu now correctly passes state into handlerPassing
Store.getState()intohandleFloodFillFromGlobalPositionmatches the new state-aware signature and keeps all arguments non-null in this branch; no further changes needed here.frontend/javascripts/viewer/model/sagas/volume/floodfill_saga.tsx (1)
158-172: Saga correctly switched topositionInLayerSpaceUsing
{ positionInLayerSpace, planeId }is consistent with the updatedfloodFillActionpayload; the rest of the saga still works with the rounded seed position, so behavior is preserved while moving to layer-space coordinates.frontend/javascripts/viewer/controller/combinations/tool_controls.ts (1)
552-561: Flood-fill tool controllers are updated to the new handler signatureBoth erase and fill tools now pass
Store.getState()intoVolumeHandlers.handleFloodFill, matching the new(state, pos, plane)API. This keeps the behavior localized at the controller layer without changing higher-level flow.Also applies to: 638-646
frontend/javascripts/viewer/model/reducers/volumetracing_reducer.ts (1)
31-46: ADD_TO_CONTOUR_LIST is wired through the main reducer correctlyThe reducer now delegates
"ADD_TO_CONTOUR_LIST"toaddToContourListReducerwithaction.positionInLayerSpace, reusing the existinggetVolumeTracingFromActiongating. This keeps contour updates localized to the active tracing and preserves immutability guarantees via the helper.Also applies to: 435-437
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (1)
131-147: Contour list reducer respects update/zoom guards and immutability
addToContourListReducerappends the new layer-space point only when updates are allowed and the current zoom/tool combination permits volume annotation, and usesupdateVolumeTracingto keep state updates immutable. This aligns with the rest of the reducer helpers.frontend/javascripts/viewer/model/sagas/volume/helpers.ts (2)
174-276: Please re-verify VoxelBuffer2D / applyVoxelMap coordinate contractsThe new
labelWithVoxelBuffer2DbuildscurrentLabeledVoxelMapfromvoxelBuffer.getTopLeft3DCoord/getBottomRight3DCoordand then passesvoxelBuffer.getFast3DCoordinateintoapplyVoxelMap. Compared to the previous bespokeget3DAddressCreator, this tightens coupling between:
- how
labelMapOfBucketis indexed (globalA/globalBand% BUCKET_WIDTH), and- what coordinate system
applyVoxelMapexpects for its(x, y)arguments when calling the provided 3D address function.Given how easy it is to get off-by-bucket or off-by-mag errors here—especially under rotation and non-unit magnifications—I’d recommend double-checking this path (ideally with rotated + scaled datasets) to ensure we’re neither double-applying nor omitting bucket offsets.
283-303: Confirm that usingvolumeTracing.tracingIdwithgetLayerByNameis always valid
createSectionLabelerlooks up the layer viagetLayerByName(dataset, volumeTracing.tracingId)before callinggetTransformsForLayer. This assumes the dataset layer’snameis equal to the tracing’stracingId. If that invariant isn’t guaranteed for all datasets, consider switching togetSegmentationLayerForTracing(or a similar accessor) or making the name↔tracingId relationship explicit to avoid failures on misaligned configurations.frontend/javascripts/test/sagas/volumetracing/volumetracing_saga.spec.ts (1)
120-191: Saga tests align well with SectionLabeler / contour-list refactorThe updates to:
- use
startEditingAction.positionInLayerSpacein the expectedupdateSegmentAction,- feed a
SectionLabelerinstance into the saga and drive contour updates viaaddToContourListActionFn, and- assert the
finishSectionLabelercall with the correct mode/overwrite/zoom parametersall mirror the new section-labeling flow and provide good coverage for the edit/finish path. No issues seen here.
Also applies to: 193-323
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
724-734:getFast3DCoordinateFnis simple and consistent with SectionLabeler’s contractUsing
Dimensions.getIndices(plane)and writing(coordX, coordY, thirdDimensionValue)into[u, v, w]matches the documented “mag-dependent layer space” convention inSectionLabeler. This is a straightforward, easily testable mapping and looks correct given the surrounding usage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (4)
637-644: Verify the scale adaptation logic as part of the TODO follow-up.The
adaptScaleFnusesoriginalPlanefor the dimension transposition, but when the plane is remapped (e.g., XY→YZ), the scale should likely be driven frombestViewto pick the correct axes. The TODO at lines 630-634 already flags the swap detection as incomplete.When addressing the TODO, confirm whether the scale adaptation should use
bestViewinstead oforiginalPlane, as suggested in past reviews.
96-106: Fix the iteration bounds inprint().The outer loop iterates
yfrom0tothis.width, but it should iterate tothis.heightto correctly print all rows of non-square buffers.Apply this diff:
print(): void { const lines = []; - for (let y = 0; y < this.width; y++) { + for (let y = 0; y < this.height; y++) { const line = []; for (let x = 0; x < this.width; x++) { line.push(this.getValue(x, y)); } lines.push(line); } console.log("VoxelBuffer content:", lines.join("\n")); }
716-718: Transform the position before delegating to base.This method currently passes the untransformed position to the base labeler, which will return incorrect 2D coordinates for rotated/transformed viewers.
Apply this diff:
globalCoordToMag2DFloat(position: Vector3): Vector2 { - return this.base.globalCoordToMag2DFloat(position); + const transformed = this.applyTransform(position); + return this.base.globalCoordToMag2DFloat(transformed); }
720-727: Transform the position before creating the circle buffer.The circle buffer is created in the untransformed coordinate system, causing misalignment on transformed planes. The position should be transformed before being passed to the base method.
Apply this diff:
getCircleVoxelBuffer2D(position: Vector3): VoxelBuffer2D { let scale = this.adaptScaleFn( getBaseVoxelFactorsInUnit(Store.getState().dataset.dataSource.scale), ); - // todo: does this need a transformation? - return this.base.getCircleVoxelBuffer2D(position, scale); + const transformed = this.applyTransform(position); + return this.base.getCircleVoxelBuffer2D(transformed, scale); }
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
485-486: Consider inlining the scale default.The conditional can be simplified using the nullish coalescing operator for better readability.
Based on past review comments, consider:
- const [scaleX, scaleY] = - scale ?? this.get2DCoordinate(getBaseVoxelFactorsInUnit(state.dataset.dataSource.scale)); + const [scaleX, scaleY] = scale ?? this.get2DCoordinate( + getBaseVoxelFactorsInUnit(state.dataset.dataSource.scale), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/test/model/transformed_section_labeler.spec.ts(1 hunks)frontend/javascripts/viewer/constants.ts(2 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts(1 hunks)frontend/javascripts/viewer/model/volumetracing/section_labeling.ts(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/test/model/transformed_section_labeler.spec.ts
- frontend/javascripts/viewer/constants.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-22T17:18:43.411Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-04-01T09:45:17.527Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8485
File: frontend/javascripts/oxalis/model/accessors/dataset_layer_transformation_accessor.ts:384-392
Timestamp: 2025-04-01T09:45:17.527Z
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.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
📚 Learning: 2025-05-07T06:17:32.810Z
Learnt from: philippotto
Repo: scalableminds/webknossos PR: 8602
File: frontend/javascripts/oxalis/model/volumetracing/volume_annotation_sampling.ts:365-366
Timestamp: 2025-05-07T06:17:32.810Z
Learning: The parameter in applyVoxelMap was renamed from `sliceCount` to `sliceOffset` to better reflect its purpose, but this doesn't affect existing call sites since JavaScript/TypeScript function calls are position-based.
Applied to files:
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/viewer/model/volumetracing/section_labeling.ts (1)
739-749: Consider whether swap handling is needed in coordinate mapping.The
mapTransformedPlanefunction computes aswappedflag (line 635), but it's only used withinadaptScaleFnand not propagated to the 3D coordinate generation. If axis swapping affects the u/v mapping (as suggested by past reviews), this function may need to accept and respect a swap parameter.As part of addressing the TODOs at lines 630-634 and 655-658, verify whether
getFast3DCoordinateFnneeds to handle axis swapping when the plane transformation results in swapped u/v axes.
This PR:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
Added changelog entry (create ano user facing changes$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)