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

Add gram-schmidt fast approximation tangent generation algorithm #17989

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aloucks
Copy link
Contributor

@aloucks aloucks commented Feb 22, 2025

Provide a fast alternative algorithm for tangent generation

Fixes: #17834
Related (maybe): #17877

Solution

Add a new enum TangentStrategy that gives the user the option to select how tangents are generated.

The GLTF loader now has a property to set which algorithm is used for models that are missing tangents.

The FastApproximation strategy is implemented with the Gram-Schmidt fast approximation technique. The HighQuality variant uses MikkTSpace.

Testing

Unit tests have been added to assert that both tangent computation strategies produce similar tangents for simple shapes. I also tried to check several examples to verify that the visual results are acceptable with both algorithms. I wasn't able to tell any visible difference.

How can other people (reviewers) test your changes? Is there anything specific they need to know?

I checked the deferred_rendering, parallax_mapping, and anisotropy examples. Suspiciously, when I produced intentionally invalid tangents, the deferred_rendering and parallax_mapping examples seemed to render fine. I didn't try the anisotropy example with intentionally invalid values. I'm curious if there is an unrelated bug in the shader where tangents are ignored (or the examples I tested somehow didn't leverage tangents).

Migration Guide

  • GenerateTangentsError::MikktspaceError has been renamed GenerateTangentsError::AlgorithmError
  • The Mesh::generate_tangents and Mesh::with_generated_tangents methods are now deprecated. They are replaced by compute_tangents and with_computed_tangents, both of which accept a TangentStrategy parameter. The naming conventino of these methods now matches compute_normals.

@aloucks aloucks force-pushed the issue-17834-compute-tangents branch 5 times, most recently from c41e77e to e409343 Compare February 22, 2025 19:39
@@ -124,6 +124,8 @@ pub mod prelude {
#[derive(Default)]
pub struct GltfPlugin {
custom_vertex_attributes: HashMap<Box<str>, MeshVertexAttribute>,
/// The strategy to use when computing mesh tangents.
pub computed_tangent_strategy: TangentStrategy,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub computed_tangent_strategy: TangentStrategy,
pub tangent_calculation_strategy: TangentCalculationStrategy,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@aloucks aloucks force-pushed the issue-17834-compute-tangents branch 2 times, most recently from b4ef353 to ec40cb6 Compare February 22, 2025 19:57
@aloucks aloucks force-pushed the issue-17834-compute-tangents branch from ec40cb6 to 3495615 Compare February 22, 2025 20:05
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-glTF Related to the glTF 3D scene/model format X-Uncontroversial This work is generally agreed upon labels Feb 23, 2025
@hukasu
Copy link
Contributor

hukasu commented Feb 23, 2025

the anisotropy example does use tangents, this issue shows some issues caused by not optimal tangents #16179

@hukasu
Copy link
Contributor

hukasu commented Feb 23, 2025

i was making tangent generation for the primitives, but with a very naive algorithm #17691, if your method works without causing seams then my PR becames redundant

@aloucks
Copy link
Contributor Author

aloucks commented Feb 23, 2025

the anisotropy example does use tangents, this issue shows some issues caused by not optimal tangents #16179

The sphere looks much better with the gram-schmidt tangents. I actually left Sphere out of the tests that I added because there was at least one vertex where the mikktspace generated tangent differed from the gram-schmidt tangent enough that it was outside of the test threshold.

mikktspace

sphere_mikktspace.mp4

gram-schmidt

sphere_gramschmidt.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-glTF Related to the glTF 3D scene/model format C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mesh tangent generation is very slow
4 participants