[opengl3] Prevent viewport's render buffers getting out of sync with render target when resized by XR interface

[dsnopek]

I've been working on getting WebXR working with Godot 4 (see draft PR #64514), and I've encountered an issue where switching to XR leads to the viewport's render buffers getting out of sync with the render target in GLES3. This is because we need to run _configure_3d_render_buffers() after the render target has been resized, so that it uses the new texture from the render target - otherwise the render buffers still have the old texture attached.

There was this comment:

				// Would have been nice if we could call viewport_set_size here,
				// but alas that takes our RID and we now have our pointer,
				// also we only check if view_count changes in render_target_set_size so we need to call that for this to reliably change

viewport_set_size() calls to _configure_3d_render_buffers(), so if this code did use viewport_set_size(), this problem wouldn't have existed! So, I made an private _viewport_set_size() that takes the Viewport pointer, which can be used here and by the public viewport_set_size(), reducing code duplication.

However, just this on its own would have led to _configure_3d_render_buffers() getting called every frame when using XR (which means destroying and re-creating the render buffers every frame) so I refactored a bunch of code to make it only resize the render target and call _configure_3d_render_buffers() if the size or view count has actually changed. This is why the RendererViewport::Viewport::get_view_count() function has turned into the RendererViewport::Viewport::view_count variable.

Hopefully, that all makes sense! But I'd be happy to discuss further :-)

[BastiaanOlij]

Not entirely sure if the changes is safe. It is totally possible the viewport size is set correctly before the XRInterface has a chance to update the viewport count, this was why get_viewport_count() was always called, it would check if the viewport count is now being overridden by the XRInterface.

Also note that I rewrote a lot of this logic in #63901 though I do not think it deals with you issue, it will require you to redo your PR I think.

[dsnopek]

Not entirely sure if the changes is safe. It is totally possible the viewport size is set correctly before the XRInterface has a chance to update the viewport count, this was why get_viewport_count() was always called, it would check if the viewport count is now being overridden by the XRInterface.

It's updating the render target and buffers if either the size OR the view count changes. In _draw_viewports() it passes xr_interface->get_view_count() to _viewport_set_size():

_viewport_set_size(vp, xr_size.width, xr_size.height, xr_interface->get_view_count());

... and in _viewport_set_size() it checks if the view count has changed:

if (p_viewport->size != new_size || p_viewport->view_count != p_view_count) {

So, assuming I'm understanding your concern correctly, I think this should be safe?

Also note that I rewrote a lot of this logic in #63901 though I do not think it deals with you issue, it will require you to redo your PR I think.

Ok, thanks! I'll keep an eye on that one and rebase my PR once it's merged.

[dsnopek]

I just tested these changes with OpenXR, and everything seemed to work fine there too! I used the Oculus runtime with the Quest 2 over Air Link.

[dsnopek]

I can't re-test this in an HTML5 export after the latest rebases, because 3D rendering is currently broken (see issue #65304), but this has a positive effect even when testing with the MobileVRInterface: if you resize the window, without this PR it goes blank; but with this PR it'll continue to render the left eye.

@BastiaanOlij @clayjohn I'd appreciate it if you guys could give this some review! Thanks :-)

[BastiaanOlij]

Hmmm, I'm wondering if we're going about this the wrong way. The whole issue happens because if use_xr is enabled, we ignore the size of the viewport and apply the size given by the XR interface inside of the render server.

Maybe we should move part of this logic out of the render server and into the viewport logic itself. I.E. if use_xr is enabled, we check the viewport size in the viewports internal process notification.

This also solves issues with the GDScript side of the engine not knowing the true size of the viewport in XR mode.

[BastiaanOlij]

Btw @dsnopek, the issue you're describing with the screen going black was one that we encountered in Godot 3 as well. Godots main viewport gets resized when you resize the screen, this logic should be disabled if the main viewport is set to 'use_xr'.

[dsnopek]

Thanks for the feedback!

Maybe we should move part of this logic out of the render server and into the viewport logic itself. I.E. if use_xr is enabled, we check the viewport size in the viewports internal process notification.

Hm. You know this code much better than me, I'd need to spend some time digging into the viewport code to imagine how that'd work - I'll try to dig into it soon. But we need to make sure to also rebuild the render buffers if the view count changes too.

Btw @dsnopek, the issue you're describing with the screen going black was one that we encountered in Godot 3 as well. Godots main viewport gets resized when you resize the screen, this logic should be disabled if the main viewport is set to 'use_xr'.

I think in this case it's simpler than that. When XR is enabled it's just going down a different code path that doesn't lead to the render buffers getting remade. This bug could actually be fixed just by adding _configure_3d_render_buffers() after the RSG::texture_storage->render_target_set_size(), but it seemed better to refactor a little bit (as explained in the original description).

[dsnopek]

I just made PR #65524 which attempts the approach described by Bastiaan above. I still personally like the bug fix here better, but I'm happy to go which ever way is deemed best :-)

[dsnopek]

Per Bastiaan on RocketChat, he'd prefer the other approach, so this one is superseded by #65524