Add view count support to Viewport#115799
Conversation
79a2b4a to
bb8cf36
Compare
bb8cf36 to
b167e95
Compare
b167e95 to
e770612
Compare
|
At a high-level, this looks really great to me! And I did some testing to make sure old code still works with these changes, for both XR output and (non-stereo) composition layers. Seems to work great! However, it's kind of hard to evaluate if this is sufficient to implement the use cases described above without any way to test them (even partially)
How big are the camera changes? Because I think it would be really nice to have them in this PR, so we could test something (if only verifying a "non-XR" stereo viewport renders correctly in RenderDoc), and see more of the whole picture of what changes will be necessary to actually use this After that, both the foveated inset and projection composition layer changes definitely make sense in their own individual PRs UPDATE: I did manage to sort of test it? I set a viewport to have 2 views and render always. When using 2D content, nothing turned up in RenderDoc. But when using 3D content (including a camera, of course), I found the render pass where it does output to texture array, but only the clear color. In both cases, the preview in the Godot editor doesn't work with a view count of 2 |
Correct, we have always disabled 2D rendering if there are multiple layers, assuming stereo projection. 2D rendering only works in very limited situations and would require us to introduce a horizontal offset, though in 3D monitor scenarios it would be nice to support it. The big no-no are slanted displays in XR devices, where its impossible to do traditional 2D output even with an offset applied. I am planning on working on the camera changes next, but I am planning on keeping that a separate PR (one that will be build ontop of this branch) as I suspect there will be a fair bit of debate around the implementation. |
|
@dsnopek further on that remark, I will look at why it doesn't render 3D, probably there is a check that if the view count doesn't match the number of projection matrices it will fail. That may be something that will still need to be part of the camera changes but I did want to make it so that it just uses the same projection matrix and you'll need to deal with it in shaders. |
e770612 to
bcae5d0
Compare
|
Out of curiosity, does this make it possible to implement godotengine/godot-proposals#2138 later on? |
I guess it would be able to use a separate layer for that, but that could also be done by just rendering the gizmo's to a separate texture buffer but re-using the depth buffer, and then mixing it in after effects have been processed. |
This is something what I am thinking of as an alternative to the separate viewport approach.
Does this require patching the renderer? |
Yes, but seeing we identify gizmo's as they are on separate rendering layers (as in Godot render layers, not texture layers), you could make something that filters them out of the normal rendering pass and applies them in a separate rendering pass. But that is a change that will need to be done during rendering. |
bcae5d0 to
c587e45
Compare
|
@dsnopek can we safely make an exception for this one? That method should have always been defined as const, not sure if it makes sense to have a compatibility function here? Or just add one to be safe? |
9cfe6e0 to
92f267b
Compare
b549a26 to
62757cd
Compare
m4gr3d
left a comment
There was a problem hiding this comment.
The code looks good to me.
But when using 3D content (including a camera, of course), I found the render pass where it does output to texture array, but only the clear color. In both cases, the preview in the Godot editor doesn't work with a view count of 2
Is this a blocker to be addressed in this PR?
62757cd to
f717002
Compare
No, the behaviour is expected. When you enable more than one layer 2D rendering is skipped completely, that has been the case for a long time because 2D rendering isn't possible in many stereoscopic applications. So it's correct that nothing was seen. If you check out #116424, that introduces camera changes that start to provide actual testable use cases for this change. |
AThousandShips
left a comment
There was a problem hiding this comment.
LGTM once the final detail is resolved
f717002 to
135184a
Compare
135184a to
f3a8152
Compare
|
Thanks! |
…m the game thread The change in where this function is called from came from: godotengine#115799
This PR adds the ability to configure the view count (layers) on a Viewport. Before this was only possible inside of the renderer itself through an overrule when
use_xris on.This allows us to create additional viewports that use the layer system and no longer be restricted to the primary XR output. This enabled:
For some of the above we also need camera changes but those our outside of the scope of this PR.
It also moves the XR viewport size overrule out of the render server and into the Viewport class. This is currently a "polling" approach just like it was in the rendering server, the logic is already smart enough not to rebuild the viewport if the size has not changed.
This also has the bonus effect that accurate size information will be available on the Viewport node when XR is used. A limitation of the old approach that caused issues in the past.
Triggering output to the HMD and other overrules such as destination buffer overrides and dynamic resolution changes remain internal to the rendering engine and triggered by the
use_xrflag. Some of these we may address at some point in the future but marking a Viewport as the primary XR output will remain.Note
This extracts some core improvements from other PRs that attempted quad view/inset rendering so they can be reviewed in isolation. We're looking into an improved implementation to foveated inset rendering that will rely on this logic.