Skip to content

fix: send full view config to openvr #2775

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

WilliamGanx
Copy link

Background: Same as #2198, rebased on the latest master. This change is specifically required for the PFDM MR device.

The PFDM MR device employs physically rotated displays to enable an expanded field of view (FOV). Previously, frame rotation was handled by the device runtime compositor to maintain 3rd party apps's backward compatibility (though this approach limited full utilization of the expanded FOV).

341272482-3b7d5d07-f06f-479f-a287-81e1b9de3e9e

The latest OTA update introduces support for the standard KHR OpenXR Loader. OpenXR apps using the KHR loader must now render pre-rotated frames based on the left/right view configurations. If the full view configurations were not provided to OpenVR, the incorrect rendering could cause binocular misalignment.

The changes maintain theoretical compatibility across headset devices, and has been verified using Quest 3 units.

@zmerp
Copy link
Member

zmerp commented Mar 27, 2025

This change breaks compatibility with the v20 protocol. I'm ok for merging this fix as version v21.0.0-dev02, but it can't be released until v21 stable. It will be a nightly-only fix for now

Copy link
Member

@zmerp zmerp left a comment

Choose a reason for hiding this comment

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

Apart from the comments above, there is a format check fail from c++ code

@WilliamGanx
Copy link
Author

Apart from the comments above, there is a format check fail from c++ code

I tried to fix the format errors from the c++ code, and there are no errors in local checks, but the online CI is still reporting errors.

cargo xtask check-format
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.20s
     Running `target\debug\alvr_xtask.exe check-format`
$ cargo fmt --all -- --check

Done [0m 1s]

@zmerp
Copy link
Member

zmerp commented Mar 28, 2025

Run simply cargo xtask format to apply both Rust and c++ autofoemat

@curoviyxru
Copy link
Contributor

curoviyxru commented Mar 28, 2025

cargo xtask format doesn't do anything on last commit in this PR. Also on master it adds the same space like here in Controller.h, but master has passed all checks, strange.

@The-personified-devil
Copy link
Collaborator

The format thing is a tad bit wonky sometimes if stuff changes between updates, I'll see if I can fix it for this pr

@shinyquagsire23
Copy link
Contributor

fwiw I suspect there's a possible non-breaking mergable tempfix for this by always rendering the frames in world space based on the view params received w/ the frame, instead of the direct blit that we're doing (identity reprojection_rotation). That way if the streamer sends frames that are not canted, they can still be drawn even if there is a resampling penalty.

(I say world space, but it doesn't actually have to be in world space, but I've found that the math is much easier with a world space render pass; the streamer compositor is a good reference)

@WilliamGanx
Copy link
Author

fwiw I suspect there's a possible non-breaking mergable tempfix for this by always rendering the frames in world space based on the view params received w/ the frame, instead of the direct blit that we're doing (identity reprojection_rotation). That way if the streamer sends frames that are not canted, they can still be drawn even if there is a resampling penalty.

(I say world space, but it doesn't actually have to be in world space, but I've found that the math is much easier with a world space render pass; the streamer compositor is a good reference)

From what I understand, in addition to the resampling penalty, there is also a FOV penalty.
Here is a user test report on the FOV.

With rotated rendering:
   horizontal: 94.46 deg
   vertical: 90.00 deg
   diagonal: 111.64 deg
   overlap: 94.46 deg

Without rotated rendering:
    horizontal: 102.46 deg
    vertical:    89.86 deg
    diagonal:   117.60 deg
    overlap:     86.46 deg

@shinyquagsire23
Copy link
Contributor

Yeah, it would require a bit of over-rendering. For visionOS I added an "FOV comfort" slider to intentionally scale the render tangents to compensate for underprediction, might be worth moving/adding to the streamer.

@zmerp
Copy link
Member

zmerp commented Mar 31, 2025

might be worth moving/adding to the streamer

That was the plan after the v21 protocol break

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

Successfully merging this pull request may close these issues.

5 participants