amber

amber copied to clipboard

Add support for ray tracing in Amber scripts. New syntax should include:

• new shader types
• geometries declaration (triangles, AABBs)
• bottom level acceleration structures declaration
• top level acceleration structures with declaration including
  1. instances
  2. transformations
  3. flags
• pipeline extension
  1. shader group declarations
  2. shader binding tables declarations
• run command extension to execute ray tracing pipeline

Syntax changes proposed at https://github.com/archimedus/amber/blob/main/docs/amber_script.md

Example CTS: https://gerrit.khronos.org/c/vk-gl-cts/+/12867

Awesome, thanks a lot for putting this together. Reading through the MD file a few questions/comments.

Listing things as I read through the doc, some of these refer to prior points I make (and some are potential modifications of those points).

1. I think we should make BLAS and TLAS more descriptive as names. Suggest something like ACCELERATION_STRUCTURE BOTTOM_LEVEL and ACCELERATION_STRUCTURE TOP_LEVEL. They're longer (we could drop the _LEVEL). But, they make it a lot clearer for someone that my not have as much ray tracing knowledge what they are.

2. SBT is very short, maybe SHADER_BIND_TABLE to be more descriptive?

3. I think AABB is fine, it's a pretty common acronym so I think having it as the shortened form doesn't hurt anything

4. BLAS_INSTANCE USE. Could this just be BOTTOM_LEVEL {name}? Is that too short? or even just something like ATTACH which would be similar to how things are attached to a pipeline?

5. For the TRIANGLE geometry, do we need the VERTEX keyword? Could we just have: TRIANGLE x0 y0 z0 x1 y1 z1 x2 y2 z2?

6. Similar for AABB geometry as it looks like you end up with AABB twice?

7. For GEOMETRY we allow multiple triangles and aabbs, so possibly pluralize the names GEOMETRY TRIANGLES and GEOMETRY AABBS ?

8. Or, maybe for both GEOMETRY TRIANGLE and GEOMETRY AABB it would be better to do something like:

GEOMETRY TRIANGLES
  TRIANGLE x0 y0 z0 x1 y1 z1 x2 y2 z2
  TRIANGLE x0 y0 z0 ...
END

GEOMETRY AABBS
  AABB x0 y0 z0 x1 y1 z1
  AABB ...
END

I think I like that better, thoughts?

9. The flags with the optional END feels a bit weird. Could we change it to just be a comma separated list, remove the ability to spread over multiple lines? (Or does that become too long because you often want to specify multiples of them?) Or, comma separated but allow line breaks, so no END needed, if there is no comma then then the list ends?

10. I don't understand what goes in transform. 12 space separated values, are those numbers, strings flags? Is this a 3x4 matrix transform?

11. For the SHADER_GROUP it says the ordering is essential. Is that the order of the SHADER_GROUP commands in the Amberscript, or the order of the shaders listed in the shader group command (like any_hit, closest_hit, etc). If it's the order in the Amber file is there something we can do to remove that restriction and fix the order up afterwards? Like SHADER_GROUP GENERATION {name} {ray_generation_shader_name}, SHADER_GROUP MISS {name} {miss_shader_name} and then I can put those in whatever order and we always reorder them to be raygen, miss, call, others after parsing the script?

12. What does the group_count mean on SBT? Does that mean that the group_name_1 shader group will be attached to the binding table that many times?

13. Reading the note on shader bind tables needed, could we do something like have named shader bind table entries. So, combined with point 11 above we end up with something like:

SHADER_GROUP MISS miss1 my_miss_shader
SHADER_GROUP MISS miss2 another_miss_shader
SHADER_GROUP RAYGEN gen my_gen_shader
SHADER_GROUP CALL call my_call_shader
SHADER_GROUP HIT hit1 my_any_hit my_closest_hit my_intersection
SHADER_GROUP HIT hit2 any_hit2 closest_hit2 intersection2

SHADER_BIND_GROUP MISS miss_group
  USE miss1
  USE miss2 MULTIPLE 3
END
SHADER_BIND_GROUP HIT hit_group
  USE hit1
  USE hit2
END
SHADER_BIND_GROUP CALL call_group
  USE call
END
SHADER_BIND_GROUP RAYGEN ray_group
  USE gen
END

We can then validate that the shader_bind_group types match up to the shader_group types what we defined above?

14. I think for BIND we should make the type required. So, BIND ACCELERATION_STRUCTURE TOP_LEVEL {name} DESCRIPTOR_SET _id_ BINDING _id_ or something like that

15. Not a huge fan of using NULL in the RUN command, what about using named params:

RUN {pipeline_name} RAYGEN {ray_gen_sbt_name} MISS {miss_sbt_name} HIT {hit_sbt_name} [CALL {call_sbt_name}]? _x_ _y_ _z_

16. After typing ACCELERATION_STRUCTURE a bunch, maybe ACCEL_STRUCTURE?

For the GEOMETRY TRIANGLES should we take something more complex then just vertex values? It seems like it actually takes a vertex and index buffer of a given stride, so, should we be able to attach a BUFFER?

For 13 above, after watching more of the rt video, I don't think it's necessarily the best way. Maybe something like the following would be better?

SHADER_BIND_GROUP gen generation_group
  RAYGEN my_gen_shader
END
SHADER_BIND_GROUP miss_group1
  MISS my_miss_shader
END
SHADER_BIND_GROUP miss_group2
  MISS another_miss_shader
END
SHADER_BIND_GROUP sphere_group
  ANY_HIT my_any_hit
  CLOSEST_HIT my_closest_hit
  INTERSECTION intersection2
END
SHADER_BIND_GROUP triangle_group
  CLOSEST_HIT closest_hit2
END
SHADER_BIND_TABLE bind_table
  generation_group
  sphere_group
  triangle_group
  miss_group1
  miss_group2
END

The bind group can contain either a MISS, a RAYGEN or (ANY_HIT?, CLOSEST_HIT, INTERSECTION?). I don't have the group_count as it maybe better to just be explicit here and duplicate the names into the bind_table?

1. OK: ACCELERATION_STRUCTURE BOTTOM_LEVEL and ACCELERATION_STRUCTURE TOP_LEVEL
2. I would use exact Vulkan spec name (i.e. bindING): SHADER_BINDING_TABLE
3. agree
4. "BLAS_INSTANCE USE. Could this just be BOTTOM_LEVEL {name}" This is heavily confusing. We include into Top Level Acceleration Structure not a Bottom Level Acceleration Structure itself, but a reference to it (that is like Vulkan specs says about it) but also names it instance. Ideal name would be BOTTOM_LEVEL_INSTANCE_REFERENCE, but I understand it is too long. May be BOTTOM_LEVEL_INSTANCE?
5. will remove VERTEX
6. will remove AABB
7. plural is reasonable.
8. I propose following:

GEOMETRY TRIANGLES
  x0 y0 z0 x1 y1 z1 x2 y2 z2
  xn yn zn ...
END
GEOMETRY AABBS
  x0a y0a z0a x0b y0b z0b  x1a y1a z1a x1b y1b z1b
  xna yna zna xnb ynb znb ...
END

9. Will force flags to be one-liner.
10. yes, 3*4 matrix. I will update doc
11. This ordering requirement was introduced due to vkGetRayTracingShaderGroupHandlesKHR. This function queries pipeline for a list of shader group and have two parameters: firstGroup and groupCount. We probably can remove group ordering and query shader groups one-by-one. Also no reason to specify shader type: it can be deduced from shader name.
12. "What does the group_count mean on SBT" This is a number of consecutive groups function vkGetRayTracingShaderGroupHandlesKHR will return. group_count is parameter groupCount to that function.
13. "SHADER_GROUP MISS miss1 my_miss_shader" What for MISS here? The type is easily deduced from shader name my_miss_shader "SHADER_BIND_GROUP MISS miss_group" Actually here we declare/define Shader Binding Table, thus I would stay with name SHADER_BINDING_TABLE. Assigning a type to shader binding table does not seem reasonable: actually SBT can contain various shader group handles. I intentionally limited shader mixing group types in amber (for now), due to start address cannot be any, it should be aligned according to VkPhysicalDeviceRayTracingPipelinePropertiesKHR::shaderGroupBaseAlignment, that depends on driver implementation. If we would like to put all groups into single SBT we need to make some padding, that is variable, that might overcomplicate support implementation in amber. "MULTIPLE 3" Should be COUNT 3, though we can skip this for now.
14. "I think for BIND we should make the type required." I doubt it. I agree we can go as "BIND ACCELERATION_STRUCTURE tlas1": no reason for TOP_LEVEL as BLAS is not allowed to be bound as descriptor set.
15. Good point
16. ACCELERATION_STRUCTURE is ok for me: I don't much like reducing names by cutting them due to cutting is ambigous.

For the GEOMETRY TRIANGLES should we take something more complex then just vertex values? It seems like it actually takes a vertex and index buffer of a given stride, so, should we be able to attach a BUFFER?

Yes, though this is complicated subject. You are right that this is just a buffer with vertices or indices and in Vulkan RT it is more advanced: several types are allowed for both vertices and indices. Vertices formats are implementation dependent. I would leave this for future. Indexing vertices does not seem natural for scripting languages, but giving names to vertices will complicate approach.

PIPELINE ...
  SHADER_GROUP generation_group
    my_gen_shader
  END
  SHADER_GROUP miss_group1
    my_miss_shader
  END
  SHADER_GROUP miss_group2
    another_miss_shader
  END
  SHADER_GROUP sphere_group
    my_any_hit
    my_closest_hit
    intersection2
  END
  SHADER_GROUP triangle_group
    closest_hit2
  END
  SHADER_BINDING_TABLE bind_table
    generation_group
    sphere_group
    triangle_group
    miss_group1
    miss_group2
  END
END

Syntax updated https://github.com/google/amber/commit/d4a407943094bff32cc498addbfae394bfbf5ad6

Also link to example CTS implementation added into head.

The changes to the doc look good to me. One note, you can't assume that you'll be able to determine the type of a thing from the name. So SHADER_GROUP MISS miss1 my_miss_shader could also be SHADER_GROUP MISS some_random_name and you won't know it's a miss shader without MISS in there.

the type of a thing from the name.

Yes I was incorrect, I meant that it is deduced from shader types included into group, not shader group name.

I merged fixes found by @nhaehnle.

I think we have enough for initial RT support, thus I created PR for this issue https://github.com/google/amber/pull/1035

Should we close this now and file followup issues for anything new to be added?

I think we can go with part 2 within this issue that is in https://github.com/google/amber/pull/1039 Edit: We have https://github.com/google/amber/issues/1038 for it, thus closing this one.