vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
453 stars 73 forks source link

Parenting Assumption in `Skin.BindJoints()` and `Skin._FindCommonAncestor()` #193

Open effs opened 10 months ago

effs commented 10 months ago

This might not be exactly a bug, not sure what the desired behavior is but it caught me by surprise so I figured I'd raise it:

_FindCommonAncestor() will not traverse to a parent of a single node.

I think it could be argued both ways whether this is correct behavior for the function itself (it does travel to parents outside of the working set if there are multiple nodes) but because BindJoints(params Node[] joints) uses it directly, it means that for single nodes the IBM is calculated against itself which may or may not be desirable.

For my purposes, I fixed the problem by explicitly passing in the parent's WorldMatrix using another overload but I think either

  1. making the behavior consistent so that BindJoints will always find the closest shared parent, not the node itself, would be ideal if it doesn't cause problems; or

  2. the function or even parameter naming could be changed to help understand the intent if this is intended behavior

What's your take on this?

vpenades commented 10 months ago

Hmm... I would need some kind of simple case scenario to see how this is a fix, I mean, the whole thing related to skinning is already mind boggling 😅