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(4D-ROI): ROI Segmentation panel (4D) #3574

Merged

Conversation

lscoder
Copy link
Collaborator

@lscoder lscoder commented Aug 2, 2023

  • Updated ToolGroupServices to make it possible to add tool instances (eg: circular brush/eraser tools are instances of BrushTool)
  • Updated calculateSUVPeak to support 4D datasets since volumes can have multiple arrays of scalarData
  • Added ROI Segmentation panel to 4D mode (started with a copy of that same panel from TMTV)
  • Updated ROI Segmentation panel layout and created a BrushConfiguration component to make it possible to change all brush-related tools settings
  • Added all segmentation tools (circular brush, circular eraser, sphere brush, sphere eraser, threshold brush, rectangle ROI threshold, rectangle scissor, circle scissor, sphere scissor and paint fill)
  • Updated the toolbar to keep all segmentation related tools in a single group
  • Updated thresholdSegmentationByRectangleROITool and calculatedSUVPeak from TMTV to also support Rectangle ROI Threshold (only Rectangle ROI Threshold (start/end) was supported before)
  • Created ToolService to be able to trigger and listen to events (activated tools)
  • Fixed 4D mode HP (PT viewports were using a wrong toolGroup) + a few other changes to make it similar to HP from TMTV
  • Fixed some issues in ViewportWindowLevel related to segmentations

@lscoder lscoder requested a review from sedghi August 2, 2023 06:32
@lscoder lscoder self-assigned this Aug 2, 2023
Comment on lines -303 to -325
toolGroupIds: ['dynamic4D-default'],
// -1 would be used to indicate active only, whereas other values are
// the number of required priors referenced - so 0 means active with
// 0 or more priors.
numberOfPriorsReferenced: 0,
// Default viewport is used to define the viewport when
// additional viewports are added using the layout tool
defaultViewport: {
viewportOptions: {
viewportType: 'volume',
toolGroupId: 'dynamic4D-default',
allowUnmatchedView: true,
initialImageOptions: {
preset: 'middle', // 'first', 'last', 'middle'
},
},
displaySets: [
{
id: 'defaultDisplaySetId',
matchedDisplaySetsIndex: -1,
},
],
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just trying to make it more similar to TMTV

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #3574 (acd14e3) into feat/preclinical-4d-base (7755cb9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           feat/preclinical-4d-base    #3574   +/-   ##
=========================================================
  Coverage                     42.75%   42.75%           
=========================================================
  Files                            82       82           
  Lines                          1450     1450           
  Branches                        338      338           
=========================================================
  Hits                            620      620           
  Misses                          667      667           
  Partials                        163      163           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7755cb9...acd14e3. Read the comment docs.

stageId: 'dataPreparation',
},
},
{
id: 'registration',
name: 'Registration',
hangingProtocol: {
protocolId: 'default4D',
protocolId: preclinical4d.hangingProtocol,
Copy link
Member

Choose a reason for hiding this comment

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

I thought in each step we are applying a new protocol no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. Each step moves to the next HP stage

Copy link
Member

Choose a reason for hiding this comment

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

hmmm this is confusing then and requires the a preexisting knowledge of the hanging protcol and that it should have multiple stages defined. Explicit code is much better in this situation so at least put a stage index or something as the argument

Copy link
Collaborator Author

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 I'm following because all workflow stages are using the same protocolId but each one of them has a different stageId.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind I understand now. Also when did we choose to add workflow to the mode root key? I thought we are using it in the onModeEnter and register the steps via the workflowService no?

Also seems like we have stages in the HP and stages in workflow which is a lot confusing we should think about how to rename the workflow stage to something else. I think we should opt in for WorkflowStep and WorkflowStepService

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed "stages" to "steps" everywhere

@@ -280,7 +277,8 @@ function getFusionViewports() {
}

const defaultProtocol = {
id: 'default4D',
id:
Copy link
Member

Choose a reason for hiding this comment

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

i don't think you should make the full pathname here, if you say 'default' it automatically should create the @ohif/extension-cornerstone-dynamic-volume.hangingProtocolModule.default if i'm not mistaken

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the id when I was comparing it with TMTV HP and I saw that one using @ohif/extension-tmtv.hangingProtocolModule.ptCT as id

Copy link
Member

Choose a reason for hiding this comment

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

does it work if you say default here? if so please do so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works if we set it to default because it would use the default HP from default mode but I've reverted my change renaming the HP back to default4D

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.

thanks, see my comments

@lscoder
Copy link
Collaborator Author

lscoder commented Aug 8, 2023

  • Renamed workflow "stages" to "steps" everywhere
  • ROI Segmentation is available only when user gets to "ROI Quantification" workflow step
  • Fixed issues when updating left/right panels content based on workflow settings or moving across different tabs because components were losing their state (broke WorkflowSteps dropdown, WindowLevels)
  • Updated ROI Segmentation panel: removed TMTV, added a default label to segmentations ("Segmentaion [number]"), fixed the selected segmentation id which was out of sync with SegmentationService
  • Fixed "Rectangle ROI Threshold (start/end)" which was not working on fusion viewports
  • Removed "Threshold" setting from circular and sphere brush tools (only brush size is available)
  • "Rectangle ROI Threshold" now have settings similar to "Rectangle ROI Threshold (start/end)" but without start/end buttons

@lscoder lscoder requested a review from sedghi August 8, 2023 07:35
@sedghi sedghi merged commit 72243fa into OHIF:feat/preclinical-4d-base Aug 8, 2023
3 of 5 checks passed
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.

2 participants