Gltf loader reads texCoord field of texture

[bugsweeper]

Objective

• Fixes Gltf texture not rendering properly #12496 .
• Fixes Incorrect UV map used when using Lightmap component #13122 .
• Fixes GLTF with 3 UV Maps throws an error  #12995 .

Solution

Gltf loader reads texCoord field of each texture except lightmap, when finds TEXCOORD_n with same index n selects ATTRIBUTE_UV_0, otherwise (in case of lightmap) selects ATTRIBUTE_UV_1. This workaround according to the way bevy_pbr uses uv attributes

[bugsweeper]

This workaround doesn't work if textures depend on different texCoords, for example #13086. Full correct solution includes change bevy_pbr to use (or abble to use) different ATTRIBUTE_UV_n for each texture. @alice-i-cecile Do we want to dig so deep?

[alice-i-cecile]

I'm personally okay with this as a partial fix, but obviously a full fix would be valuable down the line. Actually following the gltF spec is valuable, and this is a reasonable and useful part of it, but there's no need to block a 90% solution IMO.

@superdump @IceSentry @robtfm, as SME-Rendering I think this decision is up to you.

[robtfm]

apologies if i misunderstood something - it looks like this approach will apply (from the texcoords you've added to the hashmap) whichever is specified last in the mesh primitive. that sounds like it is probably worse than assuming texcoord(0).

i expect meshes which use multiple distinct uvs will normally have the base texture uv coords first, so in those cases we will be using the wrong coords for the base color texture (where previously we would use the wrong coords for metallic/occlusion/normals).

i think it would be better if the HashSet was an Option that stored the most important use, then we could choose to use the most appropriate coords (if base color texture exists, map the relevant texcoords to UV_0. otherwise if normal map exists map those, else metallic else occlusion etc).

[bugsweeper]

that sounds like it is probably worse than assuming texcoord(0)

As I mentioned earlier this workaround doesn't work in all cases, correct solution is to use separate uv_map for each texture. HashSet doesn't keep single value, it keeps all of them.
Let's try to describe situation more thoroughly. Lets imagine example when there is two textures (base color and occlusion). What happen depending on texCoord values (numbers of UV maps)?

texCoord values	Behavior before changes	Behavior after changes
Both are 0	Correct	Correct
Values don't match	Incorrect	Incorrect
Values match but not 0	Incorrect	Correct

Do you think that if mesh uses baseColorUV instead of normalUV for normal texture, the mesh will look correct?

[robtfm]

Do you think that if mesh uses baseColorUV instead of normalUV for normal texture, the mesh will look correct?

no, but i think errors in occlusion map uvs or normal map uvs are probably less noticeable than errors in base color texture uvs... and those features can be disabled, so it's still possible to get something close to correct, if not fully-featured.

how about, use an option to record what uvs we are going to take (starting from most important), and then if the uv is already chosen, discard lower-priority textures that don't use the same?

so if base color texture uses texcoords(0) and normal map uses texcoord(1), don't add the normal texture and use texcoord 0.
if normals use texcoord(1) and occlusion uses texcoord(0), don't add occlusion and use texcoord 1.
etc

[robtfm]

one issue with my suggestion is that gltfs might use multiple identical sets of texcoords, in which case it would currently be working but would lose features with this strategy. i don't know if that's common.

[robtfm]

HashSet doesn't keep single value, it keeps all of them.

(just wanted to point out that while the hashset has all of them, only one ATTRIBUTE_UV_0 can exist on the final mesh. the code would repeatedly overwrite the mesh attribute so finally the uvs would be whichever was specified last in the gltf.)

[bugsweeper]

@robtfm Looks like there is alternate PR which already fixes even #13086 , which my solution is not supposed to fix.