Add draw indirect to Rendering Device

[thimenesup]

Self explanatory.

[tetrapod00]

I couldn't find an actual proposal for indirect rendering that this would close, but there is this discussion godotengine/godot-proposals#8647. If this works as advertised, it should satisfy multiple existing proposals that are asking for similar features but not specifically asking for "draw indirect".

[DarioSamo]

I haven't reviewed in detail, but make sure to add the draw list usage for the indirect buffer just like how dispatch indirect does, otherwise the graph won't be able to accurately detect the dependencies. You can find the following example in the code for dispatch indirect.

if (buffer->draw_tracker != nullptr) {
    draw_graph.add_compute_list_usage(buffer->draw_tracker, RDG::RESOURCE_USAGE_INDIRECT_BUFFER_READ);
}

[thimenesup]

Fixed that, and added docs too.

[Lateasusual]

This would be very nice to have, and there's a lot of future improvements that could be made that would depend on this.

There's a lot of duplicated code from draw_list_draw() that probably should be shared between the two IMO.

[thimenesup]

Its been over a month, any update on the reviewing status? Or if anything else needs to be done before?

[DarioSamo]

Its been over a month, any update on the reviewing status? Or if anything else needs to be done before?

I did not see anything particularly wrong with the implementation, but new feature proposals are not usually merged in unless there's demand for it.

That said, your PR was caught in the middle of GodotCon which means a lot of maintainers were away, and this feature might prove to be hard to gather some support since most users are probably not aware of how it works. On a technical basis, it sounds completely fine to me to have it so I'll gladly support its inclusion.

It'd be nice to have some code we could use to verify if it works, because as the PR stands, we have to go out of our way to write an example ourselves to validate if it works.

[thimenesup]

I don't understand why so much hesitation about adding what effectively is the equivalent of compute dispatch indirect, and a core functionality of Vulkan and DirectX, frankly I was suprised it wasn't already implemented and therefore used by Godot renderer, since it allows offloading a lot of tasks to the GPU which provides optimization opportunities.

Anyways here is a simple example project.

[DarioSamo]

I don't understand why so much hesitation about adding what effectively is the equivalent of compute dispatch indirect, and a core functionality of Vulkan and DirectX, frankly I was suprised it wasn't already implemented and therefore used by Godot renderer, since it allows offloading a lot of tasks to the GPU which provides optimization opportunities.

The volume of PRs is really, really big and stuff can fall on the wayside if there's little demand for it. But like I said, a lot of maintainers were busy with GodotCon and the Sprint. If it happens, it's completely fine to bump it again for discussion or even attend the rendering meetings to give a reminder.

The reason I asked for a project is so maintainers don't have to hesitate on testing it out because of the need to develop an example case. The PR needs to be verified that it works after all for it to be approved.

I'll see if I can test it out soon now that there's a project.

[DarioSamo]

I've tested the project and it seems to work, but as it is I don't actually see the visible output for some reason. Checking on RenderDoc, I was able to see the triangles just fine, but it seems like the GDScript render pass gets overwritten by the swap chain clear that Godot does. Is this intentional or just a bug from testing on master?

Either way, I don't think that is a blocker for the PR in particular. There's only some extra additions that need to be made due to some recent changes, but other than that it looks good to me.

I've also verified that other drivers currently implement the required functionality for indirect drawing, although precaution must be taken with Metal to not use the 'count' variant, as it's currently unimplemented. However, this PR does not expose using the count variant as far as I could tell.

[DarioSamo]

Thread guard and check for transfer worker must be added.

[thimenesup]

Updated to work for master branch, also I can confirm that for 4.3 the output is visible as expected.

[Saul2022]

It would be nice too if there was some kind of video that showcases this in use, say as performance optimizitation with for example multi meshes or particle shader as i ‘v seen people from unity talking about this approach for more performance.

[thimenesup]

Updated with suggested formatting changes.

[thimenesup]

@Saul2022

It would be nice too if there was some kind of video that showcases this in use, say as performance optimizitation with for example multi meshes or particle shader as i ‘v seen people from unity talking about this approach for more performance.

Does this video showcase it enough?

[Calinou]

Code looks good to me.

[clayjohn]

Approving. This is something we will eventually need exposed when we do more GPU-driven rendering. The API is already exposed through the RenderingDeviceDriver, this just further exposes it through the RenderingDevice. We have already exposed the equivalent compute_list_dispatch_indirect() because it is used by VoxelGI.

The only real risk factor here is that we are exposing an API that isn't used internally, so there is a higher than normal chance of it breaking (due to lack of testing from ordinary contributors). At the same time, the code is not complex and will not likely change anytime soon, so I think the risk is minimal.

[Repiteo]

Thanks!

[jasonwinterpixel]

Found this from the changelogs. This seems like a great addition to the engine and I like the direction of adding more low level abilities. I think this should have been received more documentation, and writing 'self explanatory' on the PR description is, well, shocking to me. I'm an advanced developer, capable of using a system like this to achieve things. I've used Unity's Graphics.DrawMeshInstancedIndirect and other similar APIs to render advancd things, but the documentation on this API is insufficient for someone like me to use this API without basically reading the code. I imagine the # of people worldwide that know how to use this API as it is right now is very small. More sufficient documentation could drastically improve the adoption of this API.

[Mainman002]

Is there any reason of not using:
draw_list_indirect
Instead of:
draw_list_draw_indirect

Also noticed towards the bottom of the script a few more functions with:
draw_list_draw_

Just kinda feels unneeded to me as "draw" is already the main function. Regardless it does the thing, just curious about the function name repetition

[clayjohn]

Is there any reason of not using: draw_list_indirect Instead of: draw_list_draw_indirect

Also noticed towards the bottom of the script a few more functions with: draw_list_draw_

Just kinda feels unneeded to me as "draw" is already the main function. Regardless it does the thing, just curious about the function name repetition

Godot has a concept of a "draw_list". All operations on a draw list are prefixed with "draw_list". In this case the operation is "draw indirect" so the function name is "draw_list_draw_indirect" This matches what we do with compute lists (e.g. compute_list_dispatch_indirect)