Open sedghi opened 1 year ago
Would the rafactor of dicom-image-loader
be part of this version?
Since this version is mainly trying to catch up with the modern DX, I'd love to see that the web worker files can be loaded in a more general way, which could makes it possible to be bundled by other modern bundlers like vite
. By saying a more general way, I mean the loading path should be implemented with import.meta.url
or something like this.
For now I believe users have to manually copy all the worker files from within the dicom-image-loader
package into somewhere their bundlers can read and put the files into a path that the dicom-image-loader
code can read and load, which is exactly is what you suggested here. Though it definitely works, I think it could be better.
This is what almost 'blocks' me from using this great library the first time, with a project set up by vite
.
Thanks for your comment @shunia
Yes, it would be ideally what you are recommending here. Although I have not tried cornerstone3D with vite
. Do you have a simple repo with cs3d and default setup of the vite
so that I try to make sure in 2.x it is resolved?
Thanks for your comment @shunia Yes, it would be ideally what you are recommending here. Although I have not tried cornerstone3D with
vite
. Do you have a simple repo with cs3d and default setup of thevite
so that I try to make sure in 2.x it is resolved?
I would love to and did try to create a repo for this, but after some digging I find it becoming harder to implement a simple viewer now because of the API change, you have to create an image loader or the viewport will just not work and complains with errors.
If you are interested in how to setup a basic vite
project, I think you can simply follow the command:
npm create vite@latest
And because of the failed experience, I believe the 'image loader' concept should be improved too. I don't have enough time to dig deeper now, but I think the signature of registerImageLoader
function is quite intuitive: the second parameter with type ImageLoaderFn
is not documented well, and even after reading some of the source codes I still have no clue what should be returned for the promise
since it's a Record<string, any>
type.
I hoped the custom image loader how-to-guide adds some guidance on how to write your loaders
https://www.cornerstonejs.org/docs/how-to-guides/custom-image-loader
DONE
imageCoordinate -> worldCoordinate
function getDataInTime( dynamicVolume: Types.IDynamicImageVolume, options: { frameNumbers?; maskVolumeId?; imageCoordinate?; } ): number[] | number[][] {~~
DONE
I think there is an inconsistency with how a volume or stack triggers the NEW_VOLUME or NEW_STACK events. In BaseVolumeViewport.ts, the VOLUME_VIEWPORT_NEW_VOLUME event is triggered on the viewport element whereas in StackViewport.ts, the STACK_VIEWPORT_NEW_STACK event is triggered on the cornerstone eventTarget. Is this intentional? Relevant lines below: https://github.com/cornerstonejs/cornerstone3D/blob/c527923a8a1afd65e76f9335ac4cc0bc3a0e924b/packages/core/src/RenderingEngine/BaseVolumeViewport.ts#L779 https://github.com/cornerstonejs/cornerstone3D/blob/c527923a8a1afd65e76f9335ac4cc0bc3a0e924b/packages/core/src/RenderingEngine/StackViewport.ts#[β¦]2
I hoped the custom image loader how-to-guide adds some guidance on how to write your loaders
https://www.cornerstonejs.org/docs/how-to-guides/custom-image-loader
I believe the most important part is omitted in the example code:
// Request succeeded, Create an image object (logic omitted)
const image = createImageObject(oReq.response);
What is the actual return type?
DONE
camera rotation should be on the camera not properties
remove STACK_NEW_IMAGE and VOLUME_NEW_IMAGE after merging the NEW_IMAGE
DONE
getCalibratedLengthUnits and getCalibratedAreaUnits arugment order
DONE
resetCamera(resetPan, ....)
decache, convertToImages etc. in streaming
DONE
referenceId vs referencedId vs referencedImageId vs referenceImageId
DONE
export function triggerAnnotationRenderForViewportIds( renderingEngine: Types.IRenderingEngine, viewportIdsToRender: string[] ): void {
Should be rendering engine Id
DONE
VOLUME_SCROLL_OUT_OF_BOUNDS is VOLUME_VIEWPORT_SCROLL_OUT_OF_BOUNDS
DONE
createAndCacheVolume might be better to name createAndCacheEmptyVolume
getSegmentationRepresentationByUID requires toolGroupId which does not makes sense
getAllSegmentationRepresentations should be array instead of object
drawpath vs drawpolyline
Increase line width to 100, OHIF dev experience is much better
setSegmentationRepresentationSpecificConfig
should not rely on toolGroupId as UIDs should be uids
drawRect, drawRectByCoordinates,
I can see the types for dicomImageLoader is missing. I've been trying to fix it on main branch but I ended up with issue with @cornerstonejs/core
and other types used in the image loader. As you are using monorepo I would expect to use composite
tsc feature but thats not the case. I'm happy to contribute if necessary as the image loader is a huge part of our project. https://www.typescriptlang.org/tsconfig/#composite
@kresli I know it is a pain, we will work on it soon hopefully
We are not following the lps definition in our slices, volview does it correctly
Update the volume API to use single slices on browser side and combined slice data on volume side so that the memory is shared between single frames and multiple frames and large volume allocation works. This should allow 2 memory regions on the webGL side to be treated as a single volume for rendering so that larger volumes can be created. This would involve removing all the destination volume code where the offset to store to is included in the imageio, and the imageio would no longer need to use shared memory.
We might want to consider the API for resizing of windows - there is CSS which automatically selects the right aspect ratio, and will allow resizing the window using the browser native resize so it is really smooth, followed by automatic updates. Unfortunately, it changes the API for the element containing the viewport a tiny bit - the old ones work, but need to be fixed just a bit to work more smoothly, but the new element styling is NOT backwards compatible.
DONE
We should enable the prescale always to true
We need to add viewGroups which are different than toolGroups
Optional installs
Changing the naming of the -> change everything to Units
AreaUnits Units -> to remove this or LengthUnits ModalityUnits -> PixelUnits (in PT it can be scaled or not, so it is really PixelUnits)
display are should be one to one always
Cornerstone3D beta distribution tag is now live in npm π, and the beta branch will publish to it.
We are excited π to open this RFC for version 2.x of Cornerstone3D. Our vision for these changes includes:
Removal of cjs in favor of esm and umd: we don't really need all three π§Ή.Dropping ES5 support and targeting newer ES2022: Aligning with the latest standards π.Upgrade of TypeScript to 5.x: Enhancing the type safety and developer experience π».Rework of the web workers with comlink: Modernizing this critical aspect for better performance and maintainability π οΈ.These changes are intended to make Cornerstone3D more modern and efficient. However, we recognize that they might also lead to breaking changes, particularly in the API π¬.
If there is any part of the API that doesn't make sense to you, or if you have a better idea that you couldn't propose before due to it being a breaking change, now is the time β°. We value your input, and we're eager to hear your thoughts π.
How to Propose Changes: