Docs: Clean up #25189
Docs: Clean up #25189
Conversation
| @@ -158,7 +158,7 @@ <h3>[method:undefined pose]()</h3> | |||
| <h3>[method:Vector3 boneTransform]( [param:Integer index], [param:Vector3 target] )</h3> | |||
| <p> | |||
| Calculates the position of the vertex at the given index relative to the current bone transformations. | |||
| Target vector must be initialized with the vetrex coordinates prior to the transformation: | |||
| Target vector must be initialized with the vertex coordinates prior to the transformation: | |||
There was a problem hiding this comment.
Target vector must be initialized with the vertex coordinates prior to the transformation
I think this is a red flag that the API needs to be revisited.
There was a problem hiding this comment.
@Mugen87 Is the current API acceptable to you?
There was a problem hiding this comment.
Looking at how the method is used, the current implementation does make sense.
I suspect you want that SkinnedMesh.boneTransform() initializes the target vector by itself based on the given vertex index. The problem is that just reading the position buffer attribute isn't enough. A vertex might already be transformed like in Mesh.getVertexPosition(). So the bone transformation is something that happens on top.
There was a problem hiding this comment.
The transform can be applied to any vector. Right?
I'd feel better if the method was named SkinnedMesh.applyBoneTransform( index, vector ).
Then it is obvious that the vector to be transformed must be initialized.
Previously, we have used target to refer to an uninitialized parameter.
There was a problem hiding this comment.
The transform can be applied to any vector. Right?
The method makes the assumption that index and target are coupled. So target must represent a vertex of the geometry since the same vertex index is used to extract the respective skin indices and weights.
The issue is that the vertex might already be transformed by morphed target or another vertex displacement technique.
I'd feel better if the method was named SkinnedMesh.applyBoneTransform( index, vector ).
I'm find with that. We probably want to use vertex instead of vector, see my previous comment.
There was a problem hiding this comment.
The method makes the assumption that index and target are coupled. So target must represent a vertex of the geometry...
I do not think that is true. Any point can be transformed. (The point doesn't even have to be near the respective vertex, although it typically is.)
The issue is that the vertex might already be transformed by morphed target or another vertex displacement technique.
In fact, that is what we allow.
I'll file a PR that changes the API to SkinnedMesh.applyBoneTransform( index, vector ); -- if you agree.
No description provided.