Respect texture filtering when importing GLTF

[LunaticInAHat]

This PR updates the GLTF importer, such that imported materials have the correct texture filter modes set (with respect to the sampling specified in the imported file).

This PR builds on work that was started in PR #54044, and forward-ports it to 4.x. The delta between making this work in 4.x vs 3.x is fairly significant, so also have a 3.x branch available for after a fix gets merged to master (that 3.x branch is based on what @TheTophatDemon started, with some small corrections that make it work properly on the GLTF files they were having trouble importing)

Bugsquad edit: This closes #61232.

[LunaticInAHat]

As a sidenote that perhaps someone more experienced with C++/Godot could help me with: I would liked to have kept the filter mode enum as an enum class, for a little bit of enhanced type safety (as TheTophatDemon originally had it), but when declaring it as enum class and trying to use it as a return type / argument type (for e.g. set_min_filter()), the build just ends up blowing up with errors such as:
./core/variant/binder_common.h:705:15: error: no match for 'operator=' (operand types are 'Variant' and 'GLTFTextureSampler::FilterMode')

I was ultimately able to work around this by just using int for the return / argument types instead, and casting to the enum, but this feels like a hack. I'd rather do it the right way, I just don't know how.

[fire]

Approved cicd workflows.

[LunaticInAHat]

Another side-comment: doctool doesn't seem to mind that I've added a new class (GLTFTextureSampler), but no documentation file for that class. Shouldn't it be complaining about that?

[fire]

Approved the cicd run. Also note that there is a policy towards squashing prs. There's a pr guide.

https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html

[LunaticInAHat]

Can you be more specific about what you would like squashed? Do you want it all compacted down into a single commit? I noted that the guide mentions "we favor commits that bring the codebase from one functional state to another functional state". I tried to make sure that each commit in this PR reflects a functional state (although there is obviously a progression of increasing functionality), but if you'd rather see it mashed into a single commit, I can do that.

[fire]

Was busy. I'll review as soon as I can.

Yes. Squash into one commit. You can backup if you want to another branch.

[LunaticInAHat]

Squashed as requested.

[fire]

I will be using https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/TextureSettingsTest to check.

Edited wrong one.

Reviewing the pr and comparing against some of the test cases. https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#reference-texture

Poked the khronos community why there's no feature test for image modes and filtering.

https://github.com/KhronosGroup/glTF-Asset-Generator/blob/master/Output/Positive/Texture_Sampler/README.md

[LunaticInAHat]

I have to admit that I don't think I tested or paid close attention to the wrapping modes when I did this. My usecase was for min/mag filters, so that's what I was looking at. The wrap modes are serialized/deserialized, but at a glance I can't see them actually being applied to the materials anywhere. In fact, as I look at things, I am suddenly filled with uncertainty about whether wrap/mirror is a Material property at all, or instead a property of the underlying Texture.

[fire]

I think we should do the filtering modes and punt the wrap mirror modes.

[LunaticInAHat]

Okay. Should I remove the logic for serializing/deserializing the wrap modes, or leave it in place, just in case someone gets ambitious in the future and wants to make those modes actually work?

[fire]

I'll spend some time reviewing today.

Can you rebase?

[fire]

remove the logic for serializing/deserializing the wrap modes, or leave it in place

I think we should not speculatively add the logic for serialize or deserializing code since we have a policy in other area that comment out code should be deleted. This is not the same, but the idea is the code should not be speculative.

[Sirus97]

@fire @LunaticInAHat I'm looking forward to this for 3.x. Is there any way I can help make this possible?

[fire]

This appears salvaging.

[Sleggie]

This would certainly be an important feature for 3.x

[fire]

If remember correctly, this pr is blocked because of the Godot Engine 4 renderer. It's unclear how to store a data model of something that cannot be rendered in GodotEngine.

1. Godot Engine doesn't support repeat or clamp to edge at all I think.
2. In Godot Engine, the filtering mode is per material rather than per texture.

@clayjohn Can you provide background on these rendering details?

[LunaticInAHat]

Thanks for the suggestions on making CI pass, and for the explanation for the delayed review :)

As Fire points out, there is a certain amount of impedance-mismatch between the way that GLTF stores texture samplers, versus how the Godot renderer would like to apply them. I believe that this PR fixes the most common usecase, where users just want nearest filtering on their models (with either wrap+wrap or clamp+clamp), however it can't support all the combinations supported by GLTF. A couple examples of things that won't work are:

• Materials where the S wrapping mode != the T wrapping mode
• Materials where different map textures (normal, roughness, etc.) are supposed to be filtered/wrapped differently than the albedo texture
  Undoubtedly there are assets out there that use those unsupported combinations.

If I recall correctly, some of those properties used to be per-texture (per-image) in 3.x, so I have assumed that making them global to the material was an intentional choice in 4.x, and that there wouldn't be any interest in changing the material system / renderer to accommodate them.

[Sirus97]

I don't know how often normal maps require different flags than albedo maps.

In our use case we require:

• Mipmaps enabled
• Filter disabled
• Ansiotropic enabled

Is it feasible to implement toggleable parameters in the import tab?

[Sirus97]

I don't know how often normal maps require different flags than albedo maps.

In our use case we require:

* Mipmaps enabled

* Filter disabled

* Ansiotropic enabled

Is it feasible to implement toggleable parameters in the import tab?

@LunaticInAHat
I'm wondering because of the mismatch between glTF and Godot's renderer.
I believe added support is out of scope of this pull request.
What our team is looking for is to substitute the import of many texture files using glTF Separate (.gltf+ .bin + textures) with one glTF Binary (.glb).
Note: We use 3.x which means the properties are per texture.

Looking at the code I notice the mention of mipmaps but not ansiotropic.
Does this mean it will respect the mipmaps setting in addition to filtering but not the ansiotropic setting?

As far as I can tell the import process applies default properties.
My hope is that these be accessible so they can be altered from the default.

Edit: I'm taking look at 4.0 to understand it better and will update my comment if there are no new replies.

[clayjohn]

For now I would stick within the limits of what is exposed via StandardMaterial/BaseMaterial.

[LunaticInAHat]

Yeah, I don't have any intention of expanding this PR to embrace anything beyond what the standard material classes expose. It is unfortunate that the materials don't support all the various sampler combinations that might be exported from art tools, but I don't think this PR should be blocked by that.

As far as anisotropy goes, as far as I can tell, GLTF does not store any information relating to anisotropic filtering (thus it cannot be imported from the GLTF). So, adjusting that will have to be a manual operation performed on the materials in Godot, after import.

It's not 100% clear to me why some of the CI builds are failing, at this point. I think I've solved the issue relating to missing documentation in my latest version of the PR, but the error in "Build godot-cpp test extension" is somewhat opaque to me.

[fire]

If you're able to help out, I wanted to do image comparison before and after using this list of models.

https://github.khronos.org/glTF-Asset-Generator/Output/Positive/Texture_Sampler/

I'm busy until the weekend, hope someone can help.

Then, I can approve.

[Sirus97]

@LunaticInAHat
I want to thank you for picking up the original PR and coming through.
It's too bad about the anisotropic setting.
After this PR gets merged what would be the next step to bring it to 3.x as well?

[Sirus97]

@fire I'm not sure how to use that asset generator but I confirmed it respects closest/nearest sampling on the Windows build.

[LunaticInAHat]

I already have a 3.x branch prepared, so once this PR gets merged, I'll open the PR for the 3.x backport. Then it should just be a question of testing, and re-review. The 3.x implementation is somewhat different from the 4.x implementation, due to larger changes in how materials & textures work, so testing results from one branch don't necessarily translate to the other.

As far as the image comparison requested by @fire goes, I just want to point out that we expect to fail tests 01 - 04, because they are using wrap modes where S != T.

[Sirus97]

@fire Do you want me to import those models into Godot and screenshot the material in the inspector?

[fire]

Yeah! and if you wish, can save the project and publish it so I can add it as a pr to godot-tests

[Sirus97]

@fire The assets seems to be broken. 😕

[fire]

I think this is a good enough case for a separate bugfix. uhhh i think we have to add a fake gltf root. But you seem confident

[clayjohn]

Code changes look good now. Once the docs are filled in this should be ready to merge!

[clayjohn]

CI is failing due to missing indentation. I have made code suggestions to make CI pass and finish filling out the docs.

[clayjohn]

Thanks for sticking with this one!