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

[WebXR] Add support for getting and setting display refresh rate #72938

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 9, 2023

This is currently a draft because it's completely untested - I just whipped it up because @Malcolmnixon asked about it :-)

This is fully working and tested now!

Also, this in NOT essential for 4.0 at all, it can wait until later :-)

@dsnopek dsnopek added this to the 4.x milestone Feb 9, 2023
@dsnopek dsnopek requested a review from a team as a code owner February 9, 2023 03:05
@dsnopek dsnopek marked this pull request as draft February 9, 2023 03:05
@dsnopek dsnopek force-pushed the webxr-frame-rate branch 2 times, most recently from fde261a to 33bac1f Compare February 9, 2023 03:22
@BastiaanOlij
Copy link
Contributor

Looks like a good start! Did you find out if WebXR has the same issue as the Quest native where it defaults to Quest 1s 72hz refresh rate but reports 90hz?

@dsnopek dsnopek force-pushed the webxr-frame-rate branch 2 times, most recently from 9ceb6c5 to b82e0c1 Compare February 10, 2023 02:16
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 10, 2023

I've finally had a chance to test this, and it basically works! However, it's a little counter-intuitive.

It'll report a refresh rate of 0 until it has been explicitly set to some value. And, like all things in WebXR, it's asynchronous, so the change won't happen immediately, but some time later (after 1 or more frames; or never, if it fails). But once it has successfully changed, then it'll start reporting the actual refresh rate.

So, probably the right way to use it, is to call get_available_display_refresh_rates() right after the WebXR session starts, pick one that's closest to your desired frame rate, and then set it right away.

Because it's asynchronous, I've added a signal (display_refresh_rate_changed) so the game developer will know when to make any changes for the new frame rate, like, for example, setting the physics tick rate to match the refresh rate.

For this same reason, I'm thinking about removing the display_refresh_rate property, and just having the getter and setter methods. It's really weird to set a property to a value, but have it report a different value when you get it. (And since the requested change might actually fail, we probably shouldn't fake it with a local variable.)

Did you find out if WebXR has the same issue as the Quest native where it defaults to Quest 1s 72hz refresh rate but reports 90hz?

I guess not, because it always reports 0 until it's changed :-)

@Malcolmnixon
Copy link
Contributor

This is roughly on par with the OpenXR experience - In the frame-rate control (GodotVR/godot-xr-tools#364) we handled the odd behavior.

I figure we'll extend the same file to handle refresh rate for WebXR in exactly the same way:

  1. Let the user pick a desired refresh rate (or use the reported rate if not specified)
  2. Set the refresh rate to the supported rate closest to the desired
  3. Assume the refresh rate change will go through and set the physics rate appropriately

For OpenXR/Quest we get the oddity of it reporting 90hz while running at 72hz; so if the user doesn't request a desired rate we pick 90hz (closest to the bogus 90 reported). Given the rules above, for WebXR/Quest if the user doesn't request a desired rate we would end up picking 72Hz (closest to the bogus 0 reported).

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 10, 2023

Given the rules above, for WebXR/Quest if the user doesn't request a desired rate we would end up picking 72Hz (closest to the bogus 0 reported).

Hm, the lowest it reports supporting is 60, so if it's going to grab the lowest one, it would end up on 60 which isn't ideal.

In any case, that's not something we need to solve here :-)

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 11, 2023

I've removed the WebXRInterface.display_refresh_rate property in favor of the getter and setter methods, since setting the value is asynchronous (and may not succeed).

At this point, this functionality is working and ready for review, so I'm taking it out of draft!

@dsnopek
Copy link
Contributor Author

dsnopek commented Mar 28, 2023

Now that we're past the 4.0 stable release, this PR's time has come! :-)

@BastiaanOlij @m4gr3d @Malcolmnixon Can one of you review this PR when you have a chance? Thanks!

Copy link
Contributor

@Malcolmnixon Malcolmnixon left a comment

Choose a reason for hiding this comment

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

This looks good to me, and testing showed it works fine on my Quest 2 in WebXR.

@akien-mga akien-mga merged commit ab7cb2a into godotengine:master Apr 11, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 4.x, 4.1 Apr 11, 2023
@dsnopek dsnopek deleted the webxr-frame-rate branch July 22, 2024 15:26
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.

4 participants