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

DICOM datasets up-side-down #185

Closed
mlavik1 opened this issue May 2, 2023 · 4 comments · Fixed by #186
Closed

DICOM datasets up-side-down #185

mlavik1 opened this issue May 2, 2023 · 4 comments · Fixed by #186
Labels
bug Something isn't working DICOM

Comments

@mlavik1
Copy link
Owner

mlavik1 commented May 2, 2023

DICOM datasets consistently spawn up-side-down:

I believe it has always been like this. Before #170 they were also mirrrored. Now the coordinate system should be correct, except the rotation is wrong.

image

@mlavik1 mlavik1 added bug Something isn't working DICOM labels May 2, 2023
@mlavik1
Copy link
Owner Author

mlavik1 commented May 2, 2023

if I remove the call to ImporterUtilsInternal.ConvertLPSToUnityCoordinateSpace the dataset will have be in LPS space:

  • z towards head
  • y towards posterior
  • x towards left

image

So the reason they show up-side-down is because of the rotation applied in ImporterUtilsInternal.ConvertLPSToUnityCoordinateSpace.

@SitronX we talked about making sure the rotation stays the same as before the that coordinate space PR, to make sure people get any unpleasant surprises when upgrading.
Though, we probably don't want the head pointing downwards, so I guess it should be fine to change this? I assume your DICOM datasets also spawn up-side-down? (just not mirrored anymore 😁 )

It will only affect newly imported datasets (not those already serialized) anyway.

@SitronX
Copy link
Contributor

SitronX commented May 2, 2023

Hi @mlavik1

I assume your DICOM datasets also spawn up-side-down? (just not mirrored anymore 😁 )

Yes all my dicom, but also NRRD are upside down (i guess Niftis are also upside down due to SimpleITK?).

Though, we probably don't want the head pointing downwards, so I guess it should be fine to change this?

Ye i guess it is fine. I dont know how people use this library, but i guess they do not update their clone or whatever everytime they launch their app. Myself, I downloaded latest version of your library to my project somewhere in January and only manually updated some crucial things since then.

If somebody did use it in their app with ability to update this library automatically i would be actually curious how they have done it. I guess this could be maybe done with Unity Asset store when trying to update the asset thru Unity package manager? Or could the clone of this library be stored also in new Unity project? Would it update correctly by pulling latest commits? Hmm. But this would forbid you to do any changes to library itself due to merge conflicts with updates in the future, so i guess downloading latest version and updating manually is the only option if you wanted to make changes inside it, right? :D

@mlavik1
Copy link
Owner Author

mlavik1 commented May 2, 2023

Yes all my dicom, but also NRRD are upside down (i guess Niftis are also upside down due to SimpleITK?).

Ok, good to hear it's consistent at least :) And yes, Niftis are also affected by that, unless you disable SimpleITK (there's another Nifti importer too haha).

Ye i guess it is fine. I dont know how people use this library, but i guess they do not update their clone or whatever everytime they launch their app. Myself, I downloaded latest version of your library to my project somewhere in January and only manually updated some crucial things since then.

Hmm yes. If someone wants a very stable experience, I would hope they either use the Asset Store version or one of the packaged releases. I'll make sure to add a note about the coordinate system changes in the release notes both places!

Most people (except the very technical ones who are developing their own software using this plugin) seem to import datasets through the editor, and then they probably rotate it manually there. Either way, I guess we don't want the datasets to be up-side-down forever 😅

If somebody did use it in their app with ability to update this library automatically i would be actually curious how they have done it. I guess this could be maybe done with Unity Asset store when trying to update the asset thru Unity package manager? Or could the clone of this library be stored also in new Unity project? Would it update correctly by pulling latest commits? Hmm. But this would forbid you to do any changes to library itself due to merge conflicts with updates in the future, so i guess downloading latest version and updating manually is the only option if you wanted to make changes inside it, right? :D

Hmm, good question! I would probably recommend:

  • If they want to use it as it is: Download .unitypackage from release and push the whole plugin source code to your project. (or use asset store version).
  • If they plan to modify it: Fork the repository, and add the fork as a git submodule.

Or what do you think?
I should probably write about these things in the readme 😁

@SitronX
Copy link
Contributor

SitronX commented May 3, 2023

Hi @mlavik1
Ohh yes, git submodules are probably ideal for that. Didnt know about them until now, thanks for this info. Still relatively new with git, i should probably spent some time researching all its options :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working DICOM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants