-
Notifications
You must be signed in to change notification settings - Fork 115
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
fix: check instance's NumberOfFrames property to see if it is a multi-frame file or not #320
Conversation
… is a multi-frame file or not
There was a problem hiding this 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?
About the code auto-formatting, I used the VSCode and its recommended extensions (prettier). I even tried to use git's "Line staging/unstaging" to isolate the changes, but the auto-formatting came back. Maybe the file was an obsolete one and was not formatted after the formatter extension was added. |
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.
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? |
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 @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 |
@sedghi it seems like the adapter depends on |
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. |
🎉 This PR is included in version 0.28.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.