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

Improve ClosestPointToPoint #699

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

agargaro
Copy link
Contributor

@agargaro agargaro commented Aug 27, 2024

By not using the shapecast function, there is a slight improvement of about 25%.

I have added a page for benchmarks to be removed later called 'testToRemove.html' (I know, I have a big imagination).

I will also add the function using the sorted list shortly (#695).

Edit: I finished 3 first versions of closestPointToPoint.

@agargaro agargaro changed the title Improve ClosestPointToPoint Improve ClosestPointToPoint Aug 27, 2024
example/testToRemove.js Fixed Show fixed Hide fixed
Copy link
Owner

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Very nice work! Looking at the PQP_Distance function it looks like they use a similar approach to the hybrid method in that it starts a new sorted queue once the last one becomes full. I assume this has the benefit of limiting memory usage and preventing insertion time from becoming too long. I wonder if we'd see benefits from that solution here?

See the DistanceQueueRecurse function and this condition where it recurses and creates a new queue to traverse. Here is documentation from header file describing "qsize" use. Granted the function is a bit different because it's performing geometry-to-geometry so the queue is storing two nodes and a distance but fundamentally it's very similar.

if ( leftDistance < closestDistanceSq && leftDistance < maxThresholdSq ) {

if ( _closestPointToPoint( leftIndex ) ) return true;
if ( rightDistance < closestDistanceSq ) return _closestPointToPoint( rightIndex );
Copy link
Owner

Choose a reason for hiding this comment

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

Don't we want to check distance to maxThresholdSq here, as well?

@@ -0,0 +1,20 @@
export function closestDistanceSquaredPointToBox( nodeIndex32, array, point ) {
Copy link
Owner

@gkjohnson gkjohnson Aug 29, 2024

Choose a reason for hiding this comment

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

I'm wondering how much of the 25% improvement we would see if we used this function instead of three.js' built in version in the original "shapecast" function (see #656) - similar to the box / ray test where avoiding reading the bounds to a Box3 object gave a significant speed improvement.

Comment on lines +72 to +80
if ( count >= sortedListMaxCount ) {

if ( _closestPointToPoint( nodeIndex32 ) ) return;

continue;

}

count ++;
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain the logic and rationale of the hybrid function a bit? It looks like it will choose the "best" node from the first 16 nodes (by default) and then fall back to the basic approach of traversing to the leaves for each of those nodes in order. The count never goes down, right?

@agargaro
Copy link
Contributor Author

agargaro commented Sep 1, 2024

Regarding the use of sorted queue, I would like to do more tests to understand when it is necessary.
Using dynamic bvh I noticed that it seems useful when there are a lot of nodes.

I would also like to modify the current shapecast' function by removing the arrayToBox' conversion to understand if the performance improvement is due to that.

Edit: sorry I read later that you also recommended doing to this. I'll try these days :)

@agargaro agargaro marked this pull request as draft September 1, 2024 18:10
@agargaro
Copy link
Contributor Author

agargaro commented Sep 1, 2024

This is with the actual 'shapecast'

image

and this is with edited 'shapecast' (removing arrayToBox)

image

@agargaro
Copy link
Contributor Author

agargaro commented Sep 1, 2024

Unfortunately, modifying the shapecast function is quite a delicate process now, because box3 is also used by the shapecast caller, so this would be a breaking change.

I believe it is correct to expose a box3 to the API because it makes it much easier for the user to use.

Could we make an alternative method for internal use, which instead of passing the box3, passes the nodeIndex32 and the float32array?

Or could we stop using shapecast for internal methods (as I am doing in this PR)?

Or leave it as is.

The overhead of shapecast is mainly callbacks and superfluous data that is passed as a parameter but not used by the caller (like isLeaf here).

image

@gkjohnson
Copy link
Owner

Thanks for checking!

and this is with edited 'shapecast' (removing arrayToBox)
...
Could we make an alternative method for internal use, which instead of passing the box3, passes the nodeIndex32 and the float32array?

I was mostly curious as to how much of the improvement was coming from this conversion to a Box3. It's worth documenting in #656, I think, but given that it's a breaking change lets not make it for now.

If we have a method that would be frequently used and benefit from the performance boost (like we're doing here) then I think it makes sense. Shapecast is just easier to implement initially.

I'm curious about #699 (comment) as well as how the PQP_Distance approach compares to this hybrid approach:

Looking at the PQP_Distance function it looks like they use a similar approach to the hybrid method...

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