-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
validating inverseBindMatrix: rename and tighten #1504
Comments
@donmccurdy Could you please add this issue to the list of all open skinning questions? |
Do you mean #1403? I didn't intend for that to be a list of all skinning questions, but rather a list of specific usage the spec currently allows, which need (a) examples, (b) clarification, or (c) to be removed if they aren't correct. So everything in that list has a next step of "test the implementations." This seems like a spec issue to be clarified or fixed. I'm not immediately sure what's correct here, or why (some?) sample models are different. |
Models use the inverseBindMatrix in different ways for different purposes. I believe this is how the matrix is intended to be used although the spec. seems to be a bit misleading or self-contradicting. #1403 does not quite fit, I agree, but perhaps can be expanded to also include spec. issues. Referring to #1403 here may suffice ? |
Following the skinning spec discussion after the previous call, I created this image, which is supposed to go (non-normatively) into an appendix: Admittedly, I wasn't entirely sure about the global transform part. The spec about Skins currently says:
Of course, at some point, the transform of the node has to be taken into account - or does this statement exactly refer to "cancelling out" the global transform for the skinning computations via the |
There is a typo: Mutliplying => Multiplying |
The usual test, to see whether people are reading it ;-) Thanks, I fixed that locally. The point about the 'final' is also true. Do you think that rephrasing it t something like '...the world coordinate of the skinned vertex' could be better? In any case, this is also related to the question about the "global transform" that I mentioned. Iff I understood this correctly, then the quoted implementation note from the spec refers to the fact that in the final computations of an actual implementation, there is something like
(apologies for the informal style here), where the first I'm still not perfectly sure about the role of the skeleton root. Actually, there's still a task on my TODO list, namely to lift the old https://github.com/javagl/gltfTutorialModels/tree/2.0/SimpleSkin example to become a proper glTFSampleModel. Maybe I can then also create different variants with different skeleton roots and different skinned-mesh-node-transforms, so that one can immediately see whether the implementation is right - and ideally, what is wrong otherwise: The skinning in general causes quite some trouble for implementors... |
An open question here referred to the statement from the spec
I think my current implementation does not do this. Instead, it uses the transform of the node which actually references the More specifically, referring to the image that is shown above (supposed to go into the appendix): Should the first bullet point
be changed to
? |
I seem to remember a discussion in an Issue on why the transform of the skinned mesh node should be ignored. That might help to determine if the multiplications of joints by G-1 actually is equivalent to that, or if that was just a convention which is widely expected. |
Now that the spec is clear that the global transform of a node that contains a skinned mesh is ignored, the only remaining use of the
The updated skinning section mentions when these transformations happen. Although I agree with the property name being suboptimal, we cannot change it at this point. |
Sounds good. Thanks. |
The InverseBindMatrix of a skin is supposed
https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#skins
In fact, that is how the spec. functionally defines inverseBindMatrix.
As such, its value will be the
as the gltf tutorial points out: https://github.com/KhronosGroup/glTF-Tutorials/blob/master/gltfTutorial/gltfTutorial_020_Skins.md#the-skin
So it could be optional (#461) as it can be computed by the viewer, or alternatively preapplied, and cannot be animated.
It would be a convenience/performance feature to help with fast loading.
A validator/loader could check if the supplied inverseBindMatrix is actually the inverse of the global (or perhaps relative to root) transform.
However, for some, perhaps most, glTF scenes this is not the case. For example, https://github.com/KhronosGroup/glTF-Asset-Generator/blob/master/Output/Animation_Skin/Animation_Skin_01.gltf has inverseBindMatrices which are unrelated to their joint transforms.
This then means that these matrices actually do not bring coordinates being skinned into the same space as each joint. While the spec. seemingly is at odds with that usage, it is a result of the actual dual purpose of the matrix. The second purpose is to optionally serve as bind shape matrix, bringing the mesh into the initial pose expected by the animation of the joints. This is pointed out by the implementation note which seems to contradict the intended functional definition of the matrix.
Just from the terse spec. language and without further information from tutorials, examples or other implementations it would be neigh impossible to divine how inverseBindMatrix needs to be taken into account during skinning.
What about renaming the matrix by getting rid of 'inverse' and call it more generally jointBindMatrix ? And then redefining its function to be more general as the first and unmutable transform which needs to be applied to a skinned vertex, perhaps augmented by its intended use as a combined inverse bind and shape matrix.
At least this is how its current usage comes across. If it is too late for renaming in a future version, clarifying its functional definition may still be possible.
The text was updated successfully, but these errors were encountered: