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

Race condition in assets loading #10688

Closed
Shatur opened this issue Nov 21, 2023 · 5 comments · Fixed by #10745
Closed

Race condition in assets loading #10688

Shatur opened this issue Nov 21, 2023 · 5 comments · Fixed by #10745
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Milestone

Comments

@Shatur
Copy link
Contributor

Shatur commented Nov 21, 2023

Bevy version

0.12

What you did

My game have a plugin that generates preview images for buttons from 3D models. And I migrated from 0.11 to 0.12.

What went wrong

I noticed that my preview generation plugin sometimes generates empty images. I looked into it deeper and discovered that AssetServer sometimes returns LoadState::Loaded too early.

Additional information

I managed to extract and simplify this logic into a minimal project that reproduces the issue: assets_render.zip

If you run it, you will see that some generated images are empty (sometimes happens on second run on my machine, but usually always happen):
image

If you disable multi-threaded feature in Cargo.toml, all images will be rendered correctly:
image

@Shatur Shatur added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 21, 2023
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Nov 22, 2023
This was referenced Nov 22, 2023
@cart cart added this to the 0.12.1 milestone Nov 22, 2023
@cart
Copy link
Member

cart commented Nov 23, 2023

I can repro on my machine.

I noticed that my preview generation plugin sometimes generates empty images. I looked into it deeper and discovered that AssetServer sometimes returns LoadState::Loaded too early.

I don't think this is true. From what I'm seeing in your app / my tests, whenever LoadState::Loaded is set, the asset is available. Ex: if I read the asset data from Assets<Scene> in loading_system, the scene is there. Do you have a test that indicates otherwise?

I think this is likely a rendering delay, not an asset system delay. Looks like your app operates under the assumption that it can spawn a scene, wait for the asset to load and create a camera/render on frame X, cleanup on frame X+1, and then restart the process. I'm guessing theres something preventing the spawned scene from rendering immediately on frame X.

I do think this pattern should work in Bevy (ex: be able to easily and predictably know when a spawned scene is ready for render). Just want to make sure we're solving the right problem.

@cart
Copy link
Member

cart commented Nov 23, 2023

One issue with the pattern you are using: LoadState::Loaded is an indicator of the top level referenced asset itself being loaded. RecursiveDependencyLoadState::Loaded is an indicator of all dependencies (including recursive dependencies) being loaded. You can check both for a combined "loaded with recursive dependencies".

If the goal is to know when a scene is "fully ready", you should be checking both. Alternatively, you can just listen for AssetEvent::LoadedWithDependencies, which is fired when the asset and its dependencies are loaded.

However even when adapted to use that event, it still doesn't always render.

I have more confirmations of the theory in the previous post:

I've constrained the example to only 4 items for easier debugging. In this case, the bench asset fails to load:

image

2023-11-23T04:06:03.180682Z DEBUG assets_render::preview: loading asset for preview Some(foliage/simple_bush/simple_bush.gltf#Scene0)
Not actually ready Some(foliage/simple_bush/simple_bush.gltf#Scene0)
2023-11-23T04:06:03.332004Z DEBUG assets_render::preview: asset for preview was sucessfully loaded: Some(foliage/simple_bush/simple_bush.gltf#Scene0)
Scene { world: World { id: WorldId(7), entity_count: 5, archetype_count: 9, component_count: 11, resource_count: 0 } }
Spawned Some(foliage/simple_bush/simple_bush.gltf#Scene0)
2023-11-23T04:06:03.339946Z DEBUG assets_render::preview: rendering preview
skipping 1v0 because of missing material instance
finish
rendering 16v0
2023-11-23T04:06:03.370838Z DEBUG assets_render::preview: loading asset for preview Some(outdoor_furniture/classic_bench/classic_bench.gltf#Scene0)
skipping 1v0 because of missing material instance
skipping 17v0 because of missing material
Not actually ready Some(outdoor_furniture/classic_bench/classic_bench.gltf#Scene0)
2023-11-23T04:06:03.687814Z DEBUG assets_render::preview: asset for preview was sucessfully loaded: Some(outdoor_furniture/classic_bench/classic_bench.gltf#Scene0)
Scene { world: World { id: WorldId(8), entity_count: 37, archetype_count: 9, component_count: 11, resource_count: 0 } }
Spawned Some(outdoor_furniture/classic_bench/classic_bench.gltf#Scene0)
2023-11-23T04:06:03.701938Z DEBUG assets_render::preview: rendering preview
skipping 1v0 because of missing material instance
finish
skipping 1v0 because of missing material instance
skipping 32v0 because of missing material
skipping 33v0 because of missing material
skipping 34v0 because of missing material
skipping 35v0 because of missing material
skipping 36v0 because of missing material
skipping 37v0 because of missing material
skipping 38v0 because of missing material
skipping 39v0 because of missing material
skipping 40v0 because of missing material
skipping 41v0 because of missing material
skipping 42v0 because of missing material
skipping 43v0 because of missing material
skipping 44v0 because of missing material
skipping 45v0 because of missing material
skipping 46v0 because of missing material
skipping 47v0 because of missing material
skipping 48v0 because of missing material
skipping 49v0 because of missing material
2023-11-23T04:06:03.721036Z DEBUG assets_render::preview: loading asset for preview Some(rocks/medium_stone/medium_stone.gltf#Scene0)
Spawned Some(rocks/medium_stone/medium_stone.gltf#Scene0)
2023-11-23T04:06:03.729398Z DEBUG assets_render::preview: asset for preview was sucessfully loaded: Some(rocks/medium_stone/medium_stone.gltf#Scene0)
Scene { world: World { id: WorldId(6), entity_count: 3, archetype_count: 9, component_count: 11, resource_count: 0 } }
skipping 1v0 because of missing material instance
2023-11-23T04:06:03.737359Z DEBUG assets_render::preview: rendering preview
finish
skipping 1v0 because of missing material instance
rendering 49v1
2023-11-23T04:06:03.746069Z DEBUG assets_render::preview: loading asset for preview Some(decorations/mirror/mirror.gltf#Scene0)
Spawned Some(decorations/mirror/mirror.gltf#Scene0)
2023-11-23T04:06:03.962403Z DEBUG assets_render::preview: asset for preview was sucessfully loaded: Some(decorations/mirror/mirror.gltf#Scene0)
Scene { world: World { id: WorldId(5), entity_count: 3, archetype_count: 9, component_count: 11, resource_count: 0 } }
2023-11-23T04:06:03.970165Z DEBUG assets_render::preview: rendering preview
skipping 1v0 because of missing material instance
skipping 1v0 because of missing material instance
rendering 49v2
finish

Note that nothing was rendered for the bench and a lot of entities were skipped for missing materials / material instances.

Also note that the leaves on the bush didn't render for the same reason.

@Shatur
Copy link
Contributor Author

Shatur commented Nov 23, 2023

Do you have a test that indicates otherwise?

Honestly no, it's was just my assumption. It worked on 0.11, and since asset system heavily changed I assumed that this could be the case.

RecursiveDependencyLoadState::Loaded

Didn't know about it, will use it too. With checking RecursiveDependencyLoadState additionally to LoadState the code will be a bit messy...

AssetEvent::LoadedWithDependencies

In my game asset could already be spawned (e.g. loaded), this is why I have to check it like this.

So looks like the scene spawned partially? This is interesting.

@cart
Copy link
Member

cart commented Nov 24, 2023

Didn't know about it, will use it too. With checking RecursiveDependencyLoadState additionally to LoadState the code will be a bit messy...

Yeah a combined state seems reasonable.

So looks like the scene spawned partially? This is interesting.

I'm pretty sure the scene is fully spawned, given that we are trying to render the things that aren't showing up (and skipping them because of missing GPU-side materials). I strongly suspect it would fully render if given a couple more frames. That being said, I still consider this a bug to be fixed (somehow). Easily determining when a scene will be fully rendered is important.

@cart
Copy link
Member

cart commented Nov 25, 2023

Ok found the fix! It actually wasn't a bug in the renderer or the asset system. It was an issue with how the GLTF loader constructs its scenes. The dependencies were being added to the top level GLTF instead of to the Scene sub-assets, meaning the scenes were entering the LoadedWithDependencies state "immediately" because there were no dependencies.

This is a reasonably straightforward fix. I have a hackey one put together to prove this works, but I'll come up with something a bit nicer.

github-merge-queue bot pushed a commit that referenced this issue Nov 27, 2023
…10745)

# Objective

Fixes #10688

There were a number of issues at play:

1. The GLTF loader was not registering Scene dependencies properly. They
were being registered at the root instead of on the scene assets. This
made `LoadedWithDependencies` fire immediately on load.
2. Recursive labeled assets _inside_ of labeled assets were not being
loaded. This only became relevant for scenes after fixing (1) because we
now add labeled assets to the nested scene `LoadContext` instead of the
root load context. I'm surprised nobody has hit this yet. I'm glad I
caught it before somebody hit it.
3. Accessing "loaded with dependencies" state on the Asset Server is
boilerplatey + error prone (because you need to manually query two
states).

## Solution

1. In GltfLoader, use a nested LoadContext for scenes and load
dependencies through that context.
2. In the `AssetServer`, load labeled assets recursively.
3. Added a simple `asset_server.is_loaded_with_dependencies(id)`

I also added some docs to `LoadContext` to help prevent this problem in
the future.

---

## Changelog

- Added `AssetServer::is_loaded_with_dependencies`
- Fixed GLTF Scene dependencies
- Fixed nested labeled assets not being loaded

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
cart added a commit that referenced this issue Nov 30, 2023
…10745)

# Objective

Fixes #10688

There were a number of issues at play:

1. The GLTF loader was not registering Scene dependencies properly. They
were being registered at the root instead of on the scene assets. This
made `LoadedWithDependencies` fire immediately on load.
2. Recursive labeled assets _inside_ of labeled assets were not being
loaded. This only became relevant for scenes after fixing (1) because we
now add labeled assets to the nested scene `LoadContext` instead of the
root load context. I'm surprised nobody has hit this yet. I'm glad I
caught it before somebody hit it.
3. Accessing "loaded with dependencies" state on the Asset Server is
boilerplatey + error prone (because you need to manually query two
states).

## Solution

1. In GltfLoader, use a nested LoadContext for scenes and load
dependencies through that context.
2. In the `AssetServer`, load labeled assets recursively.
3. Added a simple `asset_server.is_loaded_with_dependencies(id)`

I also added some docs to `LoadContext` to help prevent this problem in
the future.

---

## Changelog

- Added `AssetServer::is_loaded_with_dependencies`
- Fixed GLTF Scene dependencies
- Fixed nested labeled assets not being loaded

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants