Skip swapchain logic if there is nothing to present (Android OpenXR) #84244

BastiaanOlij · 2023-10-31T11:05:46Z

When we are running XR in standalone mode (e.g. OpenXR on Android such as Quest, Lynx, ML2, etc) output to screen is handled by OpenXR however Godot still creates and attempts to display it's build in swapchain for normal output. This is causing a bundle of issues from warnings being spammed (suboptimal swapchain), outright failures (device lost), to simply an empty swapchain overwriting whatever was rendered to screen by OpenXR.

This PR solves that issue by having Godot skip it's normal output if nothing is being blit to screen (e.g. there is no output because the output has been redirected).

Testing so far has removed the issues I was having on Quest Pro, awaiting feedback to see if this solves more serious issues on Quest 3 and ML2. It also shaved 1ms of overhead per frame.

For GLES3 I had to introduce a proper end_viewport to get the logic to work correctly.

Test project:
https://www.dropbox.com/scl/fi/htl04po1ylfex3k64e1c9/TestXRv2.zip?rlkey=l7t9nmpwtlt44o5ogbib3xr8w&dl=0
(requires android build templates to be compiled and installed)

Fixes #74963

BastiaanOlij · 2023-10-31T11:18:42Z

This may make #74754 unnecessary

drivers/vulkan/vulkan_context.cpp

trent-waddington · 2023-11-01T02:50:51Z

Tested on Quest 3. Both debug and release templates are working with your test case.

BastiaanOlij · 2023-11-20T22:39:47Z

Just some extra info here for reviewers and the production team.

This was always an annoyance on Quest 2 and Pro, but on Quest 3 without this fix you can't run a Vulkan game. So far the feedback I've had is that compatibility works fine. I should be able to test this myself soon and add some extra feedback.

clayjohn

Looks good to me. I tested locally on a non-XR project and can't see any regressions.

Would be good to hear from someone with a Quest 3 about whether this fixes the issue we are having there.

BastiaanOlij · 2023-12-05T01:49:11Z

Would be good to hear from someone with a Quest 3 about whether this fixes the issue we are having there.

I've had feedback from several users on the XR discord that it does, but they don't tent to come here on github.

clayjohn · 2023-12-07T03:13:48Z

Note for merging: this should be merged after #83452. Bastiaan has some small fixups related to this PR to make it work well with the RDD changes. It makes sense to do those fixups directly on this PR after merging #83452

akien-mga · 2023-12-07T09:04:29Z

Note for merging: this should be merged after #83452. Bastiaan has some small fixups related to this PR to make it work well with the RDD changes. It makes sense to do those fixups directly on this PR after merging #83452

If this is wanted for 4.2 and 4.1 cherrypicks, a dedicated PR with the current version would likely need to be made against the 4.2 branch then, as we won't be able to cherry-pick the change after it has been rebased on top of RDD.

dsnopek · 2023-12-07T15:11:41Z

If this issue really is preventing Quest 3 from using Vulkan (I have a Quest 3 - I'll try to test this when I have a chance :-)), then I think we really should try to get a version of this PR cherry-picked into 4.2. Most folks deploying to Quest are using the compatibility renderer (myself included), but having Vulkan supported on Quest 1, Quest 2, Quest Pro, but not Quest 3, would be unfortunate.

dsnopek · 2023-12-07T16:29:48Z

Hm, I'm not entirely sure how to trigger the Quest 3 issue. Using Godot 4.2-stable (so without this PR), I tried switching one of my projects from gl_compatibility to mobile, and I ran it 3 times on the Quest 3 (only playing for about 20-30 seconds each time) and everything seemed to work fine. And there were no messages in the log like those described in issue #74963

Is this something that only happens some percentage of the time? Or, only after the app runs for awhile? Or, maybe it depends on certain setup from the test project?

Anyway, I'm happy to try testing again, but I'm not sure I fully understand what the critical Quest 3 issue is :-/

AThousandShips · 2023-12-07T16:32:25Z

drivers/vulkan/vulkan_context.cpp

@@ -2473,7 +2477,7 @@ Error VulkanContext::swap_buffers() {
 		return OK;
 	}

-	//	print_line("swapbuffers?");
+	//	print_line("swap_buffers");


Suggested change

// print_line("swap_buffers");

//print_line("swap_buffers");

Commented code, as we're already modifying

I'll fix this up after the rebase

BastiaanOlij · 2023-12-11T01:10:37Z

Anyway, I'm happy to try testing again, but I'm not sure I fully understand what the critical Quest 3 issue is :-/

Weird, it's pretty consistent for me. Interestingly enough @RandomShaper PR does prevent the crash as well, but the error spam remains a problem. Or maybe another fix snuck in since I last tested this that prevents the crash.

All in all it's still a good fix as we're never presenting this swapchain, there is extra overhead we're removing, both memory wise and performance wise.

YuriSizov · 2023-12-18T15:52:42Z

I assume this still needs a rebase?

BastiaanOlij · 2023-12-26T10:09:12Z

I assume this still needs a rebase?

It did since Pedros PR was merged :) should be all good now

AThousandShips

Looks good otherwise! 🎉

AThousandShips · 2023-12-26T10:19:42Z

servers/rendering/renderer_viewport.cpp

@@ -764,6 +763,7 @@ void RendererViewport::draw_viewports(bool p_swap_buffers) {
 							blit_to_screen_list[vp->viewport_to_screen].push_back(blits[b]);
 						}
 					}
+					RSG::rasterizer->end_viewport(p_swap_buffers && blits.size() > 0);


Suggested change

RSG::rasterizer->end_viewport(p_swap_buffers && blits.size() > 0);

RSG::rasterizer->end_viewport(p_swap_buffers && !blits.is_empty());

Optional style suggestion

akien-mga · 2024-01-02T11:22:16Z

There's a warning/error with MSVC:

.\drivers/d3d12/d3d12_context.h(235): error C3668: 'D3D12Context::flush': method with override specifier 'override' did not override any base class methods
.\drivers/d3d12/d3d12_context.h(235): error C3668: 'D3D12Context::flush': method with override specifier 'override' did not override any base class methods
.\drivers/d3d12/d3d12_context.h(235): error C3668: 'D3D12Context::flush': method with override specifier 'override' did not override any base class methods


scons: *** [platform\windows\godot_windows.windows.editor.x86_64.obj] Error 2
Error: platform\windows\display_server_windows.cpp(4663): error C2259: 'D3D12Context': cannot instantiate abstract class
.\drivers/d3d12/d3d12_context.h(68): note: see declaration of 'D3D12Context'
platform\windows\display_server_windows.cpp(4663): note: due to following members:
platform\windows\display_server_windows.cpp(4663): note: 'void ApiContextRD::flush(bool,bool,bool)': is abstract
.\servers/rendering/renderer_rd/api_context_rd.h(61): note: see declaration of 'ApiContextRD::flush'

AThousandShips · 2024-01-02T14:12:46Z

This is because that class hasn't been updated and the override has just two arguments instead of three

BastiaanOlij · 2024-01-03T12:44:54Z

There's a warning/error with MSVC:

.\drivers/d3d12/d3d12_context.h(235): error C3668: 'D3D12Context::flush': method with override specifier 'override' did not override any base class methods
.\drivers/d3d12/d3d12_context.h(235): error C3668: 'D3D12Context::flush': method with override specifier 'override' did not override any base class methods
.\drivers/d3d12/d3d12_context.h(235): error C3668: 'D3D12Context::flush': method with override specifier 'override' did not override any base class methods


scons: *** [platform\windows\godot_windows.windows.editor.x86_64.obj] Error 2
Error: platform\windows\display_server_windows.cpp(4663): error C2259: 'D3D12Context': cannot instantiate abstract class
.\drivers/d3d12/d3d12_context.h(68): note: see declaration of 'D3D12Context'
platform\windows\display_server_windows.cpp(4663): note: due to following members:
platform\windows\display_server_windows.cpp(4663): note: 'void ApiContextRD::flush(bool,bool,bool)': is abstract
.\servers/rendering/renderer_rd/api_context_rd.h(61): note: see declaration of 'ApiContextRD::flush'

Oops, forgot about that. Added the property but didn't do anything with this. The scenario where this would be set to false really only happens on Android (and possibly iOS/VisionPro some day) so there doesn't need to be a DX12 implementation. @RandomShaper I'm guessing we could skip the _wait_for_idle_queue if p_sync is false just in case? But it feels risky to me as there is no way to test the behaviour.

RandomShaper · 2024-01-03T15:28:30Z

If there's no expectation it will be called with false, we can even just ERR_FAIL_COND it as not implemented.

BastiaanOlij · 2024-01-08T10:51:30Z

If there's no expectation it will be called with false, we can even just ERR_FAIL_COND it as not implemented.

Done!

akien-mga · 2024-01-08T13:56:27Z

Thanks!

YuriSizov · 2024-01-24T15:01:29Z

This PR is not cherry-pickable because it's based on the codebase after #83452 (and D3D12). A separate PR needs to be made for the 4.2 branch.

BastiaanOlij added bug topic:rendering topic:xr cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Oct 31, 2023

BastiaanOlij added this to the 4.3 milestone Oct 31, 2023

BastiaanOlij self-assigned this Oct 31, 2023

BastiaanOlij force-pushed the skip_present_if_needed branch from 4c9a60d to d780436 Compare October 31, 2023 11:06

BastiaanOlij marked this pull request as ready for review October 31, 2023 11:18

BastiaanOlij requested a review from a team as a code owner October 31, 2023 11:18

BastiaanOlij requested review from m4gr3d and clayjohn October 31, 2023 11:18

BastiaanOlij force-pushed the skip_present_if_needed branch from d780436 to fa496b1 Compare October 31, 2023 11:25

RandomShaper reviewed Oct 31, 2023

View reviewed changes

drivers/vulkan/vulkan_context.cpp Outdated Show resolved Hide resolved

BastiaanOlij force-pushed the skip_present_if_needed branch 3 times, most recently from 1e145f2 to 8fa5aab Compare November 5, 2023 00:52

BastiaanOlij force-pushed the skip_present_if_needed branch 2 times, most recently from 8a37149 to c4882fc Compare November 20, 2023 23:21

clayjohn approved these changes Dec 5, 2023

View reviewed changes

BastiaanOlij mentioned this pull request Dec 6, 2023

Split RenderingDevice into API-agnostic and RenderingDeviceDriver parts #83452

Merged

7 tasks

AThousandShips reviewed Dec 7, 2023

View reviewed changes

BastiaanOlij force-pushed the skip_present_if_needed branch from c4882fc to 85fe843 Compare December 26, 2023 10:08

BastiaanOlij force-pushed the skip_present_if_needed branch from 85fe843 to 901624b Compare December 26, 2023 10:13

AThousandShips approved these changes Dec 26, 2023

View reviewed changes

BastiaanOlij force-pushed the skip_present_if_needed branch from 901624b to 1c0c7c6 Compare January 3, 2024 12:42

Skip swapchain logic if there is nothing to present (Android OpenXR)

d6caa69

BastiaanOlij force-pushed the skip_present_if_needed branch from 1c0c7c6 to d6caa69 Compare January 8, 2024 10:51

akien-mga merged commit 25011e9 into godotengine:master Jan 8, 2024
15 checks passed

YuriSizov removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip swapchain logic if there is nothing to present (Android OpenXR) #84244

Skip swapchain logic if there is nothing to present (Android OpenXR) #84244

BastiaanOlij commented Oct 31, 2023 •

edited

BastiaanOlij commented Oct 31, 2023

trent-waddington commented Nov 1, 2023

BastiaanOlij commented Nov 20, 2023

clayjohn left a comment

BastiaanOlij commented Dec 5, 2023

clayjohn commented Dec 7, 2023

akien-mga commented Dec 7, 2023

dsnopek commented Dec 7, 2023

dsnopek commented Dec 7, 2023 •

edited

AThousandShips Dec 7, 2023

BastiaanOlij Dec 11, 2023

BastiaanOlij commented Dec 11, 2023

YuriSizov commented Dec 18, 2023

BastiaanOlij commented Dec 26, 2023

AThousandShips left a comment

AThousandShips Dec 26, 2023

akien-mga commented Jan 2, 2024

AThousandShips commented Jan 2, 2024

BastiaanOlij commented Jan 3, 2024

RandomShaper commented Jan 3, 2024

BastiaanOlij commented Jan 8, 2024

akien-mga commented Jan 8, 2024

YuriSizov commented Jan 24, 2024

	RSG::rasterizer->end_viewport(p_swap_buffers && blits.size() > 0);
	RSG::rasterizer->end_viewport(p_swap_buffers && !blits.is_empty());

Skip swapchain logic if there is nothing to present (Android OpenXR) #84244

Skip swapchain logic if there is nothing to present (Android OpenXR) #84244

Conversation

BastiaanOlij commented Oct 31, 2023 • edited

BastiaanOlij commented Oct 31, 2023

trent-waddington commented Nov 1, 2023

BastiaanOlij commented Nov 20, 2023

clayjohn left a comment

Choose a reason for hiding this comment

BastiaanOlij commented Dec 5, 2023

clayjohn commented Dec 7, 2023

akien-mga commented Dec 7, 2023

dsnopek commented Dec 7, 2023

dsnopek commented Dec 7, 2023 • edited

AThousandShips Dec 7, 2023

Choose a reason for hiding this comment

BastiaanOlij Dec 11, 2023

Choose a reason for hiding this comment

BastiaanOlij commented Dec 11, 2023

YuriSizov commented Dec 18, 2023

BastiaanOlij commented Dec 26, 2023

AThousandShips left a comment

Choose a reason for hiding this comment

AThousandShips Dec 26, 2023

Choose a reason for hiding this comment

akien-mga commented Jan 2, 2024

AThousandShips commented Jan 2, 2024

BastiaanOlij commented Jan 3, 2024

RandomShaper commented Jan 3, 2024

BastiaanOlij commented Jan 8, 2024

akien-mga commented Jan 8, 2024

YuriSizov commented Jan 24, 2024

BastiaanOlij commented Oct 31, 2023 •

edited

dsnopek commented Dec 7, 2023 •

edited