-
Notifications
You must be signed in to change notification settings - Fork 326
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(measurement): Add calibration type labels (ERMF, PROJ, USER) #638
Conversation
✅ Deploy Preview for cornerstone-3d-docs canceled.
|
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.
Looks good, see my comments please
seems like you need to do the api build again |
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.
Just a few things thus far. Still need to look at more, but as you indicated to me in a side conversation, you have some touch-ups for it. So I will wait.
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.
Looks much better, thanks for updating the PR. I have one major request
-
which is we need a dedicated example page for calibration instead of adding it to the stack annotation tools. You dropdown is fine for calibration types, but I want to have a dialog for the user to input something via alert (similar to ohif) or a input field to be able to try various scnearios (going back to original calibration etc)
-
the aspect ratio 1:1 breaks the annotation probably a bug
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.
I'm trying to understand the calibrateImageSpacing
function.
export default function calibrateImageSpacing(
imageId: string,
renderingEngine: Types.IRenderingEngine,
calibrationOrScale: Types.IImageCalibration | number
): void {
it has calibrationOrScale, while the IImageCalibration itself is below
export interface IImageCalibration {
/** The pixel spacing for the image, in mm between pixel centers */
rowPixelSpacing?: number;
columnPixelSpacing?: number;
/** The scaling of this image - new spacing = original pixelSpacing/scale */
scale?: number;
/** The type of the pixel spacing, distinguishing between various
* types projection (CR/DX/MG) spacing and volumetric spacing (the type is
* an empty string as it doesn't get a suffix, but this distinguishes it
* from other types)
*/
type: CalibrationTypes;
/** A tooltip which can be used to explain the calibration information */
tooltip?: string;
/** The DICOM defined ultrasound regions. Used for non-distance spacing units. */
sequenceOfUltrasoundRegions?: Record<string, unknown>[];
}
and it has scale too. It is very confusing to me
*/ | ||
export default function calibrateImageSpacing( | ||
imageId: string, | ||
renderingEngine: Types.IRenderingEngine, | ||
rowPixelSpacing: number, | ||
columnPixelSpacing: number | ||
calibrationOrScale: Types.IImageCalibration | number |
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 intent here is that it is a simple scale value, to agree (almost) with the old API, OR it is a full calibration object. If it is still confusing, could you suggest a naming for it that explains that it is either a calibration object entirely or a scale value used to construct a calibration object?
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.
I mean you are changing the API from row/column pixelSpacing to a single one, so it is changing the API right now. I don't know if it really makes sense to change the API + make it backward compatible
rowScale: number; | ||
columnScale: number; | ||
scale?: number; | ||
calibration?: IImageCalibration; |
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 scale
here live because of legacy implementation? since there is scale inside calibration too.
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.
I'm removing scale, and adding some more documentation.
Also adding an aspect ratio, but the code currently does not handle it effectively. I've figured out how to fix that, but I'm not quite sure yet where the changes would be. The correct way to handle aspect ratio is to:
- Render the image in the specified aspect ratio
- Add an SVG transform to apply the aspect ratio as well as pan/zoom settings
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.
looks good, can you respond to my remaining comment only?
I pushed an update to deal with that. |
can you run |
Context
The user needs to know how the calibration of size on an image is derived, whether it is ERMF or USER projected or none for volumetric measurements. Add the ability to specify a label, and instead of updating measurement positions, provide a scaling capability to scale just the measurements and not the separate positioning of everything.
Changes & Results
Stores the calibration object inside the custom metadata provider
Uses the scale to scale all the measurements appropriately for display, but NOT changing the internal values.
Uses px/mm in the units decision.
Testing
yarn example stackAnnotationTools
Run the various measurements and see how they change with different scaling applied.
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment