Page MenuHome
Differential D7098

VR: Milestone 1 - Scene Inspection - Features
ClosedPublic
Actions

Authored by Julian Eisel (Severin) on Mar 10 2020, 8:39 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Maniphest Tasks
T71347: Virtual Reality - Milestone 1 - Scene Inspection
Summary

VR: Milestone 1 - Scene Inspection - Features

NOTE: This doesn't fully satisfy the milestone 1 goals yet, see check-list in T71347. The remaining bits can be addressed once this is in master, during Bcon2.

Patch on top of D6193.

This is the C-side implementation of the features added to the existing VR support to fulfill milestone 1. Part of the needed features are implemented in an Add-on, which will be submitted separately.

Main changes/features:

  • Option to let a regular viewport show the state (viewer position and rotation) of the VR view (VR-Mirror). The viewport remains fully interactive; data is purely runtime.
  • Improved RegionView3D locking to prevent navigation from a VR-mirror. Also support runtime-only locking, so that locking flags are removed on file read (if wanted).
  • Option to disable positional tracking. Keeps the current position (calculated based on the VR centroid pose) when enabled while a VR session is running.
  • Option for gizmo-groups to continuously redraw. Needed for virtual VR camera gizmo (see T71347). Currently this continuosly redraws the entire region, optimizing this is a separate project (see T73198).
  • Support all regular 3D-View shading options for VR views.
  • Improved management of the VR reference pose, supporting custom objects to derrive reference pose from and entirely custom location/rotation input.
  • RNA/Python-API to query and set VR session state information.
  • Notifiers for XR data changes, to trigger redraws as needed.

Diff Detail

Repository
rB Blender
Branch
vr_scene_inspection
Build Status
Buildable 7105
Build 7105: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) created this revision.Mar 10 2020, 8:39 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)
Julian Eisel (Severin) added inline comments.Mar 10 2020, 8:44 PM
build_files/utils/make_update.py
161

This is obviously just for testing and should not end up in master.

release/scripts/startup/bl_ui/space_view3d.py
5491–5504

I'm not sure about the trickery I did here with the _ex functions. It's done to show the 3D View shading options for the VR add-on. I'll probably do this differently and use utils.py instead.

Julian Eisel (Severin) added a task: T71347: Virtual Reality - Milestone 1 - Scene Inspection.Mar 11 2020, 12:49 AM
Julian Eisel (Severin) mentioned this in D7100: VR: "Basic VR Viewer" Add-on to expose VR features in the UI.Mar 11 2020, 1:46 AM
Brecht Van Lommel (brecht) requested changes to this revision.Mar 11 2020, 10:29 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenloader/intern/readfile.c
7584–7597

File reading and writing should never be conditional on build options. Otherwise it's not clear that reading and writing a file with XR settings produces a valid result when building without XR.

source/blender/editors/space_view3d/space_view3d.c
703

Does this need an #ifdef at all?

source/blender/editors/space_view3d/view3d_draw.c
344

Drawing code is not the ideal place to set this, can you make an ED_view3D_something function to set/unset this on XR session start/end and V3D_XR_SESSION_MIRROR changes?

1793

This seems like it could affect seq_render_scene_strip?

source/blender/editors/space_view3d/view3d_utils.c
884

Can this be a utility function or something, or is there a more explicit check?

Why is this new code needed here?

source/blender/makesdna/DNA_view3d_types.h
376

Rename to RV3D_LOCK_ZOOM_AND_DOLLY?

380

If some settings are runtime only, I think the better solutions is to put them in a separate runtime_viewlock and always clear that. Mixing user defined settings and runtime settings seems error prone.

source/blender/makesdna/DNA_xr_types.h
28

This contains an IDProperty that may need to be freed, I don't see that anywhere.

34

Pointing to scene data from the window manager probably needs special handling to survive undo and loading different scenes into an existing UI. I don't see the code for that.

I could find this for example in readfile.c

v3d->camera = restore_pointer_by_name(id_map, (ID *)v3d->camera, USER_REAL);
source/blender/makesrna/intern/rna_space.c
1343–1350

Don't use RNA_pointer_get() for things like this.

Can you move this code into editors/space_view3d next to the code that sets these locking flags? Code to enable/disable state like that should be in the same place with some clear symmetry, and not spread out over different modules.

4211

show_as_xr_session_mirror-> mirror_xr_session?

4215

VR Session Mirror -> Mirror VR Session? We usually start with the verb if there is one.

4216

Suggested description:

Mirror the camera perspective of virtual reality sessions in this 3D viewport

source/blender/makesrna/intern/rna_wm_gizmo.c
1383

"Main loop iteration" does not mean anything specific in the Python API as far as I know.

Is this just redrawing every time the viewport redraws?

source/blender/makesrna/intern/rna_xr.c
67

Value should not be left uninitialized.

77

Value should not be left uninitialized.

98

an transformation -> the transformation

109

XR-Session -> XR Session

120

What are the reference pose and base pose? Are they the same thing? The descriptions don't seem to explain it.

123–128

Would this typically be a camera object?

131

Which location is this, in the center between the eyes?

153

VR View -> VR viewport

165–166

This description seems flipped to me? And should this explain what those 3 degrees of freedom are?

Will this be greyed out in the interface when the device does not support it?

183

Get the WM from ptr->owner_id and make this a property?

It's strange that is_running needs a context, but then viewer_location is available without context.

195

Is there a difference between "pose" and "viewer", or can the naming be tweaked somehow to clarify the correspondence between them.

Also not sure if centroid is the right term, would use "center between the eyes"

204

Rotation -> rotation

source/blender/windowmanager/intern/wm_operators.c
3665

Same comment as for RNA, this code to toggle view locks should not be spread out over 3 different modules.

source/blender/windowmanager/intern/wm_xr.c
243

Should this quat have M_PI_2 rotation over X like the others?

288–295

Is this converting between Y-up and Z-up? Could leave a comment about that.

311

Value should not be left uninitialized.

320

Value should not be left uninitialized.

381

If you're going to access wm directly, don't have xr_context_ptr as an argument as well, just get it directly from wm.

WM_xr_session_is_running checks wm->xr.content, so if you were to pass in another context there would be a mismatch.

390

WM_xr_session_is_running doesn't mean the session has actually ended, so wm->xr.session_state` is not guaranteed to be NULL here?

399

sepecification -> specification

source/blender/windowmanager/wm.h
107–108

Some symmetry in function naming is helpful I think. Like wm_xr_init() and wm_xr_exit?

"XR data" is vague to me anyway, I can't tell what that covers.

This revision now requires changes to proceed.Mar 11 2020, 10:29 AM
Julian Eisel (Severin) mentioned this in D6193: Core XR Support [part 4]: Blender-side changes (+ the remaining bits).Mar 11 2020, 3:42 PM
Julian Eisel (Severin) updated this revision to Diff 22703.Mar 11 2020, 10:22 PM
Julian Eisel (Severin) marked 28 inline comments as done.
  • Cleanup, naming, comments, etc
  • Fix missing IDProperty freeing for View3DShading settings
  • Fix possible error with base pose object on undo or Load UI disabled
  • Fix uninitialized variables when WITH_XR_OPENXR is disabled
  • Keep VR view grid aligned, even if there's no camera
  • Don't return uninitialized values when early-exiting
  • Cleanup: Rename zoom/dolly lock flag
  • Refactor RegionView3D runtime lock flag handling
Harbormaster completed remote builds in B7105: Diff 22703.Mar 11 2020, 10:22 PM
Julian Eisel (Severin) added a comment.Mar 11 2020, 10:26 PM

Most review comments should be addressed now, the rest will follow tomorrow. The "Done" checkboxes in inline comments show what's left to address still.

source/blender/editors/space_view3d/view3d_draw.c
1793

Don't see how. Sequencer uses ED_view3d_draw_offscreen_imbuf_simple(), this is ED_view3d_draw_offscreen_simple(), which is only called from wm_xr.c.

source/blender/makesrna/intern/rna_space.c
4216

Did a slight adjustment. Wanted to avoid the term "camera" since that's used for something different already.

source/blender/makesrna/intern/rna_wm_gizmo.c
1383

It really means that we tag the viewport for redraw each main loop iteration, see wm_region_test_gizmo_do_draw(). Changed the comment.

I was thinking about ways to throttle redraws to not impact performance too much, but that may result in inconsistent frame rates for the HMD. So rather redraw every time the VR view redraws (each main loop iteration).

It's the reason why I worked on decoupling gizmo redraws from the viewport, see T73198.

source/blender/makesrna/intern/rna_xr.c
120

They are the same thing. I'm just not sure which name is better, so I ended up mixing them up a bit. I welcome suggestions for a better name.

Tried to clarify comments a bit, but the reference pose vs. base pose vs. whatever is still an open question for me.

123–128

Yes, but I don't see a reason to restrict it in the API. It may be useful to use empties for example.

131

Clarified descriptions a bit.

165–166

Clarified.
AFAIK we can't tell currently if a device/driver supports positional tracking. The option to disable positional tracking is handled entirely on our end, not using OpenXR.

183

This is currently used as a static method (FUNC_NO_SELF), XrSessionState is only allocated when the session is started. We could add an additional None-check in .py, not sure what's nicer.

195

In OpenXR terms, a "pose" is just any combination of location and rotation info. When data is attached to the "centroid of view origins" (for stereo usage), the term "viewer" is used, e.g. the viewer space or viewer pose. So I guess "viewer pose" would match the OpenXR terminology here.

https://www.khronos.org/registry/OpenXR/specs/1.0/html/xrspec.html#reference-spaces

Julian Eisel (Severin) updated this revision to Diff 22726.Mar 12 2020, 2:46 PM
Julian Eisel (Severin) marked 5 inline comments as done.
  • Refactor XR mirror toggling
  • Refactor how the user region is obtained in quad view
  • Fix XR toggle possibly starting already started session
Harbormaster completed remote builds in B7120: Diff 22726.Mar 12 2020, 2:46 PM
Brecht Van Lommel (brecht) added inline comments.Mar 13 2020, 1:58 PM
source/blender/blenloader/intern/writefile.c
2814

Remove this #ifdef, always write this data.

source/blender/makesrna/intern/rna_wm_gizmo.c
1383

Maybe rename to ALWAYS_REDRAW and change the description to Redraw the gizmo group whenever the viewport is redrawn?

To me continuous implies it's somehow causing the viewport to be redrawn continuously, while as I understand it, it always redraws the gizmo when the viewport is redrawn but does not cause any redraws itself.

source/blender/makesrna/intern/rna_xr.c
120

We should use one main name in any case. I prefer "base pose" over "reference pose", to me it's a little more descripitive that this is the pose VR starts from.

"start pose" or "starting pose" could be an alternative, but I'm not sure it's better? It sounds a bit less technical, but perhaps isn't clear enough that depending on the degrees of freedoms part of that pose might never change.

195

I like naming these properties viewer_pose_* then, for consistency with base_pose_.

Julian Eisel (Severin) updated this revision to Diff 22850.Mar 16 2020, 10:14 PM
Julian Eisel (Severin) marked 5 inline comments as done.

Did quite some further polish and general fixes. Plus I fixed some issues with
shutting down the session. E.g. when closing the session through the OpenXR
runtime, we wouldn't correctly unlock the VR mirror 3D View.

  • Remove unnecessary code, forgot to remove earlier
  • Fix division-by-zero on session start with VR mirror enabled
  • Refactor sharing of 3D View Shading layout
  • Force continuous redraws of mirrored 3D Views
  • Use preview quality settings for Eevee in VR, not high-quality ones
  • Fix broken Positional Tracking toggle behavior
  • Fix crash with some 3D View operators (e.g. Zoom to Border)
  • Refactor runtime data storage, for OpenXR side session ending
  • Cleanup: Change misleading name
  • Fix multiple issues when closing session from OpenXR runtime
  • Cleanup: Improve naming of session state queries
  • Fix crash when closing Blender with active VR session
  • Enable fixed world space lighting by default

From review:

  • Always read/write XR data, even with WITH_XR_OPENXR disabled
  • Cleanup: Rename gizmo-group-type flag for continuous redraws
  • Cleanup: Use "Viewer Pose" not just "Viewer"
Harbormaster completed remote builds in B7183: Diff 22850.Mar 16 2020, 10:14 PM
Julian Eisel (Severin) added inline comments.Mar 16 2020, 10:16 PM
source/blender/editors/space_view3d/view3d_utils.c
884

This synced some viewlock flags and which caused issues with the new flags I added. The simple solution was to skip the user region that we added new lock flags to.
Now with runtime_viewlock that's not needed anymore.

source/blender/makesrna/intern/rna_wm_gizmo.c
1383

We chatted in person and decided to go with a name suggesting that this is specific to VR, which it is.

Brecht Van Lommel (brecht) accepted this revision.Mar 16 2020, 11:02 PM
This revision is now accepted and ready to land.Mar 16 2020, 11:02 PM
Julian Eisel (Severin) mentioned this in rBdc2df8307f41: VR: Initial Virtual Reality support - Milestone 1, Scene Inspection.Mar 17 2020, 9:43 PM
Julian Eisel (Severin) closed this revision.Mar 17 2020, 9:54 PM

Committed rBdc2df8307f41 :)

Revision Contents
Changeset List

PathSize
3 lines
build_files/
utils/
2 lines
intern/
ghost/
3 lines
intern/
2 lines
2 lines
1 line
38 lines
release/
scripts/
startup/
bl_ui/
4 lines
67 lines
source/
blender/
blenkernel/
intern/
6 lines
blenloader/
intern/
36 lines
4 lines
10 lines
draw/
intern/
2 lines
editors/
include/
3 lines
screen/
3 lines
sculpt_paint/
13 lines
space_view3d/
4 lines
18 lines
67 lines
130 lines
2 lines
35 lines
25 lines
18 lines
2 lines
gpu/
3 lines
intern/
25 lines
makesdna/
23 lines
5 lines
29 lines
makesrna/
1 line
intern/
4 lines
32 lines
7 lines
6 lines
170 lines
windowmanager/
10 lines
1 line
gizmo/
2 lines
intern/
5 lines
intern/
9 lines
43 lines
34 lines
341 lines
6 lines

Diff 22703

View Options

CMakeLists.txt

Loading...
View Options

build_files/utils/make_update.py

Loading...
View Options

intern/ghost/GHOST_Types.h

Loading...
View Options

intern/ghost/intern/GHOST_IXrGraphicsBinding.h

Loading...
View Options

intern/ghost/intern/GHOST_XrGraphicsBinding.cpp

Loading...
View Options

intern/ghost/intern/GHOST_XrSession.h

Loading...
View Options

intern/ghost/intern/GHOST_XrSession.cpp

Loading...
View Options

release/scripts/startup/bl_ui/properties_render.py

Loading...
View Options

release/scripts/startup/bl_ui/space_view3d.py

Loading...
View Options

source/blender/blenkernel/intern/object.c

Loading...
View Options

source/blender/blenloader/intern/readfile.c

Loading...
View Options

source/blender/blenloader/intern/versioning_280.c

Loading...
View Options

source/blender/blenloader/intern/writefile.c

Loading...
View Options

source/blender/draw/intern/draw_manager.c

Loading...
View Options

source/blender/editors/include/ED_view3d.h

Loading...
View Options

source/blender/editors/screen/screen_ops.c

Loading...
View Options

source/blender/editors/sculpt_paint/paint_cursor.c

Loading...
View Options

source/blender/editors/space_view3d/CMakeLists.txt

Loading...
View Options

source/blender/editors/space_view3d/space_view3d.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_draw.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_edit.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_fly.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_gizmo_navigate.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_utils.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_view.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_walk.c

Loading...
View Options

source/blender/gpu/GPU_viewport.h

Loading...
View Options

source/blender/gpu/intern/gpu_viewport.c

Loading...
View Options

source/blender/makesdna/DNA_view3d_types.h

Loading...
View Options

source/blender/makesdna/DNA_windowmanager_types.h

Loading...
View Options

source/blender/makesdna/DNA_xr_types.h

Loading...
View Options

source/blender/makesrna/RNA_access.h

Loading...
View Options

source/blender/makesrna/intern/CMakeLists.txt

Loading...
View Options

source/blender/makesrna/intern/rna_space.c

Loading...
View Options

source/blender/makesrna/intern/rna_wm.c

Loading...
View Options

source/blender/makesrna/intern/rna_wm_gizmo.c

Loading...
View Options

source/blender/makesrna/intern/rna_xr.c

Loading...
View Options

source/blender/windowmanager/WM_api.h

Loading...
View Options

source/blender/windowmanager/WM_types.h

Loading...
View Options

source/blender/windowmanager/gizmo/WM_gizmo_types.h

Loading...
View Options

source/blender/windowmanager/gizmo/intern/wm_gizmo_map.c

Loading...
View Options

source/blender/windowmanager/intern/wm.c

Loading...
View Options

source/blender/windowmanager/intern/wm_draw.c

Loading...
View Options

source/blender/windowmanager/intern/wm_operators.c

Loading...
View Options

source/blender/windowmanager/intern/wm_xr.c

Loading...
View Options

source/blender/windowmanager/wm.h

Loading...