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

KHR_skin_strict #1747

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

Conversation

donmccurdy
Copy link
Contributor

Extension introduces no new features or schema to the glTF file format. Instead, it defines a strict subset of the existing skinning specification, and imposes requirements — beyond those of the original specification — on tools creating glTF assets with a skins entry.

This proposal is quite close to #1695 (by @marstaik), but omits the new skeleton-centric schema, in favor of strictly limiting existing features. My hope is that by requiring the skeleton property and enforcing a 1:1 relationship between a skin and a skeleton root, engines will be able to load skinned meshes reliably even if their internal representation is centered around skeletons rather than skins.

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Jan 22, 2020

@reduz I'm hoping this satisfies your suggestions in #1665 (comment). Please see the overview for the motivation of using an extension for this: I am open to this being a required part of glTF 3.0, but I (1) do not want to rush into that, and (2) do not think there is enough consensus in #1665 to skip directly to the core specification.

## Requirements

* All skins *must* define the `skeleton` property.
* The `skeleton` node for a skin *must* be a joint, and the direct parent of the entire joint subtree for that skin.
Copy link

Choose a reason for hiding this comment

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

A node can't be both a joint itself and a parent of the subtree of all joints. Presumably this means (combining it with the following bullet point) "The joint nodes in a skin must form a subtree of the node graph and the skeleton node must be the root of that subtree"?

Why is it important for the skeleton node to be a joint? For models exported from Blender, a slight weakening is true: the joint nodes together with the skeleton node form a subtree of the node graph and the skeleton is the root of that subtree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your clarification is what I'd meant to say, yes.

Why is it important for the skeleton node to be a joint?

Maybe it's not necessary? But some of the side effects are useful:

  • A joint node must belong only to a single skin.
  • A joint node must not define any other functional role.

I think we'd all agree that having a camera or a light as the skeleton root node would not be in the spirit of this extension. But perhaps the skeleton root node should be allowed to be a mesh — such that the skinned mesh is itself the parent of its bones?

Having the skeleton root sometimes be a joint, and sometimes not, seems like another moving piece to consider eliminating if it serves no purpose, but I'm not sure. More feedback on this would be welcome.

Copy link

Choose a reason for hiding this comment

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

Found out this was already discussed in #1669.

I'm not sure what exact balance you're going for between conservative/liberal here is. I just wondered if it could be motivated purely by a technical benefit, like skins being distinct subtrees is motivated by being able to calculate a bind pose TRS for joints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure... going to wait for more feedback on this question.

Comment on lines 30 to 32
* A non-joint node *must not* be both an ancestor and a descendant of a joint node.
* `node ⊃ joint ⊃ joint ⊃ node` is valid.
* `node ⊃ joint ⊃ node ⊃ joint` is invalid, regardless of whether the two joints are part of the same skin.
Copy link

@scurest scurest Jan 22, 2020

Choose a reason for hiding this comment

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

The intent is to forbid "child skins"? Like this?

child_skin

Is this what reduz meant here?

A mesh must only be able to bind to a single skin. No game engine supports binding a mesh to multiple skins. If an exporter wants to do this, it needs to join both skeletons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if reduz meant the same thing, but it would be forbidden by this wording, yes. A weaker restriction could be:

A non-joint node must not be both an ancestor and a descendant of a joint node, if the two joints are in the same skin.

I'm unable to think of a compelling reason for nesting one skeleton under another; it sounds unlikely to work consistently... any concerns with prohibiting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify:

  1. The main intent of this line is, strongly, to prohibit non-joint nodes being injected into a the joint subtree of a single skin.
  2. Prohibiting one skin from being nested under another is a side effect that (IMO) seems beneficial, but I don't feel as strongly about that part.

^Although, the requirement as-written would not prohibit nesting a skin under another skin without an intervening node. If we intend to keep (2), that should also be prohibited for consistency.

Copy link

@scurest scurest Jan 22, 2020

Choose a reason for hiding this comment

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

A non-joint node must not be both an ancestor and a descendant of a joint node, if the two joints are in the same skin.

This is already forbidden in the bullet point above by requiring the joints form a tree, right?

I'm unable to think of a compelling reason for nesting one skeleton under another

If the intent is to forbid one skeleton under another, this is too weak: it only forbids them when there is an intervening non-joint node. This is still allowed

child_skin

Possible phrasing to forbid this: "Given two joint nodes from different skins, neither may be a descendant of the other".

any concerns with prohibiting it?

Well, an exporter can just extend the "upper" skin into the "lower" skin and join them together. Just trying to understand the reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already forbidden in the bullet point above by requiring the joints form a tree, right?

I don't think it's explicit enough in the earlier bullet, but probably there are better ways to phrase that. How about removing this bullet and rephrasing the one above:

All joints in a skin must define a single fully-connected subtree, sharing a single root joint, with no non-joint nodes coming between the joints of the tree.

If we do want to prohibit nested skins, that can be a clearer separate bullet.

Copy link

Choose a reason for hiding this comment

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

What we need to prevent is that there doesn't exist a case where meshs[0] tries to bind to skins[0] and skins[1] simultaneously, that is a no go.

What does that mean exactly? This?

"nodes": [
  {"mesh": 0, "skin": 0},
  {"mesh": 0, "skin": 1},
  ...
]

Choose a reason for hiding this comment

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

What we need to prevent is that there doesn't exist a case where meshs[0] tries to bind to skins[0] and skins[1] simultaneously, that is a no go.

What does that mean exactly? This?

"nodes": [
  {"mesh": 0, "skin": 0},
  {"mesh": 0, "skin": 1},
  ...
]

Correct

Copy link

Choose a reason for hiding this comment

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

What about

{"mesh": 0},
{"mesh": 0, "skin": 0}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the confusing bullet and clarified the previous one.

@marstaik I don't understand your comment about not re-binding different instances of a mesh to different skins. A "mesh" in glTF is not instantiated until it is mounted at a node, and meshes may be instantiated multiple times. Assuming both skins have the same number of bones, why couldn't different instances of a mesh use different skins (or no skin)?

Certainly we don't want a single instance of a mesh bound to joints from different skins...

Choose a reason for hiding this comment

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

Sorry, I confused myself there. I was tired and missed the fact that it was a node list, haha.

Certainly we don't want a single instance of a mesh bound to joints from different skins...

Yes you are correct

@marstaik
Copy link

There is an issue still that has not been addressed by any of the propositions yet - the bind pose.
I had planned to modify #1695 to account for bind poses because they really are crucial, as well as applying only to skeleton's and not skins.

Sadly, the IBM's of a Skin do not translate properly to the "Bind Pose" needed by skeletal animation systems to work properly. Also, limiting the "Skins" to one skin per joint tree also creates a lack of flexibility that the Skeleton can solve, with the trade-off being adding additional complexity.

The Bind Pose is basically the ground truth for any form of animation blending, we need to take the Bind Pose, apply joint transforms on top (for animation), and then use the IBM's of the skin to tell the mesh how to deform.

I'll try to give as concise of an explanation as I can:

Let there be two gltf files, scene1.gltf and scene2.gltf
Let both gltf files contain the same skin hierarchy.
Now, let scene1.gltf define the skin so that mesh0 may be bound in A-Pose, and let scene2.gltf define the skin so that mesh1 may be bound in T-Pose.

With the current implementation, we have to derive the Bind Pose from the IBM's. But in the two separate gltf scenes, the same joint hierarchies have two different sets of IBM's.

Applying mesh1 to the Bind Pose of scene1.gltf will cause an explosion (literally), so so will trying to apply mesh0 to the Bind Pose of scene2.gltf.

Game Engines like to export a bunch of assets in different files and be able to put them together again, such as exporting each individual mesh with the corresponding skeleton joints individually.

Ive ran into the issue first-hand with the different IBM's, and the only solution currently its to go to the modeling program and reset the bind pose so ensure they are identical.

@vpenades
Copy link
Contributor

Ive ran into the issue first-hand with the different IBM's, and the only solution currently its to go to the modeling program and reset the bind pose so ensure they are identical.

Resetting the IBMs will affect the way in which joints are deformed.

@donmccurdy, I would like this extension to include a comment for exporters able to automatically handle KHR_skin_strict: Any exporter able to automatically re-bind a mesh+skin with new IBMs to meet the KHR_skin_strict, it should warn the artist that the skin deformation behaviour might look different in-game, compared to what the artist sees in the authoring application.

@zeux
Copy link
Contributor

zeux commented Dec 30, 2020

I really like this - this is simple and can be grafted on top of an existing file; tools that aren't aware of the extension can just skip it. If this gets merged, tools like gltfpack will be able to auto-tag output files with this extension by just checking the conformance to the specified rules.

It's not quite clear to me from the specification from this:

  • All joints in a skin must define a single fully-connected subtree, sharing a single root joint, with no non-joint nodes coming between the joints of the tree.

Whether it's valid for a joint to have a non-joint child (as long as there's no other joint descendants of that node) or not - that is, whether the notion of fully-connected subtree only excludes intermediate non-joint nodes or leaf node chains as well. It would be good to clarify this either way.

@vpenades
Copy link
Contributor

vpenades commented Dec 30, 2020

I have a question regarding the Skeleton node/joint. I understand that since it is not explicitly forbidden, Skeleton can be a child of another node hierarchy (as long as parents are not joints and just normal nodes).

Maybe it's not necessary? But some of the side effects are useful:

  • A joint node must belong only to a single skin.
  • A joint node must not define any other functional role.

I didn't know that defining a Skeleton implicitly constraints the use of a certain node to be used once on a single skin. If that's the case I think it needs to be explicitly stated in the specification. The same with functional roles.

I foresee a small problem, when a skin declares a number of joints, but the mesh vertices do not reference all these joints. I think gltf-validator logs a warning if that happens. I guess this condition might need to be eased.

I think it could not be terribly difficult to add a last-minute check to see if the model being exported meets these conditions, and then add the KHR_skin_strict extension. But then, whether an exported model has the extension or not will be a lottery, failing the check more often as the complexity of the model grows.

@zeux
Copy link
Contributor

zeux commented Jan 18, 2022

One more potential addition to this extension could include requiring that bone influences are sorted by weight, both across weight groups (WEIGHT_0/WEIGHT_1) and within each group. This makes it easier for renderers to implement shading LODs that use a (renormalized) subset of weights without having to sort the influences during loading; Unity is one particular example. cc @atteneder.

@zeux
Copy link
Contributor

zeux commented Jan 18, 2022

One more thing, this extension restricts joint graph to not have non-joint interior nodes, but does seem to allow (by omitting this restriction) non-joint nodes to be children of joint nodes. This may present challenges for some engines where skeletons aren't part of the more general node graph as it effectively allows attaching meshes/lights/etc to children of joints. Not sure if this is intentional or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stricter Skinning Requirements
7 participants