VR: Fix for Viewport Denoising Artifacts
ClosedPublic
Actions

Authored by Peter Kim (muxed-reality) on Jul 9 2021, 5:57 AM.

Details

Reviewers

Julian Eisel (Severin)
Clément Foucault (fclem)

Commits

rBc41b93bda532: XR: Fix for Viewport Denoising Artifacts

Summary

Addresses T76003. When using VR with Eevee and viewport denoising,
scene geometry could sometimes be occluded for one eye. Solution was,
as Clement suggested, to use a separate GPUViewport/GPUOffscreen for
each VR view instead of reusing a single one for rendering.

Output from master:

With patch applied:

Sample scene (credit: blendFX):

blenderXR_testlab.blend34 MBDownload

Diff Detail

Repository: rB Blender

Event Timeline

Peter Kim (muxed-reality) requested review of this revision.Jul 9 2021, 5:57 AM

Peter Kim (muxed-reality) created this revision.

Harbormaster completed remote builds in B15706: Diff 39326.Jul 9 2021, 5:57 AM

Peter Kim (muxed-reality) edited the summary of this revision. (Show Details)Jul 9 2021, 6:06 AM

Peter Kim (muxed-reality) added reviewers: Julian Eisel (Severin), Clément Foucault (fclem).

Peter Kim (muxed-reality) added a project: Virtual Reality.

Looks good for the most part, just one thing I'd prefer to see addressed.

source/blender/windowmanager/xr/intern/wm_xr_intern.h
92–93	The only thing that bothers me about this patch is that this array size is fixed. Couldn't we just allocate a dynamic array in `wm_xr_session_surface_offscreen_ensure()`?

This revision now requires changes to proceed.Jul 22 2021, 11:43 AM

Julian Eisel (Severin) added inline comments.Jul 22 2021, 11:44 AM

intern/ghost/intern/GHOST_XrSession.cpp
423	Better to `assert(view_idx < 256)` here.

Use dynamic array for surface viewports

Harbormaster completed remote builds in B15952: Diff 39804.Jul 22 2021, 3:05 PM

Peter Kim (muxed-reality) marked 2 inline comments as done.Jul 22 2021, 3:07 PM

Peter Kim (muxed-reality) added inline comments.

intern/ghost/intern/GHOST_XrSession.cpp
423	Added assertion.
source/blender/windowmanager/xr/intern/wm_xr_intern.h
92–93	Now using ListBase for surface viewports.

Looks good to me now! I'm wondering though, by how much does this increase VRAM usage? I guess duplicating the buffers, which aren't exactly small is not going to be too cheap.

@Clément Foucault (fclem) is there any good reason to even have a history buffer for VR, where we just constantly redraw with changed view matrices? I guess if the history buffer is only used for AA, that wouldn't work well in VR anyway.

In D11858#309402, @Julian Eisel (Severin) wrote:

Looks good to me now! I'm wondering though, by how much does this increase VRAM usage? I guess duplicating the buffers, which aren't exactly small is not going to be too cheap.

Just using Windows task manager,
total VRAM usage went from 1.8 -> 2.0 GB for WMR and 1.9 -> 2.1 GB for Oculus.

In D11858#309402, @Julian Eisel (Severin) wrote:

Looks good to me now! I'm wondering though, by how much does this increase VRAM usage? I guess duplicating the buffers, which aren't exactly small is not going to be too cheap.

@Clément Foucault (fclem) is there any good reason to even have a history buffer for VR, where we just constantly redraw with changed view matrices? I guess if the history buffer is only used for AA, that wouldn't work well in VR anyway.

The history buffer does serves a good purpose. If EEVEE's viewport denoising is ON (maybe it's turned OFF by default for VR i do not know) it will avoid many noise issue with stochastic effects such as AO or SSR. It does help also for AA on high frequency areas. So to me it's important for VR.

I see the increase in Vram is not that much so I'm ok with the changes.