glslang icon indicating copy to clipboard operation
glslang copied to clipboard

Make gl_HitT proper aliases of gl_RayTmax

Open Max-Andersson opened this issue 2 years ago • 3 comments

In https://github.com/KhronosGroup/glslang/pull/2759, a mechanism to better handle the aliased builtin variables gl_VertexID and gl_VertexIndex (and their Instance counterparts) was committed. There is a similar problem for gl_HitTEXT and gl_RayTmaxEXT (and their NV counterparts), where they end up handled as duplicated variables and SPIR-V generation ends up creating two OpVariables with the same builtin decoration.

This PR simply makes use of the RetargetVariable code from https://github.com/KhronosGroup/glslang/pull/2759, as well as remove some then redundant code for handling gl_HitT.

Max-Andersson avatar Mar 22 '22 14:03 Max-Andersson

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 22 '22 14:03 CLAassistant

By my reading, gl_HitTNV and gl_RayTmaxNV are not aliases in GL_NV_ray_tracing. Likewise HitTNV and RayTmaxNV are different things in SPV_NV_ray_tracing. So this should not cause changes for GL_NV_ray_tracing shaders targeting SPV_NV_ray_tracing. Right?

Yet I am seeing, for example, changes for spv.AnyHitShader.rahit test which is a GL_NV_ray_tracing shader targeting SPV_NV_ray_tracing.

Should this be changing? Or am I misunderstanding something here?

greg-lunarg avatar Mar 25 '22 17:03 greg-lunarg

I see, indeed the SPIR-V has two distinct decorations for the two in NV, unlike the EXT versions which only has RayTmaxKHR. I'll revert the changes to the NV variables, thanks for pointing it out!

Max-Andersson avatar Mar 29 '22 14:03 Max-Andersson

@Max-Andersson do you plan to pick this back up at some point?

arcady-lunarg avatar Apr 06 '23 19:04 arcady-lunarg

@greg-lunarg Looking at the GLSL_NV_ray_tracing spec, it does say "gl_HitTNV is available only in the any-hit and closest-hit shaders. This is an alias of gl_RayTmaxNV added to closest-hit and any-hit shaders for convenience." (line 657 here: https://github.com/KhronosGroup/GLSL/blob/master/extensions/nv/GLSL_NV_ray_tracing.txt). Seems like the specs are somewhat ambivalent on the issue.

arcady-lunarg avatar Apr 27 '23 22:04 arcady-lunarg