-
Notifications
You must be signed in to change notification settings - Fork 453
fix(tools): Prevent infinite update loop in SplineROITool #2453
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
base: main
Are you sure you want to change the base?
fix(tools): Prevent infinite update loop in SplineROITool #2453
Conversation
|
Hi team! 👋 The fix resolves an infinite SEGMENTATION_DATA_MODIFIED loop in SplineROI-based tools and significantly improves app stability during contour editing. Please let me know if there is anything I can improve or adjust to help move the review forward. Thanks a lot for your time and for maintaining this great project! |
|
Thanks for the PR, we will look soon |
| }; | ||
|
|
||
| // Compare polylines in canvas space with a small tolerance | ||
| private _arePolylinesEqual = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a utility in the tools module
| if (a.length !== b.length) return false; | ||
| for (let i = 0; i < a.length; i++) { | ||
| if ( | ||
| Math.abs(a[i][0] - b[i][0]) > eps || |
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.
Why not use isEqual(a[i],b[i])?
| ); | ||
| // Compare with existing contour polyline (converted to canvas) | ||
| const existingPolylineCanvas = | ||
| annotation.data.contour.polyline?.map(worldToCanvas) || []; |
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.
You don't need the || [] at the end - the compare function works without it.
| { | ||
| points: splinePolylineCanvas, | ||
| closed: isClosed, | ||
| targetWindingDirection: ContourWindingDirection.Clockwise, |
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.
Is it possible that the target winding direction will update/smooth the contour, resulting in infinite loop anyways?
| !this._arePolylinesEqual(existingPolylineCanvas, splinePolylineCanvas) | ||
| ) { | ||
| const isClosed = data.contour.closed; | ||
| this.updateContourPolyline( |
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.
Consider moving the check for is same into the updateContourPolyline itself, perhaps under a flag.
Also, the isSamePolyline with the changes I suggested works fine for either 2d or 3d point data, so you might not need to convert the old one to canvas points if the new one is being converted anyways, and might have new changes inside updateContourPolyline.
Context
When adding or editing a contour segmentation using any of the SplineROI based tools (e.g., CatmullRomSplineROI, LinearSplineROI, BSplineROI), an infinite loop of SEGMENTATION_DATA_MODIFIED events was triggered.
This issue was caused within the renderAnnotationInstance method of the SplineROITool. During every render pass, the tool would recalculate the spline's polyline and call updateContourPolyline. This process unconditionally marked the annotation as invalidated, which in turn triggered a new SEGMENTATION_DATA_MODIFIED event and a subsequent re-render, leading to a feedback loop.
Changes & Results
Modified
SplineROITool.ts:Effect of the change:
Testing
This fix was tested locally by monitoring the SEGMENTATION_DATA_MODIFIED event to ensure it no longer fires in a loop.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment