-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
IntersectUtils: improve intersectRay #654
Conversation
Thanks! In terms of making the PR more readable I think it would be best to relegate the changes to just the "intersectRay" function and not change the function signature. In terms of performance comparison I think it would be best to perform a benchmark that compares only the Also in terms of your comparison screenshot - what framerate does your monitor run at? Is it over 120hz? I'm surprised to such high fps. It might be best to perform these measurements in milliseconds, instead. |
So you would like to avoid precomputing values and calculating them each time for cleaner code? I try to run benchmarks even without them.
I could try, but I'm not sure if they would accept the change.
I have a 165hz monitor, nvidia rtx 3080 ti and amd ryzen 9 5950x (a bad setup if I want to test 😂) What about also adding a distance check (using raycaster.far) to avoid iterating nodes that are too far away? Tomorrow I'll send you a list with execution times :) |
Yes I think I'd like to prioritize cleaner code and we can ensure that this algorithm is faster, as well. If precomputing these values is where the improvement comes from then the algorithm itself isn't faster - it's just the precomputation - which can be added for the existing Box3 algorithm if we want. My understanding is that a lot of these "branchless" and vectorized algorithms are designed for GPUs and benefits don't necessarily translate to CPU implementations so it's best to make one change at a time.
You have computed
I think this is a separate concern and can be handled in another PR. As far as I know raycasting doesn't account for |
Thanks for the benchmark page - I've tried editing a few things myself and I'm seeing that if we adjust algorithm itself to match three.js' such that it reads from the array buffer there is no difference in timing at all compared to the new change. Even changing the ray definition to use Float64Arrays seems to have little to no effect on perf. See this adjusted function based on three.js': this.intersectBox = function ( nodeIndex32, array ) {
let tmin, tmax, tymin, tymax, tzmin, tzmax;
const invdirx = dirInv[ 0 ],
invdiry = dirInv[ 1 ],
invdirz = dirInv[ 2 ];
const ox = origin[ 0 ];
const oy = origin[ 1 ];
const oz = origin[ 2 ];
let minx = array[ nodeIndex32 ];
let maxx = array[ nodeIndex32 + 3 ];
let miny = array[ nodeIndex32 + 1 ];
let maxy = array[ nodeIndex32 + 3 + 1 ];
let minz = array[ nodeIndex32 + 2 ];
let maxz = array[ nodeIndex32 + 3 + 2 ];
if ( invdirx >= 0 ) {
tmin = ( minx - ox ) * invdirx;
tmax = ( maxx - ox ) * invdirx;
} else {
tmin = ( maxx - ox ) * invdirx;
tmax = ( minx - ox ) * invdirx;
}
if ( invdiry >= 0 ) {
tymin = ( miny - oy ) * invdiry;
tymax = ( maxy - oy ) * invdiry;
} else {
tymin = ( maxy - oy ) * invdiry;
tymax = ( miny - oy ) * invdiry;
}
if ( ( tmin > tymax ) || ( tymin > tmax ) ) return null;
if ( tymin > tmin || isNaN( tmin ) ) tmin = tymin;
if ( tymax < tmax || isNaN( tmax ) ) tmax = tymax;
if ( invdirz >= 0 ) {
tzmin = ( minz - oz ) * invdirz;
tzmax = ( maxz - oz ) * invdirz;
} else {
tzmin = ( maxz - oz ) * invdirz;
tzmax = ( minz - oz ) * invdirz;
}
if ( ( tmin > tzmax ) || ( tzmin > tmax ) ) return null;
if ( tzmin > tmin || tmin !== tmin ) tmin = tzmin;
if ( tzmax < tmax || tmax !== tmax ) tmax = tzmax;
//return point closest to the ray (positive side)
if ( tmax < 0 ) return null;
return true;
}; Specifically adding this line seems to cause the performance to dip from ~280ms to ~ 350ms (~25% increase in time) in Chrome: arrayToBox( nodeIndex32, array, _boundingBox ); To that end, unless you're seeing something different, I think just modifying the existing |
I agree. Cleaner and we are sure it works :) I updated the code, is that okay? we can also change
Can I open a bug so I can try it as soon as I have time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I open a bug so I can try it as soon as I have time?
That would be great. If you'd like to open an issue for supporting raycaster.near and far, as well, that would be helpful, as well.
@@ -1,10 +1,74 @@ | |||
import { Box3 } from 'three'; | |||
import { arrayToBox } from '../../utils/ArrayBoxUtilities.js'; | |||
export function intersectRay( nodeIndex32, array, ray ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment noting that this code is copied from three.js' and why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this okay?
Looks great, thanks! |
Hi :)
I tried to implement a more suitable function to control the box - ray intersection (following this article), even if it is obviously less clean than the current implementation.
There is a slight improvement on my pc, but I would like to do some more testing as well.
Do you think this approach could be valid?
I used global variables instead of passing the same arrays recursively. Obviously I don't like it, I should modify it.
Would it make sense to implement a distance control directly here to discard nodes that are too far away? (I left the check commented out)