Skip to content
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

Merged
merged 32 commits into from
Jun 21, 2024

Conversation

Celian-abd
Copy link
Contributor

Context

Updated version of the Rectangle/Circle ROIStartEndThreshold tools so they can work on other then acquisition plane.

Changes & Results

  • Update calculateCachedStats for other plane

Testing

yarn run example RectangleROIStartEndThreshold or on CircleROIStartEndThreshold

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • [] "Browser:

Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit ba773d7
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/6675d4b109a64f0008f1ba03
😎 Deploy Preview https://deploy-preview-1128--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

* @param spacingInNormalDirection - The spacing in the normal direction
* @returns The filtered `Annotations`.
*/
export default function filterAnnotationsWithinSamePlan(
Copy link
Member

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)

Copy link
Contributor Author

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.

Comment on lines 621 to 629
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;
}
Copy link
Member

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

Copy link
Contributor Author

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.

@Celian-abd Celian-abd requested a review from sedghi March 14, 2024 10:53
@Celian-abd
Copy link
Contributor Author

@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.

Copy link
Member

@sedghi sedghi left a 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.

https://share.cleanshot.com/3pGRlKPZ

@Celian-abd
Copy link
Contributor Author

@sedghi I think you might have tested an older version.

Here is what I have : https://streamable.com/yx1qd4

@sedghi
Copy link
Member

sedghi commented Jun 5, 2024

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 cs3d versions. After that is merged (pretty soon), I'll come back to this.

@salimkanoun
Copy link
Contributor

Hi @sedghi, is there a chance you could review this PR ? We enhanced it, i think it is ready for review

@sedghi
Copy link
Member

sedghi commented Jun 15, 2024

Yes, we released version 3.8.1, so I will review this issue. Apologies for the delay.

Copy link
Member

@sedghi sedghi left a 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 sedghi merged commit 7e5e892 into cornerstonejs:main Jun 21, 2024
15 checks passed
@salimkanoun
Copy link
Contributor

@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

@salimkanoun
Copy link
Contributor

(PR in ohif not cornerstone)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants