Add image sampler configuration in GLTF loader

[axlitEels]

I can't underrate anisotropic filtering.

Objective

• Allow easily enabling anisotropic filtering on glTF assets.
• Allow selecting ImageFilterMode for glTF assets at runtime.

Solution

• Added a Resource DefaultGltfImageSampler: it stores Arc<Mutex<ImageSamplerDescriptor>> and the same Arc is stored in GltfLoader. The default is independent from provided to ImagePlugin and is set in the same way but with GltfPlugin. It can then be modified at runtime with DefaultGltfImageSampler::set.
• Added two fields to GltfLoaderSettings: default_sampler: Option<ImageSamplerDescriptor> to override aforementioned global default descriptor and override_sampler: bool to ignore glTF sampler data.

Showcase

Enabling anisotropic filtering as easy as:

app.add_plugins(DefaultPlugins.set(GltfPlugin{
    default_sampler: ImageSamplerDescriptor {
        min_filter: ImageFilterMode::Linear,
        mag_filter: ImageFilterMode::Linear,
        mipmap_filter: ImageFilterMode::Linear,
        anisotropy_clamp: 16,
        ..default()
    },
    ..default()
}))

Use code below to ignore both the global default sampler and glTF data, having your_shiny_sampler used directly for all textures instead:

commands.spawn(SceneRoot(asset_server.load_with_settings(
    GltfAssetLabel::Scene(0).from_asset("models/test-scene.gltf"),
    |settings: &mut GltfLoaderSettings| {
        settings.default_sampler = Some(your_shiny_sampler);
        settings.override_sampler = true;
    }
)));

Remove either setting to get different result! They don't come in pair!

Scene rendered with trillinear texture filtering:

Scene rendered with 16x anisotropic texture filtering:

Migration Guide

• The new fields in GltfLoaderSettings have their default values replicate previous behavior.

[github-actions]

Welcome, new contributor!

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

[rparrett]

Would it be better to instead provide an override for the default sampler?

That would allow users to control the properties of the sampler that aren't specified in the glTF without overriding the properties that are specified in the glTF. I think you could then also remove the separate anisotropy_clamp setting.

That feels potentially more useful to me than a complete override, but this isn't really my area.

[axlitEels]

A complete override still feels necessary as some sort of option to me: for example, it's a common thing in games to allow choosing between bilinear and trilinear filtering (there was a performance impact back in the day, now trilinear is cheap enough I'm not testing it) and I think Bevy should still allow implementing that, which just couldn't be done as long as we can't override glTF's fields somehow.

Aside of anisotropy_clamp, the other fields wgpu SamplerDescriptor has and glTF doesn't provide are lod_min_clamp, lod_max_clamp, compare and border_color. The last one is weird because it isn't true color -- it's an enum with 4 variants, and, more importantly, using it requires address modes to be set to a variant glTF doesn't provide, yet glTF spec doesn't make these fields optional so we'd again need to override them. The other three missing fields imply some heavy tinkering with the engine (because AFAIK LOD ones don't prevent unused mip levels from loading), and the people who would actually need them probably could do it with the override.

I could replace anisotropy_clamp setting with default_sampler while leaving override_sampler in place, but A) that's weird and B) it turns my example into this:

commands.spawn(SceneRoot(asset_server.load_with_settings(
    GltfAssetLabel::Scene(0).from_asset("models/test-scene.gltf"),
    |settings: &mut GltfLoaderSettings| settings.default_sampler = ImageSamplerDesciptor{
        anisotropy_clamp: 16,
        ..default()
    }
)));

which I believe looks bloated.

[axlitEels]

wgpu's SamplerDescriptor is based off WebGPUs GPUSamplerDescriptor which is set in stone, so I don't expect it to change earlier than the entire Bevy glTF loader gets thrown out the window and rewritten for whatever internal reason it will be.

[rparrett]

My concern is coming from address_mode_u and address_mode_v. It doesn't seem desirable to ever override those globally.

[axlitEels]

In realistic project, they'd be overridden per-asset like I do in example. Setting up global override would be very intentional.
And again that override is desired (at least could be, per-asset) if we want to specify border_color.

[rparrett]

"globally" is the wrong word, but as-is, you would not be able to override one of those other fields without nuking the value of address_mode_u and address_mode_v from the glTF, right?

Sorry if I'm being obtuse here.

[axlitEels]

Yes, the only field that can be modified without nuking glTF's sampler data is anisotropy_clamp. I don't see a solution there which would be neat and flexible at the same time.
I'm actually OK to implement the default_sampler alternative, but if I was to decide I'd go with my first proposal.

[thepackett]

Most games allow setting anisotropic filtering (among other settings) in the graphics options anytime, and I think that some way to facilitate that is needed in Bevy (possibly starting out as a third party crate). This pr only allows changing the anisotropic filtering level when loading the asset, and only for Gltf assets. I suppose the question is whether or not this solution sufficient to meet the needs of users until a more sophisticated solution is made, and I don't have the expertise to make that judgement.

[rparrett]

and only for Gltf assets.

We have https://docs.rs/bevy/latest/bevy/prelude/struct.ImagePlugin.html#structfield.default_sampler, but this is not utilized by GltfLoader (and I'm not sure how it could be).

So something along the lines of this PR seems useful.

[thepackett]

We have https://docs.rs/bevy/latest/bevy/prelude/struct.ImagePlugin.html#structfield.default_sampler, but this is not utilized by GltfLoader (and I'm not sure how it could be).

Huh, I didn't know about that. Would a more desirable behavior be to use the default sampler to set any sampler settings that are unspecified by the Gltf? I took a glance at the definition, and we could store the default sampler in the GltfLoader in the same way we store it in the render app, so it should be possible. It could only be set once at the beginning of the app, but that's true for the render app too.

[axlitEels]

I looked more into it and yes, ImagePlugin.default_sampler is a no-go because it immediately gets buried in RenderApp, and then other samplers refer to it through enum variant, no way we're pulling it from there to modify it's clone.
And there's a big issue rising. AF is a must and as @thepackett said we need a sophisticated solution to tweak texture filtration anytime. My PR kinda allows for a solution to change AF at runtime by reloading all the assets (ouch!) and many games do it like that, but beyond that I'm afraid any less kludgy solution would be a total refactor of how image samplers are both stored and processed, and this will take ages.

[rparrett]

My PR kinda allows for a solution to change AF at runtime

Or by requiring an app restart. That's definitely a better situation than what we have now.

Right now, you can technically you can use a non-1 anisotropy_clamp for your glTF scenes if:

• You use RenderAssetUsages::all(), which you likely wouldn't want to do otherwise.
• You wait until your assets load, and then update all of the Image assets after-the-fact

I took a glance at the definition, and we could store the default sampler in the GltfLoader in the same way we store it in the render app

Might be able to get away with only storing a descriptor.

[thepackett]

I looked more into it and yes, ImagePlugin.default_sampler is a no-go because it immediately gets buried in RenderApp, and then other samplers refer to it through enum variant, no way we're pulling it from there to modify it's clone.

This is true as it's currently written, but there's nothing stopping us from having the ImagePlugin store a copy of the default sampler in the world for us to access during the GlftLoader initialization. I don't think that would be an issue.

This, with slightly modified form of your current solution where we have an Option<ImageSamplerDescriptor> and an "override everything" bool in the GltfLoaderSettings, would allow us to:

1. If you're content with everything using the same sampler settings, you can simply set the default sampler to your liking and then use asset_server.load() on all your Gltf assets without needing to worry about any extra boilerplate. By default, the Gltf loader will not override any asset specific settings, so this means you can still define sampler settings in your Gltf assets on an asset by asset basis.
2. If you do need special settings for a particular asset, you can use asset_server.load_with_settings().
   • If you don't want to override all of the asset's sampler settings, you could replace the default_sampler with your own. (stored_default_sampler is queried from the resource we would store the default sampler in):

commands.spawn(SceneRoot(asset_server.load_with_settings(
    GltfAssetLabel::Scene(0).from_asset("models/test-scene.gltf"),
    |settings: &mut GltfLoaderSettings| settings.default_sampler = Some(ImageSamplerDesciptor{
        anisotropy_clamp: 16,
        ..stored_default_sampler.clone()
    })
)));

• If you do want to override all the asset's sampler settings, then you could use it like this:

commands.spawn(SceneRoot(asset_server.load_with_settings(
    GltfAssetLabel::Scene(0).from_asset("models/test-scene.gltf"),
    |settings: &mut GltfLoaderSettings| settings.default_sampler = Some({
        ImageSamplerDesciptor{
            anisotropy_clamp: 16,
            ..stored_default_sampler.clone()
        });
        settings.override_asset_settings = true;
)));

3. And if you simply wanted to override an asset specific settings with the default samplers settings, then you could just do this:

commands.spawn(SceneRoot(asset_server.load_with_settings(
    GltfAssetLabel::Scene(0).from_asset("models/test-scene.gltf"),
    |settings: &mut GltfLoaderSettings| settings.override_asset_settings = true
)));

Would something like this be the best of both worlds, at least until there's a more robust system in place that supersedes it?

[axlitEels]

Perfect! I'm going to write that after #15994 gets merged.

[thepackett]

Awesome, let me know if you'd like me to help with or write any portion of this PR, I'd be glad to help! I'll take a look at #15994 as well.

[thepackett]

@axlitEels Boop.
#15994 has been merged, so this should be unblocked now.
This feature would be useful for me too, so I'd be happy to pick this up if you've gotten busy with other things!

[axlitEels]

@thepackett Yes, I noticed. Was working on a different thing in my project, didn't want to mix it up. I'll begin today or tomorrow.

I'm thinking about how the default descriptor should be stored. Rn it feels like a resource in main world with a single get() method but it could also host a more native approach to modify the default sampler (which is stored in a similar resource in render world). The issue is that this will definitely add a system to ExtractSchedule and I'm afraid to touch that. Feature for future PR or that's silly?

[axlitEels]

@thepackett thanks again for all the help!
@rparrett can you review?

[thepackett]

I made a couple of documentation suggestions, but otherwise this looks good to me!

[greeble-dev]

Clicking approve as I've partly tested the changes (overriding via GltfLoaderSettings) and can't see any issues. Just added one suggestion to fix a clippy error.

There's one problem I ran into while testing. If I load the same asset multiple times with different settings then it doesn't work - the settings from the first load seem to override later loads. I'm assuming that's a limitation of the asset system rather than a problem with this PR. If this PR lands then I might investigate some more and file a bug or feature request.

In case it's useful, the example is on this branch: https://github.com/greeble-dev/bevy/tree/pr/17875 . It technically works right now, but only by duplicating the asset.

[thepackett]

There's one problem I ran into while testing. If I load the same asset multiple times with different settings then it doesn't work - the settings from the first load seem to override later loads. I'm assuming that's a limitation of the asset system rather than a problem with this PR. If this PR lands then I might investigate some more and file a bug or feature request.

In case it's useful, the example is on this branch: https://github.com/greeble-dev/bevy/tree/pr/17875 . It technically works right now, but only by duplicating the asset.

I helped test this, and I can confirm that the issue is in the asset server and I've made an issue for it here #18267.

The TLDR is that the asset server doesn't really support reloads fully. Reloads are accomplished by forcibly spawning a load task for an asset, but this does not forcibly spawn load tasks for dependencies of an asset, such as an image in this particular case.

[greeble-dev]

Ah, gotcha, I didn't see that bug.

I'm kinda tempted to make a PR with my example since it will serve as a repro for #18267? Let me know if that sounds worthwhile.

[thepackett]

@greeble-dev IMO, It would be nice to have an example to show off the behavior in this PR, as well as to act as a gltf reloading test.

Incidentally, any assets that are loaded as labeled subassets should still reload properly (I didn't actually test), since they don't go through the asset server in the same way. The gltf loader will treat images that are embedded directly in a gltf view (ie, stored directly in the gltf/glb, or in a .bin file, ie not stored as a URI to another file) as labeled subassets. As a test case / reproduction, I suspect it would be useful to use a gltf file that stores images in both ways so that it run and test both code paths.

[alice-i-cecile]

@axlitEels this looks fantastic; can you resolve merge conflicts and I'll get this in for you?

[axlitEels]

How about making this constructor pub(crate)? This is supposed to be a resource only GltfPlugin would need to insert. Any attempt of inserting one's own DefaultGltfImageSampler would be a conflict with GltfPlugin.

[axlitEels]

I think I have to @alice-i-cecile
Because otherwise it's ready for merge

[alice-i-cecile]

I prefer the simpler and more flexible fully public design here.

[greeble-dev]

IMO, It would be nice to have an example to show off the behavior in this PR, as well as to act as a gltf reloading test.

Followed up in #19095 and #19094.