Skip to content

Conversation

@agargaro
Copy link
Contributor

@agargaro agargaro commented Jun 11, 2024

I fixed the problem related to getScaleWorld and added tests.

I need to clean up the code, but can you tell me if the InstancedMesh tests are okay?

Shall I add tests on BatchedMesh too? (after mrdoob/three.js#28462)

related #671

@agargaro agargaro marked this pull request as draft June 11, 2024 21:25
Comment on lines -20 to +21
this.getWorldScale( worldScale );
getWorldScale( this.matrixWorld, worldScale );
Copy link
Owner

Choose a reason for hiding this comment

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

Oh interesting... So the previous issue was caused by Object3D.getWorldScale implicitly updating the world matrix without having synchronized position, rotation, and scale values. Confusing.

Why don't we use Matrix4.decompose, instead, and pass a dummy position and rotation into the function? I think it will be simpler than copying logic and directly accessing matrix elements.

@gkjohnson
Copy link
Owner

Shall I add tests on BatchedMesh too? (after mrdoob/three.js#28462)

If you don't mind I think this would be good to have

@agargaro
Copy link
Contributor Author

Accelerated raycasting does not seem to work on BatchedMesh.
It is currently ignored because the temporary mesh geometry used for raycasting does not have the boundsTree property (unlike InstancedMesh, it is not replaced by another geometry).

https://github.com/mrdoob/three.js/blob/master/src/objects/BatchedMesh.js#L876

I don't think it is a good idea to create a BVH from the merged geometry (containing the repeated geometries), so I would say to wait until your new changes are released.

I have some ideas in mind, but it may be better to talk about them later, in any case I think it will be necessary to add the possibility of creating separate BVH based on a range of indexes.

I hope I am not wrong 😂

@gkjohnson
Copy link
Owner

Accelerated raycasting does not seem to work on BatchedMesh.
It is currently ignored because the temporary mesh geometry used for raycasting does not have the boundsTree property (unlike InstancedMesh, it is not replaced by another geometry).

Oh ha yes I should have known this 😅

I don't think it is a good idea to create a BVH from the merged geometry

Definitely agree.

I have some ideas in mind, but it may be better to talk about them later

Yeah I think this is separate from this PR. #660 was opened in order to afford the ability to create a BVH from the subrange of a geometry specifically for BatchedMesh but it remains unfinished. We can ping or finish up that PR if we'd like to move it along. I think in the long run we'd want to keep a list of BVH's per batched geometry and then we'd have to add a special case in the acceleratedRaycast function to take advantage of it.

@agargaro
Copy link
Contributor Author

We can ping or finish up that PR if we'd like to move it along.

Okay, if he cannot I will continue his work.

@gkjohnson gkjohnson merged commit 55bc6fa into gkjohnson:master Jul 2, 2024
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.

2 participants