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

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

Merged

Conversation

md-prog
Copy link
Contributor

@md-prog md-prog commented Oct 7, 2022

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.

Copy link
Collaborator

@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.

This looks good to me, but there are bunch of style changes that I don't like. @pieper have we done a format commit when we added the prettier config recently?

@sedghi sedghi changed the title #318 - check instance's NumberOfFrames property to see if it is a multi-frame file or not fix: check instance's NumberOfFrames property to see if it is a multi-frame file or not Oct 7, 2022
@md-prog
Copy link
Contributor Author

md-prog commented Oct 7, 2022

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

pieper commented Oct 7, 2022

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

sedghi commented Oct 10, 2022

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

md-prog commented Oct 11, 2022

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

@sedghi
Copy link
Collaborator

sedghi commented Oct 11, 2022

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

md-prog commented Oct 11, 2022

@sedghi Please take a look at #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
Copy link
Contributor Author

md-prog commented Oct 13, 2022

@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.

@pieper pieper merged commit abce232 into dcmjs-org:master Oct 13, 2022
@github-actions
Copy link

🎉 This PR is included in version 0.28.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants