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

fix example update_gltf_scene: add label to load asset #1204

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Jan 3, 2021

sorry for the mix...

@CleanCut
Copy link
Member

CleanCut commented Jan 4, 2021

That fixes it for me 👍🏼

Without the fix to the asset path, should a non-fatal error have been logged to the console? That's what I would have naively expected, not knowing how gltf files work.

@mockersf
Copy link
Member Author

mockersf commented Jan 4, 2021

That's part of the "asynchronous is hard for error reporting" category...

By calling asset_server.load, you get a Handle<T>, and the T is determined by the end user depending on the usage. So I can say let my_image: Handle<Texture> = asset_server.load("music.mp3"); and it will work...
When actually spawning the scene with the "invalid" handle, it will try to retrieve the actual scene data from the handle, which will fail but that's normal, as it may not be loaded yet.

Since #1020, loading a GLTF file won't give an handle to a scene but a handle to a gltf struct. I would like to have the default scene being used by default in this case, but haven't found a good way to do it without binding scene to how gltf works which I'd like to avoid.

I'm open for ideas if you have some on this subject!

@Moxinilian Moxinilian added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples labels Jan 4, 2021
@CleanCut
Copy link
Member

CleanCut commented Jan 6, 2021

I would expect any asset backend that's actually doing the loading to emit an error to the log if it fails to load the asset in question. I think that's both easy to do and sufficient handling for Bevy's current level of maturity. That's all I was really getting at.

But since you asked...and it was an interesting question...here some ideas for more robust handling that could be considered for the future 😄. These ideas are all about asset system as a whole, though, not just gltf. Also, I know little about the current asset system implementation, so maybe something like these ideas (or better!) are already in place!

  • Some facility for configuring callbacks in the case of asset loading failure
    • This could be some global error handler for all asset types, or an ability to specify a handler per specific asset type, or even some way to pass an error handler for any specific asset (though that last sounds tedious to use).
  • Adding something to Handle<T> itself to indicate the loading state of the asset (in progress, loaded, error, etc.), so it can simply be inspected.
  • Some sort of way to query the asset system for loading state by passing in an existing Handle<T>
  • Some way to indicate either globally or per-asset that if loading fails, a built-in or user-supplied default asset should be substituted.

@mockersf
Copy link
Member Author

mockersf commented Jan 6, 2021

I would expect any asset backend that's actually doing the loading to emit an error to the log if it fails to load the asset in question. I think that's both easy to do and sufficient handling for Bevy's current level of maturity. That's all I was really getting at.

But in this case, nothing failed. The loader was able to load something from the file, and saved it to the Handle. The scene spawner tried to load the asset from the handle and found nothing, but that's normal behaviour as the asset is maybe not done loading

  • Some facility for configuring callbacks in the case of asset loading failure
    • This could be some global error handler for all asset types, or an ability to specify a handler per specific asset type, or even some way to pass an error handler for any specific asset (though that last sounds tedious to use).

Looking at how it handles error, loading something can return a AssetServerError... which is kinda ignored (it's unwrapped in another thread, so the thread crashes).

  • Adding something to Handle<T> itself to indicate the loading state of the asset (in progress, loaded, error, etc.), so it can simply be inspected.

That's supposedly done by the AssetEvent event but it doesn't have errors...

  • Some sort of way to query the asset system for loading state by passing in an existing Handle<T>

There is get_load_state that should give you the loaded state of an handle, but I never tried it.

  • Some way to indicate either globally or per-asset that if loading fails, a built-in or user-supplied default asset should be substituted.

I think I remember a discussion on discord about that, people were splitter on whether that was a good idea...

There are a few things that could be improved, but I think it's better to wait for the atelier integration before spending too much time on that...

Thinking on the specific problem of gltf, I think a great fix would be to be able to have different default asset from the same handle, without label, if they have different type. That is, from the same handle, I could get a default scene, a default gltf, ... depending on how it's used. I'll keep that in mind for when atelier is integrated 👍

@cart
Copy link
Member

cart commented Jan 7, 2021

Yup good thoughts all around. I agree that we should probably hold off on major asset system changes until after the atelier migration (which will likely introduce new and different problems for us to solve 😄)

@cart cart merged commit 12d4184 into bevyengine:master Jan 7, 2021
@CleanCut
Copy link
Member

CleanCut commented Jan 7, 2021

But in this case, nothing failed. The loader was able to load something from the file, and saved it to the Handle. The scene spawner tried to load the asset from the handle and found nothing, but that's normal behaviour as the asset is maybe not done loading

Ah, tricky. Thanks for all the info and explanations!

I'm looking forward to seeing how things work after the atelier migration.

rparrett pushed a commit to rparrett/bevy that referenced this pull request Jan 27, 2021
@mockersf mockersf deleted the fix-update-gltf-example branch April 27, 2021 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants