Skip to content

Conversation

amarpMSFT
Copy link
Collaborator

HLSL portion of proposal to allow shaders access to raytracing hit triangle position data from BVH (so apps don't have to manually source a copy of this data).

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

As a meta point, I know that @damyanp told you to split proposals this way, but I really don't like it.

In a case like this where we wouldn't approve one without the other it duplicates the review effort and tracking. Concretely for this proposal I've had to swap back and forth between the two to see that they don't match.

I'll grab some of time from @damyanp to discuss our process more clearly.


Other approaches have been proposed for the return type of this intrinsic:

* Current approach: return built-in struct containing all three vertices as `float3` fields:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is represented in the HLSL, but not correctly reflected in the DXIL proposal (https://github.com/microsoft/hlsl-specs/pull/572/files). I believe that the document that this was taken from was not correctly updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On re-reviewing the other doc I guess it isn't so much that it was out of date for the feedback as much as it didn't factor in the other changes in SM 6.9 (specifically DXIL Vectorization). I'll comment over in that PR about more specific thoughts.


```cpp
template<uint ConstRayFlags>
class RayQuery {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will be simpler for us to comply with our namespacing policy (https://github.com/microsoft/hlsl-specs/blob/main/docs/Process.md#conforming-extensions) if we also provide this implementation for SPIRV.

I believe this is fully compatible with SPV_KHR_ray_tracing_position_fetch, but it would be good to get an opinion from @s-perron and/or @Keenuts.

@amarpMSFT
Copy link
Collaborator Author

As a meta point, I know that @damyanp told you to split proposals this way, but I really don't like it.

In a case like this where we wouldn't approve one without the other it duplicates the review effort and tracking. Concretely for this proposal I've had to swap back and forth between the two to see that they don't match.

I'll grab some of time from @damyanp to discuss our process more clearly.

Sounds good, understandable that how best to split HLSL/DXIL need some iteration. Happy to recombine, let me know once you've decided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants