search

vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
470 stars 75 forks source link

Add a skinned mesh to an existing node #40

Closed ptasev closed 4 years ago

ptasev commented 4 years ago

Currently it's possible to set a rigid mesh to an existing node with SceneBuilder.AddRigidMesh(MeshBuilder, NodeBuilder).

However, I haven't found a way to do this with SceneBuilder.AddSkinnedMesh. How can I accomplish this?

vpenades commented 4 years ago

You don't need to.

In glTF, the transform of the node containing the skinned mesh is not taken into account. The only transforms being used are the ones of the skin, so the "master node" is just a placeholder. In fact, under glTF spec, it is ilegal to have a master node with any kind of transform. Transform fields must be empty for a node containing a Skin.

SceneBuilder.AddSkinnedMesh will create the master node automatically. Since under glTF spec it is ilegal for that node to have any transformation, there's not much you can do with it besides settings its name.

ptasev commented 4 years ago

I wanted to put the mesh in a specific spot in the node hierarchy (outside of the skeleton node tree), but I suppose it's not something I have to have.

I read the part in the spec that said the transforms on the master node are ignored, but I didn't see it mention that it was outright illegal. https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#skins

vpenades commented 4 years ago

At the very least, it is deceiving..... since the transform is ignored, having a transform in a node with a skin causes some developers to misunderstand how the transform pipeline works. Not sure if it's strictly forbidden, if it's not, it's probably because backwards compatibility. I believe gltf validator treats it as warnings.

If I recall, when the final gltf model is constructed, the node containing the skin is put outside the skeleton tree. In fact, a new node is created for every AddSkinnedMesh call. That way, multiple meshes can share the same skeleton.

ptasev commented 4 years ago

You're right that it's deceiving, but it can still be considered missing functionality since you can't control the entirety of the node tree. It's not the end of the world if the functionality is missing, so it's up to you if you want to add it.

Update: By the way, you're right about the validator giving a warning if the node with a skin is not at the root, or has a transform it will give a warning. So maybe it's not good functionality to have.

The other missing functionality is controlling the name. Maybe the node created with AddSkinnedMesh should take the name of the mesh if it exists. Currently the created node looks like this:

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

Again, this is a minor enhancement, and not something that's high priority.

vpenades commented 4 years ago

The other missing functionality is controlling the name. Maybe the node created with AddSkinnedMesh should take the name of the mesh if it exists.

All SceneBuilder.Add* methods return an InstanceBuilder object that allows you to set the name.

ptasev commented 4 years ago

I don't believe the Name property on InstanceBuilder actually does anything.

vpenades commented 4 years ago

I don't believe the Name property on InstanceBuilder actually does anything.

You're right... I guess it's a leftover from when I started designing the APIs. ... I'll revisit this API and figure out a way to also set the name for skin (and matrix)

ptasev commented 4 years ago

I tried your changes, and setting the name works now. Thanks!