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

Resize Viewport for XR in the Viewport node #65524

Closed
wants to merge 1 commit into from

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Sep 8, 2022

This is an alternative approach to fixing the same bug as PR #64513 based on a suggestion from @BastiaanOlij

It doesn't totally work: weird things happen when you resize the window. I tried modifying the Window node to act differently when use_xr is true, but that got pretty messy and I couldn't get it working right (things just got weirder ;-)) so it's not included in this PR.

In any case, if this seems to @BastiaanOlij and @clayjohn like the better way to go from an architectural perspective, I can continue trying working on it and hopefully iron out those problems!

(However, to me PR #64513 seems like the simpler fix to the bug. I could make an even simpler PR using the same approach as that one, that just adds 1 line to fix the bug, but it'd be less efficient for reasons explained in the description there.)

@dsnopek dsnopek force-pushed the xr-resize-viewport-2 branch 3 times, most recently from 1cb52be to 0495f80 Compare September 9, 2022 03:17
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 9, 2022

@BastiaanOlij Ok, the latest version takes into account your review, and, in limited testing with MobileVRInterface, it works without weirdness! I still have to try it with WebXR and OpenXR.

To me, this feels kinda messy, given all the different things it needs to touch, especially since I'm not super familiar with much of this code.

Also, I don't know if there are use cases where it makes sense for SubViewport to be used for XR? Currently, this PR makes it not allowed to enable XR on a SubViewport.

@@ -648,6 +663,19 @@ void Window::_update_window_size() {
void Window::_update_viewport_size() {
//update the viewport part

#ifndef _3D_DISABLED
if (use_xr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure why this needs to be added? When XR is enabled we don't really care about the window size as we're overriding this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, it turns out that this was the fix for the window size weirdness! Switching to window_get_real_size() didn't help (and, actually, I think that's not correct, because on X11, at least, that includes the window decorations, which we don't want).

Without this bit of code, the BlitToScreens returned by MobileVRInterface don't work right. The Rect2i() that gets passed into viewport_attach_to_screen() here is what eventually gets passed to XRInterface::post_draw_viewport() as the screen rect. So, we need to keep updating that as the window changes size, in order to have the correct screen rect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I don't know enough about the internals of the window system to know it isn't getting the correct size here. It should be getting the size of the window, not the viewport. You can have situation in normal apps where the viewport only covers a portion of the screen and is only blit to that portion.

It doesn't make sense that just because we have XR mode on, that suddenly breaks.... There is something else going on here..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the viewport is holding on to the window size, which is which is what's being passed into XRInterface::post_draw_viewport(). This would probably be easier to walk through on a video call where I could share my screen, but if you walk through the code backwards:

  1. In servers/rendering/renderer_viewport.cpp it has:
    Vector<BlitToScreen> blits = xr_interface->post_draw_viewport(vp->render_target, vp->viewport_to_screen_rect);
    
  2. The vp->viewport_to_screen_rect above is set in RendererViewport::viewport_attach_to_screen()
  3. And it's Window::_update_window_size() which normally calls viewport_attach_to_screen(). We don't want any of the other things that Window::_update_window_size() does normally, but when in XR mode, we still need the viewport_attach_to_screen() calls so that the code up in point nr 1 still works.

There's probably a better way to ensure that viewport_attach_to_screen() gets called when the underlying OS window gets resized than the way I did it, but I'm not sure what that would be.

@YuriSizov YuriSizov added this to the 4.0 milestone Sep 9, 2022
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 10, 2022

@BastiaanOlij Alright, I've got all the request updates in!

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 14, 2022

This is superceded by PR #65800

@dsnopek dsnopek closed this Sep 14, 2022
@dsnopek dsnopek deleted the xr-resize-viewport-2 branch July 22, 2024 15:24
@AThousandShips AThousandShips removed this from the 4.0 milestone Jul 22, 2024
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