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.

Sign up for GitHub

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

initial commit for fixing video segmentations #1828

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

initial commit for fixing video segmentations #1828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6339cf8
initial commit for fixing video segmentations
Feb 14, 2025
8355ad4
revert previous changes
minimal-scouser Mar 4, 2025
ae3007a
Merge branch 'main' of github.com:cornerstonejs/cornerstone3D into bu…
minimal-scouser Mar 4, 2025
67e112e
add index as an argument to getDerivedImageId
minimal-scouser Mar 4, 2025
f19cfb4
fix breaking change when trying to remove actor - esp for video viewport
minimal-scouser Mar 4, 2025
959ac0f
use appropriate method for setting image data to a video viewport actor
minimal-scouser Mar 4, 2025
410eaa9
append frames to derivedImageId to comply with video viewport
minimal-scouser Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/core/src/RenderingEngine/Viewport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,8 @@ class Viewport {
return;
}
const renderer = this.getRenderer();
renderer.removeViewProp(actorEntry.actor as vtkProp); // removeActor not implemented in vtk?
// the optional chaining is particularly for video viewports
renderer?.removeViewProp(actorEntry.actor as vtkProp); // removeActor not implemented in vtk?
this._actors.delete(actorUID);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/loaders/imageLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ export function createAndCacheDerivedImage(
export function createAndCacheDerivedImages(
referencedImageIds: string[],
options: DerivedImageOptions & {
getDerivedImageId?: (referencedImageId: string) => string;
getDerivedImageId?: (referencedImageId: string, index: number) => string;
targetBuffer?: {
type: PixelDataTypedArrayString;
};
Expand All @@ -356,7 +356,7 @@ export function createAndCacheDerivedImages(
const images = referencedImageIds.map((referencedImageId, index) => {
const newOptions: DerivedImageOptions = {
imageId:
options?.getDerivedImageId?.(referencedImageId) ||
options?.getDerivedImageId?.(referencedImageId, index) ||
`derived:${uuidv4()}`,
...options,
};
Expand Down
7 changes: 5 additions & 2 deletions packages/tools/examples/videoSegmentation/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,14 @@ async function run() {
// We need the map on all image ids
const allImageIds = viewport.getImageIds();
const firstImage = allImageIds[0];
const segImages = await imageLoader.createAndCacheDerivedImages(
[firstImage],
const segImages = await imageLoader.createAndCacheDerivedLabelmapImages(
allImageIds,
{
skipCreateBuffer: true,
onCacheAdd: csUtils.VoxelManager.addInstanceToImage,
getDerivedImageId: (referenceImageId, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the multiframe utilities to update the frame reference.
Also, there is no need to use a random uid, you can use the original uid with derived in front of it, but updating the frame index so that every time you generate the same uids.
Finally, this derivation should probably go into the core code so that the handling is consistent and can be done from things like OHIF.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sedghi - should we add a vewport call 'getDerivedImageIds' It could take a range request to allow getting a sub-range of it.

Copy link
Contributor Author

@minimal-scouser minimal-scouser Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

cornerstone3D/packages/core/src/loaders/imageLoader.ts

Lines 340 to 371 in 0b67f91

export function createAndCacheDerivedImages(
referencedImageIds: string[],
options: DerivedImageOptions & {
getDerivedImageId?: (referencedImageId: string) => string;
targetBuffer?: {
type: PixelDataTypedArrayString;
};
voxelRepresentation?: VoxelManagerEnum;
} = {}
): IImage[] {
if (referencedImageIds.length === 0) {
throw new Error(
'createAndCacheDerivedImages: parameter imageIds must be list of image Ids'
);
}
const derivedImageIds = [];
const images = referencedImageIds.map((referencedImageId, index) => {
const newOptions: DerivedImageOptions = {
imageId:
options?.getDerivedImageId?.(referencedImageId) ||
`derived:${uuidv4()}`,
...options,
};
derivedImageIds.push(newOptions.imageId);
return createAndCacheDerivedImage(referencedImageId, {
...newOptions,
instanceNumber: index + 1,
});
});
return images;
}

line 360 is creating a derivedImageId using a random uuid anyway if we don't implement getDerivedImageId. so i am little confused.

here's a trace:
createAndCacheDerivedLabelmapImages
createAndCacheDerivedImages
line 360

hene i assumed i was not doing anything drastically different reg the creation of a derivedImageId. i thought getDerivedImageId lets the user create a derviedImageId in a format they want, which is why i used it to append frames to the id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wayfarer3130

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you at least move the create derived images into a specialized method in imageLoaders.ts that takes a type argument that can be video/stack/volume/wsi and returns the right type of response? It should take the same type of argument as the rendering engine create does. That way the same code can be re-used more generally, and we can update it to use non-random uids if we choose, or continue as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes more sense. on it.

return `derived:${cornerstone.utilities.uuidv4()}/frames/${index + 1}`;
},
}
);

Expand Down
7 changes: 6 additions & 1 deletion packages/tools/src/eventListeners/segmentation/imageChangeEventListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
getEnabledElementByIds,
cache,
utilities,
VideoViewport,
} from '@cornerstonejs/core';
import { triggerSegmentationRender } from '../../stateManagement/segmentation/SegmentationRenderingEngine';
import { updateLabelmapSegmentationImageReferences } from '../../stateManagement/segmentation/updateLabelmapSegmentationImageReferences';
Expand Down Expand Up @@ -203,7 +204,11 @@ function _imageChangeEventListener(evt) {
imageId: derivedImageId,
representationUID: `${segmentationId}-${SegmentationRepresentations.Labelmap}`,
callback: ({ imageActor }) => {
imageActor.getMapper().setInputData(imageData);
if (viewport instanceof VideoViewport) {
imageActor.getMapper().getInputData().setDerivedImage(imageData);
} else {
imageActor.getMapper().setInputData(imageData);
}
},
},
]);
Expand Down
Loading