Closed archimedus closed 2 weeks ago
From the description, it sounds like there are 4 separate PRs in here? Can we split them apart so they're easier to review?
What is the reason behind the INCLUDE
tag? We've historically been hesitant to add an actual include facility, adding the VIRTUAL_FILE
support to allow include
tags inside shaders for HLSL, because we've been concerned that once added, Amber files are no longer a single standalone script. It now has support files, it has paths that have to match when sending the files to someone else, etc. (https://github.com/google/amber/issues/222 is a request to allow external shader files)
Well, this feature set is just what I needed to have to implement https://gerrit.khronos.org/c/vk-gl-cts/+/14671.
From the description, it sounds like there are 4 separate PRs in here? Can we split them apart so they're easier to review?
I don't think it is worthy to split it this way: two other points are rather small improvements not worthy for separate issue.
I will move out INCLUDE as this really have few relations to name of this issue, though it will make more pain implementing issue I mentioned, due to ray scene declaration is a repetetive task that INCLUDE fits best.
It now has support files, it has paths that have to match when sending the files to someone else, etc. (https://github.com/google/amber/issues/222 is a request to allow external shader files)
That was exactly what I thought starting INCLUDE: well, does not it looks like it is time for INCLUDE?
Scanning through the gitlab PR, the INCLUDE
pulls out a few lines, but also makes it harder to read the individual scripts, so from someone just reading the .amber
files it makes them harder to understand.
For the others, I think geometry flags could be separated, the overall feature is small, but there is a bunch of stuff in there including amberscript parser changes which, would be a lot easier to review if separated from the pipeline library amberscript changes.
I'm not really sure what the null shader group entails from scanning the PR, so it maybe fine to leave as part of this PR.
Ok, here we have only library support.
I use shared_ptr<ShaderGroup>
, due to it is logically correct: ShaderGroup
really belong to several pipelines.
Please give it a run.
This is the first part of the review for what I've gotten through so far.
Is there a document on pipeline libraries I can read? I'm a bit confused by some of the things being done and am guessing it relates back to how vulkan sets things up.
I'm a bit confused as to why we jam all the things together early, like, why not keep shader groups in their individual Pipeline objects until we get to the vulkan backend and then we can stuff things together as needed? Does that remove the need for the non lib counts of shaders in pipelines?
I guess, an over arching design of how these pieces all fit together would be useful.
Is there a document on pipeline libraries I can read?
Well, I was using vulkan specification only, I cannot suggest anything better for ray tracing pipeline libraries. The most essential part that I found in spec I added as comment. As far as I understand pipeline libraries is mostly separated from graphics pipeline libraries (that is a located in different extension).
why not keep shader groups in their individual Pipeline objects until we get to the vulkan backend
I think the comment I added is best explains this:
// Here is a tricky step: add shaders and shader groups from library to
// pipeline itself. Like spec "suggests":
//
// If pipeline libraries are included in pLibraryInfo, shaders defined in
// those libraries are treated as if they were defined as additional entries
// in pStages, appended in the order they appear in the pLibraries array and
// in the pStages array when those libraries were defined.
//
// When referencing shader groups in order to obtain a shader group handle,
// groups defined in those libraries are treated as if they were defined as
// additional entries in pGroups, appended in the order they appear in the
// pLibraries array and in the pGroups array when those libraries were
// defined.The shaders these groups reference are set when the pipeline
// library is created, referencing those specified in the pipeline library,
// not in the pipeline that includes it.
On practical stage: amber file in pipeline (that uses some pipeline library) can refer to shader group names inherited from pipeline libraries used, thus we need to either parse whole tree of used pipeline libraries or do as I just did: include pipeline groups into pipeline that uses pipeline library. Same with shaders (pStages).
Does that remove the need for the non lib counts of shaders in pipelines? I am not sure what do you mean here.
As an example I suggest to take a look example I have written for CTS:
Could you please give another kokoro run?
Thank you.
Please another run to make sure that all is fixed.
Fixed few more things. I hope this time will go smooth.
It seems it's done.
Please give another try.
It seems no more fixes required here.
Extend ray tracing support in Amber with Pipeline Library support. Also add: