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.

Sign up for GitHub

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

Jump to bottom

add handling of all missing gltf extras: scene, mesh & materials #13453

Merged
merged 12 commits into from
Jun 3, 2024
Merged

add handling of all missing gltf extras: scene, mesh & materials #13453

merged 12 commits into from
Jun 3, 2024

Conversation

kaosat-dev
Copy link
Contributor

@kaosat-dev kaosat-dev commented May 21, 2024 •
edited
Loading

Objective

Solution

As outlined in the discussion in the linked issue as the best current solution, this PR adds specific GltfExtras for

  • scenes

  • meshes

  • materials

  • As it is , it is not a breaking change, I hesitated to rename the current "GltfExtras" component to "PrimitiveGltfExtras", but that would result in a breaking change and might be a bit confusing as to what "primitive" that refers to.

Testing

  • I included a bare-bones example & asset (exported gltf file from Blender) with gltf extras at all the relevant levels : scene, mesh, material

Changelog

  • adds "SceneGltfExtras" injected at the scene level if any
  • adds "MeshGltfExtras", injected at the mesh level if any
  • adds "MaterialGltfExtras", injected at the mesh level if any: ie if a mesh has a material that has gltf extras, the component will be injected there.

@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Needs-Release-Note Work that should be called out in the blog due to impact labels May 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@kaosat-dev kaosat-dev mentioned this pull request May 22, 2024
Closed
BUGO07
BUGO07 approved these changes May 27, 2024
View reviewed changes
Copy link

@BUGO07 BUGO07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great changes, this will help out other crates as well.

GitGhillie
GitGhillie reviewed May 27, 2024
View reviewed changes
Copy link
Contributor

@GitGhillie GitGhillie left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a fairly straightforward extension of what's already there, and it will simplify/improve Blender-to-Bevy workflows a lot.

Some nits here and there, and I'm wondering: The example will show up in https://bevyengine.org/examples/ right? Might be a bit confusing if there is no output on the screen.

Cargo.toml Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/3d/load_gltf_extras.rs Outdated Show resolved Hide resolved
examples/3d/load_gltf_extras.rs Outdated Show resolved Hide resolved
examples/3d/load_gltf_extras.rs Outdated Show resolved Hide resolved
hymm
hymm approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once GitGhillie's comments are addressed.

crates/bevy_gltf/src/loader.rs Outdated Show resolved Hide resolved
@kaosat-dev
feat(gltf_extras):
c821cdc 
 * tweaks & touchup adressing the review comments
 * added display of extras per entity on screen instead of console/cli
 * regenerated the example gltf file with more clear names & values for the different gltf extras
@kaosat-dev
Copy link
Contributor Author

I updated the example & the gltf file to be much clearer: it now looks like this:
Screenshot from 2024-05-28 09-20-10

GitGhillie
GitGhillie approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@GitGhillie GitGhillie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! The query for the extras is now also much easier to read.

@hymm hymm added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 3, 2024
Merged via the queue into bevyengine:main with commit d26900a Jun 3, 2024
31 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Jun 3, 2024
Closed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1329 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hymm hymm hymm approved these changes

@GitGhillie GitGhillie GitGhillie approved these changes

@BUGO07 BUGO07 BUGO07 approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Enhancement A new feature C-Needs-Release-Note Work that should be called out in the blog due to impact D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bevy is not reading all gltf extra properties
5 participants
@kaosat-dev @alice-i-cecile @hymm @GitGhillie @BUGO07