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(segmentation): Refactor segmentation and style handling #1449

Merged
merged 38 commits into from
Sep 12, 2024

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Aug 30, 2024

segmentation representation = segmentation + type + viewport

segmentation style refactored for readability , it is now similar to annotation style

…cedImageId for better overlay matching

	•	Improve VideoViewport rendering by applying device pixel ratio scaling and clearing the canvas before drawing
	•	Update WSIViewport to use getViewReferenceId instead of getReferenceId for consistency
	•	Update MetadataModuleTypes to use numbers for rows, columns, sliceThickness, and sliceLocation
	•	Add preset property to ViewportProperties for future use
	•	Introduce deepClone utility function for deep cloning objects
	•	Update SegmentationStateManager to handle labelmap image references and remove color LUTs when representations are removed
	•	Add null check for image in Cache class to prevent undefined errors
	•	Update imagePlaneModule to use numeric values for rows and columns
	•	Import and use utilities from @cornerstonejs/core in addColorLUT and ToolGroup
	•	Replace deepClone with utilities.deepClone in ToolGroup
	•	Update BrushTool to dispatch a custom error event instead of throwing an error when no active segmentation is detected
Copy link

netlify bot commented Aug 30, 2024

Deploy Preview for cornerstone-3d-docs failed. Why did it fail? →

Name Link
🔨 Latest commit 193bfac
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/66e34b4c7ea6510008f062f1

-  Introduce `setCameraClippingRange` method in `BaseVolumeViewport` and implement it in `VolumeViewport`, `VolumeViewport3D`, and `Viewport` classes
-  Move camera clipping range logic from `setCamera` method to `setCameraClippingRange` method to improve code organization and reusability
-  Update `setCamera` method to call `setCameraClippingRange` method for consistency across classes
…rollTool**

-  Remove VolumeRotateMouseWheelTool from cornerstoneTools
-  Update various examples to use StackScrollTool instead of VolumeRotateMouseWheelTool
-  StackScrollTool now has an additional configuration option to enable rotation
-  The rotation functionality is implemented in the mouseWheelCallback function of StackScrollTool
-  The rotation functionality uses the gl-matrix library for 3D transformations
This commit replaces segmentation with labelmap in various parts of the codebase, including core and tools. It updates function names, types, and logic to reflect the change from segmentation to labelmap.

Changes include:
-  Renaming functions from segmentation to labelmap (e.g., `createAndCacheDerivedSegmentationImage` to `createAndCacheDerivedLabelmapImage`)
-  Updating types and interfaces to reflect the change from segmentation to labelmap
-  Changing logic and implementation to work with labelmaps instead of segmentations
-  Updating tests and examples to reflect the change from segmentation to labelmap
	•	Update Viewport.ts to use BaseVolumeViewport instead of VolumeViewport
	•	Update loaders/index.ts to register cornerstoneStreamingImageVolumeLoader as unknown as VolumeLoaderFn
	•	Update convertLabelmapToSurface.ts to remove isVolume check and handle volumeId directly
	•	Update surfaceComputationStrategies.ts to remove isVolumeSegmentation check
	•	Update addLabelmapToElement.ts to handle missing volume and create a new volume if necessary
	•	Update labelmapDisplay.ts to use BaseVolumeViewport instead of VolumeViewport
	•	Update BrushTool.ts, CircleScissorsTool.ts, PaintFillTool.ts, RectangleScissorsTool.ts, SphereScissorsTool.ts, getStrategyData.ts, and utilities/segmentation/*.ts to use BaseVolumeViewport instead of VolumeViewport
	•	Remove stackVolumeCheck.ts file as it is no longer needed
Update Labelmap tool to handle volume ID correctly and prevent errors when volume ID is missing

-  Update addLabelmapToElement function to set volumeId property on LabelmapSegmentationDataVolume object
-  Update getStrategyData function to check for missing volumeId and dispatch an error event if found
-  Improve error handling and logging for segmentation-related issues
	•	Refactored SegmentationRenderingEngine to handle viewport cases where the enabled element is not found
	•	Simplified SegmentationStateManager to remove redundant imageIdReferenceMap lookups
	•	Added addSegmentationRepresentationUIDToViewport function to associate a segmentation representation with a given viewport
	•	Updated utilities/index.ts to include pointInSurroundingSphereCallback function
	•	Implemented pointInSurroundingSphereCallback function to run a callback for each point of imageData within a sphere defined by great circle points
	•	Updated labelmapDisplay.ts to include removeRepresentation function and _needsTransferFunctionUpdate function
	•	Updated addLabelmapToElement.ts to trigger segmentation modified event when adding a labelmap to an element
…ports

-  Remove segmentation representation from state and all viewports
-  Update segmentation representation removal functions
-  Remove obsolete functions and imports
-  Update segmentation state management and display tools
-  Fix issues with contour, labelmap, and surface display tools
…onality and performance

-  Extracted and refactored common logic in VideoViewport and VolumeViewport3D
-  Improved voxelManager.forEach method for efficient point iteration
-  Added resetCameraForResize method to VolumeViewport3D
-  Enhanced generateVolumePropsFromImageIds utility function
-  Introduced pointInShapeCallback for efficient shape-based point iteration
-  Removed deprecated methods and improved code organization
…e code organization and modularity

-  Extracted internal functions internalComputeVolumeSegmentationFromStack and internalConvertStackToVolumeSegmentation from SegmentationStateManager
-  Moved external functions computeVolumeSegmentationFromStack and convertStackToVolumeSegmentation to separate files
-  Updated imports and exports to reflect new file structure
-  Removed unused function updateSegmentationState and modified internalConvertStackToVolumeSegmentation to directly update segmentation state
…lback

-  Migrate voxelManager to new system in WSIViewport
-  Refactor pointInShapeCallback to support new system
-  Update type imports and usage in pointInShapeCallback
-  Update type imports in labelmapComputationStrategies and convertLabelmapToSurface
-  Initialize globalDefaultProperties with an empty object
-  Update globalDefaultProperties with new values instead of overwriting them
-  Remove unused cameraFocalPointOnRender property
-  Simplify _setPropertiesFromCache method by removing unused variables
-  Update camera event triggering to use the current camera state
-  Remove _restoreCameraProps method as it is no longer needed
-  Refactor _getVOIFromCache method to return the voiRange
-  Update setVOI method to use the returned voiRange
-  Remove console.log statements and unused code
-  Improve code readability and consistency
-  Change boundsIJK to private and add getDefaultBounds method in VoxelManager
-  Add clearSegmentValue function to clear specified segment value from a segmentation
-  Update segmentation helpers and index
-  Update Labelmap display to use perSegment labelmap config
-  Update BrushStrategy to include isInObject and isInObjectBoundsIJK in InitializedOperationData
-  Update various segmentation strategies to use operationData.isInObject and operationData.isInObjectBoundsIJK
-  Extract imageIdToURI function to handle image ID to URI conversion
-  Update imageLoader to store referencedImageId in localImage
-  Simplify imageChangeEventListener and remove unnecessary logic for segmentation actor updates
fix all examples based on new segmentation
segmentIndex: number,
color: Types.Color
): void {
// Get the reference to the color in the colorLUT.
const colorReference = getSegmentIndexColor(
segmentationRepresentationUID,
viewportId,
segmentationId,
segmentIndex
);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the color values are directly modified, when we add color luts, do we ensure we take a full copy so we don't later on modify it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes during add we do

*/
export function setActiveSegmentation(
viewportId: string,
segmentationId: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this include the segmentation type? Also, should it be possible to have 1 active of each type of segmentation? That is, to have both the active labelmap and the active contour.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, in the concept of activeness, only one segmentation (not representation) is active at a time. We can't have the contour of one segmentation active while the labelmap of another segmentation is active.

@@ -765,7 +773,7 @@ export default class ToolGroup {
get(this._toolInstances[toolName].configuration, configurationPath) ||
this._toolInstances[toolName].configuration;

return structuredClone(_configuration);
return utilities.deepClone(_configuration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this one deepClone and not structuredClone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since structuredClone cannot copy functions or classes, our custom deepClone function uses structuredClone for objects while also handling functions and classes.

@@ -67,12 +66,13 @@ const initializeCircle = {
segmentationImageData.getDimensions()
);

segmentationVoxelManager.boundsIJK = boundsIJK;
imageVoxelManager.isInObject = createPointInEllipse({
operationData.isInObject = createPointInEllipse({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a spot where getting the object iterator as a complete options object which includes currently the is in object and the bounds ijk would allow us in the future to define it using more efficient maps, and as long as it is just an instance of the right type, then the type can be extended in the future to allow for improved definitions. I believe this should be part of CS3D 2 as it fixes the types now, and then allows for better future extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have the options for the forEach, i added it to the pointInShape too

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.

A few bug fixes as noted, plus some naming changes a few small improvements are needed, but overall this is a much cleaner design than the CS3D 1 version.

-  Introduce `hasVolumeURI` method to `BaseVolumeViewport` to check if a volume with a given URI exists
-  Update `getViewportsWithImageURI` and `getViewportsWithVolumeId` to use `getRenderingEngines` instead of `getRenderingEngine`
-  Introduce `getViewportsWithVolumeURI` utility
-  Update various tools to use `getTargetImageData` instead of `getTargetIdImage` and remove `renderingEngine` dependency
-  Remove `getTargetVolumeId` method from `BaseTool` and update tools to use `getVolumeId` directly from `viewport` instance
-  Update segmentation tools to use `getTargetImageData` instead of `getTargetIdImage` and remove `renderingEngine` dependency
…ViewportsWithImageURI

-  Remove renderingEngineId from getViewportsWithVolumeId in WindowLevelTool
-  Remove renderingEngineId from getViewportsWithImageURI in ProbeTool
-  Remove renderingEngineId from getViewportsWithVolumeId in ViewportColorbar
* every point in the shape.
* @param boundsIJK - The bounds of the volume in IJK coordinates.
* @param options - Configuration options for the shape callback.
* @returns An array of points in the shape if returnPoints is true, otherwise undefined.
*/
export function pointInShapeCallback(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you - this feels like a better interface.

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.

Thank you for all these changes - I think they are really looking pretty good now

@sedghi sedghi changed the title more fixes feat(segmentation): Refactor segmentation and style handling Sep 12, 2024
@sedghi sedghi merged commit 51f7cde into beta Sep 12, 2024
6 of 10 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