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

Add support for custom glTF vertex attributes. #5370

Merged
merged 30 commits into from
Apr 24, 2023

Conversation

komadori
Copy link
Contributor

@komadori komadori commented Jul 18, 2022

Objective

The objective is to be able to load data from "application-specific" (see glTF spec 3.7.2.1.) vertex attribute semantics from glTF files into Bevy meshes.

Solution

Rather than probe the glTF for the specific attributes supported by Bevy, this PR changes the loader to iterate through all the attributes and map them onto MeshVertexAttributes. This mapping includes all the previously supported attributes, plus it is now possible to add mappings using the add_custom_vertex_attribute() method on GltfPlugin.

Changelog

  • Add support for loading custom vertex attributes from glTF files.
  • Add the custom_gltf_vertex_attribute.rs example to illustrate loading custom vertex attributes.

Migration Guide

  • If you were instantiating GltfPlugin using the unit-like struct syntax, you must instead use GltfPlugin::default() as the type is no longer unit-like.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds labels Jul 18, 2022
komadori added a commit to komadori/bevy_mod_gltf_patched that referenced this pull request Nov 15, 2022
@mockersf
Copy link
Member

codes looks good in the loader, but the example could do with a lot more explanation of what's going on, why there is a custom attribute, how it's used, ...

@komadori
Copy link
Contributor Author

codes looks good in the loader, but the example could do with a lot more explanation of what's going on, why there is a custom attribute, how it's used, ...

Thanks @mockersf for reviewing this. I've added some explanatory comments to the example. The choice of use-case for a custom attribute in the example is arbitrary, but I thought this was quite visually fun without being too complicated.

komadori added a commit to komadori/bevy_mod_gltf_patched that referenced this pull request Mar 11, 2023
@komadori komadori requested a review from nicopap April 22, 2023 00:46
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM. Just need to source the barycentric.glb file.

Copy link
Contributor

Choose a reason for hiding this comment

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

The bevy policy is to have gltf (plain text version) formats if possible. I think this one should be added as a gltf, especially given it won't be large.

Also: where does the file come from? Did you make it yourself? Regardless, you should add a line to the CREDITS.md stating the source and license. If you made it yourself, in this particular case, I'd recommend CC0 (public domain)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was generated by this scratch program: https://github.com/komadori/mk_bary_gltf

The program and by extension its output are dual licensed under the MIT and Apache 2.0 licenses.

crates/bevy_gltf/src/attrs.rs Outdated Show resolved Hide resolved
@nicopap nicopap added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 22, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@nicopap
Copy link
Contributor

nicopap commented Apr 22, 2023

Also, could you add the following to your PR description:

----

## Changelog

- Add loading of custom vertex attributes to bevy's glTF loader.
- Add the `custom_gltf_2d.rs` example to illustrate loading of custom vertex attributes.

## Migration Guide

- If you were manually adding `GltfPlugin` to your app, it should be replaced by `GltfPlugin::default()`

@github-actions
Copy link
Contributor

You added a new example but didn't update the readme. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@komadori komadori requested a review from nicopap April 23, 2023 23:31
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM: Let's Get This Merged.

The API is justified by the fact you need a bridge between the glTF attribute and the wgsl-accessible vertices. Otherwise, it wouldn't be possible to specify a location(X) in shader.

@komadori
Copy link
Contributor Author

Thanks @nicopap for the review.

Now this PR has two approvals, should the S-Ready-For-Final-Review label be added?

@nicopap
Copy link
Contributor

nicopap commented Apr 24, 2023

I actually do not exactly understand the rules behind "Read for final review" but it looks like it is as you describe: Point 2 of the "Maintainers" section https://github.com/bevyengine/bevy/blob/main/docs/the_bevy_organization.md#maintainer

@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 24, 2023
@alice-i-cecile
Copy link
Member

This was in fact ready-for-final-review: thanks for adding the label!

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good migration guide, solid review process, no merge conflicts. I still hate the 2D vs 3D example division, but that's not a fight for this PR. gh pr checkout 5370: let's try out the new example. Works great, clear docs.

Code itself looks solid: great error handling! Let's merge!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 24, 2023
Merged via the queue into bevyengine:main with commit d74533b Apr 24, 2023
@komadori komadori deleted the gltf-custom-attributes branch April 24, 2023 15:21
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-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants