Skip to content
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

Finish splitting functionality of the RenderingDevice backends into RenderingDeviceDriver. #87340

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

DarioSamo
Copy link
Contributor

@DarioSamo DarioSamo commented Jan 18, 2024

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:

  • Command Queues
  • CPU/GPU Fences
  • GPU/GPU Semaphores
  • Swap Chains

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.

  • Windows
  • Linux (X11 Systems)
  • Linux (Wayland)
  • Android
  • macOS
  • iOS
  • OpenXR (Desktop)
  • OpenXR (Quest)

TODO:

  • CmdBeginDebugUtilsLabelEXT and related functions should be fetched like the older versions.
  • Rendering Device should print the API name instead of "API".
  • There should be no need to print the information about the picked device for RDs that aren't the Singleton.
  • Fix all CI issues so it builds successfully in all platforms.
  • Provide compatibility methods if necessary for RenderingDevice on the functions that have been changed.
  • Finish documentation changes.

@AThousandShips AThousandShips added this to the 4.3 milestone Jan 18, 2024
@DarioSamo DarioSamo force-pushed the rd_common_context branch 12 times, most recently from 06b374c to a11417c Compare January 19, 2024 17:29
doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
drivers/d3d12/rendering_context_driver_d3d12.cpp Outdated Show resolved Hide resolved
drivers/d3d12/rendering_device_driver_d3d12.cpp Outdated Show resolved Hide resolved
drivers/d3d12/rendering_device_driver_d3d12.cpp Outdated Show resolved Hide resolved
drivers/vulkan/rendering_context_driver_vulkan.cpp Outdated Show resolved Hide resolved
modules/lightmapper_rd/lightmapper_rd.cpp Outdated Show resolved Hide resolved
@darksylinc
Copy link
Contributor

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:

RenderingDeviceGraph::frame
RenderingDevice::frame

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 RenderingDevice, we still have no guarantee RenderingDeviceGraph will notice when RenderingDevice::frame advances.

And what's missing would be to aggressively check:

DEV_ASSERT( RenderingDevice::frame == RenderingDeviceGraph::frame );

Nonetheless, as Dario explained to me and I've confirmed, RenderingDeviceGraph::frame isn't currently in use because that's only used for secondary command buffers, which had to be disabled because they were broken on NVIDIA (either because of NV errors, or because of Godot's faults).

(*) 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.

@BastiaanOlij
Copy link
Contributor

Haven't had time to dive deep into this but making sure OpenXR still works I can confirm that:

  • PCVR works which suggests our OpenXR overrules are still working
  • Running on Quest this currently hard crashes but I haven't been able to capture good crash info. Needs to be tested whether this is an Android issue or specific to Android XR devices.

@DarioSamo
Copy link
Contributor Author

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.

@clayjohn
Copy link
Member

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.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DarioSamo
Copy link
Contributor Author

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.

@reduz
Copy link
Member

reduz commented Jan 25, 2024

Fantastic work on this!!

@DarioSamo DarioSamo force-pushed the rd_common_context branch 2 times, most recently from ac06d4c to 2857015 Compare January 25, 2024 16:31
@DarioSamo DarioSamo marked this pull request as ready for review January 25, 2024 16:38
@DarioSamo
Copy link
Contributor Author

DarioSamo commented Feb 12, 2024

Rebased to fix conflicts.

@akien-mga akien-mga merged commit 3be3d50 into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks MinGW build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -5224,17 +5652,9 @@ void RenderingDeviceDriverD3D12::begin_segment(CommandBufferID p_cmd_buffer, uin
frames[frame_idx].desc_heap_walkers.aux.rewind();
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 = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, breaks MinGW build.

@bruvzg
Copy link
Member

bruvzg commented Apr 12, 2024

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.

@Alex2782
Copy link
Contributor

Alex2782 commented Apr 25, 2024

RenderPass2KHR - Compatibility fallback does not work properly on Android LG Nexus 5X, Adreno 418

Also same crash on MacOS without:

_register_requested_device_extension(VK_KHR_CREATE_RENDERPASS_2_EXTENSION_NAME, false);
details

// Compatibility fallback with regular create render pass but by converting the inputs from the newer version to the older one.

crash at:

_convert_subpass_attachments(p_create_info->pSubpasses[i].pResolveAttachments, p_create_info->pSubpasses[i].colorAttachmentCount, subpasses_attachments[resolve_attachments_index]);

r_attachment_references[i].attachment = p_attachment_references_2[i].attachment;


If I deactivate the extension (VK_KHR_CREATE_RENDERPASS_2_EXTENSION_NAME), I can also reproduce the crash on MacMini M1.

_register_requested_device_extension(VK_KHR_CREATE_RENDERPASS_2_EXTENSION_NAME, false);

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);

Bildschirmfoto 2024-04-25 um 17 53 05

@DarioSamo
Copy link
Contributor Author

@Alex2782 Can you confirm if #91169 fixes it? Thank you very much for the report by the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Baking lightmaps always prints a warning message about PSO caching