Conversation
…t_surface_connectivity()
…abstracted to MeshManager level
…get_volume_connectivity()
…rary numbers of vertices
5f20f5b to
ac4c7f3
Compare
pshriwise
left a comment
There was a problem hiding this comment.
A few thoughts and comments here.
One larger question: Where is the box expansion occurring in the GPRTRayTracer? I see the expansion using FLT_EPSILON in populate_aabb, but for larger volumes this expansion may not be enough to guarantee we don't run into false negatives when traversing the tree.
| const dp::vec3 v2, | ||
| const dp::vec3 v3) { | ||
|
|
||
| // TODO - I've decided to use Cramer's rule here instead of matrix inversion to make it easier to implement a cross-compilable version of this function |
There was a problem hiding this comment.
In general this looks fine to me. In my branch for quads/hexes I've moved to a Plucker coordinate check against face and I might recommend we change to that broadly as it naturally extends to linear elements with an arbitrary number of faces, but I'm curious, what was incompatible in the previous version?
There was a problem hiding this comment.
It's been a while, but I think the main issue was related to row/column convention differences between the matrix API used by GPRT/Slang and the linalg API used on the C++ side. The old method (still in main) was doing this matrix multiplication to solve for our barycentric coords:
// Solve T * [λ1, λ2, λ3] = rhs
double3 lambda123 = mul(inverse(T),{rhs.x, rhs.y, rhs.z});I initially tried to implement the same logic on the Slang side, but I got different barycentric coordinates on CPU vs GPU. My understanding is that the two math APIs were interpreting the matrix row/column ordering differently which was leading to differences when I would call mul(...).
I switched to Cramer's rule to avoid the matrix inversion and multiplication path entirely. That keeps the shared Slang/C++ implementation to just dot and cross products, which is easier to keep consistent across both C++ and slang.
I might recommend we change to that broadly as it naturally extends to linear elements with an arbitrary number of faces, but I'm curious, what was incompatible in the previous version?
Agreed, seems reasonable to move towards a more arbitrary method.
include/xdg/gprt/ray_tracer.h
Outdated
|
|
||
| // Ray Generation parameters | ||
| uint32_t numRayTypes_ = 1; // <! Number of ray types. Allows multiple shaders to be set to the same geometery | ||
| uint32_t numRayTypes_ = 2; // <! Number of ray types. 0=surface, 1=volume |
There was a problem hiding this comment.
Let's create a new enum for this property if there isn't one already.
There was a problem hiding this comment.
Oh, I see there are some constants in shared_structs.h. Let's apply RT_NUM_RAY_TYPES here.
There was a problem hiding this comment.
Oh, I see there are some constants in shared_structs.h. Let's apply RT_NUM_RAY_TYPES here.
Yeah, not sure what the best place for these would be since they're constants but will only ever be used inside of GPRTRayTracer and the slang shaders so it didn't feel appropriate to put them in constants.h. But open to suggestions on that front.
src/gprt/triangle_rt_shaders.slang
Outdated
| uint rayID = DispatchRaysIndex().x; | ||
|
|
||
| // There is some logic for handling next volumes inside the h5m-reader which I could make use of too | ||
| // TODO : Should the dblHit struct return the next volume ID for the ray back to the host |
There was a problem hiding this comment.
Responding to this comment randomly: We could do but I don't see a reason to necessarily. As long as we know what volume we're in and what surface was hit it's a really cheap operation to find the next volume and in the case of particle transport we may not use it if a collision will occur before the particle reaches that surface.
There was a problem hiding this comment.
Makes sense. I'll remove this comment then.
src/gprt/triangle_rt_shaders.slang
Outdated
|
|
||
| // ------------------------------------------------ CUSTOM INTERSECTION SHADERS ------------------------------------------------ | ||
|
|
||
| /* 1D ray generation intersection with a double precision triangle using the Plucker intersection algorithm*/ |
There was a problem hiding this comment.
Sorry for the stray: The term "1D" threw me here for a sec.
| /* 1D ray generation intersection with a double precision triangle using the Plucker intersection algorithm*/ | |
| /* Single ray generation intersection with a double precision triangle using the Plucker intersection algorithm*/ |
There was a problem hiding this comment.
Agreed it is a little misleading. I've changed the comment to this in the latest commit:
// Custom FP64 Plucker ray-triangle intersection algorithm for each ray-triangle pair
[shader("intersection")]
void DPTrianglePluckerIntersection(uniform DPTriangleGeomData record)
{|
@pshriwise bb61bf6 addresses some of the comments you left. Most of the changes are self-explanatory, but i'll highlight one here: I added a new I also moved the enums from A couple of small syntax changes were needed to keep the header Slang-compatible: |
I see your point, this is missing from GPRT atm and we only do the |
| #ifndef XDG_SHARED_CONSTANTS_H | ||
| #define XDG_SHARED_CONSTANTS_H | ||
|
|
||
| namespace xdg { |
There was a problem hiding this comment.
As discussed live this week, probably best to avoid changing the infrastructure too drastically to accommodate slang. Sorry for the mislead here.
This PR adds all of the necessary setup to allow GPRT to perform ray tracing against volumetric (tet) meshes.
Changes:
GPRTRayTracer::find_element()and updatedtest_find_elementto test across both Embree and GPRT backends. Note - important to stress that this is a single ray dispatch version and an batch based dispatch will come in a later PR. Single dispatch versions do remain very useful for testing parity with Embree however.plucker_tet_containment_testcross compatible between slang and C++. In doing so I have opted to make use of Cramer's rule rather than matrix inversion since I was having trouble with matrices between host and device. GPRT'sdouble3x3required implementing my own matrix inversion function and had a different row-column ordering compared to linalg's implementation.It looks like a much larger PR than it really is since this PR also makes some structural changes to the file layout across shaders. Namely it splits shaders into three .slang files
rt_common.slangh,triangle_rt_shaders.slangandtetrahedron_shaders.slang. So around 🟩 +180 additions and 🟥 −270 deletions are solely from moving the existing shaders for surface ray tracing into the new files.Extra notes:
I guess in theory should we ever need to actually perform ray tracing operations against tets it also allows for that (would just need to define a new custom intersection shader for double precision) but as far as i know there is no use case for this required by OpenMC?