feat: Add range support for key image tool #1743
Conversation
|
|
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
wayfarer3130
changed the title
wayfarer3130
changed the title
packages/core/src/types/IViewport.ts
Outdated
| /** | ||
| * The end index - this requires sliceIndex to be specified. | ||
| */ | ||
| sliceRangeEnd?: number; |
There was a problem hiding this comment.
does it makes sense to have
sliceIndex?: number -> for single index
sliceRange?: [number, number] -> for range
There was a problem hiding this comment.
No, I'm trying to get away from sliceIndex at all since it really isn't deterministic between different display sets contain the same set of images. I would have liked to get rid of it in this PR, but that was more of an API change than I was willing to contemplate here, whereas the simple change to have two different values instead of number | [number,number] is much more internally consistent with the current API, even though it is a small change.
packages/core/src/types/IViewport.ts
Outdated
| * where the sliceIndex is the first point. This is a positive value if defined, | ||
| * so can be tested for `sliceRangeEnd`. | ||
| */ | ||
| sliceRangeEnd?: number; |
There was a problem hiding this comment.
I'd rather have a separate numberr here - this one will be left as indices as it actually refers to specific slices in a consistent fashion, but even with that, the use of sliceIndex is consistent between single and ranges, so I think I'd like to leave it as a separate value.
There was a problem hiding this comment.
As discussed we will move off number and do a referencedEndId or something
packages/core/src/types/IViewport.ts
Outdated
| * An internal URI version of hte referencedImageId, used for performance | ||
| * while checking the referenced image id. | ||
| */ | ||
| referencedImageUri?: string; |
There was a problem hiding this comment.
we have FrameOfReferenceUID above, so is this referencedImageURI ?
I don't see anywhere else in the code we have referencedImageUri but i see 16 occurences of referencedImageURI
There was a problem hiding this comment.
Updated the docs to explain it - there is no relationship to FrameOfReferenceUID
There was a problem hiding this comment.
we don't have referencedImageUri at all in the codebase and have 16 referencedImageURI already, that is my point
| return false; | ||
| } | ||
| const { |
There was a problem hiding this comment.
We should start writing tests for the isReferenceViewable feature. Consider adding some for this PR; if not, we should do it in the future.
Done according to PR review comments
|
can you rename endRangeImageURI to |
Context
Key images can be used to identify several different things within a series. They might identify specific points as a Fiducial, or they might be a series label or a range, or a selection of several images within the same series. This change adds the ability to select various parts of a series within an annotation.
Also, the performance of the isReferenceViewable was very poor and the results were inconsistent. This PR addresses the performance by adding a map of image id to location and caching additional test locations where needed.
The API is nearly the same - there are a couple of small internal changes in the view references, but unless there are previously stored view references with ranges, those should not affect this PR.
The range references are not supported yet for volume viewports, but the existing view references are supported.
Changes & Results
Testing
Run the stackRange and videoRange examples
Note the consistsency between range value settings between both viewport types.
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-apito 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