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

Fix LOD generation for meshes with tangents & mirrored UVs #94682

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Jul 23, 2024

When UVs are mirrored in a mesh, collapsing vertices across the mirroring seam can significantly reduce quality in a way that is not apparent to the simplifier. Even if simplifier was given access to UV data, the coordinates would need to be weighted very highly to prevent these collapses, which would penalize overall quality of reasonable models.

Normally, well behaved models with mirrored UVs have tangent data that is correctly mirrored, which results in duplicate vertices along the seam. The simplifier automatically recognizes that seam and preserves its structure; typically models have few edge loops where UV winding is flipped so this does not affect simplification quality much.

However, pre-processing for LOD data welds vertices when UVs and normals were close, which welds these seams and breaks simplification, creating triangles with distorted UVs.

We now take tangent frame sign into account when the input model has tangent data, and only weld vertices when the sign is the same.

Fixes #67891.

When UVs are mirrored in a mesh, collapsing vertices across the
mirroring seam can significantly reduce quality in a way that is not
apparent to the simplifier. Even if simplifier was given access to UV
data, the coordinates would need to be weighted very highly to prevent
these collapses, which would penalize overall quality of reasonable
models.

Normally, well behaved models with mirrored UVs have tangent data that
is correctly mirrored, which results in duplicate vertices along the
seam. The simplifier automatically recognizes that seam and preserves
its structure; typically models have few edge loops where UV winding is
flipped so this does not affect simplification quality much.

However, pre-processing for LOD data welded vertices when UVs and
normals were close, which welds these seams and breaks simplification,
creating triangles with distorted UVs.

We now take tangent frame sign into account when the input model has
tangent data, and only weld vertices when the sign is the same.
@zeux
Copy link
Contributor Author

zeux commented Jul 23, 2024

Before:

image

After:

image

Notes:

The mesh on the left is from #67891; the mesh on the right is the same GLB file with explicitly generated tangents via gltf-transform. This PR relies on correctly generated tangents: when UVs are mirrored, the vertices on the mirror seam need to be duplicated and have two tangents, one pointing one way and the other pointing the other way (with TBN winding flipped between the two).

The reason I mention this is that the way Godot generates new tangents is actually incorrect: it does this using the current geometry indexing, instead of doing unindex/reindex. This prevents tangent generation from creating new vertices. This was changed ~6 years ago to make morph targets work, and since nobody noticed in 6 years it's probably fine (?), but this happens to work properly because the model in question has the seam in the index data in GLB whether or not tangents are generated, so things work out "magically". If you instead optimize the basketball GLB, welding the vertices in the file itself, then this PR does not fix the UV mapping issue because tangent generation is incapable of "unwelding" the vertices back.

So in that respect this is a partial fix, since the model needs to either carry tangent data or not result in incorrect tangents generated during import; otherwise the LODs may still result in significant texture distortion along the mirror seams. But it does fully fix the referenced issue.

@zeux
Copy link
Contributor Author

zeux commented Jul 23, 2024

Oh, for completeness, wireframe:

Before:

image

After:

image

You can see the simplifier preserving the tangent seams automatically which leads to correct texturing - these are the black lines. Note that the silhouette of these lines is getting simplified, it's just that vertices can't move away from that edge loop.

@Calinou Calinou added bug topic:import topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jul 24, 2024
@Calinou Calinou added this to the 4.3 milestone Jul 24, 2024
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.

Wow thanks for looking into this bug from so many years ago!

@fire
Copy link
Member

fire commented Jul 24, 2024

Do you think we should also do a unindex index operation for tangents in a separate pr?

@akien-mga akien-mga merged commit 543e438 into godotengine:master Jul 24, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@Saul2022

This comment was marked as off-topic.

@zeux zeux deleted the lod-uv-mirror branch July 24, 2024 15:07
@zeux
Copy link
Contributor Author

zeux commented Jul 24, 2024

Do you think we should also do a unindex index operation for tangents in a separate pr?

This is a little delicate: the original change I referred to from 6 years ago is #23991, which was explicitly fixing tangent generation disrupting indexing for morph targets. Properly handling this requires tracking indexing and remapping target arrays, maybe using something similar to ImporterMesh::Surface::split_normals. I'm also not sure whether unindex/reindex affects tangent generation time meaningfully, that would need to be tested. So overall ideally I do think Godot should generate the tangents correctly when UVs are mirrored regardless of the input indexing, but this is delicate enough that maybe it's fine to wait for somebody to run into this problem in a visible manner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:import topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visible LOD artifacts in Basketball
5 participants