KitwareMedical / SlicerVirtualReality

A Slicer extension that enables user to interact with a Slicer scene using virtual reality.
Apache License 2.0
119 stars 58 forks source link

Decouple update of transform node matrix and attributes #147

Closed jcfr closed 10 months ago

jcfr commented 10 months ago

The following changes are done anticipating subsequent commits introducing support for both OpenVR and OpenXR


Generalize updates of node attributes to all type of devices and introduce two distinct functions:

All transform nodes are now consistently set with these attributes:

where <DeviceType> may be "HMD", "Controller" or "Tracker"


Simplify introducing CreateDefaultTrackerTransformNode API. Similarly to HMD, left and right controllers, this commit updates the vtkMRMLVirtualRealityViewNode API to support creating default transform nodes associated with each generic trackers.

jcfr commented 10 months ago

Built and tested locally :white_check_mark:

Note that due to the lack of HTC vive hardware, the changes related to the support for generic trackers couldn't be tested.

lassoan commented 10 months ago

I'm just wondering if device type is one of "HMD", "Controller" or "Tracker" then why don't we add a VirtualReality.DeviceType attribute and then the same VirtualReality.DeviceActive and VirtualReality.DeviceConnected attributes could be used to indicate that the transform is valid. With the API proposed in this PR you need to do know what device types are possible and then do trial-and-error to find an attribute that can tell if the status is valid. The attributes can also be contradicting (if you have both HMD and Controller status attributes then which ones should be used?).

I works consider making transform status a Slicer core feature (a proper property of transform node). It would be used in SlicerIGT extensively, but it could be very useful without that, too, just to temporarily disable a transform to quickly see the transformed nodes with/without the transformation in effect. Transformable node display node could tell what to do if transform is invalid: use the current transform but make the node appear more transparent and/or different color: use identity as transform; or ignore transform status for display.

jcfr commented 10 months ago

Adding VirtualReality.DeviceType and VirtualReality.DeviceActive

This makes sense.

Also, since OpenXR runtime doesn't provide such a low level of granularity, we will likely only have VirtualReality.DeviceActive (the PoseValid, PoseStatus, and Connected attribute will not be available)

re: need to do know what device types are possible and then do trial-and-error to find an attribute

Agreed. In the context of this PR, I intended to keep the same attributes that were introduced.

jcfr commented 10 months ago

Transformable node display node could tell what to do if transform is invalid: use the current transform but make the node appear more transparent and/or different color: use identity as transform; or ignore transform status for display.

This makes sense. Additionally, we could have the idea of the transformable object being Hovered or Picked and it would be represented differently.

pieper commented 10 months ago

just to temporarily disable a transform to quickly see the transformed nodes with/without the transformation in effect

+1 for this - I would love to have a hotkey to toggle the status of transforms. Currently I do this with the transforms context menu in the Data module but it's a multi-click and error prone operation.