Details

Reviewers

Jeroen Bakker (jbakker)
Ray Molenkamp (LazyDodo)
Brecht Van Lommel (brecht)

Maniphest Tasks

T71347: Virtual Reality - Milestone 1 - Scene Inspection
T71365: GSoC 2019: Core Support of VR Headsets through OpenXR -- Review

Commits

rB3b1ef223bad5: Ghost: Support drawing OpenGL framebuffers into a DirectX 11 buffer

Summary

Needed for DirectX-only OpenXR runtimes (e.g. Windows Mixed Reality).

Adds a minimal DirectX 11 Ghost context, plus some shared DirectX-OpenGL resource interface using the NV_DX_interop2 WGL extension.
I know that the current implementation fails on some systems, but it's not something I intend to spend time on soon, so I'd consider it a known issue. Recently I also learned that OSVR uses the same extension, see https://github.com/sensics/OSVR-RenderManager/blob/master/osvr/RenderKit/RenderManagerD3DOpenGL.cpp. Their implementation may be useful to fix the issue, according to a OSVR dev, it works quite reliably for them.

Diff Detail

Repository

rB Blender

Branch

temp-openxr-directx (branched from master)

Build Status

Buildable 6872
Build 6872: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) created this revision.Nov 5 2019, 1:39 PM

Harbormaster completed remote builds in B5558: Diff 19402.Nov 5 2019, 1:39 PM

Accidentally diffed against master, rather than D6188.

Harbormaster completed remote builds in B5559: Diff 19403.Nov 5 2019, 1:43 PM

Julian Eisel (Severin) retitled this revision from Ghost DirectX compatibility from GSoC OpenXR branch to Core XR Support [part 2]: Ghost DirectX compatibility.Nov 5 2019, 1:44 PM

Julian Eisel (Severin) added a task: T71365: GSoC 2019: Core Support of VR Headsets through OpenXR -- Review.Nov 5 2019, 3:41 PM

Julian Eisel (Severin) mentioned this in D5537: GSoC 2019: Core Support of Virtual Reality Headsets through OpenXR.Nov 5 2019, 3:47 PM

Jeroen Bakker (jbakker) added inline comments.Nov 6 2019, 1:05 PM

intern/ghost/intern/GHOST_ContextD3D.cpp
107	If handy, make it more clear, now it is not clear how and when to use it.

Dalai Felinto (dfelinto) added a task: T71347: Virtual Reality - Milestone 1 - Scene Inspection.Nov 13 2019, 3:33 PM

I am not a ghost expert perhaps add a ghost expert as blocker.

intern/ghost/intern/GHOST_ContextD3D.cpp
252	Is it really needed to do a clear for each blit action? If this is for transparency?
266	Is this linear interpolation? is this needed as the areas are the same
intern/ghost/intern/GHOST_ContextD3D.h
117	Unused?

This revision now requires changes to proceed.Jan 7 2020, 3:55 PM

Add comment on DirectX device debugging
Avoid clearing render-target on each redraw in release builds
Use GL_NEAREST as interpolation mode for framebuffer blitting

Harbormaster completed remote builds in B6871: Diff 22252.Mar 2 2020, 12:42 PM

Diff'ed on wrong branch

Harbormaster completed remote builds in B6872: Diff 22253.Mar 2 2020, 12:44 PM

Julian Eisel (Severin) added inline comments.Mar 2 2020, 12:45 PM

intern/ghost/intern/GHOST_ContextD3D.cpp
252	It should not be needed, but I find it useful for debugging issues. E.g. when the interop extension fails, you get pink output. Disabled this for release builds for now.
266	The areas are the same, so there should not be any interpolation. The specification says that this has to be either `GL_LINEAR` or `GL_NEAREST` though, so passing 0 is not allowed. Changed it to `GL_NEAREST`, since I guess that would be cheaper just in case, but it really shouldn't matter.
intern/ghost/intern/GHOST_ContextD3D.h
117	It's an override to be called through the C-API, but this patch doesn't add the C-API call. Don't like it but we need this to draw the framebuffer vertically mirrored (because of OpenGL vs. DirectX Y-axis direction). See `GHOST_isUpsideDownWindow()` in D6193.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Mar 2 2020, 12:46 PM

Have not tested, but I don't see anything wrong in the code.

intern/ghost/intern/GHOST_ContextD3D.cpp
265	Tweak error message so that it's clear this is about D3D / OpenGL interop, now it could be from anywhere.
344	I don't see a reason to use `std::string` instead of `char*`, seems like an unnecessary memory allocation.
intern/ghost/intern/GHOST_SystemWin32.cpp
419	Is'nt this the window title? If so it should be something like "Blender XR", in case it gets exposed to the user.

Quick pass only, didn't build it, I am aware that error checking is not something we normally do (our motto seems to be malloc will never fail!) so i'm hesitant to pick on memory issues, however com calls are more likely to fail than an allocation so extra care must be taken there.

The Mix of raw com use and ComPtr in a single file bothers me, pick a method and stick with it! however given I'm not convinced we need the triangle code, this problem may solve it self.

intern/ghost/intern/GHOST_ContextD3D.cpp
159	result not checked tex may not have a valid pointer which you will deref when you call release a few lines down.
171	result not checked , possible null deref below
341	Do we need this at all? I can see it being useful during development, but this is about to land in master. I we want to keep this a comment explaining why would be in order here.
intern/ghost/intern/GHOST_SystemWin32.cpp
431	possible null deref

This revision now requires changes to proceed.Mar 5 2020, 3:56 PM

Julian Eisel (Severin) added inline comments.Mar 5 2020, 4:42 PM

intern/ghost/intern/GHOST_ContextD3D.cpp
341	It was useful in the beginning, didn't use it in a while. I thought it's better to keep it anyway, since this is not trivial to add. Uploaded this as a patch now (P1284) and removed the code. We can link to the patch from a code comment.

Ray Molenkamp (LazyDodo) added inline comments.Mar 5 2020, 5:26 PM

intern/ghost/intern/GHOST_SystemWin32.cpp
431	Scratch that, my bad. new throws bad_alloc doesn't return null.

Cleanup: Remove testing code to draw a triangle with D3D
Address points from review

Harbormaster completed remote builds in B6968: Diff 22429.Mar 5 2020, 5:50 PM

Remove left-overs from merge conflict errors

Harbormaster completed remote builds in B6970: Diff 22431.Mar 5 2020, 5:53 PM

Ray Molenkamp (LazyDodo) accepted this revision.Mar 5 2020, 6:08 PM

@Jeroen Bakker (jbakker) only had minor points, and said in person that he's fine with this after having it reviewed by Brecht. So I'll go ahead and push this.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 5 2020, 7:17 PM

Closed by commit rB3b1ef223bad5: Ghost: Support drawing OpenGL framebuffers into a DirectX 11 buffer (authored by Julian Eisel (Severin)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Julian Eisel (Severin) added a commit: rB3b1ef223bad5: Ghost: Support drawing OpenGL framebuffers into a DirectX 11 buffer.

Core XR Support [part 2]: Ghost DirectX compatibility
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 22253

intern/ghost/CMakeLists.txt

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_ISystem.h

intern/ghost/GHOST_Types.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_ContextD3D.h

intern/ghost/intern/GHOST_ContextD3D.cpp

intern/ghost/intern/GHOST_System.h

intern/ghost/intern/GHOST_System.cpp

intern/ghost/intern/GHOST_SystemWin32.h

intern/ghost/intern/GHOST_SystemWin32.cpp

intern/ghost/intern/GHOST_WindowWin32.cpp

Core XR Support [part 2]: Ghost DirectX compatibilityClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 22253

intern/ghost/CMakeLists.txt

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_ISystem.h

intern/ghost/GHOST_Types.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_ContextD3D.h

intern/ghost/intern/GHOST_ContextD3D.cpp

intern/ghost/intern/GHOST_System.h

intern/ghost/intern/GHOST_System.cpp

intern/ghost/intern/GHOST_SystemWin32.h

intern/ghost/intern/GHOST_SystemWin32.cpp

intern/ghost/intern/GHOST_WindowWin32.cpp

Core XR Support [part 2]: Ghost DirectX compatibility
ClosedPublic
Actions

Revision Contents
Changeset List