bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add support for custom glTF vertex attributes.

Open komadori opened this issue 1 year ago • 2 comments

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.

komadori avatar Jul 18 '22 21:07 komadori

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, ...

mockersf avatar Dec 26 '22 18:12 mockersf

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 avatar Jan 15 '23 23:01 komadori

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.

github-actions[bot] avatar Apr 22 '23 07:04 github-actions[bot]

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()`

nicopap avatar Apr 22 '23 07:04 nicopap

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.

github-actions[bot] avatar Apr 23 '23 23:04 github-actions[bot]

Thanks @nicopap for the review.

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

komadori avatar Apr 24 '23 08:04 komadori

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 avatar Apr 24 '23 09:04 nicopap

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

alice-i-cecile avatar Apr 24 '23 14:04 alice-i-cecile