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

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

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

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

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_`.

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.