-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GLTF extension support #11138
GLTF extension support #11138
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
Hi @CorneliusCornbread ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple & clear, works fine locally I think it is all good ! Thanks a lot for your PR ! :)
Some minor thoughts:
- I do not really think the meta migration is much of an issue, but that could be just me
- I usually favour having a small example for new features, but I seem to recall the Bevy team likes to keep the examples minimal to prevent too much bloat, but perhaps @alice-i-cecile has some input on that ?
@@ -35,14 +35,15 @@ bevy_tasks = { path = "../bevy_tasks", version = "0.12.0" } | |||
bevy_utils = { path = "../bevy_utils", version = "0.12.0" } | |||
|
|||
# other | |||
gltf = { version = "1.3.0", default-features = false, features = [ | |||
gltf = { version = "1.4.0", default-features = false, features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call ! v 1.4.0 has some nice improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a requirement for the extensions, 1.4.0 adds the extensions
feature flag. There's also some fixes for large gltf's with large buffers which is nice
@@ -98,6 +98,8 @@ pub struct Gltf { | |||
/// Named animations loaded from the glTF file. | |||
#[cfg(feature = "bevy_animation")] | |||
pub named_animations: HashMap<String, Handle<AnimationClip>>, | |||
/// The gltf root of the gltf asset, see <https://docs.rs/gltf/latest/gltf/struct.Gltf.html>. Only has a value when `GltfLoaderSettings::include_source` is true. | |||
pub source: Option<gltf::Gltf>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok.
I wondered if it would be better hidden behind a feature flag like named_animations
but it would not really make sense at the bevy level imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this makes sense, the idea is that if you don't need the extra data for some assets you can save on memory for those models specifically. Sometimes we only need the model data we're extracting normally.
Just an additional 2 cents regarding your comment in your example code:
you can use |
Objective
Adds support for accessing raw extension data of loaded GLTF assets
Solution
Via the GLTF loader settings, you can specify whether or not to include the GLTF source. While not the ideal way of solving this problem, modeling all of GLTF within Bevy just for extensions adds a lot of complexity to the way Bevy handles GLTF currently. See the example GLTF meta file and code
Changelog
Added support for GLTF extensions through including raw GLTF source via loader flag
GltfLoaderSettings::include_source == true
, stored inGltf::source: Option<gltf::Gltf>
Migration Guide
This will have issues with "asset migrations", as there is currently no way for .meta files to be migrated. Attempting to migrate .meta files without the new flag will yield the following error:
This means users who want to migrate their .meta files will have to add the
include_source: true,
setting to their meta files by hand.