Hey, I'm so happy to see you working on this :)

I didn't have time yet to dig too deep, but wanted to give some first feedback at least:
From a few brief looks, the way things are done here makes a lot of sense to me. They are pretty much how I imagined how things could/should look. I'm myself a bit unsure still how the event system integration should work. What you did here seems quite reasonable, but I haven't fully made up my mind on the most future proof approach yet.
I'm also a bit unsure if we should expose a UI for customizing actions yet. As a first step I think we should let add-ons define action sets for their tools/workflows, user-customization could be added later.

But these things we can discuss together. I'd love to chat a bit more directly - this came out of nowhere so I have many questions ;)
Are you on blender.chat? We can discuss in the #blender-coders or #xr channel there.

Also, if you plan to further work on this or other features, we can give you commit access so you can work in a public branch on git.blender.org. That's JFYI, we can discuss this better in the chat.

In D9124#253255, @Julian Eisel (Severin) wrote:

Hey, very sorry this has taken so long! I'm enthusiastic about this. The review process can be tedious, but let's get through it!

Just a first quick glance over the code. Here are some general points:

• My earlier proposal wasn't to make VR actions part of the normal keymap, but to register action-set configs like normal keymap-configs in a .py file. So, for example, there could be a vr_default_action_maps.py as part of the scene inspection add-on. Plus (optionally, don't think it's needed yet), a UI for editing the action set mapping via the Add-on's preferences.
• We should provide default action sets, so the system is ready to go for common devices.
• I don't think there should be a UI for editing actions. At least not by default. We could show this if "Developer Extras" are enabled but it's not something we should expose to regular users.
• The UI shouldn't contain names like "Controller 0"/"Controller 1". Just name it "Controller Left"/"Controller Right".
• Same for the code, I don't see a point in calling something 0/1 that is just left/right. Is there a case where 0/1 is more appropriate? (Other than if the view is upside-down, looking backwards or arms are crossed.)
• Can we find out ourselves which action set to use? So the user doesn't have to select between "oculus"/"wmr"/...
• Is the auto-key design really a good idea? AFAICS the recording framerate will be limited by the drawing framerate. So if the frame rate is poor, the tracking will be really poor as well. I'd expect the tracking to be on an own thread dedicated to purely to the tracking.

Hey, no problem and thanks for your feedback. I'll work on making adjustments over the coming weeks.

In response to your general points:

• VR actions in normal keymap: I actually understood your earlier proposal to register VR actions in a similar, but separate, way to keymap configs. However, I found that I would need to duplicate a lot of the work already done by the keymaps (for example, generating diffs against default configs and visualizing operator properties). Furthermore, it seemed consistent to group XR inputs together with the other types of input mappings (NDOF, Text Input, Timer, etc.). That being said, a separate system for VR actions also seems reasonable.
• Default action sets: I agree that we should provide default action sets. I didn't add any at first in order to make the interface as generic as possible but realize now that they will be necessary for regular users.
• UI for editing actions: Hmm I'd like to give all users the option to modify input mappings for default actions (since they may find a different mapping more preferable/ergonomic) as well as the ability to add their own custom actions (since they may have workflows not covered by the default action sets). Hiding this behind "Developer Extras" seems reasonable though.
• "0/1" vs "left/right": Although in most cases "left/right" will be most appropriate for identifying controllers, I decided to use "0/1" because there may be some cases where a controller or other tracked device may not be strictly left or right (such as a Vive tracker). However, excluding these special cases, I do think that "left/right" is more easily understood.
• Determining device-appropriate action set: In theory, the XR runtime is supposed to automatically select the most appropriate action set (from a list of provided action sets) for the identified device. However, in practice this hasn't worked well for me, so (unless we want to compare known device strings/IDs) I think we should still give users the option to select a specific action set.
• Auto-keying: I agree that the auto-keying has its downfalls as is and should be performed on a separate thread. Even in its current state though, I believe some people (e.g. blendFX) have found it useful for production.

Hi @Peter Kim (muxed-reality) I'm trying to create an own action using this system just to see if I understood how this works correctly. I can make it work with a button which I assign to the screen.animation_play command just fine:

Thanks for all the great efforts here!

@Rainer Trummer (aliasguru) VR navigation is not really supported at the moment, but it is planned for the future (probably in a later patch).

What you can do for now is create a VR landmark of "Custom Object" type and adjust the object's position via a custom operator. If the operator is assigned to a VR action, it will receive events of type XR_ACTION so you can then listen for this input to move the object (and thus the viewer's position).

In addition, the event will have an "xr_state" property, which gives the float value (-1.0 to 1.0) of the input, as well as "xr_controller_location" (xyz) and "xr_controller_rotation" (quaternion) properties that give the corresponding controller pose (same left/right side as the input) at the time the event occurred.

Some people have tried this approach with some success. However, it may be best to wait until VR navigation is properly supported (i.e. once there is a navigation/world matrix that can be applied to VR poses and is accessible via the Python API) .

In D9124#253432, @Peter Kim (muxed-reality) wrote:

• VR actions in normal keymap: I actually understood your earlier proposal to register VR actions in a similar, but separate, way to keymap configs. However, I found that I would need to duplicate a lot of the work already done by the keymaps (for example, generating diffs against default configs and visualizing operator properties). Furthermore, it seemed consistent to group XR inputs together with the other types of input mappings (NDOF, Text Input, Timer, etc.). That being said, a separate system for VR actions also seems reasonable.

I think we can keep this much simpler.
First, like I said I don't think we need customizable action-sets yet. For now having a default action set defined in an editable Python file should be enough. Let's see how this part works first, before doubling down on it.

If in future we want to add customizable action-sets we can indeed do it with a UI like the keymaps and I think we can actually reuse the code for that. E.g. you can call draw_kmi() in rna_keymap_ui.py and either adjust that so it works for action sets too or create temporary "fake" keymap items for the UI drawing. Both should be simple to do. These layout functions do simple things anyway.

Further I don't think we need complex diffing. For keymaps it's only needed because a number of keymaps have to be merged into one, and compatibility is very important. For action sets we can do something simpler since there will be less "layers" involved (2? One base action-set from an add-on or otherwise imported + changes by the user?).

Basically my advice is to not add complexity that isn't clearly needed at this early stage. We can still add such non-core stuff - or we might also end up deciding to go for a different design when we evaluate things later. This is typical for agile development: do just enough to learn enough for the next steps, focus on the essentials at first. It's fine if things will be limited, as long as we deliver something that gets the user from A to B and we didn't design ourselve into a corner.

(Maybe we should call our abstraction of action-sets action-maps? To keep it consistent with key-maps.)

• "0/1" vs "left/right": Although in most cases "left/right" will be most appropriate for identifying controllers, I decided to use "0/1" because there may be some cases where a controller or other tracked device may not be strictly left or right (such as a Vive tracker). However, excluding these special cases, I do think that "left/right" is more easily understood.

Yeah, think so too. I guess left/right would relate to the resting pose, which could be noted clearly in the code/API-docs.

• Determining device-appropriate action set: In theory, the XR runtime is supposed to automatically select the most appropriate action set (from a list of provided action sets) for the identified device. However, in practice this hasn't worked well for me, so (unless we want to compare known device strings/IDs) I think we should still give users the option to select a specific action set.

There's a lot to be said about adding options because it seems like the easiest/safest way forward. It's better to come up with designs that don't need such options.

So the runtime just fails to select the correct action set? Is that only a specific runtime? Maybe it's fixed by now?
I'd say temporarily having a map of known device IDs is fine, until the runtimes handle it properly. If there are issues we might want to report them upstream.

• Auto-keying: I agree that the auto-keying has its downfalls as is and should be performed on a separate thread. Even in its current state though, I believe some people (e.g. blendFX) have found it useful for production.

I think so too but I'd make it a feature that is disabled by default. It could be an add-on preference that somehow notes that it's of limited use.

In general it's good to differentiate between core functionality for the milestone, and features we add just because it's useful to some of our stakeholders (e.g. the autokeying). The latter needs less polish/design and can be moved into a separate add-on or behind a preference of the scene inspection one.

On another note, we may want to consider renaming the add-on once it becomes more than just scene inspection.

There are a number of things we could review in separate patches and move into master already/soon.

For example the Ghost-XR and WM-XR code doesn't need to follow high standards, it's internal, runtime only code. We can always change things in master as needed.
Other things that could be split off:

• Improvements to reference spaces.
• Autokeying from VR session (experimental feature of the add-on?).
• Headset object.
• The changes to support wmOperatorType.modal_3d/.invoke_3d (doesn't have to include the usages, like transform_invoke_3d(), I just want to see if other devs agree on this approach).
• Surface event handling.

Typically I'd create a bunch of branches for this, which is quite a hassle to deal with. But I think it's the fastest way forward.

I created a task to keep track of the patches: T88367: Review & Merge Controller Support Patches. Is there any particular reason to keep this patch open? Guess the fact that the branch is named xr-actions-D9124, but we can simply rename that (suggest xr-controller-support or such).

In D9124#288824, @Julian Eisel (Severin) wrote:

I created a task to keep track of the patches: T88367: Review & Merge Controller Support Patches. Is there any particular reason to keep this patch open? Guess the fact that the branch is named xr-actions-D9124, but we can simply rename that (suggest xr-controller-support or such).

Ok, I renamed the branch to xr-controller-support and updated it with the latest changes from master. Guess there's no need to keep this patch open anymore.