-
-
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
Split RenderingDevice
into API-agnostic and RenderingDeviceDriver
parts
#83452
Conversation
Looks fantastic! |
I'll be leaving feedback in general here and on a per-line basis if necessary. Some of my comments are just criticizing a few design carry-overs from the current design and aren't necessarily related to this PR being approved or not, it just feels like the right time to bring these points up. Type SafetyI already brought this up over DMs, but there's currently no type safety system for the IDs returned by RDD. Using the uint64_t's directly means we can mix and match arguments around and the compile won't indicate the error. We can solve this in a few ways. A) Every type returned gets a struct that wraps around the uint64_t. A very minor change that guarantees the types can't be mixed together.
B) Every type returned is a pointer to an interface that the backend has to inherit. This requires more work but since these handles are already pointers in disguise (they're not references you can indefinitely hold), there's not much of a downside to it. The major advantage is that debuggers can access these types natively and display their contents.
Another benefit to consider is const-correctness can be implemented into the pointer types and will therefore be directly indicated by the API whether the object's internals should be modifiable or not. Vertex FormatsThere seems to be currently a forced correlation between bindings and attributes, meaning that attributes that share the same buffer can't share the same binding (e.g. if 10 elements are bound to the same buffer, they each get their own binding instead of using the same one). I'm not familiar if this has potential performance implications, but it's worth investigating if this is correct. This is not new to this PR, as it was pretty much directly copied as is from the current Vulkan RD. Command QueuesCommand queues are not directly exposed but managed by the backend, meaning RenderingDevice has no control whatsoever on where commands are executed, on what type of queue and when exactly. This could prove to be very useful in the feature as a lot of the logic around managing command queues should be common to most backends and it takes off some responsibility for the backend maintainer. I think we could address this in this PR and move most of the logic that Vulkan Context currently handles to RD instead. I'll do a separate comment for concerns about D3D12 compatibility in the future. |
These are not something that necessarily need to be solved by this PR but are probably worth considering before doing the D3D12 version of RDD. Some are really just points to consider when doing the D3D12 refactor. Combined Image SamplersAs far as I know these are not supported by D3D12 and the backend would need to work around it in some ugly ways to support them. Are we relying on this feature a lot? What approach did the D3D12 PR take to solving this? Compute dispatch baseI'm not aware D3D12 has anything about this. This is a new addition to the API by this PR. We might want to consider it more carefully. It's also a Vulkan 1.1 feature at minimum, so we need to query device support for it. I think we should just remove it and let the user handle it by adding the offset in a UBO/Push constant. BarriersD3D12 requires resource barriers if not using enhanced barriers. I think we should take the chance to make our barrier-API only take resources instead. I don't think we ever rely on memory barriers. As far as I'm aware the current D3D12 PR needs to work around this and I think it's hard to find use cases for full memory barriers with the upcoming acyclic render graph/barrier solver design. Semantics vs LocationsWhat approach does the current D3D12 PR take for matching vertex location inputs to the semantics? Is that something that we'd need to address on this API or is it solved through some other means as part of the backend responsibility? If there's a possibility this API will take shaders in its native format, it sounds like we might need to allow for a type of configuration that allows input semantics. Unordered Access StateWhat is the suggested solution for mapping this API to the unordered access state in D3D12? Using the image layout for general and the bit for shader writes? Texel BuffersThese don't translate to a flag during creation in D3D12, so we'll just need to track that bit to enable using the format on the SRV/UAV when it's assigned. |
You may want to check the D3D12 PR (#70315) to see how I mapped the Regarding barriers, the D3D12 driver using resource-level barriers would just ignore the detailed ones (that's exactly what the PR above is doing). If enhanced barriers are eventually implemented, well, it just have to obey them. I'm addressing the type safey of ids in the next push. Regarding other things, this is just the first iteration and will for sure need changes to fit other APIs or future |
Regarding type safety, I'm taking the first approach you discussed because most |
3fd481b
to
93c191e
Compare
For the TODO: Lightmapper is broken and doesn't start, which probably means we need to review local_device implementation. Validation layer seems to indicate it's related to the Vulkan instance.
|
93c191e
to
7b80756
Compare
ea3ed6a
to
263cd28
Compare
Re-pushed. Lightmapper is fixed (or so I think). |
c3698a5
to
969ab22
Compare
8cca345
to
307b238
Compare
It seems to be a GPU driver or OS version specific issue. Works as expected on some setups, fails on another. Maybe a driver bug. We are fine with merging as it is now. The code seems to be OK. |
Credit and thanks to @bruzvg for multiple build fixes, update of 3rd-party items and MinGW support. Co-authored-by: bruvzg <[email protected]>
307b238
to
12a519b
Compare
This is never ending, is it? 😅 Pushed with a fix for the memory leaks. |
I think I'm going to force-push again... Just kidding. 😈 |
Thanks! As often is the case with such achievements, this monumental effort is not the end, but a beginning for even more exciting new things to be added into the engine in near and far future. But tonight you can rest with a proud smile. Cheers to everyone who contributed and tested this PR! |
Really happy to see this, just wanted to say thanks for the effort and can't wait for future improvements this will enable! |
This does not compile on macOS, it fails on the linking stage (I tested both arm64 and x86_64):
|
It compiles in the CI. Are you using any different parameters that would have an appreciable difference? Molten VK appears to have support for both those functions, so it is odd that it's not linking |
@clayjohn Fixed, the problem was that I was using an old version of MoltenVK. If anyone else is experiencing this issue, try updating the Vulkan SDK. |
A huge chunk of
RenderingDeviceVulkan
is API-agnostic bookkeeping, validation, etc. Such code is also shared with others, likeRenderingDeviceD3D12
. This PR keeps a single, non-abstractRenderingDevice
with all those API-agnostic elements and moves all the API-specific parts to a thin wrapper around the driver, which isRenderingDeviceDriver
for Vulkan. This makes much easier to write additional rendering drivers, by allowing one to focus on the API-specific stuff. In some cases that will require modifying the interface betweenRD
andRDD
, which is not exposed to the user.Other relevant points:
VulkanContext
(APIContextRD
) with the bare minimum set of servicesRenderingDevice
and direct usage needs. (The specificRenderingDeviceDriver
s have access to the specific context implementation, which open doors for them to a much richer, but non-standard, API.) UPDATE: Now Direct3D 12 has been merged, it gets similar love in this PR.RenderingDevice
andRenderingDeviceDriver
.RD
has been enhanced so now you can instantiate aRenderingDevice
even when Godot is running in compatibility mode (this will allow to have aRD
-based lightmapper available in a GL compatibility project). This piece of code shows how to ensure you have a usable localRenderingDevice
in any circumstances:RD::texture_get_native_handle()
(superfluous) andDRIVER_RESOURCE_VULKAN_*
(replaced by API-agnostic).This includes a couple of merged (commits will go away once rebased) and unmerged (#82797) PRs.TODO:
master
.Production edit: closes godotengine/godot-roadmap#31