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

Removed synchronous wait in GLB json loading causing spikes #716

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

umeraamir-arthur
Copy link

The synchronous wait here was causing the main thread to wait until the JSON is loaded completely which was highly dependent on the device and was varying depending on it.

This new solution is also waiting synchronously but main thread doesn't wait for it.

@umeraamir-arthur
Copy link
Author

@pfcDorn what do you say about this solution?

@pfcDorn
Copy link
Contributor

pfcDorn commented Apr 2, 2024

why not using await Task.Run( () => { ... } ); for loading and parsing the stream. Is much simpler

@umeraamir-arthur
Copy link
Author

umeraamir-arthur commented Apr 3, 2024

So technically this whole code

Thread parseJsonThread = new Thread(() => GLTFParser.ParseJson(_gltfStream.Stream, out _gltfRoot, _gltfStream.StartPosition));
parseJsonThread.Priority = ThreadPriority.Highest;
parseJsonThread.Start();
while (parseJsonThread.IsAlive)
{
Debug.Log("GLB json data is loading async...");
await Task.Delay(25);
}
Debug.Log("GLB json data is loaded.");

will become like this?
await Task.Run(() =>
{
GLTFParser.ParseJson(_gltfStream.Stream, out _gltfRoot, _gltfStream.StartPosition);
});

@pfcDorn
Copy link
Contributor

pfcDorn commented Apr 3, 2024

right! :)

@umeraamir-arthur
Copy link
Author

umeraamir-arthur commented Apr 3, 2024

Great! What about thread priority?
parseJsonThread.Priority = ThreadPriority.Highest;
Will it effect?

@pfcDorn
Copy link
Contributor

pfcDorn commented Apr 3, 2024

it don't think it's important anymore. Every device will have enough CPU cores today :)

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.

2 participants