Closed archimedus closed 1 month ago
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.
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 https://github.com/google/amber/pull/1035#discussion_r1561074103:
- (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 https://github.com/google/amber/pull/1035#discussion_r1561074103, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACLC6CATL2ZTJOCDOK3UTY42JTXAVCNFSM6AAAAABFXNG7XKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJUGI3TCOJWGY . You are receiving this because you commented.Message ID: @.***>
You can download clang-format
as part of the LLVM binaries at https://github.com/llvm/llvm-project/releases/tag/llvmorg-17.0.1
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.
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?
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?
Thanks! I'm not sure whether the formatting is as-intended, why start one line with
PIPELINE
and then have a second line starting withLIBRARY
? 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
andRAY_HIT_ATTRIBUTE_SIZE
maybe should have aMAX_
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 VkPipeline
s 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
Thanks, the syntax looks good to me.
Are there other pipeline flags we may want to set? Having it FLAGS LIBRARY
may make more sense in that case.
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.
@dj2
I accepted CLA, but it seems github added you as a co-author, thus it requires you to accept the CLA.
Fixed, needed to associate the email github picked with my account. Is this ready for me to do another round of reviews?
Thanks.
Yes, I think it is ready for review.
The library support I think I should add as next pull request.
kokoro:run
Rebased.
@dj2
Test cases executed: 183
Successes: 74
Failures: 72
Suppressed: 37
Is that expected or something was broken by this PR?
They all passed with the DeviceProperties CL, and with my fixup CL. So, it seems like something in this PR is causing the failures.
I will investigate
@dj2 I think it is ready to be merged
Merged. Thank you very much for the design, implementation, and patience as we worked through the bot issues. Very much appreciate the contribution.
Ray Tracing support in Amber #1033