Skip to content
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

GL_EXT_ray_query updates #2139

Merged
merged 26 commits into from
Mar 25, 2020
Merged

GL_EXT_ray_query updates #2139

merged 26 commits into from
Mar 25, 2020

Conversation

neslimsah
Copy link
Contributor

rayQuery extension updates.

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2020

CLA assistant check
All committers have signed the CLA.

@johnkslang
Copy link
Member

@neslimsah can you make an account and sign the CLA? Or, possibly have this come from someone who has done that?

Thanks.

SPIRV/spirv.hpp Outdated Show resolved Hide resolved
@jiaolu
Copy link

jiaolu commented Mar 23, 2020

the test case are raygeneration shader stage. should add graphics and compute shader stages test cases

@neslimsah
Copy link
Contributor Author

the test case are raygeneration shader stage. should add graphics and compute shader stages test cases

You mean graphics and compute shader stage test cases for rayQuery extention? Thanks.

Copy link

@jiaolu jiaolu left a comment

Choose a reason for hiding this comment

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

In these test case, it would be better to use rayFlags to pass in the rayQueryInitializeEXT
gl_RayFlagsOpaqueEXT
gl_RayFlagsNoOpaqueEXT
gl_RayFlagsSkipClosestHitShaderEXT

than numerical value.

@johnkslang
Copy link
Member

I see bots are reporting:

All checks have passed
2 successful checks

We will need to know why there are only 2 checks. Travis CI, for example, did not run. I see a change to allow 30 minutes, but hopefully that's actually not the point. Some bot runs occasionally fail, due to timeouts, where it appears some part of the process hung. But, I'm not sure now what the current situation is.

.travis.yml Outdated Show resolved Hide resolved
SPIRV/spirv.hpp Outdated Show resolved Hide resolved
glslang/MachineIndependent/ParseHelper.cpp Outdated Show resolved Hide resolved
glslang/MachineIndependent/Intermediate.cpp Outdated Show resolved Hide resolved
glslang/MachineIndependent/ParseContextBase.cpp Outdated Show resolved Hide resolved
glslang/MachineIndependent/ParseHelper.h Outdated Show resolved Hide resolved
@alelenv
Copy link
Contributor

alelenv commented Mar 24, 2020

@Tobski @neslimsah : rayQuery should be passed by reference even to user functions similar to samplers right?
Current code seems to do pass by value.
Also rayQuery is modifiable via intrinsics but can't be assigned to so I think the lvalue check should still be there

@Tobski
Copy link
Contributor

Tobski commented Mar 24, 2020

@Tobski @neslimsah : rayQuery should be passed by reference even to user functions similar to samplers right?
Current code seems to do pass by value.

I think this is correct? The "rayQuery" itself is a reference. It's a bit weird but I think it's consistent with how we've defined SPIR-V (which is also weird because reasons...). I could be missing something though?

SPIRV/GlslangToSpv.cpp Outdated Show resolved Hide resolved
@johnkslang
Copy link
Member

I re-reviewed the whole thing.

I only had one nit/question/desired-improvement about the odd switch statement layout above.

Modulo that, I'm okay with this change, but see there might be some open questions you still want to resolve.

After answering my switch statement question, just let me know when your satisfied with it, and I can merge it.

@dgkoch
Copy link
Contributor

dgkoch commented Mar 25, 2020

@johnkslang looks like all issues have been addressed and is passing now, can we get it merged?

Thanks.

@johnkslang johnkslang merged commit 8e26c5f into KhronosGroup:master Mar 25, 2020
@alelenv
Copy link
Contributor

alelenv commented Mar 25, 2020

My question was not resolved and i have filed issue #2150 to discuss/resolve there

@neslimsah
Copy link
Contributor Author

pass-by-reference issues #2150 and #2153 are fixed by #2157.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants