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

Implement VK_KHR_ray_tracing in HLSL #1364

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

maierfelix
Copy link

@maierfelix maierfelix commented May 16, 2020

WIP:

  • Didn't tackle callable shaders yet
  • Tests - I hope someone can help on this

The trickiest part is that HLSL only accepts structs for hit attributes and payloads. When a primitive (such as float or vec4) is used, I fallback to temporary structs and copy the values around.

Also the ray payloads are passed into the entry function as inout, so are limited in their scope. To overcome this, I create a static global variable (to mimic GLSL) and copy the input and output back in the entry function.

Also I did some NV to KHR transitioning, hope that's fine, I can revert if not

Edit: Link issue

@CLAassistant
Copy link

CLAassistant commented May 16, 2020

CLA assistant check
All committers have signed the CLA.

main.cpp Outdated Show resolved Hide resolved
spirv_hlsl.cpp Outdated Show resolved Hide resolved
spirv_hlsl.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@HansKristian-Work HansKristian-Work left a comment

Choose a reason for hiding this comment

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

Skimmed through. Seems reasonable enough, I don't anticipate any major hurdles to get this merged.

But first, we need to get two major concerns out of the way:

  • Please use the NV extension for now. Once the KHR spec is out of beta, I will migrate HLSL/GLSL implementations to use KHR instead.
  • A change on this scope must have tests. This also helps me cross-check the implementation. Please let me know if you need help writing tests. It should be fairly straight forward. You place test shaders in shaders-hlsl/rchit/ (different folders for different shader types) and run the test scripts to create reference outputs, which are committed along with the PR.

The reference HLSL files won't compile on either fxc or glslangValidator, so you should make sure the test files have .fxconly.nofxc. in the file name of the shader. I'll check them with DXC.

The tests should exercise:

  • All builtin intrinsics
  • Different use of payloads.
  • IncomingRayPayloads can be either plain variables, structs, or blocks
  • Multiple output ray payloads, they can be either plain variables, structs or blocks.

@maierfelix
Copy link
Author

I'd leave callable shaders out, someone with experience in using them should do the implementation.

If you could create a reference test which I can base off, and also show how I can run the tests, that'd be great.

@HansKristian-Work
Copy link
Contributor

Ok, I'll try to write a more complete suite of tests for ray-tracing, the current test coverage is kind of lacking.

See https://github.com/KhronosGroup/SPIRV-Cross#testing for how to run the test suite.

@HansKristian-Work
Copy link
Contributor

Here: #1369. You should be able to copy them over to shaders-hlsl and use that as the basis for the HLSL implementation.

@maierfelix
Copy link
Author

Thanks!

You place test shaders in shaders-hlsl/rchit/ (different folders for different shader types) and run the test scripts to create reference outputs, which are committed along with the PR.

Just to clear upfront, I'm using temporary variable identifiers here for example. Isn't that becoming a problem when creating reference output files?

@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented May 20, 2020

Why would that be a problem? RayDesc should be part of the "banned identifier list" though. Anyways, to start off with, please make sure you can run the test suite on plain master and get that to pass. From there you can start to migrate the new RT tests over to HLSL as well.

@maierfelix
Copy link
Author

I mean ray_uid

@HansKristian-Work
Copy link
Contributor

I mean ray_uid

For temporaries like that you can reserve an ID for it with uint32_t id = ir.increase_bound_by(1) and use to_name on it to get IDs like _1254. These are guaranteed to not conflict with anything else.

@HansKristian-Work
Copy link
Contributor

Hi. It's been a while since last update. Should I expect further updates on this PR? If not, I might be able to continue on it.

@maierfelix
Copy link
Author

Hey sorry for not updating this, I've hit an unexpected chunk of work. It'd be great if you could continue on it

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

Successfully merging this pull request may close these issues.

3 participants