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

Closed
opened 2019-11-05 15:41:01 +01:00 by Julian Eisel · 13 comments
Member

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.
Author
Member

Added subscribers: @JulianEisel, @dfelinto

Added subscribers: @JulianEisel, @dfelinto

Added subscriber: @sebastian_k

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

Added subscriber: @WilliamReynish

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

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

@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 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

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: @jrandymac
Member

Added subscriber: @Blendify

Added subscriber: @Blendify
Member

Changed status from 'Confirmed' to: 'Resolved'

Changed status from 'Confirmed' to: 'Resolved'
Aaron Carlisle self-assigned this 2019-12-28 20:22:23 +01:00
Member

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/

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

Added subscriber: @lemenicier_julien
Thomas Dinges added this to the 2.82 milestone 2023-02-08 16:41:38 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#71365
No description provided.