search

vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
482 stars 77 forks source link

VertexPreprocessor unnecesssarily normalizing all normal and tangent vectors #243

Closed vermadas closed 4 months ago

vermadas commented 4 months ago

In the SanitizeVertexGeometry lambda there are two if statements that have typos in their conditionals that's causing all normal and tangent vectors to be normalized. First here: https://github.com/vpenades/SharpGLTF/blob/32e214dc76dfa5d277b9481dd434462c9a9a3031/src/SharpGLTF.Toolkit/Geometry/VertexTypes/VertexPreprocessorLambdas.cs#L164 Second here: https://github.com/vpenades/SharpGLTF/blob/32e214dc76dfa5d277b9481dd434462c9a9a3031/src/SharpGLTF.Toolkit/Geometry/VertexTypes/VertexPreprocessorLambdas.cs#L178

I'm guessing that those if statements should read if (l < 0.99f || l > 1.01f)

Normally, this shouldn't be an issue. However, several models I was building with morph animations were failing validation because they had zero MorphTargetsCount instead of the expected. When I was saving vertices to the meshbuilder, the normals on the vertices were always being normalized, despite me normalizing them upstream. And for some reason, some of them were changing very slightly with minor floating point precision differences. So when the MorphTargetBuilder was keeping track of the vertex geometry as keys in a dictionary, they were not being found when I was calling SetVertex.

<0, -0.4185573, -0.90819037> became <0, -0.41855732, -0.9081904>, for example.

vpenades commented 4 months ago

yeah, these values are definitely wrong

I'll look into it.

vpenades commented 4 months ago

I've checked the issue, the fact that the renormalization (or any other vertex sanitization step) can alter how the morph targets are processed is an unexpected and undesirable effect I did not foresee.

I've fixed the code so it only runs when the normals and tangets are indeed out of bounds and not indiscriminately.

You can try the code with the latest commit with the bounds check fix.

Alternatively, if you know you're feeding clean vertices, you can also configure the VertexPreprocessorLambdas with empty ones that do nothing. This will make the code to run slightly faster, and will ensure the library does not modify the vertices in any way.

Feel free to close the issue if you consider this as the appropiate solution, and thanks for reporting it. I bet you expended quite some time figuring out the source of the problem.

vermadas commented 4 months ago

Yes, it was a bit frustrating to figure out. I kept making wrong assumptions about why or where it was happening. And issue #208 was a bit of a red herring since their symptoms lined up with mine.

Thank you for the quick fix though!

vpenades commented 4 months ago

Because the unit tests take so long, I got a late test error, it's fixed an the next commit.

Morph targets is a nightmare to deal with, and the fact that vertices can have any kind of morphable attribute does not help either.