Page MenuHome

Core XR Support [part 3]: Ghost-XR API based on OpenXR
ClosedPublic

Authored by Julian Eisel (Severin) on Nov 5 2019, 3:00 PM.

Details

Summary

Design Overview

  • For code using this API, the most important object is a GHOST_XrContext handle. Through it, all API functions and internal state can be accessed/modified.
  • Main responsibilities of the Ghost XR-context are to manage lifetimes of the OpenXR runtime connection (represented by XrInstance), the session and to delegate operations/data to the session.
  • The OpenXR related graphics code, which is OS dependent, is managed through a GHOST_IXrGraphicsBinding interface, that can be implemented for the different graphics libraries supported (currently OpenGL and DirectX).
  • Much of this code here has to follow the OpenXR specification and is based on the OpenXR [[https://github.com/KhronosGroup/OpenXR-SDK-Source/tree/master/src/tests/hello_xr | hello_xr]] implentation.
  • In future we may want to take some code out of the context, e.g. extension and API layer management.
  • There are runtime debugging and benchmarking options (exposed through --debug-xr and --debug-xr-time, but not as part of this patch).
  • Error handling is described in a section below.

Why have this in Ghost?

Early on, I decided to do the OpenXR level access through GHOST. Main reasons:

  • OpenXR requires access to low level, OS dependent graphics lib data (e.g. see XrGraphicsBindingOpenGLXlibKHR)
  • Some C++ features appeared handy (std::vector, RAII + exception handling, cleaner code through object methods, etc.)
  • General low level nature of the OpenXR API

After all I think much of the functionality is too high level to live in GHOST however. I would like to address this by having a separate VAMR (virtual + augmented + mixed reality) module, placed in intern/. The main issue is getting this to work well with Ghost data, especially how to get the mentioned low level data out of Ghost.
This is something I'd like to look into again before too long, but for now I think having this in Ghost is reasonable.

Error Handling Strategy

The error handling strategy I chose uses C++ exceptions, a controversial feature. Let me explain why I think this is reasonable here.

The strategy requirements were:

  • If an error occurs, cleanly exit the VR session (or destroy the entire context), causing no resource leaks or side effects to the rest of Blender.
  • Show a useful error message to the user.
  • Don't impair readability of code too much with error handling.

Here's why I chose an exception based strategy:

  • Most alternatives require early exiting functions. This early exiting has to be 'bubbled up' the call stack to the point that performs error handling. For safe code, early exit checks have to be performed everywhere and code gets really impaired by error checking. Tried this first and wasn't happy at all. Even if error handling is wrapped into macros.
  • All GHOST_Xr resources are managed via RAII. So stack unwinding will cause them to be released cleanly whenever an exception is thrown.
  • GHOST_Xr has a clear boundary (the Ghost C-API) with only a handful of public functions. That is the only place we need to have try-catch blocks at. (Generally, try-catch blocks at kinda random places are a bad code smell IMHO. Module boundaries are a valid place to put them.)
  • Exceptions allow us to pass multiple bits of error information through mulitple layers of the call stack. This information can also be made specific with a useful error message. As of now, they conain a user error message, the OpenXR error code (if any), as well as the exact source code location the error was caught at.

So the strategy I went with works as follows:

  • If a VR related error occurs within GHOST_Xr, throw an exception (GHOST_XrException currently).
  • OpenXR calls are wrapped into a macro throwing an exception if the return value indicates an error.
  • Useful debugging information and user messages are stored in the exceptions.
  • All resources must be managed through RAII, so throwing an exception will release 'dangling' ones cleanly.
  • In the GHOST C-API wrappers, the exceptions are caught and contained error information is forwarded to a custom error handling callback.
  • The error handling callback is set in wm_xr.c, prior to creating the XR-Context, and implements clean destruction of the context.

Diff Detail

Repository
rB Blender
Branch
temp-openxr-ghostxr (branched from master)
Build Status
Buildable 5561
Build 5561: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 5 2019, 3:04 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
  • Remove CMake include unneeded after recent build system changes
  • Add required OpenXR defines to Ghost CMakeLists (used to be in root one)
Brecht Van Lommel (brecht) requested changes to this revision.Mar 5 2020, 7:28 PM

Seems broadly fine.

As noted in the review for part 4, it would be nice if we could not expose the D3D stuff in the API at all and keey that private to GHOST, similar to what we do for Metal on macOS.

intern/ghost/GHOST_C-api.h
997–1016

Add documenting comments for these like other GHOST API functions.

intern/ghost/intern/GHOST_XrContext.cpp
37

Zero initialize this?

67

Add empty line between functions.

526

Return a reference instead of a pointer?

intern/ghost/intern/GHOST_XrContext.h
31

I'm fine with initializers like this, but why use braces rather than = ? To me that's more readable and consistent with initialization elsewhere.

100–106

Don't mix static and non-static members, separate them a bit.

116–119

Personally would use init instead of enumerate here, to make it clear this is only for initialization.

intern/ghost/intern/GHOST_XrException.h
31

Don't abbreviate result.

intern/ghost/intern/GHOST_XrSession.h
30

Leave out the e, we don't use hungarian notation.

35

Forward declare class GHOST_XrContext;, we don't typically use class in argument lists like this.

And same for the other struct cases in this header.

intern/ghost/intern/GHOST_Xr_intern.h
47

I would not use a macro for this and leave out FILE and LINE from the exception (is not needed to find the relevant code).

For me this makes it harder to read code, if I see the actual throw keyword I immediately know what it does, but if it's behind a macro it's not clear. It makes code shorter, but makes it harder to understand if each module has their own macros like this.

49–66

Personally I think these XR_DEBUG macros obscure more than they help.

73

Also here, I think the abstraction does not help understanding the code, keep it simple. If needed perhaps have a GHOST_XrSwapchain that does the proper cleanup in the constructor, but don't do all this templating and function pointers.

This revision now requires changes to proceed.Mar 5 2020, 7:28 PM
Julian Eisel (Severin) marked 13 inline comments as done.
  • Add comments to C-API function declarations
  • Cleanup: Address various review comments
  • Cleanup: Avoid macros, don't print source location for errors
  • Remove obscure unique_oxr_ptr, implement alternative RAII for swapchain
  • Remove old files/changes, git keeps bringing them back...
intern/ghost/intern/GHOST_XrContext.h
31

I'm mainly following the C++ guidelines syntax (i.e. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-in-class-initializer) but there's no real reason for this mentioned there. So don't mind changing.

For regular variable initializers this is a bit more involved, and I think it's by now suggested to use braces in most cases to avoid some pitfalls. For our C++ style it shouldn't matter much though, so I'll go with the more readable variant (=).

intern/ghost/intern/GHOST_Xr_intern.h
73

I knew that you'd be bothered by this :) It was a bit more useful at some point, now it's only used in one case. So indeed not worth keeping.

Regarding the D3D exposed to WM: Agree, would like to avoid this too. Since managing all the patches/branches is a bit of a nightmare currently, I'd prefer to clean this up once everything is in master though, like you suggested in part 4.

This revision is now accepted and ready to land.Mar 10 2020, 6:26 PM
Julian Eisel (Severin) updated this revision to Diff 22847.EditedMar 16 2020, 9:56 PM
  • Update to master
  • Color transforms were fixed in Windows MR -> disable our workaround
  • Cleanup: Clang-format