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(measurement): Add calibration type labels (ERMF, PROJ, USER) #638

Merged
merged 43 commits into from
Jul 21, 2023

Conversation

wayfarer3130
Copy link
Collaborator

@wayfarer3130 wayfarer3130 commented Jun 2, 2023

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

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • [] "Browser:

@netlify
Copy link

netlify bot commented Jun 2, 2023

Deploy Preview for cornerstone-3d-docs canceled.

Name Link
🔨 Latest commit e412d6c
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/64bad7960674b100086bf875

@wayfarer3130 wayfarer3130 changed the title [WIP] feat(measurement): Add calibration type labels (ERMF, PROJ, USER) feat(measurement): Add calibration type labels (ERMF, PROJ, USER) Jun 12, 2023
@wayfarer3130 wayfarer3130 requested a review from sedghi June 12, 2023 17:06
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.

Looks good, see my comments please

@wayfarer3130 wayfarer3130 requested a review from sedghi June 13, 2023 19:23
@sedghi
Copy link
Member

sedghi commented Jun 13, 2023

seems like you need to do the api build again

@wayfarer3130 wayfarer3130 requested a review from jbocce June 27, 2023 13:50
Copy link
Contributor

@jbocce jbocce left a 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.

packages/tools/examples/stackAnnotationTools/index.ts Outdated Show resolved Hide resolved
packages/tools/src/utilities/calibratedLengthUnits.ts Outdated Show resolved Hide resolved
packages/tools/src/utilities/calibratedLengthUnits.ts Outdated Show resolved Hide resolved
packages/tools/src/utilities/calibratedLengthUnits.ts Outdated Show resolved Hide resolved
packages/tools/src/utilities/roundNumber.ts Show resolved Hide resolved
packages/tools/src/utilities/index.ts Outdated Show resolved Hide resolved
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.

Looks much better, thanks for updating the PR. I have one major request

  1. 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)

  2. the aspect ratio 1:1 breaks the annotation probably a bug

@wayfarer3130 wayfarer3130 requested a review from sedghi July 10, 2023 17:04
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.

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
Copy link
Collaborator Author

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?

Copy link
Member

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;
Copy link
Member

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.

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 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:

  1. Render the image in the specified aspect ratio
  2. Add an SVG transform to apply the aspect ratio as well as pan/zoom settings

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.

looks good, can you respond to my remaining comment only?

@wayfarer3130
Copy link
Collaborator Author

looks good, can you respond to my remaining comment only?

I pushed an update to deal with that.

@sedghi
Copy link
Member

sedghi commented Jul 21, 2023

can you run yarn run build:update-api? and include them? https://www.cornerstonejs.org/docs/contribute/update-api

@wayfarer3130 wayfarer3130 requested a review from sedghi July 21, 2023 19:19
@sedghi sedghi merged commit 0aafbc2 into main Jul 21, 2023
5 checks passed
@wayfarer3130 wayfarer3130 deleted the feat/calibration-ermf-display branch August 8, 2023 11:12
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.

3 participants