-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(SR): SCOORD3D point annotations support for stack viewport #3857
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for ohif-platform-docs canceled.
|
✅ Deploy Preview for ohif-dev canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3857 +/- ##
=======================================
Coverage 44.44% 44.44%
=======================================
Files 80 80
Lines 1332 1332
Branches 327 327
=======================================
Hits 592 592
Misses 587 587
Partials 153 153 Continue to review full report in Codecov by Sentry.
|
@sedghi can you take a look at these changes and let me know what you think? This allows the rendering of scoord3d points by FOR. There's another PR here that is related: cornerstonejs/cornerstone3D#950 |
Passing run #4004 ↗︎
Details:
Review all test suite changes for PR #3857 ↗︎ |
…oint-annotations-support
…oint-annotations-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this is not in the right direction I think. Here are my thoughts.
Based on the requirement, it seems like SCCORD3D should be rendered in any viewport that are in the same FOR and looking at the nearby slice that the annotation is drawn on.
I believe that is the definition of our volume viewports. You can look at the tmtv which has volume viewports. https://viewer-dev.ohif.org/tmtv?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.7009.2403.334240657131972136850343327463
As seen the drawn annotation on the CT is rendering on PT and fusion because of the design of volume viewports (to show annotations if they are nearby, as opposed to stack viewports which needs referencedImageIds).
So I believe the cornerstony way of implementing this is to
- Import SCCORD3D like other regular SCCORDs that we have support
- Convert the referenced series into a volume viewport
- Force all other viewports that are in the same FOR and share the same normal into volume viewport if mounted
- Done
@wayfarer3130 what do you think
@sedghi I think it might be quite beneficial to support these annotations in the stack viewport. If you have a point that is not on the slice plane, you will have to interpolate to the point position. In the acquisitions that are used for prostate MRI you will be dealing with very anisotropic voxel sizes (0.5x0.5x4mm), so if you interpolate, you can get artifacts. And also I think it is very important for clinical workflow to see the point on the original slices. I think there is significant value in being able to project the point to the closest slice and show it in the stack viewport, with a disclaimer to the user that the point was projected. What do you think? |
@fedorov To see the point on the original slices we should use Stack Viewport, but the issue/complexity here is that that annotation needs to be viewed in other series in the same FOR and in the same Slice I guess (no?), how do you imagine handling those without some projection? I don't think we interpolate for volume viewports the logic is: if the annotation is within the same slice thickness of any viewport, that viewport will show it import { vec3 } from 'gl-matrix';
import { CONSTANTS, metaData } from '@cornerstonejs/core';
import type { Types } from '@cornerstonejs/core';
import { Annotations, Annotation } from '../../types';
const { EPSILON } = CONSTANTS;
const PARALLEL_THRESHOLD = 1 - EPSILON;
/**
* given some `Annotations`, and the slice defined by the camera's normal
* direction and the spacing in the normal, filter the `Annotations` which
* is within the slice.
*
* @param annotations - Annotations
* @param camera - The camera
* @param spacingInNormalDirection - The spacing in the normal direction
* @returns The filtered `Annotations`.
*/
export default function filterAnnotationsWithinSlice(
annotations: Annotations,
camera: Types.ICamera,
spacingInNormalDirection: number
): Annotations {
const { viewPlaneNormal } = camera;
// The reason we use parallel normals instead of actual orientation is that
// flipped action is done through camera API, so we can't rely on the
// orientation (viewplaneNormal and viewUp) since even the same image and
// same slice if flipped will have different orientation, but still rendering
// the same slice. Instead, we choose to use the parallel normals to filter
// the annotations and later we fine tune it with the annotation within slice
// logic down below.
const annotationsWithParallelNormals = annotations.filter(
(td: Annotation) => {
let annotationViewPlaneNormal = td.metadata.viewPlaneNormal;
if (!annotationViewPlaneNormal) {
// This code is run to set the annotation view plane normal
// for historical data which was saved without the normal.
const { referencedImageId } = td.metadata;
const { imageOrientationPatient } = metaData.get(
'imagePlaneModule',
referencedImageId
);
const rowCosineVec = vec3.fromValues(
imageOrientationPatient[0],
imageOrientationPatient[1],
imageOrientationPatient[2]
);
const colCosineVec = vec3.fromValues(
imageOrientationPatient[3],
imageOrientationPatient[4],
imageOrientationPatient[5]
);
annotationViewPlaneNormal = vec3.create() as Types.Point3;
vec3.cross(annotationViewPlaneNormal, rowCosineVec, colCosineVec);
td.metadata.viewPlaneNormal = annotationViewPlaneNormal;
}
const isParallel =
Math.abs(vec3.dot(viewPlaneNormal, annotationViewPlaneNormal)) >
PARALLEL_THRESHOLD;
return annotationViewPlaneNormal && isParallel;
}
);
// No in plane annotations.
if (!annotationsWithParallelNormals.length) {
return [];
}
// Annotation should be within the slice, which means that it should be between
// camera's focalPoint +/- spacingInNormalDirection.
const halfSpacingInNormalDirection = spacingInNormalDirection / 2;
const { focalPoint } = camera;
const annotationsWithinSlice = [];
for (const annotation of annotationsWithParallelNormals) {
const data = annotation.data;
const point = data.handles.points[0];
if (!annotation.isVisible) {
continue;
}
// A = point
// B = focal point
// P = normal
// B-A dot P => Distance in the view direction.
// this should be less than half the slice distance.
const dir = vec3.create();
vec3.sub(dir, focalPoint, point);
const dot = vec3.dot(dir, viewPlaneNormal);
if (Math.abs(dot) < halfSpacingInNormalDirection) {
annotationsWithinSlice.push(annotation);
}
}
return annotationsWithinSlice;
} |
The idea is to project, and indicate to the user that the point is not the original one, but has been projected.
I do not understand the above. "If the annotation is within the same slice thickness of any viewport" - what does this mean? |
maybe we can meet and discuss? |
…oint-annotations-support
…oint-annotations-support
…oint-annotations-support
…ohif/Viewers into feat/scoord3d-point-annotations-support
@@ -209,10 +209,17 @@ function OHIFCornerstoneSRViewport(props: withAppTypes) { | |||
onElementEnabled(evt); | |||
}} | |||
initialImageIndex={initialImageIndex} | |||
isJumpToMeasurementDisabled={true} | |||
isJumpToMeasurementDisabled={false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a comment explaining why this should be false? Why don't we want to enable jump in the SR viewport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jump was disabled. I just enabled it.
|
||
const EPSILON = 1e-4; | ||
|
||
function getRenderableData(GraphicType, GraphicData, ValueType, imageId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this just a refactor or should i review this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor + adding scoord3d conditional
@@ -0,0 +1,18 @@ | |||
import { metaData } from '@cornerstonejs/core'; | |||
|
|||
export default function getSOPInstanceAttributes(imageId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you export this from the @ohif/cornerstone
extension and then import it here? This seems to be a duplication.
import { adaptersSR } from '@cornerstonejs/adapters'; | ||
|
||
const { CodeScheme: Cornerstone3DCodeScheme } = adaptersSR.Cornerstone3D; | ||
|
||
export const CodeNameCodeSequenceValues = { | ||
ImagingMeasurementReport: '126000', | ||
ImageLibrary: '111028', | ||
ImagingMeasurements: '126010', | ||
MeasurementGroup: '125007', | ||
ImageLibraryGroup: '126200', | ||
TrackingUniqueIdentifier: '112040', | ||
TrackingIdentifier: '112039', | ||
Finding: '121071', | ||
FindingSite: 'G-C0E3', // SRT | ||
FindingSiteSCT: '363698007', // SCT | ||
CornerstoneFreeText: Cornerstone3DCodeScheme.codeValues.CORNERSTONEFREETEXT, | ||
}; | ||
|
||
export const CodingSchemeDesignators = { | ||
SRT: 'SRT', | ||
SCT: 'SCT', | ||
CornerstoneCodeSchemes: [Cornerstone3DCodeScheme.CodingSchemeDesignator, 'CST4'], | ||
}; | ||
|
||
export const RELATIONSHIP_TYPE = { | ||
INFERRED_FROM: 'INFERRED FROM', | ||
CONTAINS: 'CONTAINS', | ||
}; | ||
|
||
export const CORNERSTONE_FREETEXT_CODE_VALUE = 'CORNERSTONEFREETEXT'; | ||
|
||
const enums = { | ||
CodeNameCodeSequenceValues, | ||
CodingSchemeDesignators, | ||
RELATIONSHIP_TYPE, | ||
CORNERSTONE_FREETEXT_CODE_VALUE, | ||
}; | ||
|
||
export default enums; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to move these into adapters long-term?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are SR values so I think they could live in the SR ext.
measurementService.addMapping( | ||
source, | ||
toolNames.DICOMSRDisplay, | ||
[ | ||
{ | ||
valueType: MeasurementService.VALUE_TYPES.POINT, | ||
points: 1, | ||
}, | ||
], | ||
DICOMSRDisplayPoint.toAnnotation, | ||
csToolsAnnotation => DICOMSRDisplayPoint.toMeasurement(csToolsAnnotation, displaySetService) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems like you are adding the DICOMSRDisplay
to be inserted in the measurementService
so that we can map to it? Am I understanding this correctly? So that in the Cornerstone viewport, we can use it to display the point?
I'm trying to understand, for instance, for the length tool that is coming from SR, what would happen after hydration?
I mean, can't we just keep the DICOMSRDisplay
tool for the SR viewport and then hydrate the measurement, and you create an SCCORD3DPoint
tool to display those in the OHIFCornerstoneViewport
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use DICOMSRDisplay tool since it knows how to display all value types?
*/ | ||
const findImageIdIndexFromMeasurementByFOR = (imageIds, measurement) => { | ||
let imageIdIndex = -1; | ||
measurement.metadata.coords.forEach(coord => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a special measurement since it has coordinates. Can we call it SRMeasurement
then?
const addReferencedSOPSequenceByFOR = (measurements, displaySet) => { | ||
if (displaySet instanceof ImageSet) { | ||
measurements.forEach(measurement => { | ||
measurement.coords.forEach(coord => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a duplication of code in the other file, right?
Context
Former PRs:
#planar
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment