Details

Reviewers

Jeroen Bakker (jbakker)
Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)

Maniphest Tasks

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

Summary

Changes for the higher level, more Blender specific side of the implementation.

Main additions:

WM-XR: Layer tying Ghost-XR to the Blender specific APIs/data
wmSurface API: drawable, non-window container (manages Ghost-OpenGL and GPU context)
DNA/RNA for initial management of VR session settings (could disable saving of these for now?)
--debug-xr and --debug-xr-time commandline options
Utility batch & config file for using the Oculus runtime (Windows only)

Diff Detail

Repository

rB Blender

Branch

temp-openxr-blenderside (branched from master)

Build Status

Buildable 7128
Build 7128: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) created this revision.Nov 5 2019, 3:33 PM

Harbormaster completed remote builds in B5563: Diff 19409.Nov 5 2019, 3:33 PM

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 5 2019, 3:37 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

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

I did a quick scan Patch is fairly straight forward. Next main topics we should address:

Color management and responsibilities
When compiling without XR, how should the external API react. IMO DNA, RNA should not be effected for this patch.

I could help with the CM part as we should get this correct, otherwise it will hunt us for-ever

source/blender/blenloader/intern/versioning_280.c
4293	I would always do the versioning code, otherwise data can not get converted when using builds.
source/blender/gpu/intern/gpu_viewport.c
543	Best to add the source (image) ocio role not the target display space. The proposed API can only we used for a strict setup.
588	Use OCIO (IMB_colormanagement) I was also looking into this part and my solution was let the caller do the color management setup (set correct shader) and use the GPU package for only the low level stuff.
source/blender/makesrna/intern/CMakeLists.txt
126	I would expect that the API is always added. It could be that some functions will have a dummy implementation when XR is turned off.
source/blender/windowmanager/wm.h
102	Add to top of the header

This revision now requires changes to proceed.Jan 15 2020, 9:07 AM

Julian Eisel (Severin) mentioned this in D6190: Core XR Support [part 2]: Ghost DirectX compatibility.Mar 2 2020, 12:45 PM

Regarding color-management:
From a color-management perspective, from what I know (based on discussions with Troy) is that we should submit a linear buffer to the OpenXR runtime. That is because in theory there's no way for us to tell which display space the HMD needs. The runtime has that knowledge and should therefore do the color-management. In practice, (virtually?) all available HMD's should be sRGB though, so we could safely assume that.
Performance wise that may not always be the preferred method though, since the runtime will have to enable its color management pipeline when we don't perform CM on our end. E.g. the WinMR docs point this out, https://docs.microsoft.com/en-us/windows/mixed-reality/openxr#select-a-swapchain-format.

source/blender/blenloader/intern/versioning_280.c
4293	Indeed, this should always be executed.
source/blender/gpu/intern/gpu_viewport.c
543	We now use the `GPUViewport`'s color settings to transform to display space, but only for the Monado (Linux) runtime. For other's we submit a linear buffer.
588	See above's comment, this is now managed via the regular `GPUViewport` CM.

Bring back utility batch and config file for Oculus support
Fix linking after recent build system changes in master
Fix compile errors and broken drawing after changes in master
Don't create framebuffer for VR view, re-use GPUViewport one
Remove own GPUViewport and GPUContext binding
Cleanup: Move struct forward declaration to top of header
Always generate XR-RNA, even with WITH_OPENXR disabled
Always do versioning code, even with WITH_OPENXR disabled
Bring back vertically mirrored drawing for DirectX compatibility
Fix compiler warning on Linux, swapping with wrong type

Harbormaster completed remote builds in B6881: Diff 22268.Mar 2 2020, 3:08 PM

Update oculus.json file for the Oculus runtime on Windows

Harbormaster completed remote builds in B6882: Diff 22269.Mar 2 2020, 3:13 PM

Looks broadly fine.

intern/ghost/GHOST_C-api.h
255	This presumably is specific to XR and does not need to be taken into account into other cases (and if it does, our code is incorrect because this is not used anywhere). Please update the description and function name to make that clear.
765	Same comment as above.
intern/ghost/intern/GHOST_ContextNone.h
33–34 ↗	(On Diff #22269)	Make sure to run `make format` before committing, this seems like it should remain on one line.
release/windows/batch/blender_oculus.cmd
2–9	Is this a temporary workaround? An option only for debugging/testing but not officially supported? There should at least be a comment here explaining why it works this way, rather than being integrated in a more user friendly way.
source/blender/makesdna/DNA_xr_types.h
25	Don't use the `b` prefix, we should get rid of that at some point for consistency rather than introduce it in more places.
source/blender/windowmanager/WM_api.h
159	It's not clear to me why there is Windows/DirectX specific logic here. I would expect it's possible to hide that in GHOST entirely, so that the secondary context is automatically created and used by GHOST without the WM being aware? Not a big deal but could be a good cleanup (possibly after commit).
source/blender/windowmanager/intern/wm_operators.c
3669	Remove "Attempt to", no need to hedge words like this in descriptions.
source/blender/windowmanager/intern/wm_xr.c
99	I don't trust the use of context here, we have no idea what will be in there when this error handler is called. Why is a popup menu needed here at all, rather than just showing them in the status bar? It seems unpredictable how many errors an OpenXR implementation might send or what their severity is. Maybe there are cases where it sends them continuously which would prevent the user from doing anything. I suggest to just remove this line and `evil_C`.
192	Remove `ROT_MODE_QUAT`, already handled below. But really, this should extract rotation and translation from `obmat` so constraints are taken into account. Camera rigs like that are pretty common.
311	Use `CLOG_ERROR`.
471–475	It's how the buffers interact, maybe some comments could clarify that. I don't quite understand why `ED_view3d_draw_offscreen_simple` takes both the viewport and offscreen, but then at the end nothing was written to the offscreen buffer? Is it only user as an intermediate buffer, or is it not needed at all?
source/blender/windowmanager/wm.h
102	This does not appear to be used, so remove it from the header again?

This revision now requires changes to proceed.Mar 5 2020, 2:18 PM

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

source/blender/makesdna/DNA_xr_types.h
25	Generally agree. Issue here is that OpenXR uses the `Xr` prefix for it's structs, so I wanted to avoid mix-ups. Then again this is only used on the Blender/WM side, while OpenXR is only used on the Ghost-XR side. So it's probably fine.
source/blender/windowmanager/intern/wm_xr.c
99	I would much prefer to keep the popup. Errors here are rather fatal and should only be thrown if an operation fails that may put the session or XR-instance into an invalid state. More importantly, if we reach this point, we end the session and even disconnect from the OpenXR runtime (to play safe). Recoverable errors should be handled on the Ghost-XR side and can print debug messages there. The kind of errors shown here are supposed to give useful feedback to the user, like if we try to start a VR session with no compatible device plugged in. Pop-ups are the best way to communicate this IMHO. I'm not sure why I need to explicitly invoke the popup though. I'll look into letting the WM do that.
192	Yes, it should use `obmat`! This part of the code is reworked in the `vr_scene_inspection` branch, where we have more advanced ways to set up the base-pose. See https://developer.blender.org/diffusion/B/browse/vr_scene_inspection/source/blender/windowmanager/intern/wm_xr.c$211-244.

Julian Eisel (Severin) mentioned this in D7098: VR: Milestone 1 - Scene Inspection - Features.Mar 10 2020, 8:39 PM

Julian Eisel (Severin) mentioned this in D7113: Allow WM-reports to be displayed as popups.Mar 11 2020, 3:31 PM

Cleanup: Remove unused Ghost window function
Cleanup: Run clang-format on all affected files
Cleanup: Address various review comments
Add more info to blender_oculus batch file
Cleanup: Add comments on GPU data and Ghost context usage
Remove in-place call to create reports-popup

Harbormaster completed remote builds in B7096: Diff 22684.Mar 11 2020, 3:37 PM

Julian Eisel (Severin) added inline comments.Mar 11 2020, 3:42 PM

intern/ghost/GHOST_C-api.h
255	Function was an old leftover, removed it.
765	Noted this in the comment now. I'm not sure what a better function name would be though. Alternatively we could also assert that this is false in the main window drawing function (or even the context change ones?).
source/blender/blenloader/intern/versioning_280.c
4802–4813	From merge conflict, to be removed.
source/blender/gpu/GPU_shader.h
187 ↗	(On Diff #22684)	To be removed.
source/blender/windowmanager/intern/wm_xr.c
99	D7113: Allow WM-reports to be displayed as popups
192	Marking as done because it's already addressed in D7098.
source/blender/windowmanager/wm.h
28	To be removed.
tests/python/bl_load_addons.py
53	Should actually be `xr_openxr` I guess. Same below.

Cleanup and correct wrong bpy.app.build_options usage

Harbormaster completed remote builds in B7128: Diff 22734.Mar 12 2020, 4:03 PM

Brecht Van Lommel (brecht) accepted this revision.Mar 16 2020, 8:56 PM

Ray Molenkamp (LazyDodo) added a subscriber: Ray Molenkamp (LazyDodo).Mar 16 2020, 8:58 PM

Ray Molenkamp (LazyDodo) added inline comments.

release/windows/batch/blender_oculus.cmd
1	needs to be `@REM` or be placed after `@echo off` to prevent this text from displaying on the console.

Update to latest master
Fix comment being printed to console in Windows batch file

Harbormaster completed remote builds in B7181: Diff 22848.Mar 16 2020, 10:03 PM

Used wrong base branch again...

Harbormaster completed remote builds in B7182: Diff 22849.Mar 16 2020, 10:05 PM

Campbell Barton (campbellbarton) resigned from this revision.Mar 16 2020, 11:18 PM

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

Just as a note, there are some small changes in the draw-manager here that I should check with @Clément Foucault (fclem) on. I don't want to delay the merge for this though, so I contacted him and will check with him over the next days. Any further improvements can be done in master.

Julian Eisel (Severin) mentioned this in rBdc2df8307f41: VR: Initial Virtual Reality support - Milestone 1, Scene Inspection.Mar 17 2020, 9:43 PM

Committed together with D7098, rBdc2df8307f41. Some further development will happen in master.

Thanks!

Julian Eisel (Severin) abandoned this revision.Mar 17 2020, 9:54 PM

Core XR Support [part 4]: Blender-side changes (+ the remaining bits)AbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 22734

intern/ghost/CMakeLists.txt

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_IContext.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_Context.h

release/windows/batch/blender_oculus.cmd

release/windows/batch/oculus.json

source/blender/CMakeLists.txt

source/blender/blenkernel/BKE_global.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenlib/BLI_math_geom.h

source/blender/blenlib/intern/math_geom.c

source/blender/blenloader/CMakeLists.txt

source/blender/blenloader/intern/readfile.c

source/blender/blenloader/intern/versioning_280.c

source/blender/draw/DRW_engine.h

source/blender/draw/intern/draw_manager.c

source/blender/editors/include/ED_view3d_offscreen.h

source/blender/editors/space_view3d/view3d_draw.c

source/blender/gpu/intern/gpu_viewport.c

source/blender/makesdna/DNA_view3d_enums.h

source/blender/makesdna/DNA_windowmanager_types.h

source/blender/makesdna/DNA_xr_types.h

source/blender/makesdna/intern/makesdna.c

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/CMakeLists.txt

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_internal.h

source/blender/makesrna/intern/rna_wm.c

source/blender/makesrna/intern/rna_xr.c

source/blender/windowmanager/CMakeLists.txt

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/intern/wm.c

source/blender/windowmanager/intern/wm_draw.c

source/blender/windowmanager/intern/wm_init_exit.c

source/blender/windowmanager/intern/wm_operators.c

source/blender/windowmanager/intern/wm_surface.c

source/blender/windowmanager/intern/wm_window.c

source/blender/windowmanager/intern/wm_xr.c

source/blender/windowmanager/wm.h

source/blender/windowmanager/wm_surface.h

source/creator/CMakeLists.txt

source/creator/creator_args.c

tests/python/bl_load_addons.py

tests/python/bl_load_py_modules.py

Core XR Support [part 4]: Blender-side changes (+ the remaining bits)
AbandonedPublic
Actions

Revision Contents
Changeset List