Tests cleanup: Remove old AccelerationStructureKHR in favor of RT helper classes defined in rt::as #5456
Conversation
|
CI Vulkan-ValidationLayers build queued with queue ID 39663. |
|
CI Vulkan-ValidationLayers build # 11203 running. |
|
CI Vulkan-ValidationLayers build # 11203 failed. |
arno-lunarg
force-pushed
the
clean-rt-tests
branch
from
e67988e
to
8927ab2
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 39686. |
|
CI Vulkan-ValidationLayers build # 11204 running. |
|
CI Vulkan-ValidationLayers build # 11204 failed. |
arno-lunarg
force-pushed
the
clean-rt-tests
branch
from
8927ab2
to
8ccaea9
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 39716. |
|
CI Vulkan-ValidationLayers build # 11205 running. |
| @@ -60098,7 +60098,8 @@ safe_VkAccelerationStructureGeometryKHR::safe_VkAccelerationStructureGeometryKHR | |||
| } else { | |||
| size_t array_size = build_range_info->primitiveOffset + build_range_info->primitiveCount * sizeof(VkAccelerationStructureInstanceKHR); | |||
There was a problem hiding this comment.
Would it be enough to allocate memory just for build_range_info->primitiveCount structures? Do we need part of allocation of the size build_range_info->primitiveOffset bytes?
There was a problem hiding this comment.
I think so too, but I need to explore how safe_VkAccelerationStructureGeometryKHR are used to make sure it will not break anything. For this PR, I am just going to fix the memcpy that is just wrong. I plan to have a deeper look at safe_VkAccelerationStructureGeometryKHR next
There was a problem hiding this comment.
You make me realise that to keep the current weird logic, I also need to offset the destination address in the memcpy
There was a problem hiding this comment.
I created an issue for the dubious array_size computation: https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5458
There was a problem hiding this comment.
thanks! yes, it would be nice to figure out if some code relies on that, especially if other computations need to be adjusted to match current logic.
|
CI Vulkan-ValidationLayers build # 11205 passed. |
arno-lunarg
force-pushed
the
clean-rt-tests
branch
from
8ccaea9
to
700e0ee
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 39749. |
|
CI Vulkan-ValidationLayers build # 11207 running. |
Fix source address and copy size in the memcpy needed to initialize a safe_VkAccelerationStructureGeometryKHR when the copied acceleration structure is on the host Will also need to have a look at array_size
arno-lunarg
force-pushed
the
clean-rt-tests
branch
from
700e0ee
to
7baee85
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 39766. |
|
CI Vulkan-ValidationLayers build # 11208 running. |
|
CI Vulkan-ValidationLayers build # 11208 failed. |
arno-lunarg
force-pushed
the
clean-rt-tests
branch
from
7baee85
to
59618b7
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 39781. |
|
CI Vulkan-ValidationLayers build # 11209 running. |
|
CI Vulkan-ValidationLayers build # 11209 passed. |
| AccelerationStructureKHR& SetBufferMemoryPropertyFlags(VkMemoryAllocateFlags memory_property_flags_); | ||
| AccelerationStructureKHR& SetDeviceBuffer(vk_testing::Buffer&& buffer); | ||
| AccelerationStructureKHR& SetDeviceBufferMemoryAllocateFlags(VkMemoryAllocateFlags memory_allocate_flags); | ||
| AccelerationStructureKHR& SetDeviceBufferMemoryPropertyFlags(VkMemoryAllocateFlags memory_property_flags_); |
| vk_obj_.geometry.aabbs.stride = stride; | ||
| return *this; | ||
| } | ||
|
|
||
| GeometryKHR &GeometryKHR::SetAABBsHostBuffer(std::unique_ptr<VkAabbPositionsKHR[]> &&buffer, |
There was a problem hiding this comment.
I have impression that it's more common to pass std::unique_ptr by value. That can also be true for other move-only types where we need to call std::move anyway, but at least for std::unique_ptr that's what I saw more often. There is also this remark in c++ guidelines:
Exception. Unique owner types that are move-only and cheap-to-move, such as unique_ptr, can also be passed by value which is simpler to write and achieves the same effect. Passing by value does generate one extra (cheap) move operation, but prefer simplicity and clarity first.
p.s. that's not request for a change since it's more about style and it's up to the author if he likes this or not.
There was a problem hiding this comment.
I was thinking that only an rvalue reference was possible here, thank you for showing me this. Clarity is the priority in those classes, so I am going to make this change!
Simplify tests by using objects from the rt::as namespace, replacing usage of AccelerationStructureKHR Rename RayTracingAccelerationStructureReference to RayTracingHostAccelerationStructureReference, because it tests host acceleration structure creation Add RayTracingHostAccelerationStructureReference to test on device acceleration structure creation
Remove vk_testing::AccelerationStructureKHR in favor of new helper classes from the rt::as namespace
arno-lunarg
force-pushed
the
clean-rt-tests
branch
from
59618b7
to
d91e487
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 40768. |
|
CI Vulkan-ValidationLayers build # 11227 running. |
|
CI Vulkan-ValidationLayers build # 11227 passed. |
Bulk of the additions (~500 for a total of ~730) are in
tests\framework\ray_tracing_objects.h/.cpp. They mostly consist in new methods to handle top level acceleration structures, and host acceleration structure operations.The 1st commit fixes for
safe_VkAccelerationStructureGeometryKHR, a memcpy was incorrect. The wayarray_sizeis computed is dubious, I created an issue to investigate that: KhronosGroup/Vulkan-Utility-Libraries#205The 2nd removes
AccelerationStructureKHRdefined intests\framework\binding.hin favor of classes from thert::asnamespace. It also adds the positive testRayTracingAccelerationStructureReference: it creates a bottom level acceleration structure on the device, then a top level acceleration structure that references the bottom level one.The 3rd one removes old
AccelerationStructureKHR