Initial implementation Ray Tracing support in Amber

[archimedus]

Ray Tracing support in Amber #1033

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[dj2]

I'm not quite done the review yet, I'm about half way through engine_vulkan and then have the following files to do. Wanted to send this out as I have to leave for the day and am off until Tuesday of next week. I'll finish the review when I'm back on Tuesday.

[dj2]

The script expects to find `clang-format` on your path, so you'll need clang-format somewhere. (The script is just a thin wrapper around clang-format to call it for the various folders, if you have another way to run clang-format, that also works) dan

…

On Thu, Apr 11, 2024 at 10:01 AM archimedus ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/vulkan/blas.cc <#1035 (comment)>: > + (uint32_t)geometryData->getPrimitiveCount(), // uint32_t primitiveCount; + 0, // uint32_t primitiveOffset; + 0, // uint32_t firstVertex; + 0 // uint32_t firstTransform; + }; + + accelerationStructureGeometriesKHR_[geometryNdx] = accelerationStructureGeometryKHR; + accelerationStructureBuildRangeInfoKHR_[geometryNdx] = accelerationStructureBuildRangeInfosKHR; + maxPrimitiveCounts_[geometryNdx] = accelerationStructureBuildRangeInfosKHR.primitiveCount; + vertexBufferOffsets[geometryNdx] = vertexBufferSize; + size_t s1 = sizeof( + *geometryData->GetData().data()); + vertexBufferSize += align(geometryData->GetData().size() * s1, 8); + } + + const VkAccelerationStructureGeometryKHR* accelerationStructureGeometriesKHRPointer = accelerationStructureGeometriesKHR_.data(); ./tools/format find: ‘clang-format’: No such file or directory Any hints? — Reply to this email directly, view it on GitHub <#1035 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACLC6CATL2ZTJOCDOK3UTY42JTXAVCNFSM6AAAAABFXNG7XKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJUGI3TCOJWGY> . You are receiving this because you commented.Message ID: ***@***.***>

[ben-clayton]

You can download clang-format as part of the LLVM binaries at https://github.com/llvm/llvm-project/releases/tag/llvmorg-17.0.1

[jasilvanus]

My questions are regarding the planned final script format. I'm aware this is in an initial state as of now.

How can max payload and hit attribute sizes be specified?

Are pipeline libraries (planned to be) supported?

[jasilvanus]

How are non-existing/null shaders (e.g. null AHS) specified here?

[archimedus]

How can max payload and hit attribute sizes be specified?

I am not sure what do you mean. These values are fully inside shader interfaces and do not need any regulation through scripting language.

Are pipeline libraries (planned to be) supported?

Not sure yet.

How are non-existing/null shaders (e.g. null AHS) specified here?

I should update documentation to:

SHADER_GROUP {group_name_4} [any_hit_shader_name] [closest_hit_shader_name] [intersection_shader_name]

i.e. you can skip unused shaders, actually code does not enforce shader order two only things checked: (1) is that code does not mix general shaders with hit shaders; (2) there are no two same types within group (i.e. group does not include two+ intersection shaders or two+ anyhit shaders or two+ closesthit shaders).

[jasilvanus]

How can max payload and hit attribute sizes be specified?

I am not sure what do you mean. These values are fully inside shader interfaces and do not need any regulation through scripting language.

I was referring to https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkRayTracingPipelineInterfaceCreateInfoKHR.html but that might only be relevant when using libraries.

Or is there an alternate shader-level mechanism to specify the max payload and hit attribute size of other shaders in the pipeline, say when early-compiling an Intersection shader that doesn't know the payload types it will be used with?

Are pipeline libraries (planned to be) supported?

Not sure yet.

Supporting libraries would be very helpful. It doesn't have to be in the initial version, but we should have an idea how it should look like so we don't have to break back-compat when introducing it.

How are non-existing/null shaders (e.g. null AHS) specified here?

I should update documentation to:

SHADER_GROUP {group_name_4} [any_hit_shader_name] [closest_hit_shader_name] [intersection_shader_name]

i.e. you can skip unused shaders, actually code does not enforce shader order two only things checked: (1) is that code does not mix general shaders with hit shaders; (2) there are no two same types within group (i.e. group does not include two+ intersection shaders or two+ anyhit shaders or two+ closesthit shaders).

I see, sounds good. We should mention though that the order doesn't matter, and that shader types are automatically derived.

[archimedus]

For raytracing libraries (probably it can be later extended for graphics, though I am interested in RT only here) I think following syntax should cover subject:

PIPELINE ... [RAY_PAYLOAD_SIZE <integer>] [RAY_HIT_ATTRIBUTE_SIZE <integer>]
LIBRARY
USE_LIBRARY <name1> [<name2> [...]]
...
END

The first line should declare library.
The second line should be used in final pipeline or another library using this one.
I think this should cover raytracing pipeline libraries.

What do you think?

[jasilvanus]

Thanks! I'm not sure whether the formatting is as-intended, why start one line with PIPELINE and then have a second line starting with LIBRARY? Maybe its best to give a small full example.

The keywords RAY_PAYLOAD_SIZE and RAY_HIT_ATTRIBUTE_SIZE maybe should have a MAX_ prefix?

[archimedus]

Thanks! I'm not sure whether the formatting is as-intended, why start one line with PIPELINE and then have a second line starting with LIBRARY? Maybe its best to give a small full example.

Well, indentation is lost. It should be like this:

PIPELINE ... [MAX_RAY_PAYLOAD_SIZE <integer>] [MAX_RAY_HIT_ATTRIBUTE_SIZE <integer>]
    LIBRARY
    USE_LIBRARY <name1> [<name2> [...]]
...
END

The keywords RAY_PAYLOAD_SIZE and RAY_HIT_ATTRIBUTE_SIZE maybe should have a MAX_ prefix?

Will add MAX_

LIBRARY is a flag here to mark that we need to have VK_PIPELINE_CREATE_LIBRARY_BIT_KHR flag.
USE_LIBRARY is a list of VkPipelines we need to add into VkRayTracingPipelineCreateInfoKHR::pLibraryInfo

I was thinking about something like this (I hope it is feasible):

PIPELINE raytracing lib_a
  LIBRARY
  SHADER_GROUP g1 raygen1
  SHADER_BINDING_TABLE sbt1
    g1
  END
END

PIPELINE raytracing lib_b
  LIBRARY
  SHADER_GROUP g2 miss1
  SHADER_BINDING_TABLE sbt2
    g2
  END
END

PIPELINE raytracing lib_c
  LIBRARY
  SHADER_GROUP g3 anyhit1 closesthit1 intersection1
  SHADER_BINDING_TABLE sbt3
    g3
  END
END

PIPELINE raytracing my_rtpipeline
  USE_LIBRARY lib_a lib_b lib_c
END

RUN my_rtpipeline RAYGEN sbt1 MISS sbt2 HIT sbt3 1920 1080 1

[jasilvanus]

Thanks, the syntax looks good to me.

[dj2]

Are there other pipeline flags we may want to set? Having it FLAGS LIBRARY may make more sense in that case.

[archimedus]

Are there other pipeline flags we may want to set? Having it FLAGS LIBRARY may make more sense in that case.

Well VkPipelineCreateFlagBits have a bunch of flags, not sure which of these are supposed to be exposed.

I agree that FLAGS LIBRARY looks like a good point.

[archimedus]

@dj2

I accepted CLA, but it seems github added you as a co-author, thus it requires you to accept the CLA.

[dj2]

Fixed, needed to associate the email github picked with my account. Is this ready for me to do another round of reviews?

[archimedus]

Thanks.

Yes, I think it is ready for review.

The library support I think I should add as next pull request.

[archimedus]

kokoro:run

[archimedus]

Rebased.

[archimedus]

@dj2

Test cases executed: 183
  Successes:  74
  Failures:   72
  Suppressed: 37

Is that expected or something was broken by this PR?

[dj2]

They all passed with the DeviceProperties CL, and with my fixup CL. So, it seems like something in this PR is causing the failures.

[archimedus]

I will investigate

[archimedus]

@dj2
I think it is ready to be merged

[dj2]

This looks pretty close, I think the only outstanding was the last use of shared_ptr which I think could be a unique_ptr.

[dj2]

std::vector<std::unique_ptr<Geometry>> should work fine. It also makes the ownership clear.

[dj2]

Merged. Thank you very much for the design, implementation, and patience as we worked through the bot issues. Very much appreciate the contribution.