-
Notifications
You must be signed in to change notification settings - Fork 339
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
feat(ROIThresholdsTools): Made the Rectangle and Circle ROIStartEndThresholds works on other planes #1128
feat(ROIThresholdsTools): Made the Rectangle and Circle ROIStartEndThresholds works on other planes #1128
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…into CircleThresholdTool-improvements
…into CircleThresholdTool-improvements
…into CircleThresholdTool-improvements
…into CircleThresholdTool-improvements
packages/tools/examples/RectangleROIStartEndThreshold/preset.js
Outdated
Show resolved
Hide resolved
* @param spacingInNormalDirection - The spacing in the normal direction | ||
* @returns The filtered `Annotations`. | ||
*/ | ||
export default function filterAnnotationsWithinSamePlan( |
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.
I'm not sure why we need this function, since annotationWithinSameSlice already checks for the same plane (annotationsWithParallelNormals
)
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.
annotationWithinSameSlice
was returning the annotations that were on the same plane and slice aswell, but we wanted all the annotations for the same plane only.
packages/tools/src/tools/segmentation/CircleROIStartEndThresholdTool.ts
Outdated
Show resolved
Hide resolved
if (this._arraysEqual(viewPlaneNormal, mprValues.axial.viewPlaneNormal)) { | ||
return Math.round(imageIdIndex[2]); | ||
} else if (this._arraysEqual(viewPlaneNormal, mprValues.sagittal.viewPlaneNormal)) { | ||
return Math.round(imageIdIndex[0]); | ||
} else if (this._arraysEqual(viewPlaneNormal, mprValues.coronal.viewPlaneNormal)) { | ||
return Math.round(imageIdIndex[1]); | ||
} else { | ||
return undefined; | ||
} |
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.
I'm not sure if this is a correct logic, basically the volume has x,y,z , but it might be a sagittal acquistion volume, but still the z is the depth though sagittal. The logic is more complext than this, example of such volume can be found in this study https://viewer-dev.ohif.org/viewer?StudyInstanceUIDs=1.3.6.1.4.1.25403.345050719074.3824.20170125095438.5
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.
Yes I agree this may not be the good solution but I didn't find how to get the slice we project to get without checking in what plane we are. Maybe you can help us with that.
…into CircleThresholdTool-improvements
…into CircleThresholdTool-improvements
…into CircleThresholdTool-improvements
@sedghi We've made some changes to the way we were doing the 3D ROI. Now there is no more sliceIndex but we are using instead the world coordinates of where the annotation is drawn and we compare it to the focalpoint of the camera. Also we added the calculation of the basic stats with all the points contained in the 3D roi and the posibility to show the textbost like we have the for the other tools. |
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.
Sorry for the delay. I'm back to contributing to open source much more now.
I see some weird behavior.
@sedghi I think you might have tested an older version. Here is what I have : https://streamable.com/yx1qd4 |
…into CircleThresholdTool-improvements
…/Celian-abd/CircleThresholdTool-improvements
…/Celian-abd/CircleThresholdTool-improvements
…/Celian-abd/CircleThresholdTool-improvements
I haven't forgotten about this. We are just shipping a 3.8.1 for OHIF, and for that, I don't want to include this PR since we are upgrading |
…into CircleThresholdTool-improvements
Hi @sedghi, is there a chance you could review this PR ? We enhanced it, i think it is ready for review |
Yes, we released version 3.8.1, so I will review this issue. Apologies for the delay. |
…/Celian-abd/cornerstone3D into pr/Celian-abd/CircleThresholdTool-improvements
…/Celian-abd/CircleThresholdTool-improvements
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.
Great addition, thanks for the pull request, and apologies for the delay in merging it.
@sedghi thank you ! Next week we will make you a PR to update the command that set the start / stop slice in ohif as now the z limits are set by position and not to the viewport slice index |
(PR in ohif not cornerstone) |
Context
Updated version of the Rectangle/Circle ROIStartEndThreshold tools so they can work on other then acquisition plane.
Changes & Results
Testing
yarn run example RectangleROIStartEndThreshold or on CircleROIStartEndThreshold
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment