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

Async dataset import #151

Closed
mlavik1 opened this issue Jan 6, 2023 · 4 comments · Fixed by #158
Closed

Async dataset import #151

mlavik1 opened this issue Jan 6, 2023 · 4 comments · Fixed by #158
Labels
enhancement New feature or request performance

Comments

@mlavik1
Copy link
Owner

mlavik1 commented Jan 6, 2023

Importing large datasets can be very slow, and will cause the whole application to freeze.

Suggestion: Implement support for async import, on a separate thread. This should be optional, and possible to disable.

@mlavik1 mlavik1 added enhancement New feature or request performance labels Jan 6, 2023
@SitronX
Copy link
Contributor

SitronX commented Mar 4, 2023

Solution

Hello @mlavik1,
i have implemented async loading on separate thread in my slightly adjusted version of your library for my needs. Today i tried to incorporate it with your main version and it is working pretty well so far.

Not everything is moved to the separate thread, cause texture creation and stuff around it needs to stay on main thread (it is not supported on other threads), but majority of the expensive code is now on separate thread from the thread pool. This includes loading datasets and also CreateTextureInternal(), CreateGradientTextureInternal() methods which are the main culprits of long loads and freezes, the hiccups with Texture generation stuff are usually under one second now.

I am still testing my code on the rest of the dataset types, but i have already tested it on Dicom, Nrrd and Nifti types (with Simple ITK on and off) and it works.

I have added this option to settings, so the async load is optionable
settings

In Runtime, with Gui script, the async loading is selectable in editor
Screenshot 2023-03-04 144206

Comparison

Without async load - App freezes completely

NoAsync.mp4

With async load - Only slight hiccup with Texture generation at the end
Async.mp4

I still need to investigate some errors with memory leak. There is quite a lot of files changed, where i added async alternatives to a lot of previous methods, so i still need to properly go through it and check for possible errors. But i guess i should have it ready in few days if you are open to it.

@mlavik1
Copy link
Owner Author

mlavik1 commented Mar 9, 2023

@SitronX Thanks for sharing! This looks really promising! 👍
I think this should be a great improvement, especially people using this project in VR (where freezes can be highly problematic).
Feel free to ask if you want help looking into any of the issues!

And regarding texture upload, If you need to avoid freezes in your own project then I think it should be possible to do most of the work on a separate thread, and then do the final upload though native code. I'm planning to do that in another project I work on, so I'll let you know if I get anywhere with it :)

Have a great day!

@SitronX
Copy link
Contributor

SitronX commented Mar 10, 2023

@mlavik1 Hello, yep i am mainly working with VR/AR and without async load it was highly uncomfortable.

I have finished fixing all known problems so i think it should be ready now. I eventually changed it so it is not using EditorPrefs, but ScriptingDefinedSymbols, similarly to SimpleITK settings, so it works even in build. Lately i also added async alternative to downscaling process. So i hope i covered all performance heavy tasks and didnt miss anything.

As said previously, it is still not perfect, but that texture upload through native code you mentioned should fix the rest of the small hiccups in the future. But as of right now it is still night and day difference than it was previously especially with VR headsets.

It should work with all dataset types, except image sequence (.png,.jpg) where the whole loading loop is working with Unity Textures almost everywhere, so i dont really know how to work around it. Unfortunately i could not test loading Parchg and raw dataset types, because i dont really have any datasets like that. But it should still work, everything is prepared for it and it is not using any Unity specific things when i checked inside, so it should not give any errors on load. Could you still please check if it is working if you have the said dataset types? I will open pull request soon.

@SitronX SitronX mentioned this issue Mar 10, 2023
@mlavik1
Copy link
Owner Author

mlavik1 commented Mar 14, 2023

Oh, good to see more VR/AR users of this plugin!
And yes, this change is definitely needed for runtime import in VR/AR projects. Really a life saver 😁
Thanks a lot for the PR! I replied with some comments.
And I think it's fine to leave the old image sequence importer as it is, because the SimpleITK image sequence importer is to be preferred anyway - and you made that async already! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants