Fix: FreeHandRoiTool Statistics Text Not Displayed#2723
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@sedghi Could you please take a look at this PR and provide your feedback? |
|
|
||
| for (let j = 0; j < points.length; j++) { | ||
| const worldPosIndex = indexPoints[j].map(Math.floor); | ||
| const worldPosIndex = indexPoints[j].map((v) => Math.floor(v + 1e-3)); |
There was a problem hiding this comment.
This isn't clear why you are doing this, nor that it is valid for things like WSI images.
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
Can you strip out the changes for worldPosIndex, or if they are really needed, then give a good explanation and make sure it really works reliably for data like WSI.
If you need ceiling/floor differences, then calculating both ceiling and floor rather than trying to do some not understandable logic that might not work reliably.
|
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 (1)
📝 WalkthroughWalkthroughTwo targeted fixes in ChangesPlanarFreehandROI Stats Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
♻️ Duplicate comments (1)
packages/tools/src/tools/annotation/PlanarFreehandROITool.ts (1)
704-707:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
data.cachedStatsbefore the missing-cache recalculation path.At Line 707, missing
cachedStatsnow correctly triggers recalculation, but_calculateStatsIfActivelater readsdata.cachedStats[targetId]directly, which can throw whencachedStatsis absent (allowed by the annotation type). Please initialize the cache map before dereferencing.Suggested fix
_calculateStatsIfActive( annotation: PlanarFreehandROIAnnotation, targetId: string, viewport, renderingEngine, enabledElement ) { const activeAnnotationUID = this.commonData?.annotation.annotationUID; if ( annotation.annotationUID === activeAnnotationUID && !this.commonData?.movingTextBox ) { return; } if (!this.commonData?.movingTextBox) { const { data } = annotation; + data.cachedStats ||= {}; if (!data.cachedStats[targetId]?.unit) { data.cachedStats[targetId] = { Modality: null, area: null, max: null, mean: null, stdDev: null, areaUnit: null, unit: null, };🤖 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/annotation/PlanarFreehandROITool.ts` around lines 704 - 707, The issue is that when cachedStats is missing from the annotation data, the subsequent call to _calculateStatsIfActive will attempt to read data.cachedStats[targetId] directly, which will throw an error. To fix this, initialize data.cachedStats as an empty object before the missing-cache recalculation path executes. Specifically, when the condition at line 707 (invalidated || !cachedStats?.[targetId]) is true, ensure that data.cachedStats is initialized to an empty object if it does not already exist, so that _calculateStatsIfActive can safely write to and read from the cache map without errors.
🤖 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.
Duplicate comments:
In `@packages/tools/src/tools/annotation/PlanarFreehandROITool.ts`:
- Around line 704-707: The issue is that when cachedStats is missing from the
annotation data, the subsequent call to _calculateStatsIfActive will attempt to
read data.cachedStats[targetId] directly, which will throw an error. To fix
this, initialize data.cachedStats as an empty object before the missing-cache
recalculation path executes. Specifically, when the condition at line 707
(invalidated || !cachedStats?.[targetId]) is true, ensure that data.cachedStats
is initialized to an empty object if it does not already exist, so that
_calculateStatsIfActive can safely write to and read from the cache map without
errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21961d64-b7c7-4bdd-8e38-350bbc9cfd40
📒 Files selected for processing (1)
packages/tools/src/tools/annotation/PlanarFreehandROITool.ts
3 participants
Context
Fix issue: #2645
After drawing an annotation using the PlanarFreeHandRoiTool, the statistics text (such as area, mean, etc.) is not displayed next to the annotation when switching from 2D MPR view to Frame view. The ROI is created and rendered correctly on the viewport, but the associated statistics label is missing.
Changes & Results
Added an additional condition to ensure statistics are recalculated when cached stats for the current
targetIdare missing. The check now triggers when the annotation is invalidated or when the cached statistics for the specified target are not available, preventing missing measurement values.fix-freehand-roi-stat-display.mp4
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit