-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Vulkan raytracing plumbing #99119
base: master
Are you sure you want to change the base?
Vulkan raytracing plumbing #99119
Conversation
Commits needs to be squashed (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html). |
d5f4b02
to
4ff001d
Compare
I had a very brief look and it looks pretty good to me, although I'd hold off on making changes that add a new rendering method. Are you planning on sticking to only supporting the hit shader workflow or would you also like to add ray query support as well? If you'd like to test your rendering without adding a new rendering method, keep in mind you can also use GDScript to drive the renderer and it should be pretty sufficient to test it out. This is also a simple way you can provide us with a demo project to test it out. |
i see that you started on a renderer too on that repo, i think your renderer could be tested/based on the godot ideas https://gist.github.com/reduz/c5769d0e705d8ab7ac187d63be0099b5 |
3eea43f
to
2e1d952
Compare
2e1d952
to
61a9d1c
Compare
b456ef5
to
a784c7d
Compare
Just focusing on the hit shader for now.
There you go: https://github.com/Fahien/godot-raytracing-gdscript-demo |
de229f2
to
2474b4d
Compare
This is very cool to see! Last night I took a look at how hard it might be adding metal support and while it's definitely out of my wheelhouse here's a commit to at least get it building on macos + a new method to guard user code from invoking raytracing when it isn't supported (also includes some precommit changes) a-johnston@a220083 ed: I have some (probably very misguided and) incomplete changes on https://github.com/a-johnston/godot/tree/raytracing-metal but giving up where it is. Currently it blows up for |
f03f6a2
to
ef1d75e
Compare
2246469
to
502748c
Compare
Thanks to @Fahien for making the changes we agreed to. I won't have much time to review this until I get back in a few weeks but I'd like to know if you're willing to extend this PR to cover the other backends or if the intention is only for Vulkan to be merged first. If you wish to tackle the other APIs, I can leave you some relevant documentation on how to replicate the drivers functionality in the meantime. |
I can try, but I wonder if it would be better doing this in subsequent PRs. |
355db76
to
4e5315f
Compare
77d44b4
to
c1adab9
Compare
Done |
@Bonkahe, thank you for testing this, and do not worry for your experience level, we all started from zero and improved with time and dedication :) I suppose you expected to see the geometries in the scene, but the demo is currently not able to get them to build raytracing acceleration structures from GDscript. If you take a look at the P.S. I have been experimenting with a compositor effect in other branches, but there are still some issues to solve, but this is a story for another PR: |
df8f641
to
38b8932
Compare
- Vulkan implementations in `RenderingDeviceDriverVulkan` - Raytracing instruction list in `RenderingDeviceGraph` - Functions to create acceleration structures and raytracing pipelines in `RenderingDevice` - Raygen, Miss, and ClosestHit shader stages support - GDScript bindings - Update classes documentation - Include a-johnston RenderingDeviceDriverMetal changes - Unimplemented placeholders for Metal and D3D12. - Apply Mickeon docs suggestions - Build acceleration structure command - Expose a shader preprocessor define - Align build scratch address - Separate buffer memory barriers
38b8932
to
54b3c85
Compare
I'll see if I can get an in-depth review of this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a large batch of comments during the review.
One major thing I've avoided repeatedly mentioning throughout is there's two stages that are currently missing from hit groups: any hit and intersection shaders.
While intersection might not be used very often and I'd argue it can be ignored, any hit is absolutely essential to implementing RT renderers in a performant way, so we can't skip on them. Shadow rays gain massive performance benefits from relying on any hit and transparency sorting is possible and very performant when using it.
There's already a very good framework in place on this PR for adding the new shader stages, so I think Any Hit being added will result in a very natural extension.
Some extra reminders:
- Given the shader binary contents have changed, make sure to upgrade the version of the shader binary upon introducing any changes to invalidate user's shader caches completely.
- There's a misconception regarding transform buffer usage in BLAS and TLAS. Ideally, you want to embed the transform in the TLAS, as it gives you the highest flexibility of only rebuilding the TLAS with updated transforms when entities in the scene move. BLAS rebuilding is often avoided unless the mesh is skinned or has a dynamic vertex shader. While BLAS APIs allow you to pass through a transform, that is mostly just for pre-applying a transform to what is intended to be a static structure for the most part. If Godot implements a path traced renderer, it is very likely it'd only create and build a BLAS for a mesh during loading and never again unless the vertices themselves change.
<param index="1" name="uniform_set" type="RID" /> | ||
<param index="2" name="set_index" type="int" /> | ||
<description> | ||
Binds the [param uniform_set] to this [param raytracing_list]. Godot ensures that all textures in the uniform set have the correct Vulkan access masks. If Godot had to change access masks of textures, it will raise a Vulkan image memory barrier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this is probably just a comment that was lifted from other documentation, but we shouldn't be referring to Vulkan barriers in the scope of RenderingDevice, as it's agnostic to the underlying driver API being used.
<method name="raytracing_list_begin"> | ||
<return type="int" /> | ||
<description> | ||
Starts a list of raytracing drawing commands created with the [code]draw_*[/code] methods. The returned value should be passed to other [code]raytracing_list_*[/code] functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation refers to draw_
prefixed methods which are non-existent.
cmd_buf_info->cmd_list->SetGraphicsRoot32BitConstants(0, p_data.size(), p_data.ptr(), p_dst_first_index); | ||
} else { | ||
// TODO | ||
ERR_FAIL_MSG("Unimplemented!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On D3D12, you can share the code with the compute branch.
/********************/ | ||
/**** RAYTRACING ****/ | ||
/********************/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize most of this is unimplemented, so I'll just leave some future reference on what the relevant classes are for these operations.
// TODO | ||
ERR_FAIL_V_MSG(AccelerationStructureID(), "Unimplemented!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D3D12_RAYTRACING_GEOMETRY_DESC, D3D12_BUILD_RAYTRACING_ACCELERATION_STRUCTURE_INPUTS, GetRaytracingAccelerationStructurePrebuildInfo.
ERR_FAIL_COND_V(!vertex_formats.has(vertex_array->description), RID()); | ||
vertex_format = vertex_formats[vertex_array->description].driver_id; | ||
} | ||
_check_transfer_worker_vertex_array(vertex_array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are currently misplaced, as the check must be performed during the actual BLAS/TLAS building, not during the creation of the BLAS/TLAS driver resource. That way the check is deferred as much as possible until the acceleration structure is actually built.
// VK_PIPELINE_STAGE_*_SHADER_BIT stages except VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR | ||
|
||
uint32_t acceleration_structure_barrier_count = 0; | ||
LocalVector<uint32_t> acceleration_structure_barrier_indices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reasoning as everywhere else, thread_local
or ALLOCA must be used on hot paths such as these (preferrably TLS as it won't blow up if it runs out of stack memory).
0, nullptr, | ||
acceleration_structure_barrier_count, as_barriers, | ||
0, nullptr); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the awkwardness of this implementation, I think it'd be warranted that we make up a fake new type of barrier specifically for acceleration structures.
From the driver's abstraction side, we identify buffers, textures and AS as separate entities. Therefore it'd make perfect sense to add a new type of barrier for AS specifically and add support to the graph to track these separately. That'd allow each back-end to have an easier time implementing the synchronization primitives required for it.
memcpy(sbt_data, handles_ptr + handle_index * handle_size, handle_size); | ||
++handle_index; | ||
|
||
sbt_data = sbt_ptr + shader_info->regions.raygen.size; | ||
for (uint32_t i = 0; i < shader_info->regions.miss_count; ++i) { | ||
memcpy(sbt_data, handles_ptr + handle_index * handle_size, handle_size); | ||
sbt_data += shader_info->regions.miss.stride; | ||
++handle_index; | ||
} | ||
|
||
sbt_data = sbt_ptr + shader_info->regions.raygen.size + shader_info->regions.miss.size; | ||
for (uint32_t i = 0; i < shader_info->regions.closest_hit_count; ++i) { | ||
memcpy(sbt_data, handles_ptr + handle_index * handle_size, handle_size); | ||
sbt_data += shader_info->regions.closest_hit.stride; | ||
++handle_index; | ||
} | ||
|
||
buffer_unmap(shader_info->sbt_buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this data was filled out and remains static during the lifetime of the shader, then it must be mapped and fill during creation and only then. If this varies per dispatch call, then it'll completely overwrite the data used by the previous call. This seems to indicate to me this is in the wrong spot.
As far as I can see, it currently seems to indicate that this depends on the pipeline itself, which means the buffer might belong there and not in the ShaderInfo and can probably be created and filled right after the raytracing pipeline is created.
vkCmdBindDescriptorSets((VkCommandBuffer)p_cmd_buffer.id, VK_PIPELINE_BIND_POINT_RAY_TRACING_KHR, shader_info->vk_pipeline_layout, p_set_index, 1, &usi->vk_descriptor_set, 0, nullptr); | ||
} | ||
|
||
void RenderingDeviceDriverVulkan::command_raytracing_trace_rays(CommandBufferID p_cmd_buffer, RaytracingPipelineID p_pipeline, ShaderID p_shader, uint32_t p_width, uint32_t p_height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The need for the pipeline as an argument can be skipped by just storing the last bound pipeline passed through command_bind_raytracing_pipeline
and asserting for its existence.
I've also left comments for a potential D3D12 implementation. If you feel that is out of the scope of the PR, feel free to ignore them and I can potentially take over that task when the time arrives. |
Here's a bunch of code adding some Vulkan raytracing stuff to the rendering device:
RenderingDeviceDriverVulkan
RenderingDeviceGraph
RenderingDevice
There's more in the fahien/raytracing-test branch, with code handling raygen, miss, and closest-hit shaders, but it's a hack on top of the forward clustered renderer, and it needs some "guided" refactoring.
Here's a sample which uses GDScript to drive the renderer: raytracing-gdscript-demo
I hope this changes would look useful to jump-start raytracing support.
Relevant for godotengine/godot-proposals#5162