dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
291 stars 110 forks source link

fix: check instance's NumberOfFrames property to see if it is a multi-frame file or not #320

Closed md-prog closed 1 year ago

md-prog commented 1 year ago

Cornerstone3D Adapter - support of multi-frame

It's difficult to add all the SOPClassUIDs that supports multi-frames. So it's unrealistic to rely on checking SOPClassUID for multi-frames.

Here the code is checking Instance's NumberOfFrames attribute (the instance is fetched from metadataProvider.).

Line 286 ~ 292 is the actual change, and other changes are just caused by prettier.

md-prog commented 1 year ago

About the code auto-formatting, I used the VSCode and its recommended extensions (prettier). And it auto-formatted the code on saving the file.

I even tried to use git's "Line staging/unstaging" to isolate the changes, but the auto-formatting came back. I see that we have auto-linting command in package.json as well.

Maybe the file was an obsolete one and was not formatted after the formatter extension was added.

pieper commented 1 year ago

I'm still thinking that listing the SOPClassUIDs that could legally be multiframes is a better strategy, but I admit that checking for the existence of NumberOfFrames is likely to be a good enough proxy for this code.

Maybe the file was an obsolete one and was not formatted after the formatter extension was added.

I don't recall, and a lot of the repo infrastructure was set up by other people.

In any case though, I think the whole concept of having the adaptor code in this repo was a mistake (a mistake I was part of, I'll admit). @sedghi do you think this code belongs in cornerstone itself instead? Or perhaps in a new dcmjs-cornerstone or cornerstone-dcmjs repository?

sedghi commented 1 year ago

Yes, that is certainly something that we are interested in as well. We are thinking of porting the cs3d adapters into our mono repo and be like cornerstonejs/io.

@md-prog Could you please start the process in cs3d and we see what we need to port? It should be a very easy task

md-prog commented 1 year ago

@sedghi it seems like the adapter depends on utilities/TID300/Point which is not exported.

sedghi commented 1 year ago

The adapter have to have dependency on dcmjs, it uses a lot of things from there indeed. Just to be clear, we are discussing to port all the cs3d adapter, so it depends on the MeasurementReport as well and other stuff.

md-prog commented 1 year ago

@sedghi Please take a look at https://github.com/dcmjs-org/dcmjs/pull/323

This is to prepare dcmjs library so that we can successfully separate cornerstone3d adapter into a separate package. Simple change - exporting Point class as public, because the cornerstone3d adapter depends on it.

md-prog commented 1 year ago

@pieper @sedghi Can we merge this for now and publish with new version number? There are downstream updates that are depending on these changes. I will take care of the separation later.

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 0.28.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: