gboisse / gfx

A minimalist and easy to use graphics API.
MIT License
498 stars 35 forks source link

Add support for intersection shaders for HW study #130

Closed SShikhali closed 2 months ago

SShikhali commented 4 months ago
gboisse commented 3 months ago

This PR is looking real good, I just want to better understand why that "shifting left" is needed, did it actually cause a bug?

Otherwise, I noticed we're missing the handling of the new RaytracingPrimitive::kType_Procedural type in this switch statement: https://github.com/gboisse/gfx/blob/b67b718697af031b40a67dd704cff9e7e05196f9/gfx.cpp#L2135-L2146

Which yields a bit the question of can you instantiate a procedural primitive?

I believe it should be supported as it's just about repeating the BLAS with another transform, but unsure if the D3D12 specs have anything to say about this...

gboisse commented 3 months ago

So, investigating a bit more that "shift left" thing; this is the function that makes sure to "resolve" the array of raytracing primitives if any deletion has happened: https://github.com/gboisse/gfx/blob/fb1b2c707882bf1a4627949e0786ed76389298fd/gfx.cpp#L6675-L6693

So, we shouldn't need that newly added shift left logic in GfxInternal::destroyRaytracingPrimitive().

Possibly, what would be a more proper fix is to simply remove these 2 API functions as they needlessly expose internal state from the library that the user really shouldn't rely on: https://github.com/gboisse/gfx/blob/fb1b2c707882bf1a4627949e0786ed76389298fd/gfx.h#L190-L191

What do you think about this? would it be ok to remove these 2 functions?

gboisse commented 3 months ago

I think also that having a GfxAccelerationStructure automatically delete all GfxRaytracingPrimitive upon destruction is a design mistake too: https://github.com/gboisse/gfx/blob/fb1b2c707882bf1a4627949e0786ed76389298fd/gfx.cpp#L5160-L5166

Probably, we'd be better off removing that logic and let the user explicitly handle the lifetime of raytracing primitives.