-
Notifications
You must be signed in to change notification settings - Fork 35
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
Extend BIH capabilities for intersection operations #1479
base: develop
Are you sure you want to change the base?
Extend BIH capabilities for intersection operations #1479
Conversation
Test summary 3 401 files 5 253 suites 3m 52s ⏱️ Results for commit 4adf145. ♻️ This comment has been updated with latest results. |
ed047f4
to
c768aa4
Compare
c768aa4
to
089bde7
Compare
@sethrj this is ready. Probably better to review this one first, then I will do the vol_id + others one. |
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.
Still working on this, but gotta take the day off and here's what I have so far.
{ | ||
Real3 pos; | ||
Real3 dir; | ||
}; |
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.
I don't think this is a good idea; it forces a copy of the data and increases register pressure. If anything, make the data members Real3 const&
and pass Ray
by value.
previous_node = exchange( | ||
current_node, | ||
this->next_node( | ||
current_node, previous_node, ray, intersection.distance)); |
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.
Nice use of exchange
! 😄
* nearer intersection with an edge bbox or volume bbox. This is because is is | ||
* possible to have a ray that interects with a volume's bbox, but not the | ||
* volume itself. | ||
* |
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 you add a brief synopsis of the expected form of the F
template function? (i.e. real_type(*)(VolumeId)
, it should return the distance to the intersection with the volume.)
{ | ||
for (auto id : view_.inf_volids()) | ||
{ | ||
auto intersection = visit_vol(id); |
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.
Since we can often reduce the cost of an intersection by passing a "maximum search distance", (i.e. if we know the closest volume is nearby we don't need to evaluate logic if the nearest surface is further than that) I think the visit function should include a "minimum" as an argument, just like the SimpleUnitTracker intersect does.
This MR creates the
BIHIntersectingVolFinder
class which is used to find the surface that a ray intersects with first, and the corresponding distance to this surface. This is done using a traversal strategy that is distinct fromBIHEnclosingVolFinder
, as is documented in their respective top-level doc strings. In a subsequent MR, this will be "hooked up" for use in background volumes.