Skip to content

Conversation

@david-lively
Copy link
Contributor

@david-lively david-lively commented Oct 3, 2025

Description

Previously, the glTF importer would ignore tangents in the model. This PR ensure that tangents in the vertex stream are properly imported and transferred to the relevant vertex buffer.

(This depends on #611 so merge that first.)

Author checklist

  • I have submitted a Contributor License Agreement (only needed once).
  • I have done a full self-review of my code.
  • I have updated CHANGES.md with a short summary of my change (for user-facing changes).
  • I have added or updated unit tests to ensure consistent code coverage as necessary.
  • I have updated the documentation as necessary.

Testing plan

Load a glTF that includes tangents, such as the glTF Normal Tangent Mirror Test.
In Unity's Render Debugger, activate the Tangent material override and verify that tangents are included.
By default, Unity provides (1,0,0) for tangents if they are missing. The tangent vertex attribute override should show something other than (1,0,0) .

@david-lively david-lively requested a review from j9liu October 3, 2025 18:42
@j9liu j9liu changed the base branch from main to flip-textures-and-uvs October 3, 2025 18:47
@david-lively david-lively marked this pull request as ready for review October 3, 2025 19:01
@david-lively david-lively marked this pull request as draft October 6, 2025 18:14
@david-lively david-lively marked this pull request as ready for review October 6, 2025 18:39
Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

Thanks @david-lively! Still waiting on my slow machine to build so I can actually try this out, but I'll leave some comments in the meantime.

*reinterpret_cast<Vector3*>(pWritePos) = positionView[vertexIndex];
pWritePos += sizeof(Vector3);

if (computeFlatNormals) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I'm not sure how much of an impact it has here, but we tend to avoid putting if statements in a loop body if possible. I know in Unreal we prefer to use near-identical for loops if it can avoid a few if statements mid-loop. Maybe others have an opinion?

Copy link
Contributor Author

@david-lively david-lively Oct 10, 2025

Choose a reason for hiding this comment

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

@j9liu Hmm. Not sure I see the benefit here. The existing implementation already had if for vertex colors and normals. To completely get rid of the conditionals, we'd need 8 nearly-identical permutations of this loop.

@j9liu
Copy link
Contributor

j9liu commented Oct 9, 2025

@david-lively Forgot that you mentioned you need to fix the material, so I'll put this on draft until that is fixed. (Otherwise we have model-provided tangents with a faulty appearance...)

image

@j9liu j9liu marked this pull request as draft October 9, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants