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

Always render when XR is enabled, even if no OS windows can draw #94412

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jul 15, 2024

The short explanation:

Currently, in Main::iteration(), it will skip rendering for a particular frame if DisplayServer::get_singleton()->can_any_window_draw() returns false.

For all current DisplayServers, they either:

  1. Always return true, or
  2. Return true if at least one of Godot's windows is visible

However, when using an XR device, we need to keep rendering, even if all OS windows aren't visible. For example, if you have a VR headset connected to your desktop, you don't want it to stop rendering to the headset if the Godot window on your desktop is minimized.

So, this PR will cause rendering to happen if the primary XRInterface is initialized, even if DisplayServer::get_singleton()->can_any_window_draw() returns false.

The longer explanation:

I started looking into this because there is a case when using the Meta XR Simulator on MacOS, where we would sometimes call xrWaitFrame() twice without calling xrBeginFrame() in between, which leads to the application getting "stuck" forever (spinning beach ball and everything).

According the OpenXR specification (in Section 10.3):

A subsequent xrWaitFrame call must block until the previous frame has been begun with xrBeginFrame and must unblock independently of the corresponding call to xrEndFrame.

So, the Meta XR Simulator is acting exactly as specified, and is blocking until the next xrBeginFrame() (which will never come when Godot is using single-threaded rendering).

The reason the xrBeginFrame() sometimes doesn't happen, is that we're calling it at the beginning of rendering (as we should, per the spec!), and, per the "short explanation" above, Godot may skip rendering if all windows are momentarily not visible.

While I originally dug into this for this very specific OpenXR related bug, I think it logically makes sense that when using XR, we shouldn't make rendering conditional on if OS windows are visible.

@dsnopek dsnopek added this to the 4.3 milestone Jul 15, 2024
@dsnopek dsnopek requested a review from a team July 15, 2024 22:30
@dsnopek dsnopek requested a review from a team as a code owner July 15, 2024 22:30
main/main.cpp Outdated Show resolved Hide resolved
main/main.cpp Outdated Show resolved Hide resolved
@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 16, 2024

Alright, I've updated this PR to implement @BastiaanOlij's idea, with a few small changes:

  • I put the methods on DisplayServer rather than OS
  • I renamed the methods to register_additional_output(), unregister_additonal_output() and has_additional_outputs()
  • I used Object * rather than Ref<RefCounted>: If the system registering the output doesn't clean up after itself, with Ref<RefCounted> we run the risk of an object being freed when DisplayServer is freed, at which point most of Godot won't be operable and anything the object tries to do in its destructor could cause issues.

However, I don't particularly like this approach:

  • We're leaving it up to individual XRInterfaces to register their additional output. On the one hand, this is good, because some interfaces may not need to (for example, WebXR and MobileVR), but now it's another thing that XRInterfaces need to know might be required.
  • We need to expose this publicly, because an XRInterface could be implemented from GDExtension, and so it'll need to be able to call (un)register_additional_output()
  • This isn't the prettiest API: registering an Object * is a little awkward, but I don't know what would be a better identifier.

Anyway, please let me know if this looks better to you guys!

/cc @clayjohn

@dsnopek dsnopek force-pushed the xr-always-render branch 2 times, most recently from c1bb353 to d0e4090 Compare July 16, 2024 19:26
@BastiaanOlij
Copy link
Contributor

I agree that the Object * thing is a bit weird, we could also keep a counter and just increase/decrease it. A little more fault sensitive but maybe cleaner. The point is however that the existing behaviour is far too strict. XR is an example where this fails, but I don't believe its the only example. Like I said, I think we'll find other use cases for this as more and more people integrate Godot with other systems and would like it to keep working at its full ability even when minimized.

Heck a long standing wish of many users for XR is that we don't even have any Godot window open, I can see similar situations when integrating Godot into other apps.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

lgtm, it's worth having the discussion if passing Object is indeed the best way or if we simplify this to increasing/decreasing a counter

servers/display_server.h Outdated Show resolved Hide resolved
@akien-mga akien-mga requested review from bruvzg and removed request for clayjohn July 18, 2024 07:41
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga merged commit 1e81a94 into godotengine:master Jul 18, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@pochoco24
Copy link

I tried to capture Godot with OBS while minimized with this commit, but it still freezes in OBS when the window is minimized.

All I have in the scene is an animated moving cube, a camera, and a script:

`extends Node3D

var object = Object.new()

func _ready() -> void:
DisplayServer.register_additional_output(object)`

@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 31, 2024

@pochoco24 I responded here

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.

6 participants