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.

Sign up for GitHub

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

Jump to bottom

Docs: Clean up #25189

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Docs: Clean up #25189

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/api/en/objects/SkinnedMesh.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Mugen87 Is the current API acceptable to you?

Copy link
Collaborator

@Mugen87 Mugen87 Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me!

<code>
const target = new THREE.Vector3();
target.fromBufferAttribute( mesh.geometry.attributes.position, index );
Expand Down