Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring to support OpenVR and OpenXR, and streamline reuse in SlicerMixedReality #121

Closed
5 tasks
jcfr opened this issue Aug 28, 2023 · 8 comments
Closed
5 tasks

Comments

@jcfr
Copy link
Contributor

jcfr commented Aug 28, 2023

The refactoring is intended to help with:

  • supporting both OpenVR and OpenXR in the context of this extension (SlicerVirtualReality)
  • allowing code reuse within SlicerMixedReality extension

Approach

Similarly to the VTK refactoring introducing vtkRenderingVR common to both vtkRenderingOpenVR and vtkRenderingOpenXR, proposed approach is the following:

  • Create vtkMRMLAbstractVRViewNode that will be common ancestor to vtkMRMLVirtualRealityViewNode and vtkMRMLMixedRealityViewNode
  • Create vtkSlicerAbstractVRLogic providing generic methods like the following:
    • AddVRViewNode
    • GetVRViewNode
    • GetDefaultVRViewNode
    • GetVRConnected / SetVRConnected(bool connect)1
    • GetVRActive / SetVRActive(bool activate)
    • SetDefaultReferenceView
    • SetVolumeRenderingLogic
  • Create reusable widget qSlicerVRConnectionPropertiesWidget2, qSlicerVRDisplayPropertiesWidget (and qSlicerVRAdvancedPropertiesWidget?)
  • Create qMRMLAbstractVRView (common to qMRMLVirtualRealityView and qMRMLMixedRealityView)
    • The view will support running using either the OpenVR or OpenXR runtime
    • qMRMLAbstractVRView will provide generic methods like the following:
      • addDisplayableManager / getDisplayableManagers
      • setCamerasLogic / camerasLogic
      • mrmlAbstractVRViewNode
      • renderer / renderWindow / interactor
      • updateViewFromReferenceViewCamera 3
      • isHardwareConnected 4
      • setActionManifestPath / actionManifestPath
    • qMRMLAbstractVRViewPrivate will provide generic methods like the following:
      • startVR / stopVR
      • desiredUpdateRate / stillUpdateRate
      • doVR
      • createRenderWindow / destroyRenderWindow
  • Create vtkSlicerAbstractVRViewInteractorStyle

Footnotes

  1. To support "remoting", the function vtkSlicerMixedRealityLogic::SetVRConnected(bool connect, const std::string& playerIPAddress) will be changed to get/set the player IP using vtkSlicerMixedRealityLogic::GetPlayerIPAddress/SetPlayerIPAddress

  2. To support "remoting", the SlicerMixedReality extension will instead implement qSlicerVRRemoteConnectionPropertiesWidget

  3. Will have to define what this mean in the context of mixed reality

  4. Since OpenXR introduce the concept of XR instance and session, we may have to revisit the terminology

@jcfr
Copy link
Contributor Author

jcfr commented Aug 28, 2023

cc: @lassoan @cpinter @adamrankin

@tbirdso
Copy link

tbirdso commented Sep 12, 2023

Following discussion with @jcfr in #127, I suggest that we need to come to a common definition of terms before we can move forward with organizing proposed XR feature sets among the SlicerVirtualReality and SlicerMixedReality modules. Specifically, what do "virtual reality" and "mixed reality" mean in the context of 3D Slicer?

To be clear, both terms have been used in a variety of ways in research and industry.

  • Most commonly, "mixed reality" refers to a wide spectrum of approaches to blend physical and digital experiences, and encompasses both "virtual reality" and "augmented reality". Meanwhile, "virtual reality" refers to immersive experiences in which a user provides 3D spatial inputs (traversal, head motion, hand gestures) and similarly perceives output in a surrounding virtual space in real time, usually by way of a Head-Mounted Display (HMD).
  • Less commonly, "virtual reality" may simply refer to a digital scene composed of 3D objects.
  • Less commonly, "mixed reality" is sometimes used interchangeably with augmented reality in which digital objects are overlaid on real-world physical scenes.

It is particularly important to establish what these terms mean when we use them in 3D Slicer because underlying libraries use the same terms to mean different things.

  • OpenXR is a standard for "mixed reality" (XR) that aims to support both virtual and augmented reality experiences
  • VTK's RenderingVR, RenderingOpenVR, and RenderingOpenXR modules currently support only immersive virtual reality experiences. The RenderingOpenXRRemoting module is currently the only path to augmented reality rendering with VTK. But non-remoting options for augmented reality rendering could theoretically be introduced, such as a tethered connection to a Meta Quest 2 headset for passthrough AR rendering. On the flip side, virtual reality remoting is also theoretically possible, such as what Meta does with its Air Link platform. My point being that while there is currently overlap between library support, mixed reality != augmented reality != remoting.

I see three possible approaches to standardization:

  1. Adopt common definitions such that a new, broad SlicerMixedReality module defines features broadly applicable to mixed reality (VR/AR/etc) experiences. Refactor the existing SlicerVirtualReality to depend on SlicerMixedReality and provide a narrow feature set tailored to immersive VR experiences. Rename remoting support to SlicerMixedRealityRemoting to depend on SlicerMixedReality. This approach would require nontrivial refactoring.
  2. Adopt common definitions such that SlicerVirtualReality is renamed to SlicerMixedReality to reflect OpenXR support and SlicerMixedReality is renamed to SlicerMixedRealityRemoting to reflect narrower XR remoting support. This may not be favorable since SlicerVirtualReality already has brand adoption.
  3. Adopt VTK's approach to use the term "virtual reality" as a catch-all for discussion dealing with mixed/virtual/augmented reality in the context of 3D Slicer, and clearly define as such in the SlicerVirtualReality README. Fully avoid using the term "mixed reality" in a Slicer context and rename SlicerMixedReality to SlicerVirtualRealityRemoting. Otherwise proceed with the approach proposed by @jcfr above to provide OpenXR support in the SlicerVirtualReality module.

Given existing buy-in for the SlicerVirtualReality module and the scope of work potentially required to fully separate general XR classes from VR-HMD-specific classes, I suggest approach (3) may be the best path to pursue.

@jcfr
Copy link
Contributor Author

jcfr commented Sep 12, 2023

Thanks for the detailed comment 🙏

Considering the ambiguous meaning of XR, what about renaming SlicerMixedReality to SlicerAugmentedReality? Considering the extension is not published, there are no issues renaming it.

That way we would have:

Extension Description
SlicerVirtualReality Provide virtual reality capabilities built on either vtkRenderingOpenVR or vtkRenderingOpenXR
SlicerAugmentedReality Provide augmented reality capabilities built on vtkRenderingOpenXRRemoting1

Footnotes

  1. with the option of supporting non-remoting options for augmented reality rendering provided by either an improved vtkRenderingOpenXR or a new VTK module

@adamrankin
Copy link
Collaborator

Feel free to take over SAR, its main goal was to implement camera param for intrinsic calibration and some convenience modules for screen based augmented reality

@tbirdso
Copy link

tbirdso commented Sep 12, 2023

Feel free to take over SAR, its main goal was to implement camera param for intrinsic calibration and some convenience modules for screen based augmented reality

Thanks @adamrankin , I wasn't aware of SlicerAugmentedReality before this. That past effort does seem to illustrate the different directions that Slicer AR development can go.

Considering the ambiguous meaning of XR, what about renaming SlicerMixedReality to SlicerAugmentedReality? Considering the extension is not published, there are no issues renaming it.

@jcfr It's up to you, but my understanding of SlicerMixedReality is that its distinguishing feature is that it provides infrastructure for XR remoting, which happens to only work on the HoloLens and thus is applied for augmented reality. Unless you foresee significant infrastructure additions to encompass other augmented reality approaches, it seems better to use Remoting in the name to capture the VTK OpenXRRemoting dependency and maintain a focus on that infrastructure.

Under previous development SlicerAugmentedReality proposes an AR path using two external stereo cameras and rendering to a VR headset to perform "passthrough" AR. This is a completely different approach than the remoting approach currently contained in SlicerMixedReality.

If you prefer to maintain every augmented reality approach in one repository under different modules, then merging SlicerMixedReality code into SlicerAugmentedReality may make sense. But, that seems like an extra and unnecessary maintenance burden in my opinion. I see no reason why the Slicer XR remoting module shouldn't be maintained under a dedicated repository such as SlicerVirtualRealityRemoting or similar.

@lassoan
Copy link
Collaborator

lassoan commented Sep 12, 2023

In general, users should not be concerned about knowing the difference between local/remote rendering or what interfaces (OpenXR, OpenVR) can be used for connecting to a headset. If the user needs to install a different extension or use a different module depending on what kind of headset he wants to use then it is an unnecessary complication. We would want Slicer scenes to be portable: it should be no problem to create a surgical plan on a VR headset and then the next day display that on an AR headset in the operating room.

Of course if it was really difficult to put everything in a single module then we could consider having separate modules, or in extreme cases even different extensions. But since all headsets and ar/vr/xr should essentially work the same way in Slicer and most of the code would be shared anyway, it would add a lot of overhead if we had to keep code in sync in mulitple modules and extensions.

We should rename the extension to MixedReality (prefixing the extension name with Slicer makes it a bit harder to find among all the SlicerX extensions) and the main module in it that is responsible for basic connection to the headset should be called MixedReality. I'm against name changes in general as it causes a lot of inconvenience to both users and developers, but in this case, adding support for AR headsets should make the change tolerable. To make the transition easier, the MixedReality extension could coexist until it reaches feature parity with SlicerVR extension and then it could be removed. It would be also OK to drop OpenVR support (and SlicerVR extension) if OpenXR can be used with all commonly used headsets that previously required OpenVR.

@tbirdso
Copy link

tbirdso commented Sep 12, 2023

In general, users should not be concerned about knowing the difference between local/remote rendering or what interfaces (OpenXR, OpenVR) can be used for connecting to a headset. ...

Fully agree. If it is palatable for community stakeholders, standardizing around common MixedReality language and consolidating ongoing XR efforts is the ideal path forward.

It would be also OK to drop OpenVR support (and SlicerVR extension) if OpenXR can be used with all commonly used headsets that previously required OpenVR.

VTK OpenXR supported devices does indeed include all previous VTK OpenVR supported devices.

@jcfr Following our offline discussion, was your preference to migrate remoting files to this repo and rename from SlicerVirtualReality to SlicerMixedReality, or to migrate files from this repo to SlicerMixedReality and then have a redirect from SlicerVirtualReality in its README? I'm not sure what the best consolidation path is in regards to Slicer extension infrastructure.

@cpinter
Copy link
Collaborator

cpinter commented Sep 13, 2023

I support the single extension approach, both from the maintenance point of view and to minimize the hassle from the user side to get a certain device working. Having one extension, the user does not need to know about the difference between all these terms, especially that it is not really standardized either, as also illustrated by @tbirdso 's comment above.

The brand value of SlicerVR is questionable I think, as it worked with minimum feature set for two years, then did not work basically at all for a few years, and we are still not fully back to working again (I thought so but there are reports on discourse about intreractions). So I don't consider renaming to MixedReality a problem.

The only thing I'd consider important is that the paper has gained some traction, and currently has 25 citations.
https://scholar.google.ca/citations?view_op=view_citation&hl=en&user=LbnADQ0AAAAJ&citation_for_view=LbnADQ0AAAAJ:e5wmG9Sq2KIC
So the main links in the paper should automatically redirect to their new counterpart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants