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 GltfLoaderSettings #10804

Merged
merged 11 commits into from
Nov 30, 2023
Merged

Add GltfLoaderSettings #10804

merged 11 commits into from
Nov 30, 2023

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Nov 29, 2023

Objective

when loading gltfs we may want to filter the results. in particular, i need to be able to exclude cameras.

i can do this already by modifying the gltf after load and before spawning, but it seems like a useful general option.

Solution

add GltfLoaderSettings struct with bool members:

  • load_cameras : checked before processing camera nodes.
  • load_lights : checked before processing light nodes
  • load_meshes : checked before loading meshes, materials and morph weights

Existing code will work as before. Now you also have the option to restrict what parts of the gltf are loaded. For example, to load a gltf but exclude the cameras, replace a call to asset_server.load("my.gltf") with:

asset_server.load_with_settings(
    "my.gltf",
    |s: &mut GltfLoaderSettings| {
        s.load_cameras = false;
    }
);

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use labels Nov 29, 2023
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.

The implementation looks good, and this should definitely exist.

I'd like a bit more documentation on how to use this in practice: right now, it's quite hard to piece the usage together.

@alice-i-cecile
Copy link
Member

I think it would also make sense to add a load_lights field to GltfLoaderSettings, but that can be done in another PR :)

@alice-i-cecile alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 29, 2023
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.

@robtfm robtfm changed the title Add GltfLoaderSettings with load_cameras member Add GltfLoaderSettings Nov 29, 2023
@robtfm
Copy link
Contributor Author

robtfm commented Nov 29, 2023

added lights, and meshes for completeness.

not sure what makes this a breaking change? anyway i added the struct comment as migration guide, let me know if you want something else.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 29, 2023

Really nice, thank you!

The associated type on a public trait impl changed, which is technically semver breaking: line 144, which then cascades to the load method. I don't really think a migration guide is likely to be helpful, but we probably shouldn't ship this in a patch release to be safe.

@alice-i-cecile alice-i-cecile 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 Nov 29, 2023
@alice-i-cecile
Copy link
Member

@robftm missing import in the example :)

@robtfm
Copy link
Contributor Author

robtfm commented Nov 30, 2023

my pc is even worse at running doctests than i am at writing them ...

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 30, 2023
Merged via the queue into bevyengine:main with commit 74ead1e Nov 30, 2023
22 checks passed
james7132 pushed a commit to james7132/bevy that referenced this pull request Dec 1, 2023
# Objective

when loading gltfs we may want to filter the results. in particular, i
need to be able to exclude cameras.

i can do this already by modifying the gltf after load and before
spawning, but it seems like a useful general option.

## Solution

add `GltfLoaderSettings` struct with bool members:
- `load_cameras` : checked before processing camera nodes.
- `load_lights` : checked before processing light nodes
- `load_meshes` : checked before loading meshes, materials and morph
weights

Existing code will work as before. Now you also have the option to
restrict what parts of the gltf are loaded. For example, to load a gltf
but exclude the cameras, replace a call to
`asset_server.load("my.gltf")` with:
```rust
asset_server.load_with_settings(
    "my.gltf",
    |s: &mut GltfLoaderSettings| {
        s.load_cameras = false;
    }
);
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use 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.

None yet

3 participants