fishfolk / bones

An easy-to-use game engine for making real games.
https://fishfolk.org/development/bones/introduction/
Other
210 stars 20 forks source link

fix: Fix race condition on if EguiCtx is initialized before bones game #378

Closed MaxCWhitehead closed 5 months ago

MaxCWhitehead commented 5 months ago

Speculative fix for #377 (see for context)

It seems both the setup_egui and step_bones_game systems are gated by if assets are loaded, which is an async process. My theory is that assets finish loading after PreUpdate, before Update, so bones steps without EguiCtx.

This prevents step_bones_game from being run until EguiCtx is inserted in setup_egui. Becaues setup_egui is gated by assets loaded, step_bones_game is also implicitly gated by assets loaded. I only check one run_if condition to avoid overhead executing systems that make up the core game loop.

MaxCWhitehead commented 5 months ago

This might be wrong without also checking assets loaded - I realized that asset hot reloading may be a factor here. Gonna review code again.

EDIT: Fixed below

MaxCWhitehead commented 5 months ago

Not directly related to this change - but @zicklag I just spotted this:

https://github.com/fishfolk/bones/blob/cf03875700f59203cd0e272f0836dad40ad5bdc7/framework_crates/bones_bevy_renderer/src/lib.rs#L311-L316

This should be unwrap_or(false) right? Wonder if this contributes to crashes we sometimes see when modifying metadata / hotreloading.

MaxCWhitehead commented 5 months ago

Updated to continue checking if assets are loaded, and if EguiCtx is initialized. Is safer this way / should avoid bugs with hot reloading.

Also changed assets_are_loaded to default to false if asset server not initialized.

zicklag commented 5 months ago

This should be unwrap_or(false) right? Wonder if this contributes to crashes we sometimes see when modifying metadata / hotreloading.

That actually is as intended. The idea is that if there is no asset server at all in the bones world, then the assets ( which don't exist ) are considered "loaded".

I did that to allow you to start a game that, for example, just used Egui, without it crashing because you didn't setup an asset server.

I'm not sure if that could cause trouble in other places, though. I did that after my brother tried to start up a hello world GUI and was confused why he needed an assets folder with a pack.yaml, when there were no assets to need yet.

I think checking that the egui ctx is initialized, too, makes sense, though.

MaxCWhitehead commented 5 months ago

Ok yeah that makes sense - I'll remove the change to asset loading check.

zicklag commented 5 months ago

I suppose a comment on that line would probably be good, though, for future readers.

MaxCWhitehead commented 5 months ago

I suppose a comment on that line would probably be good, though, for future readers.

Reverted change to that function + added a comment.