Support alternative UV layers for a handful of texture types

[MAG-MichaelK]

fixes #80858

The image below shows the current result of importing in 4.1.1-stable on the left for both SheenChair and a custom GLTF that uses UV2 for baseColorTexture.texCoord, and the result of doing the same with a build of Godot that includes my changes.

I have added texCoord support for all 8 UV layers for albedo, metallic/roughness, normal, ambient occlusion, and emissive material textures. This required a breaking change in the way that alternative UV layers are specified for detail and AO textures.

[jcostello]

UV2 is not for lightmaps?

[MAG-MichaelK]

With GLTF, it seems any UV layer can be used for anything. I've seen it used for AO, Normal, Roughness, and Albedo textures. With the EPIC_lightmap_textures GLTF extension, any UV layer can be specified as the lightmap layer.

[lyuma]

Welcome! It's great to see new contributors to Godot, and from a technical perspective, your implementation looks great (though I prefer vector2 for scale and offset when triplanar is not used).

That said, this is not the only thing required for implementing a new feature, as I consider this an enhancement rather than a bugfix. Specifically, a feature proposal over at https://github.com/godotengine/godot-proposals/issues needs to be written to justify this change.

Personally, even though it's implemented correctly, I am scared of this change for two reasons. Primarily, I'm worried about overhead in the default standard shader caused by potentially having more varying data sent between stages. I'm also having a bit of trouble following the additional complexity in the shader since a lot of variable names are looked up from an array, and in particular, there is a need to ensure we keep backwards compatibility so we would need to verify it does not change behavior unless the extra UV sets are being used.

Now for the actual details...

I think there is an argument to be made for having AO on a different texcoord, as in the SheenChair example, since it is could be baked separately from the underlying texture. So I would be open to considering this specific usecase, which does not require adding the other 6 UV channels. Perhaps a separate proposal can be made for the specific features that are needed.

That said, the idea of using TEXCOORD_1 for albedo does not feel justified to me. The example you present, while valid glTF, sounds like it was done artificially. There are many cases where the glTF 2.0 specification is overly broad, and it is expected that assets follow common practice. There are cases where one may use a different texture channel with different scaling from the base texture, such as for detail texturing, but godot locks the scale to the texcoord itself, so I'm not sure this would solve the detail texturing case. I might be open to this if a concrete usecase could be presented, but it should be done in a proposal with an example model.

Regarding the additional texcoords, I cannot find in the glTF sepcification that support for 8 texcoord set indices is mandatory, and I think the level of complexity needed does not justify the change, and the sheen and cactus examples also do not require the additional UV channels. I think this should be justified in its own proposal, with concrete usecases that cannot be done with 2 texcoords.

See also: Unity glTFast also has an open issue related to this: atteneder/glTFast#206 - I agree with their initial assessment:

This is considered lowest priority, since there's no demand (zero requests) and it potentially makes the shaders more complex (or adds a significant amount of shader variants).

In short, the key here is to be specific. We can't change everything to solve a theoretical spec compliance issue. To be convinced this is worth it, I either need to see justification, in the form of specific models needed by specific applications/games, and it needs to be simplified down to specific features needed.

[MAG-MichaelK]

I made this change to fix an issue I am seeing myself in a project that I am working on. I am using Godot in my free time to create a client for a library we develop where I work. The intent is to showcase how easy it is to integrate in any editor you choose. All of our clients makes heavy use of GLTF models, most of them authored with Unreal. The Unreal GLTF exporter likes to spit out files that Godot won't load. For example, I needed to fix exported GLTF files containing mesh primitives that have 0 vertices (something that is not allowed by the specification, but that many GLTF viewers support anyway), which I was able to do using a GLTFDocumentExtension class. However, I felt that attempting to do the same to address the issue with texture UVs would be far more work, so I opted to modify the engine itself to adhere more closely to the specification.

The below image is taken from the the GLTF 3.0 reference guide. It directly relates the texCoord property of materials with the TEXCOORD_X property of mesh primitives. As mesh primitives can have TEXCOORD_0 all the way to TEXCOORD_7, it's fair to believe that texCoord can also be in the range of 0 to 7.

Although I have not yet encountered any models that do use any UV layers besides 1 and 2 for anything (besides EPIC lightmap textures), I have not had the time to check some of the other models that were not previously importing correctly.

I have to disagree with your assertion that this be considered an enhancement rather than a bug fix, as Godot currently completely ignores the texCoord property of textures, meaning any GLTF file that does not use UV1 for all of its textures would not import correctly. I feel that we should strive to be able to import all of the official GLTF sample models, taking into account any extensions that aren't yet widely adopted.

[fire]

@lyuma

The Unreal GLTF exporter likes to spit out files that Godot won't load.

This is a demonstrable usecase. I'm ok improving compatibility for Unreal Engine import.

[fire]

@MAG-MichaelK

Can you apply this diff?

diff --git a/scene/resources/material.cpp b/scene/resources/material.cpp
index 8b7c5ab..f063926 100644
--- a/scene/resources/material.cpp
+++ b/scene/resources/material.cpp
@@ -1208,7 +1208,7 @@ void BaseMaterial3D::_update_shader() {
 	} else {
 		if (texture_uvs[TEXTURE_ALBEDO] == TEXTURE_UV_1 && flags[FLAG_UV1_USE_TRIPLANAR]) {
 			code += "	vec4 albedo_tex = triplanar_texture(texture_albedo,uv1_power_normal,uv1_triplanar_pos);
";
-		} else if(texture_uvs[TEXTURE_ALBEDO] == TEXTURE_UV_2 && flags[FLAG_UV2_USE_TRIPLANAR]) {
+		} else if (texture_uvs[TEXTURE_ALBEDO] == TEXTURE_UV_2 && flags[FLAG_UV2_USE_TRIPLANAR]) {
 			code += "	vec4 albedo_tex = triplanar_texture(texture_albedo,uv2_power_normal,uv2_triplanar_pos);
";
 		} else {
 			// GLTF supports up to 8 UV channels

[AThousandShips]

Properties and methods need to be documented

[lyuma]

(TL;DR: Create a godot issue for each gltf issue with minimal project, attaching a default project zip with the affected gltf inside into the "Minimal Reproduction Project (MRP)" section. That gives us a place to work from.)

Generally speaking, contributors must be making a PR which either solves an issue (the original issue needs lists a couple specific cases that could be issues), or which addresses a feature proposal (which I suspected 8 texcoords falls under).

It should be easy to file an issue: you don't have to say much other than show the screenshots. By far the most important piece of an issue report is the " minimal reproduction project" (MRP) which demonstrates the issue on a version of Godot and can be used with a PR like this one to test that the issue is solved. With import issues, often it's just a new project with the one gltf file inside it, zipped up.

From what I understood, the specific issue here is related to the lightmap texture assigned to "texcoord 2" (UV channel 3). I've never seen such a file before, and as someone who has no unreal experience, we need at least one such a GLTF test file so we have a concrete test case to fix, ideally attached to such an issue.

I'm sorry for being so strict. If this was a one line bugfix or something relatively simple that's one thing. But this PR affects hundreds of lines of code, and so at the very least we need to have an issue being solved.

I am still concerned most specifically with the additional complexity and possibly overhead in the approach being used to extend support to 8 uv channels (CUSTOM1/2/3) into BaseMateiral3D. Aside from the compatibility breakage, the change also introduces a ton of new scale and offset variables which will be unused for 99.99% of users.

My personal opinion is to be fully compliant with glTF KHR_texture_transform, the parameters should be per-texture not per-UV slot. The technically correct fix here isn't to add uv1_scale..uv8_scale, but instead to add albedo_scale .... normal_scale. But! if they are the same, they should be shared per texture where they are the same to minimize the number of varyings and to avoid increasing the overhead for users of BaseMaterial3D who do not use different scale per texture slot. (And don't get me started on texture transform rotation, which I've never seen anybody use possibly due to lack of support in many engines including Godot)

Long story short, perfect gltf compatibility is difficult, possibly increasing performance overhead and maintainability overhead. That's why I prefer to solve issues on a case by case basis, which requires example files / issues, and might not require extending to 8 UV slots.

[clayjohn]

I fully agree with Lyuma's comment above, but I would also like to take a step back and provide some context.

First off, this is amazing work and thanks for choosing to upstream something you are working on for a personal/work project!

Second, in its current state we would have a lot of trouble merging this PR in its current state, primarily for 2 reasons:

1. There has been no user demand for this (as exemplified by the lack of issues/proposals)
2. This PR adds significant complexity to a very frequently used part of the codebase (Material) and potentially hurts performance for many users.

To understand why we are so strict about seemingly useful PRs, it is good to take a look at our developer best practices Particularly the problem always comes first and cater to common use cases, leave the door open for rare ones. The underlying principle here is that we don't add complex features in case users might need it. We only add as much complexity as is needed, and then we can adjust later.

Applied to this PR, to me it sounds like we could get away with only supporting 2 UV channels as long as we respect the user's choice between those 2 UV channels. Having support for UVs 3-8 is more added because we can, rather than because users need it. Given our best practices, it might be best to scale this PR down to supporting the first 2 UV channels until we have a tangible use case for supporting more.

Looking at the GLTF 2.0 specification, it looks like it doesn't require having any more than 1 UV channel, but it does recommend having "at least" 2.:

Client implementations SHOULD support at least two texture coordinate sets, one vertex color, and one joints/weights set.

To me, that further indicates that we might want to limit our support to 2 channels as well.

[MAG-MichaelK]

My personal opinion is to be fully compliant with glTF KHR_texture_transform, the parameters should be per-texture not per-UV slot.

This is a really good point. Put that way, it probably could do with not being addressed in this PR.

I am more than happy to reduce support for UV channels to just 2 instead of the full 8. I haven't come across any models so far that need all 8 channels for these texture types.

[Lexpartizan]

My personal opinion is to be fully compliant with glTF KHR_texture_transform, the parameters should be per-texture not per-UV slot.

This is a really good point. Put that way, it probably could do with not being addressed in this PR.

I am more than happy to reduce support for UV channels to just 2 instead of the full 8. I haven't come across any models so far that need all 8 channels for these texture types.

I use all 8 UV channels in my work. And I wouldn't mind a couple more. I'm using a texture array and to define a tile, I need 2 UV. This allows you to have 64 512 px materials on one 4096 px texture. I make the materials white and choose a color or pattern for them-a picture, etc. using another texture array. That's another 2 UV. I made a texture array with details and labels. This allows me to diversify the models. And all this in one universal material, which I have suitable for thousands of different models. Which gives significant savings in rendering calls. One channel is needed for lightmap.
So don't say that if you don't need something, then others don't need it. learn more about my methods https://lexpartizan.dreamwidth.org/

[AThousandShips]

So don't say that if you don't need something, then others don't need it. learn more about my methods

A single exception does not undo the rule, most people won't need this and adding more layers isn't free, so a balance has to be struck, and the performance might affect everyone regardless if they use the feature or not

[Lexpartizan]

@clayjohn

Applied to this PR, to me it sounds like we could get away with only supporting 2 UV channels as long as we respect the user's choice between those 2 UV channels. Having support for UVs 3-8 is more added because we can, rather than because users need it. Given our best practices, it might be best to scale this PR down to supporting the first 2 UV channels until we have a tangible use case for supporting more.

To me, that further indicates that we might want to limit our support to 2 channels as well.

Please leave the option to work with 8 channels. I use all 8. Texture array requires 2 channels in the simplest use case. And it is very convenient to store user data for shaders in them.

[MAG-MichaelK]

This PR is meant to specifically address an issue with GLTF importing. I'm sure a discussion around adding full support for 8 UV channels can be had, but I don't feel it is relevant to this thread.

[clayjohn]

@Lexpartizan it sounds like you are totally misunderstanding what this PR does and misinterpreting the discussion. We have no intention of altering our current support for 8 UV channels. What this PR allows you to do is specify which UV channel a given texture uses in the BaseMaterial. It doesn't impact your custom models or custom shaders whatsoever.

The discussion around 2 or more is linked solely to the fact that GLTF typically supports using either UV1 or UV2 for textures, but implementations are allowed to allow using more. So we have to decide if we will support the typical UV1 + UV2 or more.

[Lexpartizan]

@Lexpartizan it sounds like you are totally misunderstanding what this PR does and misinterpreting the discussion. We have no intention of altering our current support for 8 UV channels. What this PR allows you to do is specify which UV channel a given texture uses in the BaseMaterial. It doesn't impact your custom models or custom shaders whatsoever.

The discussion around 2 or more is linked solely to the fact that GLTF typically supports using either UV1 or UV2 for textures, but implementations are allowed to allow using more. So we have to decide if we will support the typical UV1 + UV2 or more.

Thanks! I have all i need!

[fire]

I will try to get some discussion on this for the next Asset pipeline meeting.

[Repiteo]

Superseded by #96748; thanks for your contribution!