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

Allow Enabling 90Hz mode for Oculus #108

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikeage
Copy link
Member

@mikeage mikeage commented Apr 22, 2021

By adding:

{
 ...,
  "Video": {
    "DisplayRefresh": 90,
  },
 ...,
}

This PR will allow setting the default display refresh to 90Hz or, presumably, if you have v28, to 120Hz.

// VrApi : FPS=90/90,Prd=33ms,Tear=0,Early=0...
float targetDisplayRefresh = App.UserConfig.Video.DisplayRefresh;
float[] freqs = OVRManager.display.displayFrequenciesAvailable;
foreach (float freq in freqs)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure if we should do this check here or in UserOptions, but it needs the OVRManager and that was throwing exceptions in UserOptions, so I left it here

m_VrCamera.gameObject.AddComponent<OculusPreCullHook>();

gameObject.AddComponent<OculusMRCCameraUpdate>();
// ---------------------------------------------------------------------------------------- //
Copy link
Member Author

Choose a reason for hiding this comment

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

The reformatting here was done manually, and wasn't caught in #81 because of dotnet/format#1101 (and probably the same basic issue in Rider)

@mikeage mikeage changed the title Enable 90Hz mode for Oculus Allow Enabling 90Hz mode for Oculus Apr 23, 2021
@@ -425,6 +426,17 @@ public float OdsPoleCollapsing
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure if Video was the right section for this, but I didn't see anything better. It seems like it's mostly video out stuff...

@mikeage mikeage marked this pull request as ready for review April 23, 2021 06:15
@mikeskydev mikeskydev added the enhancement Feature added label Aug 11, 2021
@mikeage
Copy link
Member Author

mikeage commented Oct 28, 2021

updated to tip

@andybak
Copy link
Contributor

andybak commented Nov 16, 2021

Can we make this draft?

@mikeage mikeage marked this pull request as draft November 16, 2021 12:56
@mikeage
Copy link
Member Author

mikeage commented Nov 16, 2021

Done, though I think it's ready for review, just not for merge. How can I indicate that?

@andybak
Copy link
Contributor

andybak commented Nov 16, 2021

Done, though I think it's ready for review, just not for merge. How can I indicate that?

I think maybe we just need a discussion. My feeling is that the benefits aren't enough to warrant the hassle right now and we should maybe revisit this later. So my vote is to move it to a Github issue or discussion.

@mikeage
Copy link
Member Author

mikeage commented Nov 16, 2021

90Hz mode offers a much smoother experience, if we can keep it up. Jumping framerates are probably worse, though, than locking at 72Hz.

This PR was an attempt at making it easier to test if 90Hz is suitable for real world use in OB. I still think it serves that purpose, although if we don't have anyone with time to test and validate it, then I agree that it's not really going to go anywhere. But without the PR, I know it will never get anywhere...

(so yes, I opened this to try to encourage discussion. I guess it didn't work)

@andybak
Copy link
Contributor

andybak commented Mar 22, 2022

One way to mitigate the performance cost of this would be to combine it with Application Spacewarp: https://developer.oculus.com/blog/introducing-application-spacewarp/

But our extensive use of transparancy might rule that out. Anyone got any thoughts?

@mikeskydev
Copy link
Member

One way to mitigate the performance cost of this would be to combine it with Application Spacewarp: https://developer.oculus.com/blog/introducing-application-spacewarp/

Note that Spacewarp currently hard requires URP (and OpenXR, but we're working on that) to function, so it's a bigger task! It sounds like a really useful feature though.

@mikeage
Copy link
Member Author

mikeage commented Mar 22, 2022

Before we talk about performance cost... can we try this and see?

We know that our average frame rate (from Oculus metrics) is just a hair under 72. I don't know if that means we'll never hit 90 in the real world, or it's easily capable of doing target-2.

@mikeskydev
Copy link
Member

@mikeage we've had reports that xr_v2 has gained some performance compared to the main release. is bumping to 90 by default possible?

@mikeage
Copy link
Member Author

mikeage commented Aug 2, 2022

Sure -- do we have a set of reference files to test with? (I need to confirm how we check FPS; I don't think we have the VrApi prints anymore). But we can try enabling it on xr_v2. I'll do a PR shortly #269

@andybak
Copy link
Contributor

andybak commented Aug 4, 2022

I'm mildly -1 on this. (-0.5?)

A lot of our users push performance to the limit. Gaining performance when we switch to xr is a good thing. Is this the best thing to spend it on?

If it's the default, how easy is it for people to switch back? How many will even think of switching back?

There's a bunch of things disabled going from PC to Quest (shadows, bloom after a certain complexity) that we could think about reenabling instead of this.

@mikeskydev
Copy link
Member

godotengine/godot#67179 (comment) Just wanting to note that the PR to Godot to add https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_FB_display_refresh_rate suggests that it's supported on SteamVR as well! We could make a cross-platform implementation of this.

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

Successfully merging this pull request may close these issues.

3 participants