Faster Mesh Rendering and Raycasting in 3D viewport #8106
Conversation
…peed up raycasting.
…h to optimize performance
…nto faster-caster
This reverts commit 36f5c0f.
…nto faster-caster
fm3
changed the title
|
Thanks for updating the title and PR description @fm3 :) I'll continue with restoring the 3D viewport proofreading using vertex colors once I'm back in office. I'll also have a look at the washed out colors then. |
![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 (1)
frontend/javascripts/oxalis/view/plane_view.ts (1)
138-196: Complex conditional logic in early-return checks.The early-return logic now has different behavior based on whether the active tool is "PROOFREAD". While this is commented, the complexity makes the code harder to follow.
Consider extracting these conditions into appropriately named helper methods to improve readability:
- if (storeState.uiInformation.activeTool === "PROOFREAD") { - if (hitObject == null && oldRaycasterHit == null) { - return null; - } - if (unmappedSegmentId != null && unmappedSegmentId === oldRaycasterHit?.unmappedSegmentId) { - return oldRaycasterHit; - } - } else { - // In proofreading, there is no highlighting of parts of the meshes. - // If the parent group is identical, we can reuse the old hit object. - if (hitObject?.parent === oldRaycasterHit?.node.parent) { - return oldRaycasterHit; - } - } + if (this.shouldSkipRaycastProcessing(storeState, hitObject, unmappedSegmentId, oldRaycasterHit)) { + return oldRaycasterHit; + } // Add this method to the class: + private shouldSkipRaycastProcessing( + storeState: any, + hitObject: MeshSceneNode | null, + unmappedSegmentId: number | null, + oldRaycasterHit: RaycasterHit + ): boolean { + const isProofreadingActive = storeState.uiInformation.activeTool === "PROOFREAD"; + + if (isProofreadingActive) { + // In proofreading mode, skip if no hit or if hitting the same unmapped segment + if (hitObject == null && oldRaycasterHit == null) { + return true; + } + return unmappedSegmentId != null && unmappedSegmentId === oldRaycasterHit?.unmappedSegmentId; + } else { + // In normal mode, skip if hitting the same parent group + return hitObject?.parent === oldRaycasterHit?.node.parent; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/oxalis/api/wk_dev.ts(4 hunks)frontend/javascripts/oxalis/view/plane_view.ts(7 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/oxalis/view/plane_view.ts (3)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
MeshSceneNode(37-42)SceneGroupForMeshes(43-43)frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
PositionToSegmentId(13-56)frontend/javascripts/oxalis/model/actions/settings_actions.ts (1)
updateTemporarySettingAction(75-83)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (13)
frontend/javascripts/oxalis/api/wk_dev.ts (5)
1-1: Imports are properly updated to support new benchmark functionality.The new imports for
appand additional types fromoxalis/constantssupport the enhanced benchmarking capabilities being added.Also applies to: 6-6
46-46: Good restructuring of benchmark history storage.Changing from a flat array to a structured object with separate arrays for different benchmark types improves organization and makes the code more maintainable.
144-146: Utility method for resetting benchmarks is a good addition.This helper method provides a clean way to reset the benchmark history data structure to its initial state.
194-200: Updated benchmarkMove implementation to match new data structure.The code now properly stores move benchmark results in the structured history object and logs appropriate statistics.
203-242: Comprehensive implementation of rotation benchmarking.The new
benchmarkRotatemethod effectively measures the performance of rotating through different views. The dynamic import to avoid circular dependencies is a good practice.One suggestion: Consider adding time measurement for individual rotation steps to provide more granular performance insights.
Have you tested this benchmark with larger datasets to ensure it correctly captures performance differences when working with more complex meshes?
frontend/javascripts/oxalis/view/plane_view.ts (8)
21-21: Good use of named constant for light intensity.Extracting the magic number into a named constant improves code readability and maintainability.
23-28: Well-structured type definition for raycaster hits.The new
RaycasterHittype provides a clear structure for storing detailed information about mesh intersections, including segment ID, index range, and hit point, which supports the enhanced raycasting functionality.
30-44: Improved light setup with camera as parent.Changing the
createDirLightfunction to use the camera as the parent for the light is a good architectural change. This ensures the light moves with the camera, providing consistent lighting as the view changes.
47-48: Optimized raycaster configuration.Setting
firstHitOnly = trueis a performance optimization that prevents unnecessary intersection calculations. Reducing the throttling delay from 150ms to 50ms makes the hover effect more responsive.
173-179: Improved segment identification from mesh geometry.Extracting the unmapped segment ID and index range from the geometry's position data is a good approach that supports the more detailed raycasting information needed for the BVH optimization.
198-214: Enhanced mesh hit highlighting with index range support.The updated code now clears the previous highlight and supports highlighting specific index ranges within a mesh, which is important for providing visual feedback, especially in proofreading mode.
228-235: Good extraction of cleanup logic to a separate method.Moving the cleanup logic for the previous hit into a dedicated method improves code organization and reduces duplication. This is a good refactoring.
281-286: Consider removing benchmark-specific code after development.This event listener for benchmarking is marked as only used for benchmarks. Based on previous review comments, it appears this may be intended to be removed before the final PR merge.
Will this benchmarking code be removed before the final PR is merged, as suggested in previous review comments?
…ical and felt weird to me)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
496-505:⚠️ Potential issueImprove boundary handling in positionToSegmentId operations
When accessing
positionToSegmentIdmethods, there's a risk of out-of-bounds operations as mentioned in past reviews. Ensure proper boundary checks are implemented for all methods.This applies to the following methods in the
PositionToSegmentIdclass:
getUnmappedSegmentIdForPositiongetRangeForPositiongetRangeForUnmappedSegmentIdAdd proper boundary checks to prevent potential runtime errors when positions are out of bounds.
98-113:⚠️ Potential issueAdd error handling to the async BVH computation
The
computeBvhAsynccall could potentially reject, but there's no error handling for this case.Add try-catch block to handle potential errors:
async addMeshFromVerticesAsync( vertices: Float32Array, segmentId: number, layerName: string, additionalCoordinates?: AdditionalCoordinate[] | undefined | null, ): Promise<void> { // Currently, this function is only used by ad hoc meshing. if (vertices.length === 0) return; let bufferGeometry = new THREE.BufferGeometry(); bufferGeometry.setAttribute("position", new THREE.BufferAttribute(vertices, 3)); bufferGeometry = mergeVertices(bufferGeometry); bufferGeometry.computeVertexNormals(); - bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + try { + bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + } catch (error) { + console.error("Failed to compute BVH for mesh:", error); + // Continue without BVH - raycast will be slower but still functional + }
🧹 Nitpick comments (6)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (6)
23-25: Consider the impact of modifying the THREE.Mesh prototypeModifying the prototype of THREE.Mesh with
acceleratedRaycastaffects all meshes in the application. While this improves performance, it's a global change that could potentially conflict with other code that might also modify this prototype.Consider documenting this change clearly in relevant documentation so that future developers are aware of this modification.
143-147: Optimize color buffer initializationThe loop-based initialization of the color buffer could be inefficient for large meshes with many vertices.
Consider using a more efficient approach:
- const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); - for (let i = 0; i < geometry.attributes.position.count; i++) { - colorBuffer.set(colorArray, i * 3); - } - geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3)); + // Create a buffer filled with repeating color values + const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); + if (geometry.attributes.position.count > 0) { + // Fill the first color + colorBuffer.set(colorArray, 0); + + // Double the data until we have enough + let filled = 1; + while (filled < geometry.attributes.position.count) { + const copyLength = Math.min(filled, geometry.attributes.position.count - filled); + colorBuffer.copyWithin(filled * 3, 0, copyLength * 3); + filled += copyLength; + } + } + geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3));This approach uses the more efficient
copyWithinmethod, which is much faster for large arrays.
390-394: Clear explanation needed for disabling range highlighting outside proofreading modeThe logic here forces
highlightStateto "full" when not in proofreading mode. This behavior should be documented to explain why partial highlighting is only allowed in proofreading mode.if (highlightState != null && !isProofreadingMode) { - // If the proofreading mode is not active and highlightState is not null, - // we overwrite potential requests to highlight only a range. + // In non-proofreading mode, we always highlight the entire mesh instead of + // a specific range because partial highlights are only meaningful during proofreading. + // This ensures consistent visual appearance in normal viewing mode. highlightState = "full"; }
430-435: Add check before iterating over rangesToResetFor clarity and to avoid unnecessary operations, add a check to see if there are any ranges to reset.
- // Reset ranges - if (mesh.material.originalColor != null) { - for (const rangeToReset of rangesToReset) { - setRangeToColor(mesh.geometry, rangeToReset, mesh.material.originalColor); - } - } + // Reset ranges if necessary + if (mesh.material.originalColor != null && rangesToReset.length > 0) { + for (const rangeToReset of rangesToReset) { + setRangeToColor(mesh.geometry, rangeToReset, mesh.material.originalColor); + } + }
518-521: Consider adding cleanup for throttled functionThe throttled function is created but there's no cleanup mechanism when the controller is destroyed, which could lead to memory leaks.
Consider implementing a destroy or cleanup method for the controller that cancels the throttled function:
destroy() { // Cancel the throttled function to prevent memory leaks this.throttledHighlightActiveUnmappedSegmentId.cancel(); // Other cleanup code... }And make sure to call this method when the controller is no longer needed.
450-468: Simplify uniform color handling logicThe current approach has several nested conditions and a traverse operation that could be optimized.
Consider restructuring this code to make it more readable and potentially more efficient:
- const isUniformColor = (mesh.activeState || mesh.hoveredState) === "full" || !mesh.isMerged; - - if (isUniformColor) { - let newColor = mesh.hoveredState - ? HOVERED_COLOR - : new THREE.Color(...mesh.material.originalColor); - - // Update the material for all meshes that belong to the current - // segment ID. Only for adhoc meshes, these will contain multiple - // children. For precomputed meshes, this will only affect one - // mesh in the scene graph. - parent.traverse((child) => { - if (child instanceof THREE.Mesh) { - setMaterialToUniformColor(child.material, newColor); - } - }); - - return; - } + // Determine if we need a uniform color (full highlight or non-merged mesh) + const needsUniformColor = (mesh.activeState || mesh.hoveredState) === "full" || !mesh.isMerged; + + if (needsUniformColor) { + // Select the appropriate color based on state + const newColor = mesh.hoveredState + ? HOVERED_COLOR + : new THREE.Color(...mesh.material.originalColor); + + // For non-merged meshes or full highlights, just update this mesh + if (!mesh.isMerged) { + setMaterialToUniformColor(mesh.material, newColor); + return; + } + + // For merged meshes with full highlight, update all related meshes + parent.traverse((child) => { + if (child instanceof THREE.Mesh) { + setMaterialToUniformColor(child.material, newColor); + } + }); + + return; + }This approach is more explicit about the different cases and may avoid unnecessary traversals for non-merged meshes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(11 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (5)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
BufferGeometryWithInfo(4-6)frontend/javascripts/types/api_flow_types.ts (1)
AdditionalCoordinate(30-30)frontend/javascripts/libs/compute_bvh_async.ts (1)
computeBvhAsync(34-39)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
NO_LOD_MESH_INDEX(93-93)frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts (1)
getActiveSegmentationTracing(330-330)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (3)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (3)
295-336: Well-structured lighting implementationThe lighting setup is well-organized with clearly defined intensity settings and a systematic approach to creating and positioning directional lights. The use of an array of light positions paired with a loop is an elegant solution.
27-33: Good use of constants for color managementThe addition of color constants and utility functions improves code consistency and maintainability. Converting colors to sRGB linear space ensures correct color rendering in the THREE.js environment.
45-56: Efficient utility for range-based color settingThe
setRangeToColorfunction provides a clean abstraction for setting colors on specific ranges of a geometry, which is essential for partial highlighting of meshes.
|
@MichaelBuessemeyer Thank you for your valuable feedback! And sorry about the whole todop comments. I must have mistakenly thought that I already self-reviewed the PR when I in fact did not. Please have another look. I incorporated/commented on your feedback :) |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/javascripts/oxalis/view/plane_view.ts (1)
140-184: 💡 Verification agent🧩 Analysis chain
Add boundary checking for unmapped segment ID determination.
If the face position is out of bounds, the call to
getUnmappedSegmentIdForPositioncould result in errors.Add proper error handling:
if (hitObject && face) { if ("positionToSegmentId" in hitObject.geometry) { const positionToSegmentId = hitObject.geometry.positionToSegmentId as PositionToSegmentId; - unmappedSegmentId = positionToSegmentId.getUnmappedSegmentIdForPosition(face.a); - indexRange = positionToSegmentId.getRangeForUnmappedSegmentId(unmappedSegmentId); + try { + unmappedSegmentId = positionToSegmentId.getUnmappedSegmentIdForPosition(face.a); + if (unmappedSegmentId !== null) { + indexRange = positionToSegmentId.getRangeForUnmappedSegmentId(unmappedSegmentId); + } + } catch (error) { + console.error("Error determining unmapped segment ID:", error); + } } }This prevents potential runtime errors if the face position is out of bounds.
🏁 Script executed:
#!/bin/bash # Check how face.a is being generated and if there are possible out-of-bounds scenarios rg -A 3 "intersections\[0\]\.face" --glob "*.ts" # Also check the implementation of getUnmappedSegmentIdForPosition rg -A 10 "getUnmappedSegmentIdForPosition" --glob "*.ts"Length of output: 2638
Action Required: Enhance Error Handling in Mesh Hit Test for Out-of-Bound Face Position
The current implementation in
frontend/javascripts/oxalis/view/plane_view.tscallsgetUnmappedSegmentIdForPosition(face.a)without checking ifface.ais in a valid range. As verified, the helper method inmesh_helpers.tsthrows an error when the position is out of bounds. To prevent potential runtime exceptions, please wrap the call in a try-catch block and only proceed to callgetRangeForUnmappedSegmentIdif a valid unmapped segment ID is returned.Changes to apply:
- Wrap the call to
getUnmappedSegmentIdForPositionin a try-catch.- Check that the returned
unmappedSegmentIdis non-null before invokinggetRangeForUnmappedSegmentId.- Log an error if an exception occurs.
Proposed diff snippet:
if (hitObject && face) { - if ("positionToSegmentId" in hitObject.geometry) { - const positionToSegmentId = hitObject.geometry.positionToSegmentId as PositionToSegmentId; - unmappedSegmentId = positionToSegmentId.getUnmappedSegmentIdForPosition(face.a); - indexRange = positionToSegmentId.getRangeForUnmappedSegmentId(unmappedSegmentId); - } + if ("positionToSegmentId" in hitObject.geometry) { + const positionToSegmentId = hitObject.geometry.positionToSegmentId as PositionToSegmentId; + try { + unmappedSegmentId = positionToSegmentId.getUnmappedSegmentIdForPosition(face.a); + if (unmappedSegmentId !== null) { + indexRange = positionToSegmentId.getRangeForUnmappedSegmentId(unmappedSegmentId); + } + } catch (error) { + console.error("Error determining unmapped segment ID:", error); + } + }
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
98-113:⚠️ Potential issueAdd error handling for the BVH computation.
The method
computeBvhAsyncreturns a Promise that could potentially reject, but there's no error handling in place. This could lead to unhandled promise rejections if the BVH computation fails.Implement proper error handling:
async addMeshFromVerticesAsync( vertices: Float32Array, segmentId: number, layerName: string, additionalCoordinates?: AdditionalCoordinate[] | undefined | null, ): Promise<void> { // Currently, this function is only used by ad hoc meshing. if (vertices.length === 0) return; let bufferGeometry = new THREE.BufferGeometry(); bufferGeometry.setAttribute("position", new THREE.BufferAttribute(vertices, 3)); bufferGeometry = mergeVertices(bufferGeometry); bufferGeometry.computeVertexNormals(); - bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + try { + bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + } catch (error) { + console.error("Failed to compute BVH for mesh:", error); + // Continue without BVH - raycast will be slower but still functional + }
🧹 Nitpick comments (7)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (3)
23-25: Add JSDoc comments for the acceleratedRaycast override.The code is overriding the default raycast method on THREE.Mesh.prototype, which is a significant modification to the Three.js library's behavior. Add a comment explaining why this override is necessary and what performance benefits it provides.
// Add the raycast function. Assumes the BVH is available on // the `boundsTree` variable +/** + * Override the default Three.js raycast method with an accelerated version. + * This provides significant performance improvements for raycasting operations + * by utilizing the Bounding Volume Hierarchy (BVH) structure. + * @see https://github.com/gkjohnson/three-mesh-bvh + */ THREE.Mesh.prototype.raycast = acceleratedRaycast;
467-468: Enhance clarity with a descriptive comment for isUniformColor.The
isUniformColorvariable's purpose and logic could be better explained.-const isUniformColor = (mesh.activeState || mesh.hoveredState) === "full" || !mesh.isMerged; +// Determine if we should use a uniform color for the entire mesh. This is the case when: +// 1. The mesh is fully active or hovered (state === "full") +// 2. The mesh is not merged (each mesh represents a single segment) +const isUniformColor = (mesh.activeState || mesh.hoveredState) === "full" || !mesh.isMerged;
535-538: Consider a more semantic throttle time constant.The throttle value (150ms) is hardcoded directly in the method call. For better maintenance, consider defining it as a named constant at the top of the file.
+const HIGHLIGHT_THROTTLE_DELAY = 150; // ms + throttledHighlightActiveUnmappedSegmentId = _.throttle( this.highlightActiveUnmappedSegmentId, - 150, + HIGHLIGHT_THROTTLE_DELAY, );frontend/javascripts/oxalis/view/plane_view.ts (4)
25-30: Add JSDoc comment for RaycasterHit type.The newly added
RaycasterHittype would benefit from documentation explaining its purpose and fields.+/** + * Represents the result of a raycasting operation against meshes. + * Contains information about the hit node, the range of indices affected, + * the unmapped segment ID, and the position where the ray hit the mesh. + * Can be null if no hit was detected. + */ type RaycasterHit = { node: MeshSceneNode; indexRange: Vector2 | null; unmappedSegmentId: number | null; point: Vector3; } | null;
49-50: Move MESH_HOVER_THROTTLING_DELAY higher in the file.Constants should be grouped together at the top of the file for better readability and maintenance.
+const MESH_HOVER_THROTTLING_DELAY = 50; const raycaster = new VisibilityAwareRaycaster(); raycaster.firstHitOnly = true; -const MESH_HOVER_THROTTLING_DELAY = 50;
185-198: Improve early return conditions with clearer comments.The early return conditions based on the active tool and hit status are complex and not well-documented. Add comments to explain the logic.
if (storeState.uiInformation.activeTool === "PROOFREAD") { + // In proofreading mode: + // 1. If nothing was hit now and nothing was hit before, return early + // 2. If the same unmapped segment ID is hit again, reuse the existing hit if (hitObject == null && oldRaycasterHit == null) { return null; } if (unmappedSegmentId != null && unmappedSegmentId === oldRaycasterHit?.unmappedSegmentId) { return oldRaycasterHit; } } else { - // In proofreading, there is no highlighting of parts of the meshes. - // If the parent group is identical, we can reuse the old hit object. + // In non-proofreading mode: + // If the same parent mesh group is hit, we can reuse the old hit object + // as we don't need to track individual segments within a mesh if (hitObject?.parent === oldRaycasterHit?.node.parent) { return oldRaycasterHit; } }
286-291: Consider removing or documenting WkDev benchmarking code.The code includes a comment about "Only used for benchmarks (see WkDev)" which suggests this might be development/debugging code that should either be removed or properly documented.
this.unsubscribeFunctions.push( - // Only used for benchmarks (see WkDev) + /** + * Force an immediate rerender. + * Note: This is only used for benchmarking purposes and can be + * removed once performance testing is complete. + * @see WkDev benchmark scripts + */ app.vent.on("forceImmediateRerender", () => { this.renderFunction(true); }), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts(0 hunks)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(11 hunks)frontend/javascripts/oxalis/model/actions/annotation_actions.ts(0 hunks)frontend/javascripts/oxalis/model/reducers/annotation_reducer.ts(0 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(7 hunks)frontend/javascripts/oxalis/store.ts(0 hunks)frontend/javascripts/oxalis/view/plane_view.ts(7 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(3 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(9 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(1 hunks)
💤 Files with no reviewable changes (4)
- frontend/javascripts/oxalis/model/reducers/annotation_reducer.ts
- frontend/javascripts/oxalis/store.ts
- frontend/javascripts/oxalis/controller/scene_controller.ts
- frontend/javascripts/oxalis/model/actions/annotation_actions.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.unreleased.md
- frontend/javascripts/test/helpers/apiHelpers.ts
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
🧰 Additional context used
🧬 Code Definitions (2)
frontend/javascripts/oxalis/view/plane_view.ts (5)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
MeshSceneNode(37-42)SceneGroupForMeshes(43-43)frontend/javascripts/oxalis/constants.ts (2)
Vector2(12-12)Vector3(13-13)frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
PositionToSegmentId(13-62)frontend/javascripts/oxalis/model/actions/settings_actions.ts (1)
updateTemporarySettingAction(75-83)frontend/javascripts/oxalis/controller/scene_controller_provider.ts (1)
getSceneController(4-10)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (5)
frontend/javascripts/oxalis/constants.ts (2)
Vector3(13-13)Vector2(12-12)frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
BufferGeometryWithInfo(4-6)frontend/javascripts/types/api_flow_types.ts (1)
AdditionalCoordinate(30-30)frontend/javascripts/libs/compute_bvh_async.ts (1)
computeBvhAsync(34-39)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
NO_LOD_MESH_INDEX(94-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
| const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); | ||
| for (let i = 0; i < geometry.attributes.position.count; i++) { | ||
| colorBuffer.set(colorArray, i * 3); | ||
| } | ||
| geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize vertex color buffer initialization.
The current implementation uses a loop to initialize the color buffer. For large meshes, this could be performance-intensive.
Consider using typed array bulk operations instead:
// Theoretically, this is not necessary for meshes that don't need non-uniform
// colors, but measurements showed that this only takes up ~0.03 ms per mesh
// (initialization, at least). We can optimize this later if necessary.
const colorBuffer = new Float32Array(geometry.attributes.position.count * 3);
-for (let i = 0; i < geometry.attributes.position.count; i++) {
- colorBuffer.set(colorArray, i * 3);
-}
+// Fill the entire array with the same color values
+for (let i = 0; i < colorArray.length; i++) {
+ const value = colorArray[i];
+ for (let j = i; j < colorBuffer.length; j += colorArray.length) {
+ colorBuffer[j] = value;
+ }
+}
geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3));Alternatively, look into using WebGL-specific techniques like uniform buffers if the color is uniform across the mesh.
📝 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.
| const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); | |
| for (let i = 0; i < geometry.attributes.position.count; i++) { | |
| colorBuffer.set(colorArray, i * 3); | |
| } | |
| geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3)); | |
| const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); | |
| // Fill the entire array with the same color values | |
| for (let i = 0; i < colorArray.length; i++) { | |
| const value = colorArray[i]; | |
| for (let j = i; j < colorBuffer.length; j += colorArray.length) { | |
| colorBuffer[j] = value; | |
| } | |
| } | |
| geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3)); |
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks a lot for taking care of all my comments 🙏
But I am terribly sorry: I found another bug that looks pretty tedious to debug. Here is the video of me reproducing this:
opacity-bug-2025-04-08_15.07.44.mp4
I gave this a little more time to get more details on how to reproduce:
- I use the l4_sample v2 dataset with mapping 80 turned on
- Load one precomputed mesh next to the other mesh you want to load
- Load the other mesh ad hoc and set its opacity < 1.
- Then you can observe the strange rendering behaviour.
The same should also be reproducable when loading (1st ad-hoc & 2nd precomputed). But if you use only ad-hoc or only precomputed, this bug does not occur
| for (let i = 0; i < geometry.attributes.position.count; i++) { | ||
| colorBuffer.set(colorArray, i * 3); | ||
| } | ||
| geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3)); |
There was a problem hiding this comment.
oh right, sorry 🙈
| meshGroup.children.forEach((child: MeshSceneNode) => { | ||
| child.material.originalColor = colorArray; | ||
| if (child.material.vertexColors) { | ||
| setRangeToColor(child.geometry, null, colorArray); |
There was a problem hiding this comment.
Yeah, making all meshes use vertex color might be nicer as this way all meshes would behave the same way 👍
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts
Outdated
Show resolved
Hide resolved
| // This is a workaround for when groupsSegmentsVisibilityStateMap does not contain | ||
| // an entry for groupId. This can currently happen when moving segments into a new group | ||
| // and rightclicking that group. | ||
| areSomeSegmentsInvisible: false, | ||
| areSomeSegmentsVisible: false, | ||
| }); |
There was a problem hiding this comment.
Damn nice :D This commit is on 🔥
Ok turns out, this is already buggy on the current master :/ So I'd say this isn't a must fix before merging this PR 👍 But I did some more testing an ended up with a mesh in a rendered state that does not support highlighting on hover 🤔 hovering-bug-2025-04-08_15.57.11.mp4I found 2 strange behaviour out of which the 1st is known to me: When the user loads a ad-hoc mesh & precomputed for the same user, the hovering is off, as both meshes exist in the scene. The second one is that the mesh is rendered in the scene but now hoverable (see at the end of the video) But here as well: I am not sure whether this PR is responsible for this behaviour. IMO this shouldn't block this PR as the reproduction steps are a bit of try an error loading the same mesh as ad-hoc / precomputed over and over again and deleting it. So this strange behaviour shouldn't really impact our users. What do you think @philippotto I would vote for creating an issue for this |
… are removed before they are re-computed; await adding of mesh to scene to avoid that ad-hoc item is immediately removed from list
I had another look and fixed it (and some other parts, too) hopefully :) thanks for testing thoroughly! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (3)
140-147: Optimize vertex color buffer initialization.The current implementation uses a loop to initialize the color buffer with the same values. For large meshes, this can be performance-intensive. Consider using more efficient methods for initializing the buffer.
// Theoretically, this is not necessary for meshes that don't need non-uniform // colors, but measurements showed that this only takes up ~0.03 ms per mesh // (initialization, at least). We can optimize this later if necessary. const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); -for (let i = 0; i < geometry.attributes.position.count; i++) { - colorBuffer.set(colorArray, i * 3); -} +// Fill the entire array with the same color values +for (let i = 0; i < colorArray.length; i++) { + const value = colorArray[i]; + for (let j = i; j < colorBuffer.length; j += colorArray.length) { + colorBuffer[j] = value; + } +}
513-522:⚠️ Potential issueAdd proper error handling for position-to-segment mapping.
The code calls
positionToSegmentId.getRangeForUnmappedSegmentId(activeUnmappedSegmentId)without any error handling. If the segment ID is not found, this could throw an error. Consider adding a try-catch block to handle potential errors.const positionToSegmentId = obj.geometry.positionToSegmentId; let indexRange = null; let containsSegmentId = false; if (positionToSegmentId && activeUnmappedSegmentId) { containsSegmentId = positionToSegmentId.containsSegmentId(activeUnmappedSegmentId); if (containsSegmentId) { - indexRange = positionToSegmentId.getRangeForUnmappedSegmentId(activeUnmappedSegmentId); + try { + indexRange = positionToSegmentId.getRangeForUnmappedSegmentId(activeUnmappedSegmentId); + } catch (error) { + console.error("Error getting range for unmapped segment ID:", error); + containsSegmentId = false; + } } }
98-113:⚠️ Potential issueAdd error handling for async BVH computation.
The async BVH computation could potentially fail, but there's no error handling for this case. Consider adding a try-catch block to handle potential errors.
async addMeshFromVerticesAsync( vertices: Float32Array, segmentId: number, layerName: string, additionalCoordinates?: AdditionalCoordinate[] | undefined | null, ): Promise<void> { // Currently, this function is only used by ad hoc meshing. if (vertices.length === 0) return; let bufferGeometry = new THREE.BufferGeometry(); bufferGeometry.setAttribute("position", new THREE.BufferAttribute(vertices, 3)); bufferGeometry = mergeVertices(bufferGeometry); bufferGeometry.computeVertexNormals(); - bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + try { + bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + } catch (error) { + console.error("Failed to compute BVH for mesh:", error); + // Continue without BVH - raycast will be slower but still functional + }
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (1)
467-485: Consider early return for performance optimization.The code checks if a uniform color should be used and then returns early, which is good for performance. However, the condition check could be moved up in the method to avoid unnecessary computations.
updateMeshAppearance( mesh: MeshSceneNode, isHovered: boolean | undefined, isActiveUnmappedSegment?: boolean | undefined, highlightState?: HighlightState, ) { // This method has three steps: // 1) Check whether (and which of) the provided parameters differ from the actual // appearance. // 2) Clear old partial ranges if necessary. // 3) Update the appearance. const isProofreadingMode = Store.getState().uiInformation.activeTool === "PROOFREAD"; if (highlightState != null && !isProofreadingMode) { // If the proofreading mode is not active and highlightState is not null, // we overwrite potential requests to highlight only a range. highlightState = "full"; } + const isUniformColor = (mesh.activeState || mesh.hoveredState) === "full" || !mesh.isMerged; + if (isUniformColor) { + let newColor = mesh.hoveredState + ? HOVERED_COLOR + : new THREE.Color(...mesh.material.originalColor); + + // Update the material for all meshes that belong to the current + // segment ID. Only for adhoc meshes, these will contain multiple + // children. For precomputed meshes, this will only affect one + // mesh in the scene graph. + parent.traverse((child) => { + if (child instanceof THREE.Mesh) { + setMaterialToUniformColor(child.material, newColor); + } + }); + + return; + } let wasChanged = false; const rangesToReset: Vector2[] = []; if (isHovered != null) { // ...rest of the method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(12 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(11 hunks)frontend/javascripts/oxalis/model/sagas/proofread_saga.ts(2 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx(4 hunks)frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/oxalis/model/sagas/proofread_saga.ts
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segment_list_item.tsx
- frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (4)
frontend/javascripts/oxalis/constants.ts (2)
Vector3(13-13)Vector2(12-12)frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
BufferGeometryWithInfo(4-6)frontend/javascripts/libs/compute_bvh_async.ts (1)
computeBvhAsync(34-39)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
NO_LOD_MESH_INDEX(94-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
312-353: Excellent improvement to lighting setup.The lighting setup has been significantly improved with a more structured approach using multiple directional lights from different angles. This will provide better visual quality and more realistic rendering of the 3D models.
394-502: Excellent color handling implementation.The updateMeshAppearance method has been significantly improved to handle both uniform colors and vertex-specific colors. The code now correctly manages highlighting and updating the mesh appearance based on hover and active states.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (2)
56-62: 🛠️ Refactor suggestionAdd check for negative index in getRangeForPosition.
Similar to the previous method, this one should also handle the case where the position is less than the first element in
cumulativeStartPosition.getRangeForPosition(position: number): [number, number] { const index = _.sortedIndex(this.cumulativeStartPosition, position) - 1; + if (index < 0 || index + 1 >= this.cumulativeStartPosition.length) { - if (index + 1 >= this.cumulativeStartPosition.length) { throw new Error(`Could not look up range for position=${position} in PositionToSegmentId.`); } return [this.cumulativeStartPosition[index], this.cumulativeStartPosition[index + 1]]; }
48-54: 🛠️ Refactor suggestionAdd check for negative index in getUnmappedSegmentIdForPosition.
The method should handle the case where the position is less than the first element in
cumulativeStartPosition, which would result in a negative index.getUnmappedSegmentIdForPosition(position: number) { const index = _.sortedIndex(this.cumulativeStartPosition, position) - 1; + if (index < 0 || index >= this.unmappedSegmentIds.length) { - if (index >= this.unmappedSegmentIds.length) { throw new Error(`Could not look up id for position=${position} in PositionToSegmentId.`); } return this.unmappedSegmentIds[index]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/controller/mesh_helpers.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (4)
4-11: Good type definitions with clear structure.The type definitions are well-structured and clearly express the relationship between THREE.BufferGeometry and the additional properties. The optional
positionToSegmentIdin both types and mandatoryunmappedSegmentIdin the unmerged version are properly defined.
14-26: Great documentation with clear explanations.The class documentation thoroughly explains the purpose and functionality of the PositionToSegmentId class. It clearly describes how the class handles buffer geometries, the concatenation process, and the types of queries it supports. This level of detail helps other developers understand the class's role in the 3D mesh rendering system.
64-70: Solid implementation of getRangeForUnmappedSegmentId.This method correctly uses
_.sortedIndexOffor efficient lookup in a sorted array and properly handles the case where the segment ID is not found by returning null. The implementation correctly accesses the range fromcumulativeStartPositionwithout needing additional bounds checks due to the sentinel value added in the constructor.
72-74: Clean implementation of containsSegmentId.This method provides a concise way to check for the existence of a segment ID, using the efficient
_.sortedIndexOffunction which is appropriate for the sorted array structure maintained by this class.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
98-113: 🛠️ Refactor suggestionAdd error handling to the asynchronous BVH computation.
The current implementation awaits
computeBvhAsyncwithout any error handling. If the computation fails, the promise will reject and could lead to an unhandled promise rejection.async addMeshFromVerticesAsync( vertices: Float32Array, segmentId: number, layerName: string, additionalCoordinates?: AdditionalCoordinate[] | undefined | null, ): Promise<void> { // Currently, this function is only used by ad hoc meshing. if (vertices.length === 0) return; let bufferGeometry = new THREE.BufferGeometry(); bufferGeometry.setAttribute("position", new THREE.BufferAttribute(vertices, 3)); bufferGeometry = mergeVertices(bufferGeometry); bufferGeometry.computeVertexNormals(); - bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + try { + bufferGeometry.boundsTree = await computeBvhAsync(bufferGeometry); + } catch (error) { + console.error("Failed to compute BVH for mesh:", error); + // Continue without BVH - raycast will be slower but still functional + }
513-522: 🛠️ Refactor suggestionAdd error handling for position-to-segment mapping.
The code retrieves a range for an unmapped segment ID without checking if the position-to-segment mapping might throw errors. While error handling occurs at lower levels, adding a try-catch block here would make the code more robust.
const vertexSegmentMapping = obj.geometry.vertexSegmentMapping; let indexRange = null; let containsSegmentId = false; if (vertexSegmentMapping && activeUnmappedSegmentId) { containsSegmentId = vertexSegmentMapping.containsSegmentId(activeUnmappedSegmentId); if (containsSegmentId) { - indexRange = vertexSegmentMapping.getRangeForUnmappedSegmentId(activeUnmappedSegmentId); + try { + indexRange = vertexSegmentMapping.getRangeForUnmappedSegmentId(activeUnmappedSegmentId); + } catch (error) { + console.error("Error getting range for unmapped segment ID:", error); + containsSegmentId = false; + } } }frontend/javascripts/oxalis/controller/mesh_helpers.ts (3)
32-51: 🛠️ Refactor suggestionHandle empty input arrays in the constructor.
The constructor doesn't explicitly handle the case where
sortedBufferGeometriesis empty. This could lead to unexpected behavior when trying to look up positions later.constructor(sortedBufferGeometries: UnmergedBufferGeometryWithInfo[]) { let cumsum = 0; this.cumulativeStartPosition = []; this.unmappedSegmentIds = []; + // Handle empty input array case + if (sortedBufferGeometries.length === 0) { + this.cumulativeStartPosition.push(0); + return; + } for (const bufferGeometry of sortedBufferGeometries) { const isNewSegmentId = this.unmappedSegmentIds.length === 0 || bufferGeometry.unmappedSegmentId !== this.unmappedSegmentIds.at(-1); if (isNewSegmentId) { this.unmappedSegmentIds.push(bufferGeometry.unmappedSegmentId); this.cumulativeStartPosition.push(cumsum); } cumsum += bufferGeometry.attributes.position.count; } // Add sentinel value at the end - this implements an offset table pattern // where the last entry indicates the total size of all vertices this.cumulativeStartPosition.push(cumsum); }
61-67:⚠️ Potential issueAdd check for negative index in getRangeForPosition.
Similar to getUnmappedSegmentIdForPosition, this method should also check for negative indices.
getRangeForPosition(position: number): [number, number] { const index = _.sortedIndex(this.cumulativeStartPosition, position) - 1; - if (index + 1 >= this.cumulativeStartPosition.length) { + if (index < 0 || index + 1 >= this.cumulativeStartPosition.length) { throw new Error(`Could not look up range for position=${position} in VertexSegmentMapping.`); } return [this.cumulativeStartPosition[index], this.cumulativeStartPosition[index + 1]]; }
53-59:⚠️ Potential issueAdd check for negative index in getUnmappedSegmentIdForPosition.
The method checks if the index is too large but not if it's negative, which could happen if the position is smaller than the first entry in cumulativeStartPosition.
getUnmappedSegmentIdForPosition(position: number) { const index = _.sortedIndex(this.cumulativeStartPosition, position) - 1; - if (index >= this.unmappedSegmentIds.length) { + if (index < 0 || index >= this.unmappedSegmentIds.length) { throw new Error(`Could not look up id for position=${position} in VertexSegmentMapping.`); } return this.unmappedSegmentIds[index]; }
🧹 Nitpick comments (4)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
23-25: Add more detailed documentation for the global THREE.Mesh.prototype override.The comment provides basic information about adding the raycast function, but it should explain why this global modification is necessary and any potential implications for other parts of the application that use THREE.Mesh instances.
-// Add the raycast function. Assumes the BVH is available on -// the `boundsTree` variable +// Override THREE.Mesh.raycast with acceleratedRaycast for improved performance. +// This global change affects all THREE.Mesh instances, significantly speeding up +// raycasting operations when a boundsTree (BVH) is available. Any mesh that uses +// raycasting should have a boundsTree computed and attached to its geometry. +// This approach avoids having to modify each individual mesh instance. THREE.Mesh.prototype.raycast = acceleratedRaycast;
140-147: Optimize vertex color buffer initialization for large meshes.The current implementation uses a loop to set the same color value for each vertex, which could be inefficient for large meshes with many vertices.
// Theoretically, this is not necessary for meshes that don't need non-uniform // colors, but measurements showed that this only takes up ~0.03 ms per mesh // (initialization, at least). We can optimize this later if necessary. const colorBuffer = new Float32Array(geometry.attributes.position.count * 3); -for (let i = 0; i < geometry.attributes.position.count; i++) { - colorBuffer.set(colorArray, i * 3); +// Fill all elements with the same color values more efficiently +colorBuffer.fill(colorArray[0]); +for (let i = 1; i < colorArray.length; i++) { + for (let j = i; j < colorBuffer.length; j += colorArray.length) { + colorBuffer[j] = colorArray[i]; + } } geometry.setAttribute("color", new THREE.BufferAttribute(colorBuffer, 3));frontend/javascripts/oxalis/view/plane_view.ts (2)
49-50: Consider making the throttling delay configurable.The throttling delay has been reduced from 150ms to 50ms. While this improves responsiveness, it might affect performance on lower-end devices. Consider making this value configurable based on device capabilities or user preferences.
-const MESH_HOVER_THROTTLING_DELAY = 50; +// Reduced from 150ms to improve responsiveness. Lower values are more responsive +// but may impact performance on lower-end devices. +const MESH_HOVER_THROTTLING_DELAY = 50;
186-199: Improve code readability with clearer condition structure.The conditional logic for determining when to return early from hit testing is complex and could be clarified with more descriptive variable names or additional comments.
-if (storeState.uiInformation.activeTool === "PROOFREAD") { - if (hitObject == null && oldRaycasterHit == null) { - return null; - } - if (unmappedSegmentId != null && unmappedSegmentId === oldRaycasterHit?.unmappedSegmentId) { - return oldRaycasterHit; - } -} else { - // In proofreading, there is no highlighting of parts of the meshes. - // If the parent group is identical, we can reuse the old hit object. - if (hitObject?.parent === oldRaycasterHit?.node.parent) { - return oldRaycasterHit; - } -} +const isProofreadingMode = storeState.uiInformation.activeTool === "PROOFREAD"; +// Handle early returns based on whether we're in proofreading mode +if (isProofreadingMode) { + // In proofreading mode, we only care about segment IDs, not mesh objects + if (hitObject == null && oldRaycasterHit == null) { + // No hit now, no hit before - nothing to do + return null; + } + if (unmappedSegmentId != null && unmappedSegmentId === oldRaycasterHit?.unmappedSegmentId) { + // Same segment ID as before - reuse old hit + return oldRaycasterHit; + } +} else { + // In regular mode, highlighting is based on the mesh object itself + // If the parent group is identical, we can reuse the old hit object + if (hitObject?.parent === oldRaycasterHit?.node.parent) { + return oldRaycasterHit; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/oxalis/controller/mesh_helpers.ts(1 hunks)frontend/javascripts/oxalis/controller/segment_mesh_controller.ts(12 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(11 hunks)frontend/javascripts/oxalis/view/plane_view.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/javascripts/oxalis/view/plane_view.ts (4)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (2)
MeshSceneNode(37-42)SceneGroupForMeshes(43-43)frontend/javascripts/oxalis/constants.ts (2)
Vector2(12-12)Vector3(13-13)frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
VertexSegmentMapping(13-80)frontend/javascripts/oxalis/controller/scene_controller_provider.ts (1)
getSceneController(4-10)
frontend/javascripts/oxalis/controller/segment_mesh_controller.ts (4)
frontend/javascripts/oxalis/constants.ts (2)
Vector3(13-13)Vector2(12-12)frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
BufferGeometryWithInfo(4-6)frontend/javascripts/libs/compute_bvh_async.ts (1)
computeBvhAsync(34-39)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (1)
NO_LOD_MESH_INDEX(94-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (1)
13-29: Great documentation for the VertexSegmentMapping class.The class documentation clearly explains the purpose and behavior of the class, making it easier for other developers to understand and use it correctly.
There was a problem hiding this comment.
Thanks for implementing all the feedback 🙏
During testing I could no longer reproduced the second bug 🎉
But the first one still exists :/. But as this is already the case on the master and the opacity feature is new I'd say it is ok if we fix this in a follow up. This shouldn't block this PR imo
Co-authored-by: MichaelBuessemeyer <[email protected]>
|
thanks for having another look! will merge now :)
yes, I think, that's orthogonal to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (2)
53-59:⚠️ Potential issueAdd a boundary check for negative index.
If
_.sortedIndex(...)returns 0 (meaning position is smaller than the first element incumulativeStartPosition), subtracting 1 yields an index of -1, which isn't checked here. This can cause[this.unmappedSegmentIds[-1]]to returnundefinedinstead of properly throwing an error. Consider adding a guard forindex < 0.const index = _.sortedIndex(this.cumulativeStartPosition, position) - 1; -if (index >= this.unmappedSegmentIds.length) { +if (index < 0 || index >= this.unmappedSegmentIds.length) { throw new Error(`Could not look up id for position=${position} in VertexSegmentMapping.`); }
61-67: 🛠️ Refactor suggestionCheck for negative index to prevent potential out-of-bounds access.
Similar to the previous method, a negative index could occur if the queried position is smaller than the first entry in
cumulativeStartPosition. Prevent unexpected array access by checkingindex < 0.const index = _.sortedIndex(this.cumulativeStartPosition, position) - 1; -if (index + 1 >= this.cumulativeStartPosition.length) { +if (index < 0 || (index + 1) >= this.cumulativeStartPosition.length) { throw new Error(`Could not look up range for position=${position} in VertexSegmentMapping.`); }
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (2)
25-26: Fix minor spelling mistake in doc comment.There's a small spelling error: "Similarily" → "Similarly."
- * Similarily, one can obtain the range that covers all vertices + * Similarly, one can obtain the range that covers all vertices
32-51: Consider handling empty geometry arrays more explicitly.When
sortedBufferGeometriesis empty, we end up withunmappedSegmentIds = []andcumulativeStartPosition = [0]. If later queries pass a position ≥ 0, index calculations in the lookup methods might yield –1, leading to undefined behavior. You may want to handle this case explicitly or document the assumption that an empty geometry array never occurs.constructor(sortedBufferGeometries: UnmergedBufferGeometryWithInfo[]) { let cumsum = 0; this.cumulativeStartPosition = []; this.unmappedSegmentIds = []; + if (sortedBufferGeometries.length === 0) { + this.cumulativeStartPosition.push(0); + return; + } for (const bufferGeometry of sortedBufferGeometries) { ... } this.cumulativeStartPosition.push(cumsum); }
📜 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/mesh_helpers.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.unreleased.md
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/oxalis/controller/mesh_helpers.ts (2)
69-75: LGTM!The boundary checks for the segment ID case look solid. The sentinel value ensures no out-of-bounds access for
index + 1. Good job!
77-79: LGTM!This method correctly leverages Lodash to determine membership in
unmappedSegmentIds.
4 participants
Initially started in a Hackathon project in 2024.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)