Finish splitting functionality of the RenderingDevice backends into RenderingDeviceDriver.#87340
Conversation
06b374c to
a11417c
Compare
|
I've been reviewing it today, the Vulkan & generic parts look alright (I did not review D3D12). As I already told Dario personally, the only thing that makes me nervous is this: They're both cloned versions and there's no guarantee(*) they're in sync. But on the other hand, if we'd unify them into just And what's missing would be to aggressively check: Nonetheless, as Dario explained to me and I've confirmed, (*) By "no guarantee" I mean if someone in the future touches this code and inadvertently introduces subtle bugs. Even if the current code is 100% bug free; checks need to be done to ensure it doesn't get broken tomorrow. |
|
Haven't had time to dive deep into this but making sure OpenXR still works I can confirm that:
|
aa07e0b to
dbab811
Compare
|
It was indeed crashing in Android due to an implementation error on my part. Now I've checked and it seems to be working. Weirdly enough, it also seems to fix a bug where using the controls in the editor would stretch the contents of the viewports to be twice its scale. |
I've been seeing this issue, but I see it in the OpenGL backend as well. So there might be something deeper going on in addition. |
0851988 to
6cc4366
Compare
There was a problem hiding this comment.
Tested locally on Linux + NVIDIA, it works as expected.
However, while this resolves #87532, every time I bake lightmaps, I now see this message in the Output panel:
API 1.3.260 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 4090
It's missing "Vulkan" before "API", and I think there should be a way to silence it as it may be excessive for certain local RenderingDevices.
Noted, I'll implement these suggestions. |
|
Fantastic work on this!! |
ac06d4c to
2857015
Compare
e971299 to
8824dc2
Compare
…RenderingDeviceDriver.
8824dc2 to
73eff10
Compare
|
Rebased to fix conflicts. |
|
Thanks! Amazing work 🎉 |
| texture_info.owner_info.states.subresource_states.push_back(D3D12_RESOURCE_STATE_PRESENT); | ||
| texture_info.states_ptr = &texture_info.owner_info.states; | ||
| texture_info.format = swap_chain->data_format; | ||
| texture_info.desc = CD3DX12_RESOURCE_DESC(render_target->GetDesc()); |
There was a problem hiding this comment.
I checked your other PR, how strange that the headers literally define it differently. Now it's a bit late but maybe we have to make a wrapper around that function that works for both compilers without putting the compiler paths inside the caller methods.
| frames[frame_idx].desc_heap_walkers.rtv.rewind(); | ||
| frames[frame_idx].desc_heaps_exhausted_reported = {}; | ||
| frames[frame_idx].null_rtv_handle = { 0 }; | ||
| frames[frame_idx].null_rtv_handle = {}; |
|
Seems like some change in this PR broke multi-threaded renderer (both compatibility and Vulkan) on macOS (see #90268 (comment)), but I'm not what's exactly wrong. |
|
RenderPass2KHR - Also same crash on MacOS without: _register_requested_device_extension(VK_KHR_CREATE_RENDERPASS_2_EXTENSION_NAME, false);detailscrash at:If I deactivate the extension ( Error RenderingDeviceDriverVulkan::_initialize_device_extensions() {
enabled_device_extension_names.clear();
_register_requested_device_extension(VK_KHR_SWAPCHAIN_EXTENSION_NAME, true);
_register_requested_device_extension(VK_KHR_MULTIVIEW_EXTENSION_NAME, false);
_register_requested_device_extension(VK_KHR_FRAGMENT_SHADING_RATE_EXTENSION_NAME, false);
//_register_requested_device_extension(VK_KHR_CREATE_RENDERPASS_2_EXTENSION_NAME, false);
_register_requested_device_extension(VK_KHR_SHADER_FLOAT16_INT8_EXTENSION_NAME, false);
|
This is a follow up PR to @RandomShaper's PR #83452 with the intention of cleaning up the loose ends that weren't put behind an abstraction yet. This also includes the changes identified by @darksylinc's PR #80566 as well as the new project settings that allow configuration of the swap chain image count and the buffered frame count of RenderingDevice.
The main reason behind pushing for this change is that the additional flexibility provided by the PR is essential to providing new asynchronous transfer queues, which will lead to massive improvements in loading times (not included in this PR). This will also enable to provide additional multi-queue functionality in the future for the Acyclic Command Graph that was recently merged. However, the current design makes it impossible to implement this without making awkward changes to all backends and maintaining all of the backends at once.
Another important reason for this change is that this PR will finally enable the possibility of the RenderingDevice's local device working independently of the renderer that was chosen, which means the GPU lightmapper can finally work in Compatibility mode! @clayjohn has proven this to be possible in a separate experiment (not part of this PR but based on top of it) where he added the required UV2 rendering to the compatibility renderer.
The overall net result means a lot of the logic is now shared across backends and the only thing the backend needs to worry about is exposing the following functionality:
The resulting code is also a lot easier to follow and it should be much simpler to fix some of the strange bugs that have been plaguing the backends for a while. However, I do not expect my first attempt at this to have been perfect, since swap chain logic can be particularly hard to handle correctly, so we'll need some extensive testing to confirm that this doesn't introduce any regressions.
NOTE: No performance improvements or regressions are expected from this PR, it is purely an organizational change and functionality wise it should be identical in the default case.
Confirm the changes work as intended in all the affected platforms.
TODO: