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

Add Mesh.getVertexPosition #25049

Merged
merged 4 commits into from Dec 22, 2022
Merged

Add Mesh.getVertexPosition #25049

merged 4 commits into from Dec 22, 2022

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Nov 30, 2022

Somewhat related issues: #18334, #14499

Description

I'm implementing dynamic hotspots for model-viewer, which requires me to find the current position of a vertex in a possibly animated mesh. Three.js does this internally for raycasting, but doesn't expose the method. I've refactored slightly to expose it and slightly simplify the raycasting code. I'm pretty sure I've seen forum discussions where this feature was asked for, so hopefully I'm not the only user. I have tested this (and the updated reduceVertices) on my dynamic hotspots branch of model-viewer and it's all working properly, handling morph targets, skinned meshes, etc.

@gkjohnson
Copy link
Collaborator

It would be nice to make this generic so it works across any vertex attribute (normal, tangent, etc). For three-mesh-bvh (and ultimately three-gpu-pathtracer) the StaticGeometryGenerator applies the morph target and skinned transforms manually:

https://github.com/gkjohnson/three-mesh-bvh/blob/7941ca674f3aad85fae13d00050ddc69b6675585/src/utils/StaticGeometryGenerator.js#L458-L497

@mrdoob
Copy link
Owner

mrdoob commented Dec 1, 2022

Handy! How about getVertexPosition?

@gkjohnson
Copy link
Collaborator

Handy! How about getVertexPosition?

Can this be something like getVertexAttribute( attrName, index, target ) so it can be used for any skinned or morphed attribute? Or perhaps getVertexPosition can be added along side getVertexAttribute for convenience?

@elalish
Copy link
Contributor Author

elalish commented Dec 1, 2022

@gkjohnson That sounds like a good idea to me. @mrdoob, would you prefer getVertexAttribute or getVertexPosition, getVertexNormal, and getVertexTangent? Either way I can update this and merge it for the position part and @gkjohnson can add the others; sound good?

@gkjohnson
Copy link
Collaborator

or getVertexPosition, getVertexNormal, and getVertexTangent?

Heh, just position, normal, and tangent are not necessarily enough. All vertex attributes include color, uv, and other custom attributes can all be morphed, no? I think it should be a general function and then implentations like getVertexPosition can look like so if needed:

getVertexPosition( i, target ) {

  return this.getVertexAttribute( 'position', i, target );

}

@elalish
Copy link
Contributor Author

elalish commented Dec 1, 2022

@mrdoob I've updated this to getVertexPosition; I think you can safely merge this to allow @gkjohnson to add his related methods more easily as a follow-on PR.


}

if ( !child.isSkinnedMesh ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linting:

The indentation is incorrect.

Also, it should be ! child.isSkinnedMesh -- with a space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good call; I thought the auto-linting would catch that kind of thing now.

@WestLangley
Copy link
Collaborator

Unrelated: reduceVertices() does not appear to be documented.

In what coordinate space is the returned position in getVertexPosition()?

@elalish
Copy link
Contributor Author

elalish commented Dec 2, 2022

Unrelated: reduceVertices() does not appear to be documented.

Sorry about that, I'll add it.

In what coordinate space is the returned position in getVertexPosition()?

It's in local space. Honestly though, I feel like there's some kind of bug, or something I don't understand, because I don't think that if ( ! child.isSkinnedMesh ) should be needed at all, yet it very much breaks when I remove it. The raycasting code doesn't need that, but I can't figure out what the difference is. I wonder if the updateMatrixWorld is somehow not updating the bone matrices?

@elalish elalish changed the title Add Mesh.getUpdatedVertex Add Mesh.getVertexPosition Dec 2, 2022
@mrdoob mrdoob added this to the r148 milestone Dec 21, 2022
@mrdoob mrdoob merged commit 87223b8 into mrdoob:dev Dec 22, 2022
@WestLangley
Copy link
Collaborator

I feel like there's some kind of bug, or something I don't understand

Please, please pursue this until it is understood.

@0b5vr 0b5vr mentioned this pull request Dec 23, 2022
10 tasks
0b5vr added a commit to three-types/three-ts-types that referenced this pull request Dec 27, 2022
- add `Mesh.getVertexPosition()`
- add doc comment of `SceneUtils.reduceVertices`

See: mrdoob/three.js#25049
@elalish elalish deleted the getUpdatedPosition branch December 27, 2022 21:32
@donmccurdy
Copy link
Collaborator

if ( ! child.isSkinnedMesh ) {

    vertex.applyMatrix4( child.matrixWorld );

}

The glTF specification requires that the world transform of the SkinnedMesh have no direct effect on the final position of a vertex during skinning. Only the world transform of the bones should be used. The bones may or may not be descendants of the SkinnedMesh.

The code above seems correct from this standpoint, if that's our intention for three.js as well.

@elalish
Copy link
Contributor Author

elalish commented Dec 28, 2022

Ah, thank you for that explanation @donmccurdy, that mostly makes sense. I think that means getVertexPosition returns things in a different coordinate space depending on whether the mesh is skinned or not, which is confusing. I'm guessing the reason it works with raycasting is that the ray is also transformed into two different possible coordinate spaces to match (though I have yet to figure out where that happens). @WestLangley do you think a small refactor is in order?

@WestLangley
Copy link
Collaborator

@elalish Skinning in three.js is implemented in world space on the GPU. As I have said elsewhere, I think that is unadvisable due to the precision problems it can introduce.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 28, 2022

@WestLangley
Copy link
Collaborator

Also related: #24479 (comment).

@donmccurdy
Copy link
Collaborator

Whatever we decide for skinning computations, the intention is for reduceVertices to produce output in world space, correct? We should clarify in documentation what boneTransform produces, too.

joshuaellis added a commit to three-types/three-ts-types that referenced this pull request Jan 3, 2023
* docs: update constructor doc comment of geometries

See: mrdoob/three.js#25086

* docs: update doc comment of the constructor of BufferAttribute

See: mrdoob/three.js#25046

the `normalized` was already optional in the typedef

* feat: add TwoPassDoubleSide

See: mrdoob/three.js#25165

also add doc comment of Side

* feat: add `Mesh.getVertexPosition()`

- add `Mesh.getVertexPosition()`
- add doc comment of `SceneUtils.reduceVertices`

See: mrdoob/three.js#25049

* feat: add `Object3D.getObjectsByProperty()`

- add `Object3D.getObjectsByProperty()`
- add doc comment of `Object3D.getObjectByProperty()`

See: mrdoob/three.js#25006

* docs: add a doc comment of `PointLight.castShadow`

See: mrdoob/three.js#25136

* feat (GLTFLoader): add loadNode hook

See: mrdoob/three.js#25077

* feat (Nodes): New features and revisions

- add three new nodes: `CacheNode`, `StackNode`, and `SpecularMIPLevelNode`
- add `NodeCache`
- add `getDefaultUV()` to `CubeTextureNode` and `TextureNode`
- add `isGlobal()` to `Node`
- add `cache` and `globalCache` to `NodeBuilder`
- add missing `flowsData` to `NodeBuilder`
- add `hasDependencies` to `TempNode`
- add `maxMipLevel` and `cache` to `ShaderNodeBaseElements`
- replace `maxMipLevel` -> `specularMIPLevel` of `ShaderNodeElements`
- change constructor type and texture type of `MaxMipLevelNode`
- add `SpecularMIPLevelNode`

* feat (Nodes): add FogExp2 node

Co-authored-by: Josh <37798644+joshuaellis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants