-
Notifications
You must be signed in to change notification settings - Fork 29
Transform meshes and bboxes according to transformation settings #8493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis set of changes restructures the scene graph and mesh management logic in the frontend, particularly within the 3D scene and annotation tools. The mesh and bounding box groups are reorganized to enable proper application of affine transformations, ensuring that user bounding boxes and meshes rotate in sync with their respective segmentation or volume layers. The logic for disabling annotation tools, especially the bounding box tool, is updated to reflect geometry transformation states. Various function and property names are updated for clarity, and related test cases are modified to align with the new behavior. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
blocked by #8106 to creating headache inducing merge conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment refine with AI:
Hej 😄
Here's the explanation for the fix that makes both bounding boxes and especially meshes rotate correctly based on how the dataset trees are transformed.
📦 Background
For demonstration lets assume a dataset with scale factor [10, 10, 40]
, and magnification levels mag 1, mag 2-2-1, mag 4-4-1. Moreover, the dataset extent is is 256³
voxels.
Before this PR, the scene graph looked like this:
- scene
- rootGroup (scaled by dataset scale factor)
- userBoundingBoxesGroup ← dataset transformations applied here
- ...
- meshesLODRootGroup (!not scaled by dataset scale factor!)
- outerMeshGroup (holds mesh-specific scale; see *1)
- innerMeshGroup ← dataset transformations applied here (❌ broken)
- mesh
*1: The mesh-specific scale comes from either precomputed metadata (e.g. mag 4-4-1 → scale [40, 40, 40]
) or is identity if the mesh is already in mag 1 or ad-hoc.
This setup worked fine until we tried applying dataset-wide transformations (e.g., rotation or centering). These are applied in the dataset-scaled space — i.e., they assume the scale [10, 10, 40]
has already been applied. This works for bounding boxes (they're under rootGroup
), but fails for meshes, which live outside of that scale space.
🔁 The Problem
For example, rotating the dataset 180° around the x-axis gives us this matrix (column-major):
[ 1, 0, 0, 0,
0, -1, 0, 0,
0, 0, -1, 0,
0, 256, 256, 1 ]
This works as expected for the bounding boxes. But when applied to the meshesLODRootGroup
, the transform has no real effect — because it's missing the dataset scale.
Even worse, if you apply the transform inside the mesh hierarchy oninnerMeshGroup
s, the mesh’s own scale (e.g. [40, 40, 40]
from mag 4-4-1) multiplies the transformation's translation — resulting in a world position 4× too far in the Y direction (✅ correct Z, because the dataset scale is [10,10,40]
→ same Z scale *2).
✅ The Fix
We now match the meshes hierarchy to the bounding boxes hierarchy. The idea: split the mesh’s scale into two parts:
[40, 40, 40] = [10, 10, 40] * [4, 4, 1]
[10, 10, 40]
→ dataset scale (applied at the root level, viarootGroup
)[4, 4, 1]
→ mesh-specific mag scale (applied at the mesh group level)
📐 Updated Scene Graph
- scene
- rootGroup (scaled by dataset scale factor)
- userBoundingBoxesGroup
- ...
- meshesLODRootGroup ← dataset transformations applied here
(!now correctly scaled by dataset factor!)
- outerMeshGroup ← mag scale factor applied here
- innerMeshGroup
- mesh
Now, both bounding boxes and meshes live in the same coordinate space, and transformations behave consistently 🎉
Hope that clarifies the change! Let me know if anything’s unclear or if you have a better way to structure the scale split.
*2: *2: A rotation round 180 degrees on the x axis only changes the y and z coordinates (see the matrix) and the different scale [40,40,40]
<-> [10,10,40]
z does not differ -> only a displacement in y direction with the factor of 4.
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
// If no scale was given, the meshes coordinates are already in scale of dataset and | ||
// thus the scaling done by the root group needs to be inverted. | ||
scale = scale || [1, 1, 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an important thing here imo.
A alternative solution would be to return ad hoc meshes in the backend by not already applying the dataset scale factor and for precomputed meshes return the relative factor / mag factor on which the mesh was calculated (e.g. mag 4,4,1 -> [4,4,1] and not [40,40,10] or so). Then this whole division by dsScaleFactor
could be left out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But on second thought, this would require to change the mesh file descriptor entries which is very likely not what we want, correct @fm3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! thanks for making this possible :) left mostly cosmetic feedback.
during review, I noticed another potential problem. bounding boxes can currently be stored for skeleton and volume layers. this is a known "code smell" which bites us every now and then. the current semantics are that there is one set of bboxes and they are stored in "whatever layer is available". in the typical hybrid annotation, they would be stored in the skeleton layer. in a volume-only annotation, the bboxes are stored in the volume layer. does the current PR use the transforms of that layer in that case?
afaik it should never ™️ (cc @fm3) happen that some bboxes are stored in the skeleton layer and some bboxes are stored in a volume layer. so, we should be fine with giving all bboxes the transforms of one layer (deleting a layer might be weird, because the transforms could change then? but maybe this is an okay side-effect).
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
Yes, my understanding is that all bounding boxes are read from and saved to the “precedence” layer. This is the skeleton layer if it exists, and otherwise the volume layer with the lexicographically smallest tracingId. When the layer set changes, and this changes which layer has precedence, we copy the boxes to the new precedence layer. |
Oh that's actually quite the problem you noticed there :/. I thought of applying the same to bboxes as to the potential skeleton would keep things consistent as the skeleton layer just rotates according to what is currently rendered natively. Thus, doing the same for bboxes independent of whether the data is stored on a skeleton or volume annotation also sounds consistent to me. So regarding:
That's a no. It always renders like the skeletons would. Even if there are none. So in short my open questions:
But what's more of a problem now is, that the meshes also rotate as if they were the rendered trees. but they should be transformed like the segmentation layer they belong to. And there can be meshes from different segmentation layers active at the same time, if I am not wrong. So, they should be transformed exactly like their segmentation layers would, but the scene hierarchy does not support retrieving the segmentation layer and render meshes differently depending on that or does it? I see 2 possible solutions for this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback and mentioning the thought about how bboxes should be rendered. It opened up a new question for me. See above.
Moreover, I gave your refactoring suggestion a try. Not sure how much improvement that is. Happy to hear you comments on that :D
the semantical correct approach would be that bounding boxes should always get the transformation of the layer that is returned by
yes, I think, this would be a good approach (probably simpler than your second suggestion). meshes from invisible segmentation layers could then be made invisible. this would be nice in general, I think (skeletons are also hidden in the 3D viewport when the skeleton layer is disabled <-- so, it would be similar for meshes then). |
…o active segmentation layer
…nsform-meshes-and-bboxes
…nsform-meshes-and-bboxes
const transformForMeshes = getTransformsForLayer( | ||
state.dataset, | ||
visibleSegmentationLayers[0], | ||
state.datasetConfiguration.nativelyRenderedLayerName, | ||
); | ||
const transformMatrix = transformsForGeometries?.affineMatrix; | ||
if (transformMatrix) { | ||
const matrix = new THREE.Matrix4(); | ||
// @ts-ignore | ||
matrix.set(...transformMatrix); | ||
// We need to disable matrixAutoUpdate as otherwise the update to the matrix will be lost. | ||
this.userBoundingBoxGroup.matrixAutoUpdate = false; | ||
this.userBoundingBoxGroup.matrix = matrix; | ||
this.segmentMeshController.meshesLODRootGroup.matrixAutoUpdate = false; | ||
this.segmentMeshController.meshesLODRootGroup.matrix = matrix; | ||
} | ||
this.applyTransformToGroup(transformForMeshes, this.segmentMeshController.meshesLODRootGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we discussed here, this will transform meshes of all layers with the transforms of the visible segmentation layer 👍 I think, this is fine if meshes of invisible layers also get hidden automatically (which isn't done in master). does this happen in this pr? I think, we need this for correct mesh rendering.
maybe "refactor SegmentMeshController to grouping meshes by their layer" (quote by you) is necessary for doing the invisible stuff, too? so, one could also do the transform per mesh-layer with not a lot of extra work, but I'm fine with either way 🙈
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I overlooked the part about making meshes of invisible layers invisible as well. The scene hierarchy should now be adjusted to make this relativly easy. The structure is now: Each layer has its own lod group with the respective meshes and the lod group for each layer can be made invisible on demand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
259-270
:⚠️ Potential issueRemoving meshes crashes – wrong parent assumed
meshGroup.parent
is the LOD–level group, not theCustomLOD
instance.
CallingremoveLODMesh
/removeNoLODSupportedMesh
on it raisesTypeError: parentLODGroup.removeLODMesh is not a function
.-const parentLODGroup = meshGroup.parent as CustomLOD; +const parentLODGroup = + (meshGroup.parent?.parent as CustomLOD) ?? + this.getLODGroupOfLayer(layerName); // fallback safetyAlternatively fetch the
CustomLOD
throughgetLODGroupOfLayer(layerName)
directly.
🧹 Nitpick comments (5)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
203-211
: Name‑based lookup is O(n) and yields false‑positives
getObjectByName
performs a depth‑first traversal and returns the first matching node.
If a mesh (or any sub‑group) happens to havename === layerName
, you will accidentally retrieve that object instead of the expectedCustomLOD
. Consider keeping an explicitMap<string,CustomLOD>
or storing auserData.type = 'layerLOD'
flag for robust retrieval.
303-314
: Visibility helper ignores nested children
Togglingvisible
on theCustomLOD
hides the outer group but ThreeJS still traverses into its children during ray‑casting unlesslayers
are also masked. Consider iterating overlevel.object.visible
instead for perf‑critical scenes.frontend/javascripts/oxalis/view/plane_view.ts (1)
140-167
: Null‑safety aroundmeshesLayerLODRootGroup
segmentMeshController
is guaranteed in normal runs, but in failure states (e.g. scene teardown)meshesLayerLODRootGroup
could beundefined
, leading toCannot read properties of undefined (reading 'children')
.-const { meshesLayerLODRootGroup } = segmentMeshController; +const { meshesLayerLODRootGroup } = segmentMeshController; +if (!meshesLayerLODRootGroup) return null;frontend/javascripts/oxalis/controller/scene_controller.ts (2)
108-123
: Chainedadd
call hides intent
scene.add(rootGroup.add(...))
relies onGroup.add
returningthis
, which is terse but non‑obvious. Splitting into two statements improves readability:-this.scene.add(this.rootGroup.add(this.rootNode, this.segmentMeshController.meshesLayerLODRootGroup)); +this.rootGroup.add(this.rootNode, this.segmentMeshController.meshesLayerLODRootGroup); +this.scene.add(this.rootGroup);
482-492
: Visibility update may run every store tick
_.isEqual
on arrays allocates and is invoked on every state change; consider computing a hash (visibleLayerNames.join('|')
) to compare strings instead of deep equality to reduce GC churn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx
(1 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts
(10 hunks)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
(8 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
(2 hunks)frontend/javascripts/oxalis/view/plane_view.ts
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
frontend/javascripts/oxalis/controller/custom_lod.ts (1)
CustomLOD
(5-65)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
NO_LOD_MESH_INDEX
(95-95)
🔇 Additional comments (4)
frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
362-362
: Good defensive programming practice!Adding optional chaining (
?.
) when accessingdataLayers[index]
prevents potential runtime errors if the index is out of bounds or if the data layer at that index is undefined. This makes the code more robust when handling potentially undefined layer data.frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
218-232
: Potential unit mismatch in scale calculation
dsScaleFactor
is assumed to be a 3‑tuple (Vector3
). In several datasets this field is a scalar; indexing it would therefore returnundefined
and yieldNaN
scales, producing invisible meshes.Add a runtime safeguard:
-const dsScaleFactor = Store.getState().dataset.dataSource.scale.factor; +const dsScaleFactor = Store.getState().dataset.dataSource.scale.factor; +const dsScaleVec = + typeof dsScaleFactor === "number" ? [dsScaleFactor, dsScaleFactor, dsScaleFactor] : dsScaleFactor; ... -const adaptedScale = [ - scale[0] / dsScaleFactor[0], - scale[1] / dsScaleFactor[1], - scale[2] / dsScaleFactor[2], -]; +const adaptedScale = [0, 1, 2].map((i) => scale[i] / dsScaleVec[i]);frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
934-938
:new CustomLOD()
is detached – zoom heuristics may be off
When no LOD group exists yet you instantiate a freshCustomLOD
, query it, then discard it.
BecauselodLevelCount === 0
,getCurrentLOD
always returns 0 and biases the loading order towards the lowest LOD. If the first chunk you fetch is not level‑0 the sort order becomes arbitrary.Either:
- seed
lodLevelCount = maxLod + 1
before callinggetCurrentLOD
, or- fall back to the middle LOD (
Math.round(maxLod/2)
) when no group exists.frontend/javascripts/oxalis/controller/scene_controller.ts (1)
450-480
: Meshes are transformed by the first visible segmentation layer only
If multiple segmentation layers are visible, meshes belonging to other layers will get the wrong transform but remain visible (depending onupdateMeshesAccordingToLayerVisibility
).
Either iterate over all visible layers and apply their individual transforms to their respectiveCustomLOD
, or hide meshes of non‑primary layers to avoid mis‑alignment.
this.lightsGroup = new THREE.Group(); | ||
this.meshesLayerLODRootGroup = new THREE.Group(); | ||
this.addLights(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lights are never attached to the scene – meshes will render un‑lit
lightsGroup
is instantiated and populated, but nothing ever adds it to either meshesLayerLODRootGroup
, rootGroup
, or the THREE.Scene
. Consequently the ambient / directional lights you create will never influence the scene.
- this.lightsGroup = new THREE.Group();
- this.meshesLayerLODRootGroup = new THREE.Group();
+ this.lightsGroup = new THREE.Group();
+ this.meshesLayerLODRootGroup = new THREE.Group();
+
+ // Make sure the lights are part of the visible scene graph
+ // (rootGroup is available from SceneController soon after construction)
+ // SceneController should add: sceneController.rootGroup.add(this.lightsGroup);
Either expose lightsGroup
so that SceneController
can add it, or add it directly in this constructor.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for noticing. should be fixed now 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh sorry, I missed the feedback regarding making meshes of not rendered layers invisible. This should now be done. Please re-review again 🙏
} | ||
|
||
private applyTransformToGroup(transform: Transform, group: THREE.Group | CustomLOD) { | ||
if (transform?.affineMatrix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well, I think I had some typing problems while developing and this was a left over. The ?
can indeed be left out in the current code 👍
this.lightsGroup = new THREE.Group(); | ||
this.meshesLayerLODRootGroup = new THREE.Group(); | ||
this.addLights(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for noticing. should be fixed now 🙏
const transformForMeshes = getTransformsForLayer( | ||
state.dataset, | ||
visibleSegmentationLayers[0], | ||
state.datasetConfiguration.nativelyRenderedLayerName, | ||
); | ||
const transformMatrix = transformsForGeometries?.affineMatrix; | ||
if (transformMatrix) { | ||
const matrix = new THREE.Matrix4(); | ||
// @ts-ignore | ||
matrix.set(...transformMatrix); | ||
// We need to disable matrixAutoUpdate as otherwise the update to the matrix will be lost. | ||
this.userBoundingBoxGroup.matrixAutoUpdate = false; | ||
this.userBoundingBoxGroup.matrix = matrix; | ||
this.segmentMeshController.meshesLODRootGroup.matrixAutoUpdate = false; | ||
this.segmentMeshController.meshesLODRootGroup.matrix = matrix; | ||
} | ||
this.applyTransformToGroup(transformForMeshes, this.segmentMeshController.meshesLODRootGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I overlooked the part about making meshes of invisible layers invisible as well. The scene hierarchy should now be adjusted to make this relativly easy. The structure is now: Each layer has its own lod group with the respective meshes and the lod group for each layer can be made invisible on demand
The code looks good 👍 however, I cannot load precomputed meshes in view mode. I get:
can you have a look? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/controller/scene_controller.ts (1)
448-457
: 🛠️ Refactor suggestionMatrix world needs update after manual matrix assignment.
After overwriting
group.matrix
and disablingmatrixAutoUpdate
, the world matrices of descendants can remain stale until something triggers an update.Add a line to explicitly trigger matrix update to prevent visual glitches:
group.matrixAutoUpdate = false; group.matrix = matrix; + group.matrixWorldNeedsUpdate = true;
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/controller/scene_controller.ts (1)
491-502
: Remove debug console log in production code.There's a console.log statement that should be removed before merging to production.
updateMeshesAccordingToLayerVisibility(): void { const state = Store.getState(); - console.log("updating according to visible layers"); const visibleSegmentationLayers = getVisibleSegmentationLayers(state); const allSegmentationLayers = getSegmentationLayers(state.dataset);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/oxalis/controller/scene_controller.ts
(10 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
(2 hunks)frontend/javascripts/test/sagas/annotation_tool_disabled_info.spec.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/test/sagas/annotation_tool_disabled_info.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (8)
frontend/javascripts/oxalis/controller/scene_controller.ts (8)
64-68
: Good use of memoization for visible layer detection.Using
reuseInstanceOnEquality
to memoize the visible segmentation layer names is an excellent performance optimization. This prevents unnecessary re-renders and computations when the layer selection hasn't changed.
114-121
: Improved scene graph organization.The refactored scene graph structure with a clearly defined hierarchy improves maintainability. By grouping the
rootNode
,meshesLayerLODRootGroup
, andlightsGroup
under a singlerootGroup
, you've created a cleaner and more manageable scene organization.
243-255
: Clean and concise root node construction.The refactored approach to creating the root node and adding all the necessary meshes and geometry groups is much clearer. Using method chaining and spread operators makes the code more readable.
330-330
: Correctly updated mesh visibility.Using the new
meshesLayerLODRootGroup
reference for visibility control in the TDView is consistent with the refactored mesh controller organization.
459-489
: Well-implemented transformation logic for bounding boxes and meshes.The implementation correctly handles different transformations for different object types:
- Bounding boxes are transformed based on their owning layer
- Meshes are transformed based on the active segmentation layer
This aligns with the PR objectives to properly transform meshes according to transformation settings.
658-664
: Good separation of concerns in event handling.Separating the update of layer bounding boxes and the transformation of user bounding boxes/meshes into separate callbacks improves code readability and maintainability.
665-668
: Effective use of optimized selector for layer visibility.Using the memoized
getVisibleSegmentationLayerNames
selector in the store property listener ensures that mesh visibility updates only happen when the visible layers actually change.
583-585
: Consistent null check for meshes layer group.The null check here ensures that visibility updates don't attempt to access undefined properties, which is good defensive programming.
@philippotto could you please give this another look? The bug should be fixed now. It was due to the context not being bonded in two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/controller/scene_controller.ts (1)
446-455
:⚠️ Potential issueFix matrix world update after manual matrix assignment
When manually setting a group's matrix and disabling
matrixAutoUpdate
, you need to explicitly flag that the world matrix needs updating to avoid potential one-frame glitches or rendering inconsistencies.Apply this fix:
group.matrixAutoUpdate = false; group.matrix = matrix; +group.matrixWorldNeedsUpdate = true;
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/controller/scene_controller.ts (1)
489-500
: Remove debug console.log statementThere's a console.log statement that should be removed before merging.
updateMeshesAccordingToLayerVisibility(): void { const state = Store.getState(); - console.log("updating according to visible layers"); const visibleSegmentationLayers = getVisibleSegmentationLayers(state); const allSegmentationLayers = getSegmentationLayers(state.dataset); allSegmentationLayers.forEach((layer) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/controller/scene_controller.ts
(10 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (6)
frontend/javascripts/oxalis/controller/scene_controller.ts (6)
64-66
: Good optimization using reuseInstanceOnEqualityThis is a nice improvement over storing the visible segmentation layer names in an instance variable. Using
reuseInstanceOnEquality
prevents unnecessary re-renders by reusing the same array instance when the values haven't changed.
112-119
: Good scene graph reorganizationThe scene hierarchy is now more structured and logical, with proper grouping of related elements. This organization makes it easier to apply transformations at the appropriate level in the hierarchy, which aligns with the PR objective of applying transformations to the inner meshes group rather than the root group.
241-253
: Improved scene graph construction using fluent add methodNice refactoring that makes the scene graph hierarchy more readable by using the fluent
.add()
method with multiple arguments. This provides a clearer picture of the scene structure than the previous approach.
457-487
: Logic for transforming bounding boxes and meshes looks correctThe implementation correctly applies different transformations to bounding boxes and meshes based on their associated layers:
- Bounding boxes receive transforms from their associated layer (skeleton or volume)
- Meshes receive transforms from the first visible segmentation layer
This implements the agreed-upon approach from the PR discussion.
657-662
: Good combination of layer updates on nativelyRenderedLayerName changeThis correctly updates both the layer bounding boxes and the transforms when the natively rendered layer changes.
663-665
: Nice use of the optimized selector for updating mesh visibilityUsing the
getVisibleSegmentationLayerNames
selector withlistenToStoreProperty
ensures that mesh visibility is updated only when the set of visible segmentation layers actually changes.
|
||
updateMeshesAccordingToLayerVisibility(): void { | ||
const state = Store.getState(); | ||
console.log("updating according to visible layers"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, I used that to confirm no unnecessary recomputation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! only remove the one console.log before merging please :)
by the way, I really like that the meshes now turn invisible when the segmentation layer is invisible. feels way cleaner UX wise :) |
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
forEveryMesh
to easily manipulate all meshes.Issues:
(Please delete unneeded items, merge only when none are left open)