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: PlanarFreehandROI stats #326

Merged
merged 11 commits into from May 8, 2023

Conversation

m00n620
Copy link
Contributor

@m00n620 m00n620 commented Dec 5, 2022

Display PlanarFreehandROI stats like other ROI tools (Elliptical, Rectangle).

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit bf7458c
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/644140f907a0f6000884ec51
😎 Deploy Preview https://deploy-preview-326--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 settings.

@m00n620 m00n620 changed the title PlanarFreehandROI stats feat: PlanarFreehandROI stats Dec 6, 2022
@sedghi sedghi requested a review from lscoder December 8, 2022 19:04
@m00n620
Copy link
Contributor Author

m00n620 commented Dec 21, 2022

@lscoder, can you please review this as soon as possible?

@lscoder
Copy link
Collaborator

lscoder commented Dec 22, 2022

@m00n620 Users should be able to drag and drop labels but it is broken

2022-12-22 13-08-11

@m00n620
Copy link
Contributor Author

m00n620 commented Dec 29, 2022

lscoder

@lscoder , it requires heavy calculation for stats, so I didn't implement moving stats, should we essentially need this feature?

@lscoder
Copy link
Collaborator

lscoder commented Jan 9, 2023

lscoder

@lscoder , it requires heavy calculation for stats, so I didn't implement moving stats, should we essentially need this feature?

All labels work in the same way and we follow the same standard to all tools. Why does it need heavy calculation when moving the label? The points should be the same and it could use the stats from the cache.

@m00n620 m00n620 requested review from lscoder and sedghi and removed request for lscoder and sedghi February 7, 2023 23:15
@m00n620
Copy link
Contributor Author

m00n620 commented Feb 7, 2023

@lscoder , @sedghi , moving textbox feature has been added. please take a look again.

@sedghi
Copy link
Member

sedghi commented Feb 8, 2023

There are some improvements to this PR, but not there yet

  • textboxes are removed when drawing happens that means all textboxes are recalculated?
  • Drawing on the second viewport make the textbox on the first one to disappear
cb22d03c-1614-4dbc-be79-57d93e791cdf.webm

@wayfarer3130
Copy link
Collaborator

Please ensure that you have tested store/remember with the @cornerstonejs/adapters DICOM SR reader and writer. Those should be able to record and restore the Planar Freehand object with the updated measurements data and position.

@m00n620
Copy link
Contributor Author

m00n620 commented Feb 13, 2023

There are some improvements to this PR, but not there yet

  • textboxes are removed when drawing happens that means all textboxes are recalculated?
  • Drawing on the second viewport make the textbox on the first one to disappear

cb22d03c-1614-4dbc-be79-57d93e791cdf.webm

@sedghi , this comment has been resolved, please take a look again.

@m00n620
Copy link
Contributor Author

m00n620 commented Feb 13, 2023

Please ensure that you have tested store/remember with the @cornerstonejs/adapters DICOM SR reader and writer. Those should be able to record and restore the Planar Freehand object with the updated measurements data and position.

@wayfarer3130 , can you please be more specific? it would be great if you can comment more for how to test storing/remembering with the @cornerstonejs/adapters DICOM SR reader and writer on this tool.

@m00n620 m00n620 requested review from sedghi and removed request for lscoder February 14, 2023 12:46
@sedghi
Copy link
Member

sedghi commented Feb 14, 2023

@m00n620 @wayfarer3130 We can focus on performance for stats for this PR and deal with SR later.
@m00n620 I see a lot of performance issue on this tool, have you tried it locally?
This demo is basically not usable https://deploy-preview-326--cornerstone-3d-docs.netlify.app/live-examples/planarfreehandroitool

@m00n620
Copy link
Contributor Author

m00n620 commented Feb 14, 2023

@sedghi , can you please be more specific about performance issues? I tried locally and I think it's working well.

@sedghi
Copy link
Member

sedghi commented Feb 14, 2023

Can you send me a video capturing the link I sent you when you draw a contour and enable text box and enable again?

@m00n620
Copy link
Contributor Author

m00n620 commented Feb 15, 2023

@sedghi , I found out that there are some performance issues when I draw large-area contours. I will do more adjustments and let you know when it's ready.

@wayfarer3130
Copy link
Collaborator

Please ensure that you have tested store/remember with the @cornerstonejs/adapters DICOM SR reader and writer. Those should be able to record and restore the Planar Freehand object with the updated measurements data and position.

@wayfarer3130 , can you please be more specific? it would be great if you can comment more for how to test storing/remembering with the @cornerstonejs/adapters DICOM SR reader and writer on this tool.

You need to embed this within a framework such as OHIF that has a store/remember facility. I'd like to add this to the CS3D with a set of examples, but that isn't available yet. I think proceeding without the adapters is probably worthwhile, as the save/load can then be added separately.

@m00n620 m00n620 requested a review from lscoder April 11, 2023 14:56
@lscoder
Copy link
Collaborator

lscoder commented Apr 13, 2023

@m00n620 It seems stats are being recalculated when user draws new ROIs even when it is not needed because it gets slower considerably. After comparing to the video shared by Alireza the textboxes were being removed but not recalculated in the middle of the drawing process and now they are not being removed but recalculated on every mouse move event.

2023-04-13.11-01-11.mp4

1st ROI - OK
2nd ROI - slower
3rd ROI - hard to draw

@m00n620
Copy link
Contributor Author

m00n620 commented Apr 20, 2023

@m00n620 It seems stats are being recalculated when user draws new ROIs even when it is not needed because it gets slower considerably. After comparing to the video shared by Alireza the textboxes were being removed but not recalculated in the middle of the drawing process and now they are not being removed but recalculated on every mouse move event.

2023-04-13.11-01-11.mp4
1st ROI - OK 2nd ROI - slower 3rd ROI - hard to draw

@lscoder , this should be fixed now. please check again.

Copy link
Collaborator

@wayfarer3130 wayfarer3130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sedghi - I've tested the various cases that were complained about, and also looked at the various comments, and I think they are all addressed. If you are ok with that, then I can merge this. I think it is sufficiently ready to be worth including now - not perfect, but better than before.

@m00n620
Copy link
Contributor Author

m00n620 commented May 5, 2023

@lscoder , can you please take a look again?

@lscoder
Copy link
Collaborator

lscoder commented May 8, 2023

LGTM

@sedghi sedghi merged commit 9240862 into cornerstonejs:main May 8, 2023
10 checks passed
@rghgit1
Copy link

rghgit1 commented May 15, 2023

OHIF

Have been looking for PlanarFreehandROI stats in OHIF Viewer V3. Has this been integrated with OHIF Viewer? If not, can you please guide me with the steps to do the same. Thanks

Adding some additional details on the steps tried so far:

  1. Updated the dependencies to latest versions of cornerstonejs
    "@cornerstonejs/core": "^0.46.1",
    "@cornerstonejs/tools": "^0.67.1",
  2. Ran 'yarn install'
  3. The code changes done as part of this feature is now available under extensions\cornerstone\node_modules@cornerstonejs\tools\src\tools\annotation\PlanarFreehandROITool.ts

However, this is not getting called when the PlanarFreehandROI tool is used and no stats are displayed in OHIF Viewport.
Any pointers on how to invoke this when PlanarFreehandROI tool is used in OHIF would be great. Thanks

@m00n620
Copy link
Contributor Author

m00n620 commented May 17, 2023

OHIF

Have been looking for PlanarFreehandROI stats in OHIF Viewer V3. Has this been integrated with OHIF Viewer? If not, can you please guide me with the steps to do the same. Thanks

Adding some additional details on the steps tried so far:

  1. Updated the dependencies to latest versions of cornerstonejs
    "@cornerstonejs/core": "^0.46.1",
    "@cornerstonejs/tools": "^0.67.1",
  2. Ran 'yarn install'
  3. The code changes done as part of this feature is now available under extensions\cornerstone\node_modules@cornerstonejs\tools\src\tools\annotation\PlanarFreehandROITool.ts

However, this is not getting called when the PlanarFreehandROI tool is used and no stats are displayed in OHIF Viewport. Any pointers on how to invoke this when PlanarFreehandROI tool is used in OHIF would be great. Thanks

@rghgit1 , you should set calculateStats option in the configuration.

@rghgit1
Copy link

rghgit1 commented May 18, 2023

OHIF

Have been looking for PlanarFreehandROI stats in OHIF Viewer V3. Has this been integrated with OHIF Viewer? If not, can you please guide me with the steps to do the same. Thanks
Adding some additional details on the steps tried so far:

  1. Updated the dependencies to latest versions of cornerstonejs
    "@cornerstonejs/core": "^0.46.1",
    "@cornerstonejs/tools": "^0.67.1",
  2. Ran 'yarn install'
  3. The code changes done as part of this feature is now available under extensions\cornerstone\node_modules@cornerstonejs\tools\src\tools\annotation\PlanarFreehandROITool.ts

However, this is not getting called when the PlanarFreehandROI tool is used and no stats are displayed in OHIF Viewport. Any pointers on how to invoke this when PlanarFreehandROI tool is used in OHIF would be great. Thanks

@rghgit1 , you should set calculateStats option in the configuration.

Thanks a ton. I had set calculateStats option to true, but was not at the right place. After setting calculateStats to 'true' in toolsConfig for PlanarFreehandROI, stats are getting displayed.

@rghgit1
Copy link

rghgit1 commented May 18, 2023

For certain scans, only Area is getting calculated. Max is shown as Infinity. Mean and Std Dev are not displayed. Is this a knwon issue?

image

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.

None yet

7 participants