-
Notifications
You must be signed in to change notification settings - Fork 30
Fix ad-hoc mesh closing edges on layer borders #9143
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
📝 WalkthroughWalkthroughReworks ad-hoc mesh padding and cube-size handling across frontend sagas and the admin REST API, removes a bounding-box clipping helper, tightens backend cuboid/bucket intersection bounds to non-negative values, and adds a changelog entry for an edge-case mesh fix. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
philippotto
left a comment
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 digging into this and fixing the problem!! I left a couple of comments 😇
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
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: 1
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
195-195: Consider removing debug console.log statement.The
console.login the error handler is not standard for production code. SinceErrorHandling.notifyalready captures the exception, the console logging is redundant. Consider removing it or using a proper logging framework if additional context is needed.Apply this diff:
- console.log("Exception when loading ad-hoc mesh for segment", action.segmentId, ":", exc);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2024-11-22T17:17:39.914Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts:125-125
Timestamp: 2024-11-22T17:17:39.914Z
Learning: In the `createNode` function within `skeletontracing_reducer_helpers.ts`, the property `resolution` is intentionally kept even when the parameter is renamed to `mag`.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2025-12-11T15:25:53.526Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/admin/statistic/time_tracking_overview.tsx:261-279
Timestamp: 2025-12-11T15:25:53.526Z
Learning: Ant Design v6 Select: when using the Select component, consider supplying a prefix prop to render an icon or element before the input for better visual context. Apply this guideline to TS and TSX files across the codebase where Ant Design Select is used; ensure prefix usage is accessible (e.g., provide meaningful aria-label if needed) and avoid unnecessary prefixes on simple inputs.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
🧬 Code graph analysis (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (2)
frontend/javascripts/libs/mjs.ts (1)
V3(372-372)frontend/javascripts/viewer/constants.ts (1)
Vector3(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (2)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (2)
474-487: Excellent documentation and implementation of the padding logic.The comment clearly explains both effects: (1) overlapping cubes to fill gaps and (2) extending beyond layer borders for closing surfaces. The implementation correctly:
- Shifts position by subtracting
magto enable negative coordinates at top-left borders- Delegates size computation to the new helper function
- Handles both segment-index and non-segment-index cases
565-586: Well-implemented helper function for computing padded cube size.The function correctly:
- Adds
[1,1,1]to fill grid gaps between chunks- Detects when a cube aligns precisely with the layer's bottom-right border
- Extends the cube by an additional voxel in those dimensions to provide closing surfaces
The logic is sound:
cubeBottomRightis computed in mag1 coordinates (by scalingcubeSizebymag), making the comparison withlayerBottomRightvalid. The local mutation ofcubeSizeat line 582 is safe since it operates on the array created at line 573.
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
Outdated
Show resolved
Hide resolved
philippotto
left a comment
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.
awesome! works well & looks good. thanks for fixing this bug!
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
🧹 Nitpick comments (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (1)
564-585: Helper function correctly implements bottom-right border detection and cube enlargement.The logic is sound:
- Starts with base cube size + [1,1,1] for gap filling (line 572)
- Converts cube size to mag1 coordinates for comparison with layer bounds (line 577)
- Detects when a cube precisely touches the bottom-right layer border (line 580)
- Increases the cube size in that dimension to extend beyond the border (line 581)
For style consistency, consider using V3 operations for the dimension-specific increment as well:
for (let dimension = 0; dimension < 3; dimension++) { if (cubeBottomRight[dimension] === layerBottomRight[dimension]) { - cubeSize[dimension] += 1; + cubeSize = V3.add(cubeSize, dimension === 0 ? [1, 0, 0] : dimension === 1 ? [0, 1, 0] : [0, 0, 1]); } }However, the current imperative style is more readable for this use case, so this is purely a stylistic nitpick.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2024-11-22T17:17:39.914Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts:125-125
Timestamp: 2024-11-22T17:17:39.914Z
Learning: In the `createNode` function within `skeletontracing_reducer_helpers.ts`, the property `resolution` is intentionally kept even when the parameter is renamed to `mag`.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
📚 Learning: 2025-12-11T15:25:53.526Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/admin/statistic/time_tracking_overview.tsx:261-279
Timestamp: 2025-12-11T15:25:53.526Z
Learning: Ant Design v6 Select: when using the Select component, consider supplying a prefix prop to render an icon or element before the input for better visual context. Apply this guideline to TS and TSX files across the codebase where Ant Design Select is used; ensure prefix usage is accessible (e.g., provide meaningful aria-label if needed) and avoid unnecessary prefixes on simple inputs.
Applied to files:
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/viewer/model/sagas/meshes/ad_hoc_mesh_saga.ts (4)
195-195: Console logging provides useful debugging context.The
console.logadds thesegmentIdto the console output, which can be helpful when debugging mesh loading failures alongside the existing error notification.
489-498: Cache key updated consistently to use paddedPosition.The cache lookup (line 489) and storage (line 498) both use
paddedPosition, ensuring consistent cache behavior. Since the padding is deterministic based onclippedPositionandmag, the same logical cube will always map to the same cache key.
520-524: Request payload correctly uses padded values.The API call now sends
paddedPositionandpaddedCubeSize, which aligns with the new padding strategy to generate closed mesh surfaces at layer borders.
474-487: Padding logic is well-documented and correctly implements the border-closing approach.The comprehensive comment clearly explains the two-part padding strategy:
- Adding 1vx overlap between cubes to fill gaps
- Extending cubes beyond layer borders to enable marching cubes to generate closing surfaces
The implementation correctly shifts positions by
-magand delegates cube size calculation to thegetPaddedCubeSizeInTargetMaghelper function, which properly increases size by [1,1,1] for gaps and handles lower layer border extensions. The API correctly passespositionWithPaddingto the backend, where the MarchingCubes implementation accepts negative offsets without validation and directly applies them in vertex calculations, confirming support for negative coordinates.
Ad-hoc meshing showed some open edges
The topleft problem was caused because the backend didn’t like negative requested coordinates, leading to crash fixes which shifted the toplefts into the layer bbox. This PR fixes this in the backend and undoes this change in the frontend, so negative coordinates are again sent, giving a clean upper edge.
The bottomright problem was caused because the segment index doesn’t indicate anything below the layer bbox. The neighbors case would add a cube outside of the layer, providing the outer edge, but the segment index case would skip that. Now the frontend embiggens the cubes to request if they align with the bottomright edges.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)