Clean up green task bounding boxes #8535
Conversation
…ll tracing layers
📝 Walkthrough""" WalkthroughThe changes refactor the management of task bounding boxes in the frontend scene controller to support multiple bounding boxes, each associated with a tracing ID, instead of a single bounding box. The code introduces a new accessor function to retrieve all relevant bounding boxes from an annotation and updates the scene controller to build, update, and manage these bounding boxes collectively. The new approach iterates over all bounding boxes, updating their visibility and scene presence based on the current annotation state, and removes outdated cubes as needed. The exported API and internal logic are adjusted to accommodate this multi-bounding box handling. Changes
Assessment against linked issues
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts (1)
94-101: Consider clarifying the function’s doc comment.The
_getTaskBoundingBoxesfunction is self-explanatory, but adding a short doc comment about what layers it processes and how it handles null bounding boxes can help future maintainers understand its behavior more quickly.frontend/javascripts/oxalis/controller/scene_controller.ts (1)
272-301: Method name might more accurately reflect multiple bounding boxes.Since this method handles multiple tracing IDs, renaming
buildTaskBoundingBoxtobuildTaskBoundingBoxescould clarify that it processes a set of bounding boxes rather than a single one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/controller/scene_controller.ts(8 hunks)frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts (1)
frontend/javascripts/oxalis/store.ts (1)
StoreAnnotation(285-290)
frontend/javascripts/oxalis/controller/scene_controller.ts (2)
frontend/javascripts/oxalis/constants.ts (1)
BoundingBoxType(41-44)frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts (1)
getTaskBoundingBoxes(100-100)
⏰ 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 (4)
frontend/javascripts/oxalis/controller/scene_controller.ts (4)
39-39: Import usage is clear.No concerns regarding the new import statement.
66-66: Good initialization of the bounding box record.The
taskCubeByTracingIdrecord is well-structured and clearly named. No issues here.
303-309: Helper function usage is well-organized.
forEachTaskCubenicely abstracts iteration over the bounding box cubes. The code is succinct and improves maintainability.
610-613: Validate the single-task assumption.By requiring
Store.getState().task != nullbefore creating bounding boxes, the code presumes a single active task. If multiple tasks may exist in the future, consider extending or revisiting this logic.
|
@hotzenklotz ping :) but feel free to delegate |
|
Sorry, overlooked this. Will review. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
20-20: Improve clarity of the changelog entry by adding a comma.To enhance readability and align with standard punctuation, add a comma after "green":
- Fixed that layer bounding boxes were sometimes colored green even though this should only happen for tasks. [#8535](https://github.com/scalableminds/webknossos/pull/8535) + Fixed that layer bounding boxes were sometimes colored green, even though this should only happen for tasks. [#8535](https://github.com/scalableminds/webknossos/pull/8535)🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...r bounding boxes were sometimes colored green even though this should only happen for...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts(10 hunks)frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/oxalis/model/accessors/tracing_accessor.ts
- frontend/javascripts/oxalis/controller/scene_controller.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...r bounding boxes were sometimes colored green even though this should only happen for...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/javascripts/oxalis/controller/scene_controller.ts (1)
226-236:⚠️ Potential issue
this.rootNodeis used before it is initialised – causes runtime crash
this.rootNodeis stillundefinedat this point increateMeshes().
Calling.add()on it will throw, preventing the scene from being created.- this.datasetBoundingBox.getMeshes().forEach((mesh) => this.rootNode.add(mesh));The dataset cube is already added a few lines later via the spread operator when
this.rootNode
is created, so this early call is both unsafe and redundant.
Please remove the highlighted line (or move it afterthis.rootNodeis assigned) to avoid the
crash.
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/controller/scene_controller.ts (2)
649-665: Visibility toggling works, but duplicates logic
stopPlaneMode()andstartPlaneMode()now correctly reuseforEachTaskCube, nice!
Consider extracting identical visibility toggling foruserBoundingBoxGroupand task cubes into a
helper to DRY these two methods, but this is purely optional.
695-696: Clear reference map on destroyAfter destroying each task cube you might also want to reset the map to free references:
this.forEachTaskCube((cube) => cube.destroy()); + this.taskCubeByTracingId = {};Not critical, but helps GC.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
| updateTaskBoundingBoxes( | ||
| taskCubeByTracingId: Record<string, BoundingBoxType | null | undefined>, | ||
| ): void { | ||
| /* | ||
| Ensures that a green task bounding box is rendered in the scene for | ||
| each layer. | ||
| The update is implemented by simply removing the old geometry and | ||
| adding a new one. Since this function is executed very rarely, | ||
| this is not a performance problem. | ||
| */ | ||
| for (const [tracingId, boundingBox] of Object.entries(taskCubeByTracingId)) { | ||
| let taskCube = this.taskCubeByTracingId[tracingId]; | ||
| // Remove the old box if it exists | ||
| if (taskCube != null) { | ||
| taskCube.getMeshes().forEach((mesh) => this.rootNode.remove(mesh)); | ||
| } | ||
| this.taskCubeByTracingId[tracingId] = null; | ||
| if (boundingBox == null || Store.getState().task == null) { | ||
| continue; | ||
| } | ||
|
|
||
| const { viewMode } = Store.getState().temporaryConfiguration; | ||
| this.taskBoundingBox = new Cube({ | ||
| min: taskBoundingBox.min, | ||
| max: taskBoundingBox.max, | ||
| taskCube = new Cube({ | ||
| min: boundingBox.min, | ||
| max: boundingBox.max, | ||
| color: 0x00ff00, | ||
| showCrossSections: true, | ||
| isHighlighted: false, | ||
| }); | ||
| this.taskBoundingBox.getMeshes().forEach((mesh) => this.rootNode.add(mesh)); | ||
| taskCube.getMeshes().forEach((mesh) => this.rootNode.add(mesh)); | ||
|
|
||
| if (constants.MODES_ARBITRARY.includes(viewMode)) { | ||
| this.taskBoundingBox?.setVisibility(false); | ||
| taskCube?.setVisibility(false); | ||
| } | ||
|
|
||
| this.taskCubeByTracingId[tracingId] = taskCube; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leaking geometry & leaving stale cubes in updateTaskBoundingBoxes
- Old cubes are detached from the scene but never
.destroy()-ed → GPU memory leak. - Cubes belonging to tracing IDs that disappear from the new map are never removed at all, so
obsolete green boxes stay around. Store.getState()is called in every iteration although the value is constant within the
function.
A minimal, safe fix:
updateTaskBoundingBoxes(
taskCubeByTracingId: Record<string, BoundingBoxType | null | undefined>,
): void {
- for (const [tracingId, boundingBox] of Object.entries(taskCubeByTracingId)) {
+ const previousIds = new Set(Object.keys(this.taskCubeByTracingId));
+ const state = Store.getState(); // single lookup
+
+ for (const [tracingId, boundingBox] of Object.entries(taskCubeByTracingId)) {
+ previousIds.delete(tracingId); // mark as still in use
let taskCube = this.taskCubeByTracingId[tracingId];
// Remove the old box if it exists
if (taskCube != null) {
- taskCube.getMeshes().forEach((mesh) => this.rootNode.remove(mesh));
+ taskCube.getMeshes().forEach((mesh) => this.rootNode.remove(mesh));
+ taskCube.destroy();
}
this.taskCubeByTracingId[tracingId] = null;
- if (boundingBox == null || Store.getState().task == null) {
+ if (boundingBox == null || state.task == null) {
continue;
}
+ // … create & add new Cube as before …
}
+
+ // Remove cubes that are no longer present
+ for (const staleId of previousIds) {
+ const staleCube = this.taskCubeByTracingId[staleId];
+ if (staleCube) {
+ staleCube.getMeshes().forEach((m) => this.rootNode.remove(m));
+ staleCube.destroy();
+ }
+ delete this.taskCubeByTracingId[staleId];
+ }
}This prevents memory leaks and guarantees that the scene exactly mirrors the current set of
task bounding boxes.
📝 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.
| updateTaskBoundingBoxes( | |
| taskCubeByTracingId: Record<string, BoundingBoxType | null | undefined>, | |
| ): void { | |
| /* | |
| Ensures that a green task bounding box is rendered in the scene for | |
| each layer. | |
| The update is implemented by simply removing the old geometry and | |
| adding a new one. Since this function is executed very rarely, | |
| this is not a performance problem. | |
| */ | |
| for (const [tracingId, boundingBox] of Object.entries(taskCubeByTracingId)) { | |
| let taskCube = this.taskCubeByTracingId[tracingId]; | |
| // Remove the old box if it exists | |
| if (taskCube != null) { | |
| taskCube.getMeshes().forEach((mesh) => this.rootNode.remove(mesh)); | |
| } | |
| this.taskCubeByTracingId[tracingId] = null; | |
| if (boundingBox == null || Store.getState().task == null) { | |
| continue; | |
| } | |
| const { viewMode } = Store.getState().temporaryConfiguration; | |
| this.taskBoundingBox = new Cube({ | |
| min: taskBoundingBox.min, | |
| max: taskBoundingBox.max, | |
| taskCube = new Cube({ | |
| min: boundingBox.min, | |
| max: boundingBox.max, | |
| color: 0x00ff00, | |
| showCrossSections: true, | |
| isHighlighted: false, | |
| }); | |
| this.taskBoundingBox.getMeshes().forEach((mesh) => this.rootNode.add(mesh)); | |
| taskCube.getMeshes().forEach((mesh) => this.rootNode.add(mesh)); | |
| if (constants.MODES_ARBITRARY.includes(viewMode)) { | |
| this.taskBoundingBox?.setVisibility(false); | |
| taskCube?.setVisibility(false); | |
| } | |
| this.taskCubeByTracingId[tracingId] = taskCube; | |
| } | |
| } | |
| updateTaskBoundingBoxes( | |
| taskCubeByTracingId: Record<string, BoundingBoxType | null | undefined>, | |
| ): void { | |
| /* | |
| Ensures that a green task bounding box is rendered in the scene for | |
| each layer. | |
| The update is implemented by simply removing the old geometry and | |
| adding a new one. Since this function is executed very rarely, | |
| this is not a performance problem. | |
| */ | |
| const previousIds = new Set(Object.keys(this.taskCubeByTracingId)); | |
| const state = Store.getState(); // single lookup | |
| for (const [tracingId, boundingBox] of Object.entries(taskCubeByTracingId)) { | |
| previousIds.delete(tracingId); // mark as still in use | |
| let taskCube = this.taskCubeByTracingId[tracingId]; | |
| // Remove the old box if it exists | |
| if (taskCube != null) { | |
| taskCube.getMeshes().forEach((mesh) => this.rootNode.remove(mesh)); | |
| taskCube.destroy(); | |
| } | |
| this.taskCubeByTracingId[tracingId] = null; | |
| if (boundingBox == null || state.task == null) { | |
| continue; | |
| } | |
| const { viewMode } = Store.getState().temporaryConfiguration; | |
| taskCube = new Cube({ | |
| min: boundingBox.min, | |
| max: boundingBox.max, | |
| color: 0x00ff00, | |
| showCrossSections: true, | |
| isHighlighted: false, | |
| }); | |
| taskCube.getMeshes().forEach((mesh) => this.rootNode.add(mesh)); | |
| if (constants.MODES_ARBITRARY.includes(viewMode)) { | |
| taskCube?.setVisibility(false); | |
| } | |
| this.taskCubeByTracingId[tracingId] = taskCube; | |
| } | |
| // Remove cubes that are no longer present | |
| for (const staleId of previousIds) { | |
| const staleCube = this.taskCubeByTracingId[staleId]; | |
| if (staleCube) { | |
| staleCube.getMeshes().forEach((m) => this.rootNode.remove(m)); | |
| staleCube.destroy(); | |
| } | |
| delete this.taskCubeByTracingId[staleId]; | |
| } | |
| } |
Only show task bounding boxes (in green) if a task exists. Also, show one for each layer (relevant for hybrid tasks).
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)