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

GLTF: Read material texture "texCoord" property on import #96748

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 9, 2024

Fixes #80858 and #93884 to the extent which is possible in the glTF code. Supersedes PR #80522.

Godot uses UV1 for the base color, normal, and metallic/roughness. It can also have occlusion and emission on either UV1 or UV2. The glTF spec allows any texture to use any UV map, so not everything in glTF is possible in Godot.

In the current master, the glTF import code does not read this property, and expects that every texture is on UV1. We can do better than that. With this PR, the glTF import code will do its best to consider the UV preferences of material textures, allowing for many more configurations such as color+normal+metallic on a different texCoord, or occlusion/emission on a separate texCoord. When the code encounters a configuration not supported by Godot, it will print a warning.

Here is the test file from #93884. Left is current master, right is with this PR.

Screenshot 2024-09-09 at 2 31 49 AM

Here is the test file from #80858.

Screenshot 2024-10-09 at 9 08 48 PM

This PR also handles the case of exporting materials with emission and/or occlusion on UV2, while previously they always exported as if they were on UV1. The code for exporting is much more straightforward than importing, the only changes are in the _serialize_materials function.

While not all glTF files will have their UV maps perfectly imported into Godot, this should be enough to ensure that exporting from Godot and importing back into Godot will preserve any UV2 map usage.

Also, I renamed some local variables for readability (ex: p -> mesh_prim_dict, sgm -> spec_gloss_ext_dict).

I'm labeling this as an enhancement because support for UV2 in glTF is a new feature, but arguably it's a bug because this is a case of Godot not following the glTF specification.

@fire
Copy link
Member

fire commented Sep 9, 2024

I am in favour of the proposal. Did not technical review yet.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronfranke what's a prose version of the algorithm for picking the coordinates?

@aaronfranke
Copy link
Member Author

@fire The prose version is that we want to read what's in the file, except that to fit within Godot's material system, we can only read at most two UVs, and the second UV can only be used for certain things. Therefore, we prefer reading the most important UV maps first to provide the best mapping, starting with the base color / albedo texture, and continuing on to less important UV maps.

@fire
Copy link
Member

fire commented Oct 7, 2024

So like a greedy algorithm that has 2 slots and from the inputs we fit the base color / albedo texture to the first slot and then the others.

@lyuma
Copy link
Contributor

lyuma commented Oct 30, 2024

I am not personally in favor of this proposal as is. In short, this creates a few hardcoded rules which impact the mesh depending on a series of conditions, and this logic will never be able to change without breaking compatibility.

Most importantly, I couldn't find the import option to control this. Having an option to control this behavior is an absolute necessity. For example, the change may alter the mesh channels of content already imported into Godot, perhaps using a custom ,tres material.

The thing I am worried about is that this unconditionally changes the order of the UV channels, based on materials which may even be replaced or swapped during the import process.

In my experience, it is common for the UV channels to have semantic meaning, for example TEXCOORD_1 is often used for lightmapping while TEXCOORD_0 is conventionally used for albedo. This flips it around, so that the material attached to a particular mesh determines the UV channels to more closely match with this semantic. While there is some sense in this, it means that the mesh data will be drastically affected by the presence of various settings on the materials.

For example, if the glTF is exported without materials, or perhaps if the user changes some of their material nodes in Blender before export, the order of mesh channels in Godot will be unexpectedly flipped, and the cause will be difficult to diagnose, for example depending on the presence of albedo or emission in blender nodes which may not even be used in Godot.

Still, I might be swayed to be okay with this as an import option.

@aaronfranke
Copy link
Member Author

aaronfranke commented Oct 30, 2024

@lyuma Currently, meshes are plainly broken on import if they are using different UV maps from what you mention is the common convention. I'm not inclined to keep adding more and more import options to preserve broken behavior. I prefer to just have the correct behavior be the only option.

If users are expecting UV maps to be preserved as-is, that is already not the case. Godot doesn't support more than 2 UV maps, so any others are discarded. Furthermore, currently UV2 is always unused on import, and can only be used with custom materials, this PR fixes that.

In my opinion, the primary focus should be to ensure that the glTF's intended appearance is correct in Godot, not in keeping texCoord 0 mapped to Godot's UV1. I don't believe it is worth supporting import of a file where the user explicitly wants Godot to use the wrong UV map. To me it just seems like nonsense behavior that there would be an option that says "yes, import me with the wrong visuals" for the sake of always using the first UV map as the used one. It's better to just always use the UV map that the glTF file says is the correct UV map.

As for a user exporting a glTF without materials and getting different behavior - I don't think it's reasonable to expect the same behavior between different files. Actually, in fact, it may be good that this is the case. This way, if users want the behavior you want, where texCoord 0 is imported as UV1, they can just not export materials. If the user is not putting materials in the glTF file, then they need to specify them in Godot anyway if they want materials.

@huwpascoe
Copy link
Contributor

I agree with what this PR is doing, the GLTF should be visually represented in engine as closely as possible, even if that means there are no guarantees of how data is mapped.

The thing I am worried about is that this unconditionally changes the order of the UV channels, based on materials which may even be replaced or swapped during the import process.

Yes, sometimes you do want explicit control over how things are mapped: #97756.
Perhaps rather than a script extension, it could be fully realized as a property group to override default behavior?
image

@aaronfranke
Copy link
Member Author

Updated to account for PR #94165, supporting animated UV maps on textures on both import and export to the best extent possible with Godot's material system. I tested UV map animation on the red chair and it works, and also tested the animation pointer test file again just in case and that also works.

@fire
Copy link
Member

fire commented Nov 6, 2024

I would ideally want to see something like #96748 (comment), do you think it's possible for us to do in this or another pr?

@aaronfranke
Copy link
Member Author

aaronfranke commented Nov 6, 2024

@fire There is PR #97756 from @huwpascoe which implements the ability to import custom attributes. That way users can use that system to preserve a specific UV map, separate from the material's UV1/UV2 maps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GLTF mesh primitive texCoord property ignored
4 participants