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 optionalTarget parameter to Raycaster.intersectObject(s) to allow array reuse #12872

Merged
merged 1 commit into from
Mar 10, 2018

Conversation

ngokevin
Copy link
Contributor

@ngokevin ngokevin commented Dec 12, 2017

Description of the Problem

Allow optionalTarget for raycaster.intersectObject and raycaster.intersectObjects. This lets developers reuse array and reduce array instantiations.

For A-Frame/WebVR, an application often has two raycasters, one for each hand, updating up to 90 times a second. Two hands would have 180 raycaster intersects per second. So this could save 180 array instantiations per second for us.

@WestLangley
Copy link
Collaborator

This PR has implications for #12231.

@ngokevin
Copy link
Contributor Author

Cool, that looks agreeable, I always try to pass in a target. Does this look good in the mean time so it can get picked up as part of that overhaul?

@WestLangley
Copy link
Collaborator

Can you please explain your use case? What is the compelling reason for this change?

@ngokevin
Copy link
Contributor Author

We have raycasters that raycast frequently for a VR app (10 to potentially 90 times a second each). Saving unnecessary arrays will reduce GC and memory usage to increase performance. Not much different from wanting to save memory in the other optionalTarget-type patterns.

@WestLangley
Copy link
Collaborator

How are you clearing out the array between raycast calls so previous intersections are not sorted?

@ngokevin
Copy link
Contributor Author

I'm using array.length = 0;

@ngokevin
Copy link
Contributor Author

ngokevin commented Jan 7, 2018

I can change the PR to automatically clear the optional target with array.length = 0.

@WestLangley
Copy link
Collaborator

Ugh... I was confused because Raycaster.intersectObject() calls intersectObject()-- a different function with the same name that, itself, is recursive.

I do not have a problem with this PR as written, but I'll defer to others. Thank you for the suggestion.

If this is merged, please update the docs.

@ngokevin
Copy link
Contributor Author

I've updated the docs and made it clear the array/optional target if specified (since there's no reason not to clear the array).

r? @Mugen87

@mrdoob mrdoob added this to the r90 milestone Feb 5, 2018
var intersects = [];
var intersects = optionalTarget || [];

// Clear target array if specified.
Copy link
Owner

Choose a reason for hiding this comment

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

I think I would prefer if the clearing logic was in the application (ie. aframe) layer instead.

@mrdoob mrdoob modified the milestones: r90, r91 Feb 13, 2018
@ngokevin
Copy link
Contributor Author

Updated to not clear the array automatically.

@ngokevin
Copy link
Contributor Author

Merge conflicts fixed.

<div>
<p>
[page:Object3D object] — The object to check for intersection with the ray.<br />
[page:Boolean recursive] — If true, it also checks all descendants. Otherwise it only checks intersecton with the object. Default is false.
[page:Boolean recursive] — If true, it also checks all descendants. Otherwise it only checks intersecton with the object. Default is false.<br />
[page:Array optionalTarget] — (optional) target to set the result. Otherwise a new [page:Array] is instantiated. Application is responsible for clearing this array on each call (e.g., array.length = 0;).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of

Application is responsible for clearing this array on each call (e.g., array.length = 0;).

It may be more clear like so:

If set, you must clear this array prior to each call (e.g., array.length = 0;).

@ngokevin
Copy link
Contributor Author

ngokevin commented Mar 9, 2018

Updated the docs comment.

@WestLangley
Copy link
Collaborator

I think this is fine.

@mrdoob mrdoob merged commit 578273a into mrdoob:dev Mar 10, 2018
@mrdoob
Copy link
Owner

mrdoob commented Mar 10, 2018

Thanks!

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.

4 participants