454 refactored Encoding/Decoding logic to expose DICOM encoding mappings for consumption#455
Conversation
…t singleVRs array.
…the same logic can be used elsewhere and hopefully it is clearer. Functionality should remain the same.
… makes more sense for it to be there.
…used by BufferStream but also can be used elsewhere. Added export of new DicomBufferCODEC class.
✅ Deploy Preview for dcmjs2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@pieper I went ahead and did some cleanup based on your comments in #454. In terms of the other issue (#373), I do not do anything here other than change the location of the logic in question which I partially move lower in the abstraction hierarchy. However, if I get some time in the near future and no one has addressed it, I might take a stab. I think that the reasonable approach is to find the specific character set at the level of the tree in the header and use that for that level only. That can get a bit complex than I have time at the moment and I will need to consult the standard for other potential cases. |
|
From a quick look I think the PR looks good 👍 I'd like another set of eyes on it though, so hopefully @wayfarer3130 can take a look too. |
|
@wayfarer3130 checking in if there is anything you think I should improve on in this PR. No rush, I know you have been very helpful in other PRs and very busy. I am just helping keep this PR in people's minds in case there is a chance to merge. Thank you! |
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…rts. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…y changing the stream decoder. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
… sure it is clear that regardless of encoding we reset it to utf-8 after setting up the internal buffer stream for decoding. chore added unit tests to make sure future changes to this logic clearly shows that we expect one encoding. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…er and decoder sides use the same default encoding. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
… to .has() since constants were converted to Sets, and cleaned up some vars into lets and consts. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
… less confusing name to this class. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
… previous change. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…rived buffer classes. I also added a warning to let users know that passing a boolean instead of including littleEndian in the options object is a mistake and perhaps unintended. Finally, updated the tests to properly use the ReaderStreamBuffer class. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…mBuilder defaults to BigEndian by default. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…ccept an options object only. Refactored every usage of the WriteStreamBuffer Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
… to properly build the instance of this class. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
… refactor of the stream buffer classes. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…es for the BufferStream class specializations. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
|
@pieper and @wayfarer3130, I made the last set of changes requested. While remaining within scope. I kept the changes in this pass strictly to what was requested. I went ahead and opened issues #480 (attempting to address items that I found during VR handling) and #484 (to remind me to refactor the handling of character set switching per our discussion). Let me know if the current changes work for everyone. Thank you for your time here! |
|
Looks like a couple remaining check issues. I'm thinking someone should try building OHIF or other projects off this branch for some testing. I wonder if we should add that as a build check on PRs to get a netlify hosted test version or similar. |
I was going to do that after the merge, but you are right. I will do that now instead in my OHIF PR branch to see if there are issues I will need to address. |
There is a change just being worked on right now in CS3D that allows building OHIF previews/running tests and running the OHIF PR tests with a just built CS3D version - it would be very nice to have the same sort of thing. The OHIF side works well now, but the CS3D side isn't quite there yet. |
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
|
A few updates from my side:
|
…file and turned it into a Map. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
…adjusted its usage in normalizers. Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
|
Assuming I did not miss an unused import, all that is left is for me to figure out how to make OHIF accept my branch in package.json. |
|
Nvm, I figured it out. I needed to import from my local project directory instead from github because the repo lacks a copy of the built bundle by default. Oops. I am currently testing the viewer using my built copy of dcmjs. |
|
Tests done with my OHIF branch pointing to this dcmjs branch:
The Cypress tests showed an error. This error was present regardless of which version of 
My conclusion is that the dcmjs changes here do not affect the OHIF side. Do try yourselves in case I missed a way to force bugs out of this dcmjs repo from the OHIF Viewer. |
Signed-off-by: Luis M. Santos <lsantos@medicalmasses.com>
|
@igoroctaviano do you want to take a look at this - mostly to make sure this does not negatively affect functionality important for IDC? |
From static analysis alone of the code, for dicom-microscopy-viewer, ohif fork and slim looks like this does not introduce compatibility problems. Residual risk is behavioral, like string decoding for certain character sets, but I ran slim/dmv/ohif linked here, and it seems fine.. |
|
@pieper Just a small poke to ask if there is anything else needed to merge this change. |
|
Sorry for the delays @luissantosHCIT - I've become a bit gunshy in merging. This one's a lot to parse, especially with all the formatting and other small changes that make it hard to see the big picture. I kind of wish those were a separate PR but that may just be extra work at this point. Looks like it's now got a conflict with the main branch. Assuming that gets resolved it sounds like Igor is okay so maybe @wayfarer3130 can chime in if all his issues got resolved. |
I completely understand. This is a foundational project that affects many people! Let me see what the conflict is and resolve it for you. I am just making sure that the PR does not get forgotten if it can be merged since at some point I might just forget about it as well. Haha. |
|
Looks like in this PR I had moved the VR constants to their own location for usage in multiple sources but main had the pre-PR change. I kept my change here since we handle the refactor here. This reminds me of the VR refactor/clean up I was trying to do a while back and which I forked into its own ticket. I am currently working on optimizing my Rust-based HL7 interface toolkit so I will not get to finish that work (#480) for some time now. If it sits without progress for a year, feel free to ask me to close it or you can close it yourself at that point. I agree about keeping the PRs simpler moving forward. I prefer to reach "checkpoints" in a complex objective myself. I will try to improve on where to draw the line on changes and break the PRs further. |
5 participants
Fixes #454.
Although, I did not find any bugs with dcmjs, some of the tooling in the library could be better exposed for consumption in other projects. I focus on exposing the DICOM buffer decoding here given an initial
SpecificCharacterSet. If translation is not necessary, a few convenience methods are provided to set the correct encoder or decoder on demand.This is a code cleanup / reorganization kind of PR.
Jest tests passed.
I did not add any new tests since there is coverage from the current tests and the one class I added is trivial.