-
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
Add implementation notes about which transforms apply to skinned meshes #1195
Conversation
> **Implementation Note:** Matrix, defining how to pose the skin's geometry for use with the joints ("Bind Shape Matrix") should be premultiplied to mesh data or to Inverse Bind Matrices. | ||
> **Implementation Note:** The matrix defining how to pose the skin's geometry for use with the joints ("Bind Shape Matrix") should be premultiplied to mesh data or to Inverse Bind Matrices. | ||
|
||
> **Implementation Note:** Client implementations should apply only the transform of the skeleton root node to the skinned mesh while ignoring the transform of the skinned mesh node. In the example below, the position of `node_0` and the scale of `node_1` are applied while the position of `node_3` and rotation of `node_4` are ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @takahirox this is consistent with the three.js defaults, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes if THREE.SkinnedMesh.bindMode
is attached
.
So, this means skeleton can't be shared among multiple skinned mesh nodes in glTF as with Three.js + attached
mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The skeleton can be shared by skinned mesh nodes that all inherit the transform of the skeleton root. The Brainstem model is an example, with 59 SkinnedMeshes all composing a single figure (open JS console in the link to see structure).
But having multiple distinct figures share the same skeleton, like mrdoob/three.js#11666, is not supported in glTF with this clarification, as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having multiple distinct figures share the same skeleton
Yeah, what I wanted to point out was this case
I find it quite odd that a client implementation should ignore scene information provided by the exporter/author. Can we get more detail on why this is the best choice to remedy this situation? My implicit expectation is that both transforms should apply. If I wanted poly to be in the box for example I'd add a transform node to place her there. Also if I want the root of poly to translate as a walk animation plays I'd expect that translation to originate from the box and not at the identity/model center. This note implies that I may need a walk animation from box and walk animation from center. Our engine (Windows Mixed Reality) only applies the skinning transforms when the model's associated animation is executing. To follow this note I believe we'll need to multiply in the inverse of node_4's "world transform" to the root of the joint tree's local transform (node_1). Then if the animation is executing we'll have an appropriate transform and when its in its binding pose it will respect the scene, |
It took me quite some time to understand what this means. So as a clarification on the specification I think it should be rephrased at the very least. Also I agree with @najadojo, I don't see why mesh transform should be ignored. I personally don't have this issue in my implementation and both transform are applied. I feel like it's very dependent on the client implementation, and thus this brings more confusion than clarification. |
Are you able to load the Polly model with the eyelids moving properly? |
Indeed the eyeballs are going out of her head ... EDIT: eyelids however are working fine |
A bit off topic but... is this polly model supposed to be the reference? |
I think it depends on the order of the transforms being multiplied. It's interesting that the eyelids animate correctly though. This is tricky because some engines do different things. Unity3D, for example, ignores the skinned mesh node's transform. Unreal's skeleton hierarchy is separated from the node hierarchy, so it doesn't have this issue. At the end of day, it would be better if there is some kind of implementation note explaining what the expected behavior is for the transforms. Otherwise, models like Polly will behave differently between engines.
Not necessarily, but it is exported by the official Khronos glTF exporter. |
That's the same in my case too, in jMonkeyEngine the skeleton is separated from the scene graph. That's probably why the eyelids work. I guess I have another issue for the eye balls. Anyway to go back to the PR at hand, maybe phrasing with caution like "depending on implementation...... client loader may choose to... etc". To put the emphasis on the fact that the spec does not enforce anything on this, but this might be something to consider.
Which is still a WIP, especially when it comes to bone animation. My point is, let's not spec something if it's to work around an issue in a gltf produced by exporter X. (not saying this is the case here, but in a more general case). |
Implementation notes are non-normative. They inform the client implementation of how to get similar results with other implementations. Implementations are free to ignore the note if they don't want to conform to the note. I don't think of it like we are adding this to the spec because of this specific exporter. We are adding it because we want exporters and importers to behave consistently. The exporter may be a WIP, but this issue points out that the spec isn't clear on how it should work. That said, if there is a better wording/behavior that still ensures consistency across implementations, then I'm all for it. |
Well in my humble opinion replacing the first "should" by "may choose to" would add the proper nuance that this is not enforced by the spec. Even if it's already in an implementation note. |
Even if it's only an implementation note now, eventually there must be a "correct" way to interpret the transforms of the skeleton and the skinned mesh, and that should make it's way into a fully-enforced part of the spec. So, wording is probably less important than making sure our change will result in models that behave consistently, which the current spec does not. |
One thought... if some engines will always ignore the skinned mesh's transform, and others always ignore the skeleton's, could we recommend that the skeleton root node be a child of the skinned mesh with no local transform? Then whichever interpretation the client takes, the result should be the same. |
@donmccurdy how would that solve the problem of needing a poly walk animation from two different locations within the scene? |
If I follow that problem correctly, then:
By transforming the mesh, which is a parent of the skeleton, to be inside the box, both are placed inside the box.
Animation translating the mesh from the box origin will translate both mesh and skeleton alike. That said, I'm not sure it's the best solution, just one option. Note that three.js has two modes supported:
|
Although I know that's just recommendation and it'd be over the specification I have one concern in my mind, that is glTF models are sometimes or often created from other formats, like converting from FBX to glTF. Maybe there'd be no such recommendation/restriction in other formats. Would we expect exporter/converter to convert well? |
We continue to struggle to implement the behavior described in this note. Parsing and translation into our engine is a recursive single pass series of functions. This note has required adding an additional pass so that we can lookup parents and their transforms from the skin root. We also have addition data passing needs to know which node is the scene root at any time during the parsing. |
@takahirox As the primary author of FBX2glTF this is on my mind as well! It's been a problem in the past that the way the FBX SDK does translation of units, is by adding nodes high in the hierarchy (children of root) whose only purpose seems to be to carry e.g. a 0.01 homogenous scale factor, and then hang everything else off of those nodes. This factor has been doubly applied in the past for precisely the reasons this PR is meant to address. |
Skeleton Re-use across mesh is a primary feature of skinning. What is the rationale of not keeping that feature intact for gltf ? |
Some thoughts...
I think we need to reconsider skin structure to fix these issues in glTF 2.x or glTF next (sooner is better). BTW Three.js has the same latter issue and we're trying to fix by adding special property child-bone map to SkinnedMesh. |
@Kuranes could you describe an example of how this feature would be used?
Unlike exchange formats, glTF deliberately restricts some features that certain engines or modeling tools allow, because they can't be widely supported. That choice is critical for glTF models to be portable and reliable. That's not to say that skeleton reuse should be disallowed, necessarily — just that we need to understand how it would be used, and how widely it might be supported, before deciding whether to include it in the core spec. If the decision is not to include it in the core spec, it is always possible to support it through an extension (e.g. |
Some use case is vegetation (Speedtree), Crowds (Mixamo, VR Chats), reuse Motion Capture tracks across mesh without editing needed. Afaict, Any people buying models in GLTF format wanting to reuse the skeleton/mesh, as in import in any other tool (3D engine, or just a tool) will lose that ?
|
Thanks for the detailed answer
yes exactly.
trying to make sure here, Particularly if it works. Would love explicit spec details on it ?
I mean "no loss of information",
|
Hoping to close this out. I'll try to summarize. When we say "re-use skeleton" or "shared skeleton", there are two scenarios.
Even before the proposed implementation note in this PR, neither of these scenarios are fully supported in core glTF 2.0 without duplicating the skeleton hierarchy and corresponding animation JSON descriptions. Note that the data for the inverse bind matrices and animation curve can be shared. The only case I see where the JSON descriptions can be shared is the first scenario if the additional transform is not necessary (i.e. identically animating an army of identical meshes in a scene). In my mind, this is because the joint hierarchy in the node hierarchy represents an instance of a skeleton. I believe this is like how Unity handles skeletons (i.e. the SkinnedMeshRenderer encapsulates a skeleton, morph targets, and a skinned mesh all in one component). I will try to explain why this implementation note is necessary given how the spec is currently structured. Without the proposed implementation note, there will be an issue with attached objects in the skeleton instance. Since the skeleton instance is part of the node hierarchy, any attached objects in the node hierarchy should be rendered as if the skeleton is not present. If we allow the skinned mesh transform to also apply to the skeleton, then the attached object will no longer be in the proper place relative to the skeleton instance. I believe sharing the JSON description for the skeleton hierarchy requires a different approach to defining the skeleton than can be done with core glTF 2.0. If we want to discuss this further, I suggest we create another issue to discuss sharing the JSON description for skeleton hierarchy and animations. This implementation note is intended to ensure that client implementations behave consistently. |
Agreed that (2) is not possible regardless of this PR, and would require future changes to glTF. I think that (1) is not strictly impossible — except for the part about shared attachments — depending on how you interpret the current spec. But given the ambiguity and the likelihood that (1) would be implemented inconsistently, I think a PR like this one, tightening the spec to disallow both (1) and (2) is best. The immediate goal should be to ensure that glTF 2.0 behavior is clear and reliable, and we can add more advanced functionality through extensions. My only remaining concern is that a PR specifying that only the mesh transform — or even both transforms — should apply might provide similar clarity. Do we know pros/cons of those options? We've mentioned above in this thread:
That's a good start, and supports this PR... Unreal's approach seems equivalent to ignoring the skeleton hierarchy's translation in favor of the meshes, which presumably means any animations targeting the skeleton root would need to be changed to the local coordinates of the mesh object when loading in Unreal? From another thread there is a comment —
@PatrickRyanMS building on that, would you expect (1) movement of the skeleton root in isolation affects all vertices with non-zero weights, and (2) movement of the |
I don't have a full list of pros and cons. The main con I have with with applying only the mesh transform or both is that attached objects won't work properly.
I think an Unreal implementation can instantiate a skeleton based on the joints in the skin but it must walk all the way up to the root of the hierarchy. This is exactly what I did in the BabylonJS implementation. BabylonJS, like Unreal, also has a separated skeleton. |
Responding with @PatrickRyanMS...
Yes, this is expected.
Assuming when you say "parent", it is the parent of the skinned mesh node and not the parent of any part of the skeleton, then yes, this is expected also. |
Oops, yes that's what I meant.
That seems like a compelling reason not to use both transforms, yes. Would be great if we could create a test model to help test this, and perhaps include some validation warning if animation targets a skinned mesh node. |
This time I’m going to make a comment with my day job cap. This PR, triggered a lot of talking at Sketchfab and we actually struggle to understand the case this PR is supposed to fix. @bghgary Could you give us a simple use case that this PR would fix. It would help everyone to understand, and maybe find another solution that would not have as many down sides. This PR raises the following issues: 1. As stated before it prevents the re use of a skeleton for multiple meshes: In this example and with the current PR being applied, we end up with the man and the woman models overlapping. To avoid this, exporters will have to duplicate the skeleton. 2. It prevents partially skinned meshes:
3. It forces to bake a skinned mesh’s animation into the root bone’s animations if the format is meant as a portable format, it should not destroy this information. To conclude, in my opinion all theses issues are due to the fact that the skeleton structure is part of the nodes structure in the first place. And that’s a flaw this PR is trying to circumvent. But As @takahirox suggested we should rework how the skeleton hierarchy is described for gltf 2.X. For the PR at hand we should try to find another solution. But for this, the initial issue needs to be crystal clear to everyone. At the very least, this PR should be put in stand by, because there is clearly no consensus among the implementers on the matter. |
Yes, I will work on this. I will try to illustrate what happens in BabylonJS for the different ways the transforms can be applied. |
This is a bit hard to illustrate, so I will try my best. Feel free to ask questions on what I did. Here is what I did:
The resulting glTF is here: https://github.com/bghgary/glTF/tree/gh-pages/skinning-issue/RiggedSimple. We can decide whether this asset is correct or not after discussing what are the appropriate transforms to apply. From here, I will try to illustrate 3 possible ways to interpret the skin.
There is a fourth possibility where neither of the transforms are applied, but I think we can safely say we don't want this. |
@Nehon Any thoughts on if there is another solution to this? |
After a lot of discussions, we didn't find any real solution for this, but we would like to discuss about two potential solutions that are not far from the suggested one: Solution 1: enforce the root bone to be attached to the the skinned mesh Node. “nodes”: [
{
"name":"human_1",
"mesh": 0,
"children": [ 1 ]
},
{
"name":"RootBone",
"translation":[ 5.0, 0.0, 0.0]
}
] It seems to make no sense to have a transformed mesh that is placed at coordinates A, a skeleton placed at coordinated B, and ignore coordinates A or B. Cases that become not possible with this solution: two meshes cannot be skinned by the same skeleton. Solution 2: The skinned mesh node and the root bone are siblings in the scene graph (they have the same parent node). "nodes": [
{
"name":"human_1",
"children": [ 1, 2 ]
},
{
"name":"RootBone",
"translation":[ 5.0, 0.0, 0.0]
},
{
"name":"Human_mesh",
"Mesh": 0,
"Translation": [10.0, 0.0, 0.0]
}
] Cases that become not possible with this solution: a mesh attached to a bone will not be aligned with skinned meshes. In the end, our conclusion is that to be able to support all these cases we’d need to completely separate the skeleton from the node structure. But since it’s too much of a change in the spec, it’s outside of the scope of this PR. (It could be nice to have more feedbacks from developers with background in skinning animation, though. I feel that we focus essentially on implementation and not on best practices) |
There are plenty of models where multiple parts of a single object point to the same skin. For example, this one: https://sketchfab.com/models/f2f13a8630004b1c82730d8b9ffa0e1f. I don't think we can live with this kind of restriction.
I'm not sure if we are okay with not allowing mesh attached to a bone. What about attaching a skinned mesh to bones? My understanding is that these cases are common.
+1, this isn't something we can tackle until a future version. From my stand point, both of these solutions have restrictions that will break existing assets in glTF 2.0 ecosystem. I still feel ignoring the skinned mesh transform as proposed is the right solution for glTF 2.0. Thoughts? |
For what it's worth, we currently apply the skinned mesh node transform only to determine the original position of the vertices. From there on, the (animated) hierarchy does all the work through the joints transforms. This seems to work fine for us, at least with the Babylon exporter and a few of the examples I have tried. I have the impression this aligns with your proposed solution, @bghgary? |
Yes, I believe so. |
To summarize — in glTF 2.0, the behavior of transforms on skinned meshes is not well defined, and we haven't been able to arrive at a backwards-compatible fix better than the one proposed in this PR. In the interest of having an unambiguous spec, and unless there are any further options to consider, we should probably merge this PR's clarifications. We realize this doesn't allow glTF 2.0 to support several features that are important for skinned animation, but the current spec cannot support them now without breaking changes. From @Nehon's earlier comments:
Un-blocking those features are certainly possibilities for a glTF 2.X version, or for an extension to the glTF 2.0 spec (e.g. Also — I'm not sure how "partially skinned meshes" are used, but if this involves vertices having |
Agree with that. It sounds reasonable. In addition what you mentioned, another concern is (perhaps) it seems like some engines don't support some skinning features (ex. shared skeleton) now. For such features, extension first would be also good. |
I have created a new issue #1285 where we can further discuss a better solution for skinned animations. We are still planning on merging this implementation note. Please give a thumbs up if you are okay with this. |
🚀 |
This change adds an implementation note about how transforms of the root node of the skeleton hierarchy and the skinned mesh node affect the skinned mesh. The working group has been discussing this for a while now and it would great to get some feedback from the community.
For some background, the original issue came from the Polly model from the glTF Blender exporter. The eyelids were authored in a way that didn't work in some engines (e.g. Babylon.js).