Repository navigation
Implement Buffer Device Address for Rendering Device Vulkan and DirectX12 - #100062
Conversation
d527b1b to
649f172
Compare
|
From what I have seen the implementations are fairly similar, albeit with some small differences, mine loads the extension either from core 1.2 or the KHR version which has a wider support compared to the EXT used in the other branch (sourced from the gpuinfo database), and also flags index and vertex buffers by default, which likely won't play nicely on devices where its not supported. I think the buffer device address feature its worthwhile on its own and should be merged beforehand, then the other ray tracing branch could be rebased from it. Went ahead an added it as optional functionality to index, vertex and uniform buffers while preserving compatibility, and updated the documentation. |
I agree. |
I believe so as well, it's easier to integrate it piece-meal like this. I just wanted to make sure you were both in agreement as it'll break the RT branch as soon as it's merged. I have no qualms with integrating the feature as I know how necessary it is for advanced rendering. |
e8b36c3 to
0326684
Compare
|
Looks good, but there are compatibility breakages. |
0326684 to
4226d3f
Compare
4226d3f to
f91ffc7
Compare
|
Added the GDExtension compatiblity methods and rebased with the current master again to fix merge conflicts. |
|
Do we need something like this in VkPhysicalDeviceBufferDeviceAddressFeaturesKHR device_address_features = {};
if (buffer_device_address_support) {
device_address_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_BUFFER_DEVICE_ADDRESS_FEATURES_KHR;
device_address_features.pNext = create_info_next;
device_address_features.bufferDeviceAddress = buffer_device_address_support;
create_info_next = &device_address_features;
} |
|
I think it might be needed for the devices that aren't initializing core Vulkan 1.2 but a prior version instead. |
f91ffc7 to
93fdcc3
Compare
stuartcarnie
left a comment
stuartcarnie
left a comment
There was a problem hiding this comment.
I've left a couple of notes for Metal. Your implementation is correct, so I suggest you add a property to the MetalFeatures structure, that is set on initialisation, so you can return the correct value for the SUPPORTS_BUFFER_ADDRESS feature.
There was a problem hiding this comment.
This can be true, but I would like to implement it by adding a new boolean property on the MetalFeatures struct:
There was a problem hiding this comment.
Could this be addressed in this PR?
|
I'd like to push for this to be merged, but it seems there's some compatibility breakage that must be addressed. I'll give it a thorough review soon. |
There was a problem hiding this comment.
So far this looks excellent to me, but I've noticed there's some inconsistency when it comes to choosing these terms:
- Device Address (
buffer_get_device_address) - Shader Address (
enable_shader_address) - Shader Device Address (
STORAGE_BUFFER_USAGE_SHADER_DEVICE_ADDRESS) - Buffer Address (
SUPPORTS_BUFFER_ADDRESS)
If we can settle on one consistent terminlogy (device address sounds like the best one to me), and fix the conflicts on the API change, I think this is good to go.
buffer_get_device_address()
enable_device_address
STORAGE_BUFFER_USAGE_DEVICE_ADDRESS
SUPPORTS_BUFFER_DEVICE_ADDRESS
b4c8a18 to
b5f7359
Compare
|
Approved, thank you very much for your work! |
b5f7359 to
3d92f40
Compare
|
Updated with the changes requested by ATS. Should be ready to merge now |
|
Thanks! |
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
Implement Buffer Device Address for Rendering Device Vulkan and DirectX12
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
Implement Buffer Device Address for Rendering Device Vulkan and DirectX12
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
Implement Buffer Device Address for Rendering Device Vulkan and DirectX12
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
Implement Buffer Device Address for Rendering Device Vulkan and DirectX12
PR godotengine#100062 introduced BUFFER_USAGE_DEVICE_ADDRESS_BIT. However it did so by adding a boolean to uniform_buffer_create(), called "bool p_enable_device_address". This makes maintaining backwards compatibility harder because I am working on another feature that would require introducing yet another bit flag. This would save us the need to add fallback routines when the feature I am working on makes it to Godot 4.5. Even if my feature doesn't make it to 4.5 either, this PR makes the routine more future-proof. This PR also moves STORAGE_BUFFER_USAGE_DEVICE_ADDRESS into BUFFER_CREATION_DEVICE_ADDRESS_BIT, since it's an option available to both storage and uniforms. This PR also moves the boolean use_as_storage into BUFFER_CREATION_AS_STORAGE.
10 participants
This is an extremely useful feature that is supported on many GPUs which may be worthwhile to have it since the changes are minimally invasive, and allows for a lot of flexibility, specially for compute shaders and a requirement for ray tracing.
Tested to work on Windows 10 with Vulkan driver.
On DirectX12 the SPIR-V to NIR shader compiler doesn't have implemented this extension (yet?), so remains untested.
Test project used available here
Do note that it only has been implemented for storage buffers, to not change function signatures, but it could be implemented for the other buffer types, either by adding another default parameter, or breaking compatibility and using bitwise flags.