GSoC 2019: Core Support of VR Headsets through OpenXR -- Review #71365

New Issue

Parent task to manage review of the Core Support of VR Headsets through OpenXR GSoC 2019 project. All these changes are a target for 2.82.
Relevant patches are attached.

The project aimed at bringing stable, well performing VR rendering support to Blender, based on the new OpenXR specification. Further, debugging utilities should be added. A fully fledged VR experience, e.g. with support for editing 3D content with controllers, was not in scope of the project.

How to Test

NOTE: Requries an Add-on. Get this by checking out the soc-2019-openxr branch in the Add-on repository.

Testing this patch isn't as simple as applying it and compiling with it. Information on how to test can be found here.
In short, the following is needed:

Install an OpenXR runtime.
Checkout the soc-2019-openxr Add-ons branch.
Install/build the OpenXR-SDK (already bundled with precompiled Windows libs, use install_deps.sh on Linux).
After a successful build, enable Basic VR Viewer Add-on.
Launch the session through {nav Window > Toggle VR Session}.

Main Features

OpenXR loader from the OpenXR SDK to connect to the System’s active OpenXR runtime.
OpenXR extension (and API-layer) management.
Basic OpenXR event management.
VR session management.
Well performing VR rendering - more performance improvements are possible, but we have a quite decent baseline.
Carefully designed error handling strategy, cancelling the VR session with a useful user error message (e.g. “Failed to get device information. Is a device plugged in?") and no side-effects to the rest of Blender.
Compatibility with DirectX-only runtimes.
--debug-xr command line option enabling our own debug/information prints, OpenXR debug prints and the OpenXR core validation layer.
--debug-xr-time command line option to print frame render times and FPS information.
wmSurface API to manage offscreen drawables without a wmWindow.
Abstraction (currently a GHOST_Xr-API) for all OpenXR specific code. Makes higher level usage easier, but most importantly, improves maintenance (esp. when updating OpenXR versions).
Add-on to hide VR features by default from the UI.

If needed I can explain features in more detail, for now keeping it short.

Visibility for Users

Showing a "Toggle VR Session" button in the UI by default may fool users into thinking there was full fledged VR support in Blender. To not disappoint users with false promises, I wrapped this button into an Add-on which is disabled by default. The Add-on description clearly warns that support is limited and an early preview.

Parent task to manage review of the *Core Support of VR Headsets through OpenXR* GSoC 2019 project. All these changes are a target for 2.82. Relevant patches are attached. The project aimed at bringing stable, well performing VR rendering support to Blender, based on the new [OpenXR specification](https://www.khronos.org/openxr). Further, debugging utilities should be added. A fully fledged VR experience, e.g. with support for editing 3D content with controllers, was not in scope of the project. ## How to Test NOTE: Requries an Add-on. Get this by checking out the [`soc-2019-openxr`](https://developer.blender.org/diffusion/BA/history/soc-2019-openxr/) branch in the Add-on repository. Testing this patch isn't as simple as applying it and compiling with it. Information on how to test can be found [here](https://wiki.blender.org/wiki/User:Severin/GSoC-2019/How_to_Test). In short, the following is needed: * Install an OpenXR runtime. * Checkout the `soc-2019-openxr` Add-ons branch. * Install/build the OpenXR-SDK (already bundled with precompiled Windows libs, use `install_deps.sh` on Linux). * After a successful build, enable *Basic VR Viewer* Add-on. * Launch the session through {nav Window > Toggle VR Session}. ## Main Features * OpenXR loader from the OpenXR SDK to connect to the System’s active OpenXR runtime. * OpenXR extension (and API-layer) management. * Basic OpenXR event management. * VR session management. * Well performing VR rendering - more performance improvements are possible, but we have a quite decent baseline. * Carefully designed error handling strategy, cancelling the VR session with a useful user error message (e.g. “Failed to get device information. Is a device plugged in?") and no side-effects to the rest of Blender. * Compatibility with DirectX-only runtimes. * `--debug-xr` command line option enabling our own debug/information prints, OpenXR debug prints and the OpenXR core validation layer. * `--debug-xr-time` command line option to print frame render times and FPS information. * `wmSurface` API to manage offscreen drawables without a `wmWindow`. * Abstraction (currently a `GHOST_Xr`-API) for all OpenXR specific code. Makes higher level usage easier, but most importantly, improves maintenance (esp. when updating OpenXR versions). * Add-on to hide VR features by default from the UI. If needed I can explain features in more detail, for now keeping it short. ## Visibility for Users Showing a "Toggle VR Session" button in the UI by default may fool users into thinking there was full fledged VR support in Blender. To not disappoint users with false promises, I wrapped this button into an Add-on which is disabled by default. The Add-on description clearly warns that support is limited and an early preview.

Added subscribers: @JulianEisel, @dfelinto

Added subscriber: @sebastian_k

Hi Julian, may I suggest you to remove the reviewers from the patch until you (or rather @sebastian_k or me?) call the patch as done, fully complying to Milestone 1. Or add us both as reviewer and once Sebastian greenlights it we move to assign reviewers for the code itself.

We shouldn't waste time with patch review (in particular the part 4) if you think you still will need to do some changes before the patch is ready to merge.

Nice work on splittling up the patch by the way.

Hi Julian, may I suggest you to remove the reviewers from the patch until you (or rather @sebastian_k or me?) call the patch as done, fully complying to Milestone 1. Or add us both as reviewer and once Sebastian greenlights it we move to assign reviewers for the code itself. We shouldn't waste time with patch review (in particular the part 4) if you think you still will need to do some changes before the patch is ready to merge. Nice work on splittling up the patch by the way.

Added subscriber: @WilliamReynish

Friendly poke to @WilliamReynish to sign of on the proposed UI for the settings.

@dfelinto I don't know why we should hold back review or merges until everything's there for the milestone? From what I can tell there are only minor changes needed (e.g. get start position from camera in a more useful way) and additions for the navigation - nothing that conflicts with the current patch scope really.

To be clear, my idea was merging the individual patches in order as soon as they are ready. E.g. once the build system stuff is approved, merge it even if it's not used yet. From our dev meetings here I got that this is the generally agreed on way of managing big patches.

Not totally against your suggestion, it just sounds like delaying the review for no real benefit from my perspective. Especially given that the feature will be disabled by default until it's more useful (right?).

@WilliamReynish this is the current settings UI: Not a big fan of it myself, longer term we should look into better solutions. For now this is added through an Add-on, which I think makes this acceptable.

@dfelinto I don't know why we should hold back review or merges until everything's there for the milestone? From what I can tell there are only minor changes needed (e.g. get start position from camera in a more useful way) and additions for the navigation - nothing that conflicts with the current patch scope really. To be clear, my idea was merging the individual patches in order as soon as they are ready. E.g. once the build system stuff is approved, merge it even if it's not used yet. From our dev meetings here I got that this is the generally agreed on way of managing big patches. Not totally against your suggestion, it just sounds like delaying the review for no real benefit from my perspective. Especially given that the feature will be disabled by default until it's more useful (right?). @WilliamReynish this is the current settings UI: ![screenshot.png](https://archive.blender.org/developer/F7949216/screenshot.png) Not a big fan of it myself, longer term we should look into better solutions. For now this is added through an Add-on, which I think makes this acceptable.

Added subscriber: @Jaydead

Hi @JulianEisel,

The way I see it, once we send the patch for review we may expect the developer to respond to the review suggestions asap (same way the developer expects the review to happen soon). But if there is still anything pending in the patch itself, now the developer will have to decided whether to address the new raised issues, or keep working on finishing it. Ultimately this split attention may slow the review process more than speed up by sending things ahead before the entire system is working.

Also, if the patch as a whole is not ready, there is no real benefit to have it merged. Unless merge conflicts are getting on the way, is this the case?

And breaking down big patches (thumbs up) is different than sending an unfinished patch for review. You can separate the backend from the frontend and have the backend be reviewed and merged separately. But as I mentioned, in that case I would expected at least the patch #4 to not be up for review just yet (or have it there but not assigned to anyone).

Especially given that the feature will be disabled by default until it's more useful (right?).

If we get the milestone 1 in place, it is useful enough I believe. Sure we can /(should?) hide it under experimental (leaving the addon enabled). I'm even fine to get this figured out after the merge. But either way if we are merging something we presume it is useful, otherwise what is the point?

Small note in that regard. Unlike the other scenarios, the milestone 1 brings the interface that would be present in all the other milestones as well. In fact anything we do on the non-vr side should likely be common and exposed the same to any thinkable scenario, thus not part of the addon (considering that any new milestone can be delivered as a new add-on/template).

Hi @JulianEisel, The way I see it, once we send the patch for review we may expect the developer to respond to the review suggestions asap (same way the developer expects the review to happen soon). But if there is still anything pending in the patch itself, now the developer will have to decided whether to address the new raised issues, or keep working on finishing it. Ultimately this split attention may slow the review process more than speed up by sending things ahead before the entire system is working. Also, if the patch as a whole is not ready, there is no real benefit to have it merged. Unless merge conflicts are getting on the way, is this the case? And breaking down big patches (thumbs up) is different than sending an unfinished patch for review. You can separate the backend from the frontend and have the backend be reviewed and merged separately. But as I mentioned, in that case I would expected at least the patch #4 to not be up for review just yet (or have it there but not assigned to anyone). > Especially given that the feature will be disabled by default until it's more useful (right?). If we get the milestone 1 in place, it is useful enough I believe. Sure we can /(should?) hide it under experimental (leaving the addon enabled). I'm even fine to get this figured out after the merge. But either way if we are merging something we presume it is useful, otherwise what is the point? Small note in that regard. Unlike the other scenarios, the milestone 1 brings the interface that would be present in all the other milestones as well. In fact anything we do on the non-vr side should likely be common and exposed the same to any thinkable scenario, thus not part of the addon (considering that any new milestone can be delivered as a new add-on/template).

Added subscriber: @jrandymac

Added subscriber: @Blendify

Changed status from 'Confirmed' to: 'Resolved'

Hi thank you for submitting a patch, unfortunately, we no longer use the task subtype "Patch" please submit new patches through the differential tool: https://developer.blender.org/differential/

Added subscriber: @lemenicier_julien

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GSoC 2019: Core Support of VR Headsets through OpenXR -- Review #71365

How to Test

Main Features

Visibility for Users