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

Infrastructure for reporting progress #5190

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jakrams
Copy link

@jakrams jakrams commented Aug 1, 2023

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.

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.
@jakrams
Copy link
Author

jakrams commented Aug 1, 2023

@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.

@turol
Copy link
Member

turol commented Aug 1, 2023

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.

@turol
Copy link
Member

turol commented Aug 31, 2023

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.

@kimkulling
Copy link
Member

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.

@jakrams
Copy link
Author

jakrams commented Sep 23, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants