-
Notifications
You must be signed in to change notification settings - Fork 84
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
[pixeldata] Apply VOI LUT when rendering images #599
base: master
Are you sure you want to change the base?
Conversation
woops didn't realize I had broken the gdcm feature, will fix |
I think this will conflict with #598 i just created earlier today |
This does look substantial, thank you for working on this!
That would be greatly appreciated. I wonder if some of the DICOM test files available feature VOI LUT sequences. I also happen to have a test enhanced CT scan which I think features a VOI LUT sequence, but I need to check.
From the quick glance I found nothing egregiously wrong, but I will make a better review when I am able.
In this case there is already some value in the library picking up VOI LUT sequences when the object uses that instead of window levels, so we can start with crate-level visibility and expose it later. |
If it helps create some coordination, we can decide an order of contribution merges so that we know which one should be rebased. |
yup, very likely :-D but I'm happy for you to merge first, I checked your PR and the conflict resolving shouldn't be too hard to do. |
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.
Thank you again for working on this. I will only leave a few comments inline. Overall this seems to be heading in the right direction. Please let me know when the pull request is ready for a more thorough test.
/// | ||
/// See [section C.8.11.3.1.5][1] of the standard for more details. | ||
/// | ||
/// [1]: https://dicom.nema.org/dicom/2013/output/chtml/part03/sect_C.8.html#sect_C.8.11.3.1.5 |
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.
That's a pretty old version of the standard. Search engines are often guilty of not providing pages to the latest versions.
/// [1]: https://dicom.nema.org/dicom/2013/output/chtml/part03/sect_C.8.html#sect_C.8.11.3.1.5 | |
/// [1]: https://dicom.nema.org/medical/dicom/2024d/output/chtml/part03/sect_C.8.11.3.html#sect_C.8.11.3.1.5 |
@@ -642,6 +658,36 @@ impl DecodedPixelData<'_> { | |||
} | |||
} | |||
|
|||
pub fn voi_lut_sequence(&self) -> Result<Option<&[VoiLut]>> { |
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.
Consider adding a simple doc-comment to this public method.
Thanks for the comments! I'll try to take a look next week. I unfortunately haven't found the time yet to finalize the tests - but that's still on my todo 😬 |
Fixes: #232
Tested on couple of images locally, this seems to do the job.
There are couple of items I'd like to address before considering this one ready:
VoiLut
are in the right place...pub(crate)
for now