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

Asset system usability and load state bugs #7479

Closed
djeedai opened this issue Feb 2, 2023 · 12 comments
Closed

Asset system usability and load state bugs #7479

djeedai opened this issue Feb 2, 2023 · 12 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@djeedai
Copy link
Contributor

djeedai commented Feb 2, 2023

Bevy version

0.9.1

What you did

Attempt to write a non-trivial yet reasonably simple asset loading system with dependencies.

What went wrong

Panics. Panics everywhere.

The loading state (LoadState) of assets and their dependent assets makes little sense to the non-expert eye. And writing a system that loads an asset and its dependencies requires heavy work and deep understanding from the user.

Additional information

I'm very frustrated by the asset system.

I've been trying to do some level loading for my game for the past week. This involves loading a core file (database; text) referencing other files (models; text), each referencing a gltf which contains a scene to spawn. A non-trivial but still relatively simple dependency tree situation. All files are small, the tree is balanced, no loops, really nothing crazy. Yet I've been having hundreds of panics on assets not loaded (which are non-deterministic due to threading).

First, after AssetServer::load() is called the main asset might still be loading. This is kind of well known, but already annoying to handle; you have to write a system that needs to poll each frame, and does different things depending on the loading state, which makes reasoning more complex. Or you have to write two systems and use stages. Either way, this is quite the overhead. And this traps every single new Bevy user. On top of that, that asset may even sometimes be in weird states like NotLoaded, even though I have a strong reference to it! 🤨

Next is GLTF. I just had an assert where the Handle<Gltf> asset is loaded, but Gltf::default_scene points to a Scene that is not in the Assets<Scene> (yet?). And this is not a malformed asset; those same assets loaded ten times without issue in previous runs, and fine in subsequent runs too, and I didn't touch them in-between. Just randomly the scene is not ready (race condition).

Dependent assets set with LoadedAsset::with_dependency() also seem to count only for triggering the loading, but again are completely uncoupled from the loading state of their parent, so the parent can be in LoadState::Loaded but a dependent asset not loaded yet. Does that make sense? If the parent is loaded, shouldn't that imply that any dependencies are loaded too? Otherwise can we really call those dependencies? In any case, this means your system that was polling assets load state now needs to poll multiple assets AND know about the graph of dependencies so it can poll all of them and report when a subtree is ready (parent and all children loaded). And again, I even saw those dependent assets still in NotLoaded state like if they had never been referenced; I would have expected their parent to keep a strong dependency to them no? Otherwise, what's LoadedAsset::with_dependency() doing then?

I had already written a simple Loader component to manage a flat list of assets, because it was a pain to write again and again the same polling code (loading queue, sync primitives, atomic counter, etc. to have a single polling call for a "transaction" of assets) for 2-3 assets that need to be loaded for some system to work. But it seems that now I also need to write an entire asset dependency graph polling system? Is that by design really?

I've read #3972 and its thread on asset dependencies but I can't see any plan or discussion that would address the above issues. I may have missed something, or I'm doing something completely wrong (which is entirely possible). But for now the way I've experienced it so far is that any non-trivial asset loading requires a mountain of work from users. Documentation on LoadState and the asset system in general would also benefit from some extra attention; I'm available to make PRs, provided I can understand how things actually work and are designed to be used.

@djeedai djeedai added C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 2, 2023
@nicopap
Copy link
Contributor

nicopap commented Feb 3, 2023

Have you looked at the source for bevy_gltf (loader.rs) or bevy_mod_fbx? Having developed my own asset loader, I didn't encounter any of the issues you describe.

You seem to have a drastically different approach to asset loading as bevy_gltf and bevy_mod_fbx. What's the limitation of the async approach that forces you to use systems for asset loading?

@djeedai
Copy link
Contributor Author

djeedai commented Feb 3, 2023

Thanks, that's a good suggestion. I had a look at the GLTF loader, and unfortunately I suspect I had already looked at when I wrote my custom asset loader because it's very similar.

  • I call LoadContext::set_default_asset(LoadedAsset::new(...)) on the primary asset, like the model file
  • I call LoadContext::set_labeled_asset("name", LoadedAsset::new(...)) on the secondary assets like the GLTF file referenced by the model or the StandardMaterial I create on the fly

The only extra thing I do is call LoadedAsset::with_dependency() on the primary asset (model file) to add a dependency to the secondary asset it creates (material). I'm not even sure that's correct because the docs don't explain what adding a dependency means / what effect it has. The GLTF loader doesn't seem to do that anywhere, curiously.

I don't think what I do is drastically different from anything. I only seem to do something a tiny bit less trivial because the database asset doesn't directly produce secondary assets (model) but simply references them, while the model files both reference assets (GLTF) and create secondary ones on the fly (materials). So there's one extra "depth" to the dependency tree compared to GLTF loading I think.

Having a system which polls until an asset is loaded sounds pretty standard; I don't know how you can manage that otherwise since AssetServer::load() immediately returns. Are you saying that you can have a top-level asset with dependencies, and the top-level one will only get into the LoadState::Loaded once all dependencies are too? I've never observed that.

The only thing I see that possibly makes things more complex in my case as opposed to the GLTF loader, if I understand how that code works, is that the GLTF loader calls LoadContext::read_asset_bytes() to immediately read the buffers and textures while inside the AssetLoader::load() call. I guess I should try that to load the GLTF model from the model asset, since the model depends on it. But I'm not even sure what that would look like because I don't want to rewrite a GLTF loader, I just want the existing one to be called, so what would I do with the raw GLTF bytes? Assuming that works somehow, that may solve the issue of the model file being ready before its GLTF asset is. Other than that, I don't see what I do differently that may cause so much issues.

There's also some obscure use of IoTaskPool which seems to load textures in parallel, or at least do something like that. But I'm not sure if that's idiomatic or not, as the docs don't really go into details about what to use that IoTaskPool for.

If anyone can shed some light on part or all of this, so I can document those, that'd be very helpful.

@djeedai
Copy link
Contributor Author

djeedai commented Feb 3, 2023

I should mention, currently I reference the GLTF from my custom asset via a string, and during AssetLoader::load() I do something like:

// Load GLTF
let gltf_path = AssetPath::new_ref(...);
let gltf: Handle<Gltf> = load_context.get_handle(gltf_path.clone());
// Inline-create secondary asset for material
let material = StandardMaterial { ... };
let material: Handle<StandardMaterial> = load_context.set_labeled_asset("material", LoadedAsset::new(material));
// Primary asset (model)
let model = Model { gltf, material, ... };
let primary_asset = LoadedAsset::new(model).with_dependency(gltf_path);
load_context.set_default_asset(primary_asset);

which is how I ensure the GLTF is loaded via the existing GLTF loader.

@mockersf
Copy link
Member

mockersf commented Feb 3, 2023

You're right on your interpretation of the gltf loader.

It's loading its subassets files and waiting for the files to be loaded, to be able to work on their bytes. It's allowed to do that as it's in an async context. The IoTaskPool part is to be able to read many files in parallel, as at that point it's loading textures and this can be slow with image conversions.

From a system, you don't have a choice and must load asset asynchronously, getting back only an handle. From a loader, you are already in an async context, so you have the choice when loading subassets of either getting an handle, or getting their bytes.

All this is very underdocumented... I'm one of the person who edited the most the gLTF loader, but I would still have to re-read the code to explain what is happening. It has never been documented because bevy_asset has been under a looming rewrite for a long time...

@djeedai
Copy link
Contributor Author

djeedai commented Feb 3, 2023

either getting an handle, or getting their bytes.

But getting a handle does not guarantee completion of loading so you end up with an asset "loaded" but with unloaded dependencies. And getting the bytes doesn't allow you to reuse an existing asset loader. Did I get that right?

@mockersf
Copy link
Member

mockersf commented Feb 4, 2023

But getting a handle do not guarantee completion of loading so you end up with an asset "loaded" but with unloaded dependencies.

Yup

And getting the bytes doesn't allow you to reuse an existing asset loader. Did I get that right?

That will depend on how the asset loader is implemented. The asset loader itself can't be called if I remember correctly, but nothing stops the asset loader implementor to also expose a function taking bytes. The gltf loader almost does that, there is a reusable function... but it's not public.

@djeedai
Copy link
Contributor Author

djeedai commented Feb 4, 2023

It seems unsatisfactory and error prone that we have to rely on how the loader is implemented. We should try to encode that use case in the trait itself.

@mockersf
Copy link
Member

mockersf commented Feb 4, 2023

It seems unsatisfactory and error prone that we have to rely on how the loader is implemented. We should try to encode that use case in the trait itself.

Totally!
The issue is that the method defined in the trait doesn't ask for anything returned, just to put in the load context. That would be an easy thing to change, but again, no real effort went into improving assets for some time...

@djeedai
Copy link
Contributor Author

djeedai commented Feb 4, 2023

So in summary:

  • references to inline assets require using read_asset_bytes(), but this doesn't play nice with existing loaders / there's no idiomatic way to invoke an existing loader.
  • we should better document the IoTaskPool idiom for parallel inline asset loading.
  • LoadContext::get_handle() gets you a handle and I think triggers loading, but doesn't wait for it to finish, which is neither a "weak ref" (doesn't trigger loading) nor a "strong ref" (waits for loading to end), but instead some kind of intermediate hybrid.
  • we don't know what LoadContext::with_dependency() does; it seems to do nothing.

Are there plans already or on-going work to tackle this, since 0.10 was supposed to focus on the asset pipeline? @cart anything you're working on maybe?

Ideally I'd rather have a proper asset dependency graph where I can specify strong/weak refs, and where the LoadState::Loaded status is only reached when all strong deps are also loaded.

@nicopap
Copy link
Contributor

nicopap commented Feb 13, 2023

I'm implementing a more exotic asset loader now. And I'm hitting the exact same limitations as @djeedai. I did some research specifically on LoadedAsset::add_dependency and it turns out that it is only used once in bevy. Seemingly for loading Shader dependencies (#include I imagine?), but I am having difficulties finding where Handle<Shader> of dependencies are used. with_dependenc{y,ies} is actually never used (and not tested) so of course it's broken :/

@mockersf
Copy link
Member

It used to be used by the gltf loader, but that was removed to ensure the same result from external files or from included files in a glb.

Not sure what you mean by with_dependenc{y,ies} that seems broken, it's just a very thin layer above add_dependency to allow chaining calls. Those three methods actually trigger the loading of a file from an asset loader.

@alice-i-cecile
Copy link
Member

Closing this out now following Assets v2.

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 C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

4 participants