"IsValid" checks may produce false negatives

[Morilli]

In the project that I'm working on, the package SharpGLTF was updated from version 1.0.0-alpha0020 to 1.0.0-alpha0025, and I'm noticing failures now when calling AddSkinnedMesh. I could track those down to the IsValid check being performed, specifically the check for IdentityColumn4: https://github.com/vpenades/SharpGLTF/blob/3dc26e16ec4fe75a4a8aa5cfcd33082e965ba570/src/Shared/_Extensions.cs#L217-L223

As the warnings that I get in Rider, for example, already suggest, this comparison with 1 is not a great check in case the passed-in matrix has been transformed and a tiny floating-point error has been introduced (very likely, and precisely the failure condition in my case). See this link for more details on the warning.

Here's an example of a matrix that fails this check, but should pass instead: I would appreciate if you could take the time to go through and remove or loosen up these checks to prevent any false negatives.

[vpenades]

Mathematically speaking, the skin matrices are calculated from the bone matrices, which under normal conditions should be orthogonal. In a perfect world the inverse of such a matrix would yield a (0,0,0,1) 4rth column.... So the fact the the bottom right corner is not exactly 1 is not because it has some meaning, but because rounding errors, the inverse operation fails to yield an exact 1.

Speaking with other people, the impression I get is that the forth column must always be 0,0,0,1 because some engines assume it's going to be that way, and discard the 4th column when uploading to the constant shaders.

So letting M44 to have values other than 1 would introduce tiny inconsistencies between engines that handle the full matrix, and the engines that only handle the 1st 3 comumns.

So my recomendation is that it's safe to force M44 to always be 1.... another matter is whether the library should have some tolerance, and force M44 to be 1 if it's within the tolerance.

[Morilli]

So my recomendation is that it's safe to force M44 to always be 1

how so? The matrix that I'm getting is an invertion I believe, so I cannot just change values there. Floating-point error, especially when dealing with matrixes, are exceptionally common, so I cannot imagine that the values are supposed to be enforced without regard to small errors.

[vpenades]

Not sure where I read it, or if it was in a discussion with someone from Khronos, but I'm sure the glTF standard requires the 4th column to be (0,0,0,1)

The proof can be found here

As you can see, the official Khronos glTF validator checks that the forth column must be (0,0,0,1) otherwise it will consider the file is a malformed glTF. So I must assume it's mandatory by the standard.

how so? The matrix that I'm getting is an invertion I believe, so I cannot just change values there.

What I can do is to check the code and see if there's some inversions within the code that yield non 1 M44 values, but in general, M44 should be set to 1 manually before sending it to the API

[vpenades]

I've just pushed some changes that might help with the issue.

Let me knows if it works for you

[Morilli]

The docs you mentioned appear to be correct in that they enforce an exact match. The commit you pushed seems to be incorrect tho, did you mean to replace some other code? It's still failing in the AddSkinnedMesh function, as that's the only one that is called really.

I will definitely have to check whether there is something to be done on my side too, as you're right that when gltf enforces those values to be exact, they shouldn't be passed in incorrectly.

[vpenades]

@Morilli AddSkinnedMesh is part of the SceneBuilder framework, which is somewhat independent from the actual Skin object of the schema.

The fix is set in the method that runs when you convert a SceneBuilder into an actual glTF object, so it will run when you convert.

I'm going to add a check in the AddSkinnedMesh too, but the fix is in the right location.

[Morilli]

Thanks, all works fine now with your latest changes.