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

Move dispatchMainThreadTasks out of updateView #605

Open
kring opened this issue Mar 12, 2023 · 0 comments
Open

Move dispatchMainThreadTasks out of updateView #605

kring opened this issue Mar 12, 2023 · 0 comments
Labels
quality Improve code quality or encourage developer success

Comments

@kring
Copy link
Member

kring commented Mar 12, 2023

Tileset::updateView calls AsyncSystem::dispatchMainThreadTasks near its beginning. This is convenient, because cesium-native's clients don't need to remember to call this explicitly themselves. But it has some downsides:

One minor downside is that dispatchMainThreadTasks is called multiple times per frame, once per tileset, which has a small amount of overhead.

The bigger downside is that because a Tileset is actively running (we're executing one of its methods) when the main thread tasks are dispatched, we have to be very careful about what the tasks do, especially to the tileset. For example, if a main thread task (for example, one doing some work on behalf of a user interface) happens to destroy the Tileset, we're sure to crash or corrupt memory. That's what happened in CesiumGS/cesium-unreal#1047.

So, a better pattern is require users to call dispatchMainThreadTasks explicitly. This is only a small additional burden for users, and eliminates an entire class of bugs.

Originally mentioned here:
CesiumGS/cesium-unreal#1047 (comment)

@kring kring added the quality Improve code quality or encourage developer success label Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality Improve code quality or encourage developer success
1 participant