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.

Sign up for GitHub

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

Jump to bottom

Correct occlusion culling viewport location calculation when projection uses asymmetric FOV #104249

Merged
merged 1 commit into from
Mar 19, 2025
Merged

Correct occlusion culling viewport location calculation when projection uses asymmetric FOV #104249

merged 1 commit into from
Mar 19, 2025
+22 −12

Conversation

jamie-pate
Copy link
Contributor

Fixes #104193

In OpenXR the viewport location is not centered on the transform origin

@jamie-pate jamie-pate requested a review from a team as a code owner March 16, 2025 18:27
@jamie-pate jamie-pate force-pushed the fix_104193 branch 2 times, most recently from 5cdc7d4 to b929b67 Compare March 16, 2025 18:36
@AThousandShips AThousandShips added bug topic:xr regression cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Mar 17, 2025
@AThousandShips AThousandShips added this to the 4.5 milestone Mar 17, 2025
@AThousandShips AThousandShips requested a review from a team March 17, 2025 13:17
Flarkk
Flarkk suggested changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@Flarkk Flarkk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In its current state this PR will introduce inconsistent jittering on the Y-axis for OpenXR users, in addition to some performance penalty to everyone.

I believe there's a cleaner way to solve it - see my detailed comments.

As a side effect, this will also fix occlusion culling with FRUSTUM camera mode as it likely suffers from the same issue.

@dsnopek
Copy link
Contributor

dsnopek commented Mar 17, 2025

I can confirm that this PR fixes the issue as shown in the MRP on #104193

I'd be happy to re-test again after it's been updated

@akien-mga akien-mga changed the title Use p_cam_projection.get_endpoints() to calculate the viewport location Use camera projection get_endpoints() to calculate the viewport location in occlusion jitter Mar 17, 2025
@jamie-pate jamie-pate requested a review from a team as a code owner March 17, 2025 23:02
@jamie-pate jamie-pate changed the title Use camera projection get_endpoints() to calculate the viewport location in occlusion jitter Correct occlusion culling viewport location calculation when projection uses asymmetric fov Mar 18, 2025
@jamie-pate jamie-pate requested a review from Flarkk March 18, 2025 00:03
@dsnopek
Copy link
Contributor

dsnopek commented Mar 18, 2025 •
edited
Loading

Something I'm a little confused about with this PR, is that it appears to only be changing the jitter calculation, but isn't changing the code from PR #98758 that apparently caused the regression. That code still uses get_viewport_half_extents() - is that OK? Also, is this PR in some way better than reverting that PR, and trying to come up with a different way to fix the bug it intended to fix?

@Flarkk Flarkk mentioned this pull request Mar 18, 2025
Merged
@Flarkk
Copy link
Contributor

Flarkk commented Mar 18, 2025 •
edited
Loading

That code still uses get_viewport_half_extents() - is that OK?

I believe it's not OK (I just left a comment there).
To me get_viewport_half_extents() should be replaced in both places by something that supports non-symmetrical frustums.

I didn't look close enough yet to explain why this PR alone seems to resolve the issue.
Side effects with jittering can be very subtle and I wouldn't be surprised some have remained unnoticed yet.

@jamie-pate
Copy link
Contributor Author

Something I'm a little confused about with this PR, is that it appears to only be changing the jitter calculation, but isn't changing the code from PR #98758 that apparently caused the regression. That code still uses get_viewport_half_extents() - is that OK? Also, is this PR in some way better than reverting that PR, and trying to come up with a different way to fix the bug it intended to fix?

I had to put the fix in the calling function at the current HEAD because the change in #98758 has actually been refactored here: c7895ca

@Flarkk
Copy link
Contributor

Flarkk commented Mar 18, 2025 •
edited
Loading

Ah okay, I understand better now.
c7895ca refactored #98758 in a way it supports asymmetrical frustums already.

So this PR is the only remaining place with get_viewport_half_extents() causing problems, and should completely fix the issue by itself.

@jamie-pate jamie-pate requested a review from Flarkk March 18, 2025 18:54
Flarkk
Flarkk reviewed Mar 18, 2025
View reviewed changes
Copy link
Contributor

@Flarkk Flarkk left a comment

Choose a reason for hiding this comment

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

I find it now in a pretty good shape speaking of the code. I left a few comments.

I didn't check it against the MRP yet.

@jamie-pate jamie-pate requested a review from Flarkk March 18, 2025 20:31
@jamie-pate jamie-pate force-pushed the fix_104193 branch 2 times, most recently from 6fc180e to 11c6593 Compare March 18, 2025 20:33
Flarkk
Flarkk reviewed Mar 18, 2025
View reviewed changes
Copy link
Contributor

@Flarkk Flarkk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Last last round - please don't hate me, it's done with good intentions for the sake of code clarity.

Also I did it on my smartphone, so forgive any typo.

Thank you for your patience !

@jamie-pate
Copy link
Contributor Author

Last last round - please don't hate me, it's done with good intentions for the sake of code clarity.

Also I did it on my smartphone, so forgive any typo.

Thank you for your patience !

I don't think this improves anything. (I previously commented but it probably got lost in the churn)

I believe bottom_left += _get_jitter(...) is more readable than the changes you are requesting.

@Flarkk
Copy link
Contributor

Flarkk commented Mar 19, 2025 •
edited
Loading

Look, it's pretty much the same either way so it's fine by me if you don't make the change.

It's just that changing where the jitter is added doesn't contribute to the fix anymore.

I couldn't test whether this still solves the issue because I don't have a XR device, but I'm pretty confident.

Thanks !

@akien-mga akien-mga requested a review from dsnopek March 19, 2025 09:27
@Flarkk Flarkk mentioned this pull request Mar 19, 2025
Open
dsnopek
dsnopek approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks, this is working in my testing on Quest 3!

I think the code is fine as it is, even without Flarkk's suggested refactor to return Rect2. The priority at this point is fixing the regression - a re-organization can be discussed in a follow-up.

But I think the changes to the comments that Akien suggests should be made, so that if we don't change this, we don't end up with out-of-date comments

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@dsnopek
Copy link
Contributor

dsnopek commented Mar 19, 2025

While squashing, it would be nice if you could also rewrite the commit message to match the PR title - the message on the current 1st commit is out-of-date. Thanks!

@jamie-pate
Calculate the viewport location correctly in OpenXR
8e3e2a8 
Fixes godotengine#104193

In OpenXR the viewport location is not centered on the transform origin
@Repiteo Repiteo merged commit 5bb1a2c into godotengine:master Mar 19, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 19, 2025

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.4.1.

@akien-mga akien-mga removed the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label Mar 20, 2025
@akien-mga akien-mga changed the title Correct occlusion culling viewport location calculation when projection uses asymmetric fov Correct occlusion culling viewport location calculation when projection uses asymmetric FOV Mar 21, 2025
@Flarkk Flarkk mentioned this pull request Mar 24, 2025
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@Flarkk Flarkk Flarkk approved these changes

@dsnopek dsnopek dsnopek approved these changes

Assignees
No one assigned
Labels
bug regression topic:xr
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

Occlusion Culling on OpenXR is unstable (meshes flickering) in Godot4.4-stable
6 participants
@jamie-pate @dsnopek @Flarkk @akien-mga @Repiteo @AThousandShips