-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Infrastructure for reporting progress #5190
base: master
Are you sure you want to change the base?
Conversation
This commit adds two classes: * ProgressTracker * ProgressScope The first is for users to implement, and to instantiate when they desire to get informed about the overall progress. The second is to be added to all functions that may take a considerable amount of time, such that they can report back how far along they are. These are much more convenient to use than the existing ProgressHandler. ProgressScope is designed such that it only requires "local knowledge" about upcoming and finished work. Scopes are nested and combined to form the final global progress. The active ProgressTracker is stored in a thread_local pointer. This is a consicius decision since in assimp there is often no 'context' passed through. The ProgressTracker may be needed anywhere, and it would be tedious and a huge change to pass it through to every function. Therefore, using a thread_local variable makes it accessible everywhere, without a major interface change. Since assimmp is single-threaded, but may be run in parallel on multiple threads, a thread_local is a good trade-off, in my opinion. This change only adds few uses of ProgressScope, to generally show how it would be used. Also for our use cases these where the most pressing places to add progress reporting, so this already covers loading from FBX files pretty well.
@kimkulling This is a proposal for discussion, this isn't meant to be final. When you have some time to look at it, I'd be happy to get your feedback. We need progress reporting in our application and will add it one way or another, but would be happy if we can give this feature back to assimp as well. We only need the FBX and GLTF code paths to be covered, so if this design (or whatever we can agree upon) is acceptable, I would also add progress reporting to the GLTF loader. |
Thread locals are a bad idea. They're not supported everywhere (Mac had trouble at some point) and they break if you use a user space fiber scheduling approach where fibers can migrate to other threads. |
I still think this is a very bad idea. If you want more fine-grained progress updating it should be mostly possible with the current infrastructure. You could have a local progress scope helper which temporarily replaces the one in Importer. The sub-steps would always report their progress in [0, 1] range and the local helper fixes up the range and reports that to the previous progress handler. |
I like the idea. But for me, it does not feel right to manually calculate the number of steps and add them to the code. It would be more useful to configure the import and the post-processing and get the values calculated automatically. |
I'm not sure how you would want to automatically calculate that. High level stuff, like which processor is running, is way to rough. With the models that we import, some of them can take minutes, so we want way more fine grained reporting. I see no other way than what I've proposed here. Or do you have some solution in mind? |
|
This commit adds two classes:
The first is for users to implement, and to instantiate when they desire to get informed about the overall progress.
The second is to be added to all functions that may take a considerable amount of time, such that they can report back how far along they are.
These are much more convenient to use than the existing ProgressHandler. ProgressScope is designed such that it only requires "local knowledge" about upcoming and finished work. Scopes are nested and combined to form the final global progress.
The active ProgressTracker is stored in a thread_local pointer. This is a consicius decision since in assimp there is often no 'context' passed through. The ProgressTracker may be needed anywhere, and it would be tedious and a huge change to pass it through to every function. Therefore, using a thread_local variable makes it accessible everywhere, without a major interface change. Since assimmp is single-threaded, but may be run in parallel on multiple threads, a thread_local is a good trade-off, in my opinion.
This change only adds few uses of ProgressScope, to generally show how it would be used. Also for our use cases these where the most pressing places to add progress reporting, so this already covers loading from FBX files pretty well.