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

Add stereoscopic rendering through multiview #48207

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Apr 26, 2021

godotengine/godot-proposals#2648

The idea here is too re-introduce stereoscopic rendering into Godot 4 but take a different approach to Godot 3. Instead of rendering one eye after the other we're going to provide multiple cameras to the render server and have it make the right decision. The initial implementation will be using multiview which means that all render buffer textures get an extra layer and we'll end up rendering left and right eye images in parallel with the Vulkan driver performing the duplication. This means we get stereoscopic rendering with a minimum of state changes and a minimum of changes to the render engine itself.

To support this there are a number of areas that are changing.

While we are focusing on implementing stereoscopic rendering wherever possible we're building things in a way that additional views can be supported going forward.

Adding Multiview to render device
While detection of Multiview capabilities was already merged this PR introduces further changes that enable the Vulkan implementation of Multiview. These include:

  • proper initialization of the Vulkan device so Multiview features are enabled if available.
  • creating texture arrays instead of textures for our render buffers with a layer for each view
  • creation of a Multiview render buffer instead of a normal render buffer if more then one view is requested

Changes to XRInterface
XRInterface is changing to accommodate this new approach.

  • is_stereo is replaced with get_view_count which return the number of views we need to support
  • get_camera_transform has been added to get the center position and is used to place our node and determine our reference frame when recentering our player. This replaces calling get_transform_for_eye(EYE_MONO, ...
  • get_transform_for_view replaces get_transform_for_eye and now takes a view index
  • get_projection_for_view replaces get_projection_for_eye and now takes a view index, note that this removes the ability to call this with EYE_MONO but as that never worked for any stereoscopic implementation and for mono applications it returns the correct projection matrix it isn't a great loss
  • commit_eyes replaces commit_for_eye and is now called after rendering of both eyes is finished with a render buffer RID that contains our render result. This functions can optionally return a blit list that tells the render engine whether output to screen is required (many VR runtimes will output to the headset only).
  • get_external_texture_for_eye for now is deprecated. A new implementation of this will need to be introduced.

Note, further changes are delegated to #49301

Changes to viewports
The use_xr flag (was arvr) has been re-enabled and will trigger the use of the primary XR interface for this viewport.

  • if use_xr is true get_view_count is used to ensure the render buffer related to the viewport will contain the correct number of layers
  • if use_xr is true get_transform_for_view and get_projection_for_view are called for each view to create the camera data send to the renderer

2D rendering if more then 1 view is requested is defunct. There are a few strategies but most are questionable especially if we're not looking at run of the mill stereoscopic rendering. There is an argument for disabling 2D all together when more then 1 view is in play.

Changes to shader compilation
Where shaders are used for stereo rendering they need to be altered to support Multiview. The changes themselves are generally minor mostly revolving around choosing a different projection matrix based on ViewIndex. SpirV already adds the requires support.

  • if Multiview is available we declare a new define: has_VK_KHR_multiview
  • when compiling a shader for multiview we define MULTIVIEW in the shader version
  • uniform buffers are used to contain the additional projection matrices for each view

Currently we define additional shader versions for stereo/multiview but it gets a little unwieldy. I'm looking into an alternative where we can create a normal and multiview version of the same shader similar to how cull variants are created

Changes to the render engine

edit note that the changes to the clustered forward renderer were moved to PR #49092, this PR will focus purely on the mobile renderer implementation.

This is where there is still a lot of work to do with only the core rendering now working properly:

  • it is here where the limitation of 2 views currently exists and mostly due to limited storage for projection matrices
  • culling currently uses the combined frustum that is created but both occlusion culling and light culling in the cluster implementation are broken because they do not take stereo into account
  • scene_data now contains additional projection matrices for each view
  • the main render loop in both clustered and mobile renderer now switch between normal and stereo variants of the shaders
  • the sky shader has been changed to support stereo rendering
  • the tonemap logic has been changed to support tonemapping all views in one go
  • blitting our output to screen

There are still many parts here that need updating, one is re-introducing external textures/render targets to render to. The other is to make an option in the mobile render to skip the copy logic and use a single render buffer (possibly the external one). This involves disabling DOF/Glow/SSR and a bunch of other post effects and moving basic tonemapping into the main shader.

We may introduce a fallback if required later on in case multiview is not supported though according to the Vulkan documentation any Vulkan 1.1 device should provide a multiview implementation.

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch 3 times, most recently from f364d64 to 1df8d27 Compare April 28, 2021 12:48
@BastiaanOlij
Copy link
Contributor Author

Currently broken and just prototyping with loads of shortcuts but getting closer to it actually rendering in stereo. Its now creating the main render buffers as multiview buffers and compiling shaders with the main code hopefully properly rendering stuff in stereo.

Need to figure out why it currently has a mismatch in buffers causing it to fail rendering

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from 1df8d27 to 9ad7787 Compare April 29, 2021 11:21
@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch 3 times, most recently from 38a5848 to 02e55fa Compare May 12, 2021 13:15
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented May 12, 2021

Starting to get somewhere with this:
image

So it is duplicating the drawcalls nicely, but it's either rendering to both layers or it's not splitting the layers correctly.... RenderDoc is crashing with a weird error so I can't debug very properly at the moment.

I'll figure it out :)

edit one thing I noticed I did wrong is that I need to create the textures as 2D_ARRAY, getting closer :)

@BastiaanOlij
Copy link
Contributor Author

Why RenderDoc? Why?!? :)

CrashHandlerException: Program crashed
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] RENDERDOC_MakeDebuggablePackage
[1] RENDERDOC_MakeDebuggablePackage
[2] RENDERDOC_EndProfileRegion
[3] RENDERDOC_EndProfileRegion
[4] RENDERDOC_EndProfileRegion
[5] RENDERDOC_EndProfileRegion
[6] RENDERDOC_EndProfileRegion
[7] VK_LAYER_RENDERDOC_CaptureNegotiateLoaderLayerInterfaceVersion
[8] VK_LAYER_RENDERDOC_CaptureNegotiateLoaderLayerInterfaceVersion
[9] RENDERDOC_EndProfileRegion
[10] vkCreateRenderPass (D:\Development\godot4-git\thirdparty\vulkan\loader\trampoline.c:1467)
[11] RenderingDeviceVulkan::_render_pass_create (D:\Development\godot4-git\drivers\vulkan\rendering_device_vulkan.cpp:3494)
[12] RenderingDeviceVulkan::framebuffer_format_create (D:\Development\godot4-git\drivers\vulkan\rendering_device_vulkan.cpp:3516)
[13] RenderingDeviceVulkan::framebuffer_create (

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from 02e55fa to 51990fc Compare May 13, 2021 09:41
@BastiaanOlij
Copy link
Contributor Author

Working on changing things over to use texture arrays, I missed that they should be configured as such.. still not rendering properly

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from 51990fc to acc49fc Compare May 13, 2021 10:53
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented May 13, 2021

image

It's now rendering correctly but only when started from RenderDoc. Examining a frame from within RenderDoc I can see stereo is being applied correctly.

edit looks like the issue running it without RenderDoc is related to the shaders being bound to the wrong pipeline. Multiview shaders must be bound to a multiview pipeline and normal shaders to a normal pipeline. Need to think about how to improve this.

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from acc49fc to 8958a5a Compare May 13, 2021 11:48
@BastiaanOlij
Copy link
Contributor Author

ERROR: VALIDATION - Message Id Number: -1480880714 | Message Id Name: VUID-VkShaderModuleCreateInfo-pCode-01091
	Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-01091 ] Object 0: handle = 0x1ba4e0882f8, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0xa7bb8db6 | vkCreateShaderModule(): The SPIR-V Capability (MultiView) was declared, but none of the requirements were met to use it. The Vulkan spec states: If pCode declares any of the capabilities listed in the SPIR-V Environment appendix, one of the corresponding requirements must be satisfied (https://vulkan.lunarg.com/doc/view/1.2.176.1/windows/1.2-extensions/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-01091)
	Objects - 1
		Object[0] - VK_OBJECT_TYPE_DEVICE, Handle 1899684725496
   at: VulkanContext::_debug_messenger_callback (drivers\vulkan\vulkan_context.cpp:157)

Currently stuck on this, when using a multiview shader something we are sending to vkCreateGraphicsPipelines is not correct. As far as I can see we're sending it the correct framebuffer, must be one of the other bits of the shader.

As mentioned before, the weird thing is that it works when running using RenderDoc

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch 2 times, most recently from a30aba2 to 0eb6667 Compare May 15, 2021 11:45
@BastiaanOlij
Copy link
Contributor Author

With some help from people on the Vulkan Slack I was able to solve all the validation issues and this now runs properly.

Still a LOT of work to do to make everything that needs stereo support have stereo support but most of the difficult stuff is now done. Will first work on making this work on the mobile renderer too.

Would be good to get some early eyes on things because this is a BIG job that touches on a lot of parts.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented May 15, 2021

Just a recap on my discussion with @reduz last night on Godot chat about the next steps.

First in relation to our camera data. The plan we discussed yesterday is to change the data structure to the following:

  • A single camera transform which in monoscopic is our camera transform and in stereo is a center point between the two eye transforms possibly moved back to support our clipping frustum.
  • A single camera projection matrix which in monoscopic is our camera projection matrix and in stereo is a combined frustum that encompasses the two eyes as best as possible
  • For stereo only, a projection matrix for each eye that combines the offset between the center position and the eye position with the eyes original projection matrix.

These will be determined either at the start of render_scene in our culling class or in the function that currently handles the cameras and calls that render_scene function.

This approach means that we can keep a lot of the code that doesn't need to change for stereoscopic the same and more efficiently handle things that would otherwise need to be handled for stereoscopic just to accommodate the two eye positions.

The logic that combines the two frustums for clipping will initially be the implementation we currently have in Godot 3 which only works properly if the two eyes share the same near and far clipping planes. But we do need to improve on this as not all devices have that guaranteed.

Second, we're going to concentrate on making the rest of the stereoscopic rendering work properly in the mobile renderer first, and then port the solution to the clustered renderer.

Finally seeing this PR is getting pretty big I'm going to look at which changes could be moved to separate PRs. For instance multiview support in the driver could be a separate PR we can apply without it getting in the way of anyone.

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from 0c4d05e to e1e0830 Compare June 4, 2021 10:32
@BastiaanOlij
Copy link
Contributor Author

Rebased and fixed up the Transform -> Transform3D rename.

I'm setting this ready for review. There is more to do around the XRInterface (for which I've started a new PR) and solving a lighting issue if the stereo camera isn't in the same plane (a future use case) but I think this is getting to a point where the work done so far needs to be evaluated and may even be ready to merge.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review June 4, 2021 10:34
@BastiaanOlij BastiaanOlij requested review from a team as code owners June 4, 2021 10:34
@akien-mga akien-mga requested a review from reduz June 4, 2021 12:10
@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from e1e0830 to 2e56b05 Compare June 6, 2021 10:53
@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch 3 times, most recently from 31dabcd to be7a45b Compare June 10, 2021 11:39
@@ -656,6 +657,11 @@ void SceneShaderForwardClustered::init(RendererStorageRD *p_storage, const Strin
actions.renames["CUSTOM2"] = "custom2_attrib";
actions.renames["CUSTOM3"] = "custom3_attrib";

// not implemented but need these just in case code is in the shaders
actions.renames["VIEW_INDEX"] = "0";
actions.renames["VIEW_MONO_LEFT"] = "0";
Copy link
Member

Choose a reason for hiding this comment

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

I would kinda expect that VIEW_INDEX 0 and 1 should be enough. Also should it not be MULTIVIEW_INDEX ? I am not sure this is very useful outside multiview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VIEW_INDEX is the actual variable, it's only 0 here because its turned off in the forward clustered, this code is only there so the compiler doesn't throw up. See the mobile implementation for the proper code.

@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch 3 times, most recently from 2e131c7 to a4fee11 Compare June 13, 2021 12:07
@BastiaanOlij BastiaanOlij force-pushed the multiview_stereoscopic branch from a4fee11 to 15c1a76 Compare June 13, 2021 12:52
@reduz
Copy link
Member

reduz commented Jun 13, 2021

Should be ready to merge once it passes CI

@reduz
Copy link
Member

reduz commented Jun 13, 2021

The only problem I see to this is that its still missing the usage of the proper eye vector on each view, so reflections and specular blobs will most likely not be correct. We can add this later as a separate PR if you wish.

@akien-mga
Copy link
Member

The only problem I see to this is that its still missing the usage of the proper eye vector on each view, so reflections and specular blobs will most likely not be correct. We can add this later as a separate PR if you wish.

Yeah let's do this as a separate PR I think, so that we can merge this and @JFonS' #48847.

@akien-mga akien-mga merged commit 4ebf248 into godotengine:master Jun 13, 2021
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the multiview_stereoscopic branch June 14, 2021 01:35
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.

5 participants