Details

Reviewers

Julian Eisel (Severin)
Jeroen Bakker (jbakker)
Clément Foucault (fclem)

Commits

rB9dda65455b54: XR Controller Support Step 4: Controller Drawing

Summary

Addresses T77127 (Controller Drawing).

Adds VR controller visualization and custom drawing via draw
handlers. Add-ons can draw to the XR surface and mirror window by
adding a View3D draw handler of region type 'XR' and draw type
'POST_VIEW'. Controller drawing and custom overlays can be toggled
individually as XR session options.

For the actual drawing, the OpenXR XR_MSFT_controller_model extension
is used to load a glTF model provided by the XR runtime. The model's
vertex data is then used to create a GPUBatch in the XR session
state. Finally, this batch is drawn via the XR surface draw handler
mentioned above.

For runtimes that do not support the controller model extension, a
a simple fallback shape (sphere) is drawn instead.

Diff Detail

Repository

rB Blender

Branch

temp-xr-controller-drawing (branched from master)

Build Status

Buildable 16391
Build 16391: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Peter Kim (muxed-reality) edited the summary of this revision. (Show Details)Aug 5 2021, 4:52 PM

Add controller model drawing

Uses the OpenXR XR_MSFT_controller_model extension to load a glTF
model provided by the XR runtime. The model's vertex data is then
used to create a GPUBatch in the XR session state. Finally, this
batch is drawn via an XR surface draw callback.

Harbormaster completed remote builds in B16289: Diff 40462.Aug 9 2021, 10:38 AM

Peter Kim (muxed-reality) edited the summary of this revision. (Show Details)Aug 9 2021, 10:48 AM

Fix crash/assert on loading controller model

When calculating controller model component transforms from glTF node
data, it was previously wrongly assumed that the glTF nodes would
always contain transform matrix values. However, nodes can instead
store their transforms as separate translation/rotation/scale values,
in which case the transform matrix needs to be explicitly calculated.
This was the case for the Reverb G2 controller models using the
Windows Mixed Reality runtime.

Special thanks to Werner Trunk for help with testing/debugging.

Harbormaster completed remote builds in B16391: Diff 40652.Aug 13 2021, 1:20 AM

Support dynamic controller model components

Updating the transforms, or "animating", supported controller
model parts (trigger, grip, thumbstick, etc.) provides better visual
feedback for the user and adds little overhead due to caching of node
transforms/indices.

Harbormaster completed remote builds in B16431: Diff 40724.Aug 16 2021, 4:59 AM

Don't store controller node world transforms

Removes unnecessary storage of world space transforms for controller
model nodes. Now, only local space transforms are stored and the
world transforms are calculated from these local transforms when
needed.

Harbormaster completed remote builds in B16439: Diff 40734.Aug 16 2021, 12:17 PM

Vitalii Vlasov (Fracture128) awarded a token.Aug 20 2021, 10:42 AM

Fix controller model world transforms

Fixes incorrect display of Reverb G2 controller model/animated parts.

Thanks to Werner Trunk for help with debugging.

Harbormaster completed remote builds in B16535: Diff 40903.Aug 20 2021, 2:21 PM

Peter Kim (muxed-reality) mentioned this in rB5a0ec2302eeb: XR: Enable controller profile extensions.Aug 25 2021, 2:38 PM

Peter Kim (muxed-reality) mentioned this in D12344: Add glTF library for loading OpenXR controller model.Aug 30 2021, 6:27 AM

Peter Kim (muxed-reality) mentioned this in D10944: XR Controller Support Step 3: XR Event Handling.Sep 4 2021, 3:46 AM

Some general requests:

Please split the TinyGLTF addition into a separate patch, so it can be reviewed by platform maintainers and get in ASAP.
There's quite some complexity here to introduce the region-type callback, that I'm not so happy about. Jeroen once proposed having a batch in the WM or so, that the draw-manager would simply draw if set. Seems like this would simplify things quite a bit? E.g. GPUViewport could contain a ListBase custom_overlay_batches and we make sure it contains the XR controller batch(es?) when drawing the surfaces. @Jeroen Bakker (jbakker) @Clément Foucault (fclem) could you give some feedback here?

intern/ghost/intern/GHOST_XrControllerModel.h
52	Can't you use a `std::vector` for this? There's a constructor taking iterators, so I think you can construct it like this: `std::vector<uint8_t> data(buffer, buffer + length);`

This revision now requires changes to proceed.Sep 9 2021, 2:31 PM

Julian Eisel (Severin) added subscribers: Clément Foucault (fclem), Jeroen Bakker (jbakker).Sep 9 2021, 2:31 PM

At least one of the GPU module guys should review the drawing/draw-manager code.

source/blender/windowmanager/xr/intern/wm_xr_draw.c
302–304	Using the immediate-mode API here is a no-go I think, since it's not thread save and we need to be able to draw this outside the main thread. Currently it wouldn't cause issues I think, in future it probably would.

In D10948#323837, @Julian Eisel (Severin) wrote:

Please split the TinyGLTF addition into a separate patch, so it can be reviewed by platform maintainers and get in ASAP.

The patch for the glTF extern lib is D12344: Add glTF library for loading OpenXR controller model. There has been some discussion but no one has claimed it for review yet.

intern/ghost/intern/GHOST_XrControllerModel.h
52	Ok thanks, will switch to `std::vector`.

Clément Foucault (fclem) added inline comments.Sep 9 2021, 5:33 PM

source/blender/windowmanager/xr/intern/wm_xr_draw.c
302–304	I'm pretty sure this is incorrect nowadays. There is one `Immediate` object per context and the `imm` global is `thread_local`. So as long as a GL context is not active in 2 threads (which is impossible) we are safe.
304	Use GPU_SHADER_3D_POLYLINE_FLAT_COLOR instead and group your drawcalls. Even if you send more data you do less drawcalls and less buffer mapping which is many time more costly than the few extra bytes of color data you send. Using polyline make sure the result is the same on all platform and is future oriented.
320	Put the `immBegin` and `immEnd` out of the loop and count the number of vertices in before starting to draw.
337	Same thing for this loop.

I have no issue with the drawing being a callback in the draw manager.

For the parts I'm concerned with, excluding what I've commented on, it's good to go.

Refactor immediate-mode drawing

Also, use std::vector instead of std::unique_ptr to receive glTF binary
data.

Harbormaster completed remote builds in B16912: Diff 41646.Sep 10 2021, 9:49 AM

Peter Kim (muxed-reality) marked 4 inline comments as done.Sep 10 2021, 10:03 AM

Peter Kim (muxed-reality) added inline comments.

intern/ghost/intern/GHOST_XrControllerModel.h
52	As it is, there was actually no need to store the glTF binary data as a member variable. However, in the function that accesses the data (`GHOST_XrControllerModel::loadControllerModel()`), `std::unique_ptr` was replaced with `std::vector`. Note that depending on the external glTF library decision, it may be necessary to keep the binary data around as a member variable to pass to WM, RNA, etc.
source/blender/windowmanager/xr/intern/wm_xr_draw.c
304	Using `GPU_SHADER_3D_POLYLINE_FLAT_COLOR` and grouped drawcalls.
320	Moved `immBegin`/`immEnd` out of the loop.

Peter Kim (muxed-reality) marked 2 inline comments as done.Sep 10 2021, 10:49 AM

Clément Foucault (fclem) requested changes to this revision.Sep 10 2021, 1:52 PM

Clément Foucault (fclem) added inline comments.

source/blender/windowmanager/xr/intern/wm_xr_draw.c
304	You could use `GPU_vertformat_attr_add(format, "color", GPU_COMP_U8, 4, GPU_FETCH_INT_TO_FLOAT_UNIT);` to reduce data transfer. But that's just me nitpicking here. :)
328	Specify the color before every vertex. That's clearer and I think will avoid problem under certain situation.
358	You need to use `immAttrSkip(col)` for the vertex you do not specify.

This revision now requires changes to proceed.Sep 10 2021, 1:52 PM

Use u8 color, immAttrSkip() for imm-mode drawing

Harbormaster completed remote builds in B16918: Diff 41656.Sep 10 2021, 2:39 PM

Peter Kim (muxed-reality) marked 3 inline comments as done.Sep 10 2021, 2:46 PM

Peter Kim (muxed-reality) added inline comments.

source/blender/windowmanager/xr/intern/wm_xr_draw.c
304	Using `GPU_COMP_U8` and `GPU_FETCH_INT_TO_FLOAT_UNIT` for vertex color attribute.
328	Ok thanks. Actually I used `immAttrSkip()` here since it appears that both vertex colors didn't need to be specified.
358	Using `immAttrSkip()` for unspecified vertex colors.

For me it's good to go.

In principle I also don't mind using a callback to "inject" the controller drawing. But I'm not a fan at all of doing this by adding a fake region type, and adding a bunch of 3D View options that could easily be dealt with on the XR level. Can't the draw-manager or viewport allow registering custom overlay call-backs that the XR logic can manage?

@Julian Eisel (Severin) I do agree that creating a dedicated function call instead of using a registered callback is preferable. Just doing something like ED_annotation_draw_view3d should be enough and the XR draw routine should decide what to draw.

In D10948#324525, @Clément Foucault (fclem) wrote:

@Julian Eisel (Severin) I do agree that creating a dedicated function call instead of using a registered callback is preferable. Just doing something like ED_annotation_draw_view3d should be enough and the XR draw routine should decide what to draw.

That's fine with me as well.

In D10948#324525, @Clément Foucault (fclem) wrote:

@Julian Eisel (Severin) I do agree that creating a dedicated function call instead of using a registered callback is preferable. Just doing something like ED_annotation_draw_view3d should be enough and the XR draw routine should decide what to draw.

I'd also be fine with a dedicated function call, however I'd like to clarify a bit more about the use of callbacks and a (fake) region type.
The primary motivation was to make it easy for add-ons to draw to the XR surface/viewports and mirror window by extending the existing draw handler API.
In this way, an add-on could register their custom drawing simply by doing bpy.types.SpaceView3D.draw_handler_add(draw_func, (), 'XR', 'POST_VIEW').

It's true that using a dedicated function call will make the XR drawing code more contained, but in that case add-ons will have to register custom overlay callbacks via XrSessionState (or the draw manager, as Julian proposed) using some new API.

Peter Kim (muxed-reality) mentioned this in rB3fa64263927f: Extern: Add TinyGLTF to load XR controller model.Sep 22 2021, 7:59 AM

Remove TinyGLTF additions

TinyGLTF and JSON were added to master separately.

Harbormaster completed remote builds in B17243: Diff 42258.Sep 22 2021, 9:06 PM

I'm still not that happy about the XR region type. It makes sense as a simple way to support custom/scriptable draw callbacks, but a) we could still support this in better ways and b) it's actually not too necessary yet, we could live with a more limited version to start with.
But I'm willing to accept that approach. We could implement things properly later and deprecate the old way to register the draw callback via BPY.

intern/ghost/intern/GHOST_XrControllerModel.cpp
57–58	Use doxygen style comments.
139–141	Use doxygen style comments.
177	Use doxygen style comments.
231	Use doxygen style comments.
source/blender/makesdna/DNA_screen_types.h
669–671	Could you make this comment more elaborate? E.g. that it's not actually used by any real region (or is it?) but just a way to allow internal code and add-ons to register a draw callback that's only executed in the XR context.
source/blender/windowmanager/xr/intern/wm_xr_draw.c
217	Could you split this function up? I'd at least put the model and the aim into separate functions.

Just some smaller tweaks requested.

This revision now requires changes to proceed.Sep 27 2021, 11:34 AM

is this still go for 3.0?

Use doxygen style comments
Improve comment for XR region type
Split controller draw callback into separate model and aim functions

Also, fix controller model loading with TINYGLTF_NO_STB_IMAGE.

Harbormaster completed remote builds in B17519: Diff 42842.Oct 3 2021, 6:45 AM

In D10948#333345, @Jacob Merrill (blueprintrandom) wrote:

is this still go for 3.0?

Yes, this is still planned for 3.0 (before bcon3).

source/blender/makesdna/DNA_screen_types.h
669–671	Added more descriptive comment.
source/blender/windowmanager/xr/intern/wm_xr_draw.c
217	Split function into separate ones for model and aim.

Julian Eisel (Severin) accepted this revision.Oct 11 2021, 5:54 PM

This revision is now accepted and ready to land.Oct 11 2021, 5:54 PM

Closed by commit rB9dda65455b54: XR Controller Support Step 4: Controller Drawing (authored by Peter Kim (muxed-reality)). · Explain WhyOct 12 2021, 9:18 AM

This revision was automatically updated to reflect the committed changes.

Peter Kim (muxed-reality) marked 2 inline comments as done.

Peter Kim (muxed-reality) added a commit: rB9dda65455b54: XR Controller Support Step 4: Controller Drawing.

Peter Kim (muxed-reality) mentioned this in D11501: XR Controller Support Step 5: Navigation.Oct 12 2021, 10:39 AM

Peter Kim (muxed-reality) mentioned this in D11271: VR Add-on: Default Actions, Controller Gizmos.Oct 12 2021, 11:29 AM

Lopo Isaac (lopoIsaac) awarded a token.Oct 19 2021, 11:00 PM

XR Controller Support Step 4: Controller Drawing
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 40652

extern/tinygltf/LICENSE

extern/tinygltf/README.blender

extern/tinygltf/README.md

extern/tinygltf/json.hpp

extern/tinygltf/stb_image.h

extern/tinygltf/stb_image_write.h

extern/tinygltf/tiny_gltf.h

intern/ghost/CMakeLists.txt

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_Types.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_XrContext.h

intern/ghost/intern/GHOST_XrContext.cpp

intern/ghost/intern/GHOST_XrControllerModel.h

intern/ghost/intern/GHOST_XrControllerModel.cpp

intern/ghost/intern/GHOST_XrSession.h

intern/ghost/intern/GHOST_XrSession.cpp

source/blender/draw/intern/draw_manager.c

source/blender/editors/include/ED_space_api.h

source/blender/editors/include/ED_view3d_offscreen.h

source/blender/editors/space_api/spacetypes.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/editors/space_view3d/view3d_draw.c

source/blender/makesdna/DNA_screen_types.h

source/blender/makesdna/DNA_view3d_enums.h

source/blender/makesdna/DNA_view3d_types.h

source/blender/makesdna/DNA_xr_types.h

source/blender/makesrna/intern/rna_screen.c

source/blender/makesrna/intern/rna_xr.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/xr/intern/wm_xr_draw.c

source/blender/windowmanager/xr/intern/wm_xr_intern.h

source/blender/windowmanager/xr/intern/wm_xr_session.c

XR Controller Support Step 4: Controller DrawingClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 40652

extern/tinygltf/LICENSE

extern/tinygltf/README.blender

extern/tinygltf/README.md

extern/tinygltf/json.hpp

extern/tinygltf/stb_image.h

extern/tinygltf/stb_image_write.h

extern/tinygltf/tiny_gltf.h

intern/ghost/CMakeLists.txt

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_Types.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_XrContext.h

intern/ghost/intern/GHOST_XrContext.cpp

intern/ghost/intern/GHOST_XrControllerModel.h

intern/ghost/intern/GHOST_XrControllerModel.cpp

intern/ghost/intern/GHOST_XrSession.h

intern/ghost/intern/GHOST_XrSession.cpp

source/blender/draw/intern/draw_manager.c

source/blender/editors/include/ED_space_api.h

source/blender/editors/include/ED_view3d_offscreen.h

source/blender/editors/space_api/spacetypes.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/editors/space_view3d/view3d_draw.c

source/blender/makesdna/DNA_screen_types.h

source/blender/makesdna/DNA_view3d_enums.h

source/blender/makesdna/DNA_view3d_types.h

source/blender/makesdna/DNA_xr_types.h

source/blender/makesrna/intern/rna_screen.c

source/blender/makesrna/intern/rna_xr.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/xr/intern/wm_xr_draw.c

source/blender/windowmanager/xr/intern/wm_xr_intern.h

source/blender/windowmanager/xr/intern/wm_xr_session.c

XR Controller Support Step 4: Controller Drawing
ClosedPublic
Actions

Revision Contents
Changeset List