fix: improved brush shape for rotated images#2743
Open
Devu-trenser wants to merge 3 commits into
Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Devu-trenser
force-pushed
the
fix-improved-brush-shape-for-rotated-images
branch
from
2c133c5 to
c9fd4f5
Compare
Devu-trenser
changed the title
…ape-for-rotated-images
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThree segmentation fill strategies ( ChangesOblique-plane fill strategy corrections
Sequence DiagramssequenceDiagram
participant initializeCircle as initializeCircle
participant createPointInEllipse as createPointInEllipse
participant voxelCenterConv as toVoxelCenterIJK
participant membershipTest as ellipse/sphere membership
initializeCircle->>createPointInEllipse: cornersInWorld, {viewRight, viewUp, viewNormal, radii}
createPointInEllipse->>createPointInEllipse: compute planeTolerance from spacing·viewNormal
createPointInEllipse->>createPointInEllipse: precompute invXRSq, invYRSq from radii
createPointInEllipse->>membershipTest: pointIJK, center, projection axes, precomputed radii
membershipTest->>voxelCenterConv: pointIJK + 0.5
voxelCenterConv->>membershipTest: pointWorld at voxel center
membershipTest->>membershipTest: u = dot(centerToPoint, viewRight), v = dot(centerToPoint, viewUp)
membershipTest->>membershipTest: ellipseVal = u²·invXRSq + v²·invYRSq
membershipTest->>membershipTest: depth = abs(dot(centerToPoint, viewNormal))
membershipTest-->>initializeCircle: ellipseVal ≤ 1 AND depth ≤ planeTolerance
sequenceDiagram
participant initializeRectangle as initializeRectangle
participant orderRectangleCorners as orderRectangleCorners
participant createPointInRectangle as createPointInRectangle
participant pointInShapeFn as pointInShapeFn
initializeRectangle->>orderRectangleCorners: unordered corners
orderRectangleCorners-->>initializeRectangle: corners sorted by polar angle
initializeRectangle->>createPointInRectangle: ordered corners, segmentationImageData
createPointInRectangle->>createPointInRectangle: transform corners to IJK, compute boundsIJK
createPointInRectangle->>createPointInRectangle: compute rectangle basis (u-axis, v-axis, normal)
createPointInRectangle->>createPointInRectangle: thickness from spacing·normal
createPointInRectangle->>pointInShapeFn: point candidate, rectangle geometry
pointInShapeFn->>pointInShapeFn: check u-range, v-range, distanceToPlane ≤ thickness
pointInShapeFn-->>initializeRectangle: true/false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/tools/src/tools/segmentation/strategies/fillCircle.ts (1)
71-76: 💤 Low valueCorner ordering inconsistency with destructuring at line 314.
The return order is
[topRight, topLeft, bottomRight, bottomLeft]but the destructuring at line 314 expects[topLeft, bottomRight, bottomLeft, topRight]. This mismatch will cause incorrect axis calculations when the corners are consumed.However, looking at the usage at line 265,
strokeCornersWorldis only used to computeboundsIJKviagetBoundingBoxAroundShapeIJK, which is order-agnostic. ThecornersInWorldused increatePointInEllipsecomes from the canvas-derived corners at lines 223-226, not fromcreateCircleCornersForCenter. So this ordering doesn't affect the actual shape test.Consider aligning the return order for consistency
return [ - topRight as Types.Point3, - topLeft as Types.Point3, - bottomRight as Types.Point3, - bottomLeft as Types.Point3, + topLeft as Types.Point3, + bottomRight as Types.Point3, + bottomLeft as Types.Point3, + topRight as Types.Point3, ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/segmentation/strategies/fillCircle.ts` around lines 71 - 76, The return statement in the function (around lines 71-76) returns corners in the order [topRight, topLeft, bottomRight, bottomLeft], but the destructuring at line 314 expects them in the order [topLeft, bottomRight, bottomLeft, topRight]. Reorder the return statement to match the expected destructuring order at line 314. Change the return array to put topLeft first, then bottomRight, then bottomLeft, then topRight to maintain consistency with how the corners are consumed elsewhere in the code.packages/tools/src/tools/segmentation/strategies/fillSphere.ts (1)
14-14: ⚡ Quick winUnused import:
getSphereBoundsInfoFromViewport.The import is still present but the function is no longer used after the bounds computation was changed to use direct IJK calculation.
Remove unused import
-import { getSphereBoundsInfoFromViewport } from '../../../utilities/getSphereBoundsInfo';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/segmentation/strategies/fillSphere.ts` at line 14, Remove the unused import statement for getSphereBoundsInfoFromViewport from the imports at the top of the fillSphere.ts file. This function is no longer needed since the bounds computation has been changed to use direct IJK calculation instead of relying on the getSphereBoundsInfoFromViewport utility function.packages/tools/src/tools/segmentation/strategies/fillRectangle.ts (1)
151-160: 💤 Low valueFull
projectedSpacingused as thickness may be too permissive.The comment says "Using full projected spacing gives more stable voxel occupancy," but using the full spacing rather than half-spacing as a tolerance means voxels up to one full voxel away from the plane are included. This could include voxels from adjacent slices in thin-slice scenarios.
For consistency with
fillCircle.tswhich usesspacingInNormal / 2forplaneTolerance, consider whether this intentional difference is desired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/segmentation/strategies/fillRectangle.ts` around lines 151 - 160, The thickness variable is set to the full projectedSpacing value, which may be overly permissive and include voxels from adjacent slices in thin-slice scenarios. For consistency with fillCircle.ts which uses spacingInNormal divided by 2 for planeTolerance, evaluate whether the thickness assignment should be changed to use projectedSpacing divided by 2 instead of the full value. Review the intention and adjust if the full spacing is not intentionally required for this particular case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/src/tools/segmentation/strategies/fillCircle.ts`:
- Around line 330-338: The spacingInNormal calculation in the fillCircle
strategy currently assumes viewNormal is always available when calling
getSpacingInNormalDirection, but viewNormal is optional and may be undefined.
Refactor the conditional logic to explicitly check if viewNormal is defined
before attempting to call getSpacingInNormalDirection with the cast to
Types.Point3. If viewNormal is undefined, provide an appropriate fallback that
does not assume Z-axis spacing is suitable for all orientations. Additionally,
reconsider whether the current condition (spacing && direction) is sufficient,
as getSpacingInNormalDirection might handle cases where only one of these
parameters is defined.
- Around line 427-441: The ellipse equation test does not check the
strokePredicate when a point falls outside the static ellipse boundary (when
xTerm + yTerm > 1), unlike the sphere case which checks strokePredicate before
its sphere test. To fix this, after the ellipse equation check at line 434 (the
condition checking if xTerm + yTerm <= 1), add a check for the strokePredicate
similar to how it is handled in the sphere case. This will ensure that
stroke-based fills work correctly for ellipses during brush strokes by
evaluating the strokePredicate for points outside the static ellipse.
In `@packages/tools/src/tools/segmentation/strategies/fillRectangle.ts`:
- Around line 120-135: The axis construction in the fillRectangle function
assumes that after angular sorting, p0, p1, and p3 form adjacent edges from a
corner, but there is no validation that this assumption holds. If the sorting
produces a different order like diagonal pairs, then axisU and axisV would not
be orthogonal and the normal calculation would be incorrect. After obtaining
orderedPoints, validate adjacency by comparing distances from p0 to each other
point—the two shortest distances should correspond to p1 and p3 (the adjacent
edges), while the longest should be to p2 (the diagonal). If this condition is
not met, reorder the points so that p1 and p3 are correctly identified as the
adjacent corner points before constructing the axes and normal.
In `@packages/tools/src/tools/segmentation/strategies/fillSphere.ts`:
- Around line 97-112: The boundsIJK variable is computed without clamping to
valid image dimensions, unlike the fillRectangle.ts strategy. After calculating
the boundsIJK array using centerIJK and radiusIJK values, clamp each dimension's
min and max bounds to the range [0, dims[i]-1] for each of the three dimensions
I, J, and K, where dims are the image dimensions. This should be done before
assigning the clamped bounds to operationData.isInObjectBoundsIJK to prevent
out-of-bounds array access errors.
---
Nitpick comments:
In `@packages/tools/src/tools/segmentation/strategies/fillCircle.ts`:
- Around line 71-76: The return statement in the function (around lines 71-76)
returns corners in the order [topRight, topLeft, bottomRight, bottomLeft], but
the destructuring at line 314 expects them in the order [topLeft, bottomRight,
bottomLeft, topRight]. Reorder the return statement to match the expected
destructuring order at line 314. Change the return array to put topLeft first,
then bottomRight, then bottomLeft, then topRight to maintain consistency with
how the corners are consumed elsewhere in the code.
In `@packages/tools/src/tools/segmentation/strategies/fillRectangle.ts`:
- Around line 151-160: The thickness variable is set to the full
projectedSpacing value, which may be overly permissive and include voxels from
adjacent slices in thin-slice scenarios. For consistency with fillCircle.ts
which uses spacingInNormal divided by 2 for planeTolerance, evaluate whether the
thickness assignment should be changed to use projectedSpacing divided by 2
instead of the full value. Review the intention and adjust if the full spacing
is not intentionally required for this particular case.
In `@packages/tools/src/tools/segmentation/strategies/fillSphere.ts`:
- Line 14: Remove the unused import statement for
getSphereBoundsInfoFromViewport from the imports at the top of the fillSphere.ts
file. This function is no longer needed since the bounds computation has been
changed to use direct IJK calculation instead of relying on the
getSphereBoundsInfoFromViewport utility function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d759a66a-da29-4877-a2bd-3a73b4936c03
📒 Files selected for processing (3)
packages/tools/src/tools/segmentation/strategies/fillCircle.tspackages/tools/src/tools/segmentation/strategies/fillRectangle.tspackages/tools/src/tools/segmentation/strategies/fillSphere.ts
Comment on lines
+330
to
+338
| const spacingInNormal = | ||
| spacing && direction | ||
| ? csUtils.getSpacingInNormalDirection( | ||
| { spacing, direction }, | ||
| viewNormal as Types.Point3 | ||
| ) | ||
| : Math.max(spacing?.[2] ?? 1, Number.EPSILON); | ||
|
|
||
| const planeTolerance = Math.max(spacingInNormal / 2, Number.EPSILON); |
There was a problem hiding this comment.
Potential fallback to spacing[2] when viewNormal is undefined.
When viewNormal is undefined (since it's optional), getSpacingInNormalDirection may not be called correctly. The fallback Math.max(spacing?.[2] ?? 1, Number.EPSILON) assumes Z-axis spacing is appropriate, which may not hold for oblique orientations.
Additionally, if spacing is defined but direction is undefined, the fallback path is taken even though getSpacingInNormalDirection could potentially handle this.
Suggested fix to handle undefined viewNormal explicitly
const spacingInNormal =
- spacing && direction
+ spacing && direction && viewNormal
? csUtils.getSpacingInNormalDirection(
{ spacing, direction },
viewNormal as Types.Point3
)
- : Math.max(spacing?.[2] ?? 1, Number.EPSILON);
+ : spacing
+ ? Math.max(...spacing, Number.EPSILON)
+ : 1;📝 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.
Suggested change
| const spacingInNormal = | |
| spacing && direction | |
| ? csUtils.getSpacingInNormalDirection( | |
| { spacing, direction }, | |
| viewNormal as Types.Point3 | |
| ) | |
| : Math.max(spacing?.[2] ?? 1, Number.EPSILON); | |
| const planeTolerance = Math.max(spacingInNormal / 2, Number.EPSILON); | |
| const spacingInNormal = | |
| spacing && direction && viewNormal | |
| ? csUtils.getSpacingInNormalDirection( | |
| { spacing, direction }, | |
| viewNormal as Types.Point3 | |
| ) | |
| : spacing | |
| ? Math.max(...spacing, Number.EPSILON) | |
| : 1; | |
| const planeTolerance = Math.max(spacingInNormal / 2, Number.EPSILON); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tools/src/tools/segmentation/strategies/fillCircle.ts` around lines
330 - 338, The spacingInNormal calculation in the fillCircle strategy currently
assumes viewNormal is always available when calling getSpacingInNormalDirection,
but viewNormal is optional and may be undefined. Refactor the conditional logic
to explicitly check if viewNormal is defined before attempting to call
getSpacingInNormalDirection with the cast to Types.Point3. If viewNormal is
undefined, provide an appropriate fallback that does not assume Z-axis spacing
is suitable for all orientations. Additionally, reconsider whether the current
condition (spacing && direction) is sufficient, as getSpacingInNormalDirection
might handle cases where only one of these parameters is defined.
Comment on lines
+427
to
+441
| const xDist = vec3.dot(pointVec, viewRight); | ||
| const yDist = vec3.dot(pointVec, viewUp); | ||
|
|
||
| // Use the Ellipse Equation with the specific xRadius and yRadius | ||
| // Ellipse equation: (x/xRadius)^2 + (y/yRadius)^2 <= 1 | ||
| return (x * x) / (xRadius * xRadius) + (y * y) / (yRadius * yRadius) <= 1; | ||
| const xTerm = xDist * xDist * invXRadiusSquared; | ||
| const yTerm = yDist * yDist * invYRadiusSquared; | ||
|
|
||
| if (xTerm + yTerm <= 1) { | ||
| // Depth Check (Z-axis) | ||
| // Ensure the voxel is physically close to the plane we are looking at | ||
| const zDist = Math.abs(vec3.dot(pointVec, viewNormal)); | ||
|
|
||
| return zDist <= planeTolerance; | ||
| } |
There was a problem hiding this comment.
Ellipse test returns false without checking strokePredicate for points outside the static ellipse.
The sphere case (line 398-400) checks strokePredicate before the sphere test. However, in the ellipse case, if a point fails the ellipse equation (xTerm + yTerm > 1), the function returns false at line 443 without checking the strokePredicate. This means stroke-based fills won't work correctly for ellipses during brush strokes.
Add strokePredicate check to ellipse case
// Otherwise, treat as ellipse in oblique plane
return (pointLPS: Types.Point3 | null, pointIJK?: Types.Point3) => {
let worldPoint: Types.Point3 | null = pointLPS;
if (!worldPoint && pointIJK && options.segmentationImageData) {
worldPoint = transformIndexToWorld(
options.segmentationImageData,
toVoxelCenterIJK(pointIJK as Types.Point3)
) as Types.Point3;
}
if (!worldPoint) {
return false;
}
+ if (strokePredicate?.(worldPoint)) {
+ return true;
+ }
+
// Get the vector from the center of the brush to the current voxel
const pointVec = vec3.create();
vec3.subtract(pointVec, worldPoint, center);📝 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.
Suggested change
| const xDist = vec3.dot(pointVec, viewRight); | |
| const yDist = vec3.dot(pointVec, viewUp); | |
| // Use the Ellipse Equation with the specific xRadius and yRadius | |
| // Ellipse equation: (x/xRadius)^2 + (y/yRadius)^2 <= 1 | |
| return (x * x) / (xRadius * xRadius) + (y * y) / (yRadius * yRadius) <= 1; | |
| const xTerm = xDist * xDist * invXRadiusSquared; | |
| const yTerm = yDist * yDist * invYRadiusSquared; | |
| if (xTerm + yTerm <= 1) { | |
| // Depth Check (Z-axis) | |
| // Ensure the voxel is physically close to the plane we are looking at | |
| const zDist = Math.abs(vec3.dot(pointVec, viewNormal)); | |
| return zDist <= planeTolerance; | |
| } | |
| const xDist = vec3.dot(pointVec, viewRight); | |
| const yDist = vec3.dot(pointVec, viewUp); | |
| // Use the Ellipse Equation with the specific xRadius and yRadius | |
| // Ellipse equation: (x/xRadius)^2 + (y/yRadius)^2 <= 1 | |
| const xTerm = xDist * xDist * invXRadiusSquared; | |
| const yTerm = yDist * yDist * invYRadiusSquared; | |
| if (xTerm + yTerm <= 1) { | |
| // Depth Check (Z-axis) | |
| // Ensure the voxel is physically close to the plane we are looking at | |
| const zDist = Math.abs(vec3.dot(pointVec, viewNormal)); | |
| return zDist <= planeTolerance; | |
| } | |
| if (strokePredicate?.(worldPoint)) { | |
| return true; | |
| } | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tools/src/tools/segmentation/strategies/fillCircle.ts` around lines
427 - 441, The ellipse equation test does not check the strokePredicate when a
point falls outside the static ellipse boundary (when xTerm + yTerm > 1), unlike
the sphere case which checks strokePredicate before its sphere test. To fix
this, after the ellipse equation check at line 434 (the condition checking if
xTerm + yTerm <= 1), add a check for the strokePredicate similar to how it is
handled in the sphere case. This will ensure that stroke-based fills work
correctly for ellipses during brush strokes by evaluating the strokePredicate
for points outside the static ellipse.
Comment on lines
+97
to
+112
| const boundsIJK = [ | ||
| [ | ||
| Math.floor(centerIJK[0] - radiusIJK[0]), | ||
| Math.ceil(centerIJK[0] + radiusIJK[0]), | ||
| ], | ||
| [ | ||
| Math.floor(centerIJK[1] - radiusIJK[1]), | ||
| Math.ceil(centerIJK[1] + radiusIJK[1]), | ||
| ], | ||
| [ | ||
| Math.floor(centerIJK[2] - radiusIJK[2]), | ||
| Math.ceil(centerIJK[2] + radiusIJK[2]), | ||
| ], | ||
| ]; | ||
|
|
||
| operationData.isInObjectBoundsIJK = boundsIJK as Types.BoundsIJK; |
There was a problem hiding this comment.
Bounds not clamped to image dimensions.
Unlike fillRectangle.ts which clamps bounds to [0, dims[i]-1], the sphere bounds are computed without clamping. This could result in out-of-bounds IJK indices being passed to the voxel iteration, potentially causing array access errors or wasted computation on non-existent voxels.
Add dimension clamping to boundsIJK
+ const dims = segmentationImageData.getDimensions();
+
// Define bounds that always encompass the sphere
const boundsIJK = [
[
- Math.floor(centerIJK[0] - radiusIJK[0]),
- Math.ceil(centerIJK[0] + radiusIJK[0]),
+ Math.max(0, Math.floor(centerIJK[0] - radiusIJK[0])),
+ Math.min(dims[0] - 1, Math.ceil(centerIJK[0] + radiusIJK[0])),
],
[
- Math.floor(centerIJK[1] - radiusIJK[1]),
- Math.ceil(centerIJK[1] + radiusIJK[1]),
+ Math.max(0, Math.floor(centerIJK[1] - radiusIJK[1])),
+ Math.min(dims[1] - 1, Math.ceil(centerIJK[1] + radiusIJK[1])),
],
[
- Math.floor(centerIJK[2] - radiusIJK[2]),
- Math.ceil(centerIJK[2] + radiusIJK[2]),
+ Math.max(0, Math.floor(centerIJK[2] - radiusIJK[2])),
+ Math.min(dims[2] - 1, Math.ceil(centerIJK[2] + radiusIJK[2])),
],
];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tools/src/tools/segmentation/strategies/fillSphere.ts` around lines
97 - 112, The boundsIJK variable is computed without clamping to valid image
dimensions, unlike the fillRectangle.ts strategy. After calculating the
boundsIJK array using centerIJK and radiusIJK values, clamp each dimension's min
and max bounds to the range [0, dims[i]-1] for each of the three dimensions I,
J, and K, where dims are the image dimensions. This should be done before
assigning the clamped bounds to operationData.isInObjectBoundsIJK to prevent
out-of-bounds array access errors.
wayfarer3130
requested changes
Jun 17, 2026
wayfarer3130
left a comment
wayfarer3130
left a comment
Collaborator
There was a problem hiding this comment.
I'm needing to make more exhaustive changes to correct a number of edge cases/flaws in how this is currently working. Will see if I can get this fixed this week still.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Fixes incorrect shape rendering when using brush tools with circular/spherical/rectangle shapes in rotated viewports.
Fixes : #2651
arul-trenser:feat-axis-based-image-stretchingbranch from #2443.Changes & Results
Changes
Results
Before
After
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit