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

Move input subsystem implementations to the input package. #11641

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

shaynie
Copy link
Collaborator

@shaynie shaynie commented Jun 22, 2023

Overview

Move input subsystem implementations to the input package.

Changes

  • Fixes: # 11592.

Verification

This optional section is a place where you can detail the specific type of verification
you want from reviewers. For example, if you want reviewers to checkout the PR locally
and validate the functionality of specific scenarios, provide instructions
on the specific scenarios and what you want verified.

If there are specific areas of concern or question feel free to highlight them here so
that reviewers can watch out for those issues.

As a reviewer, it is possible to check out this change locally by using the following
commands (substituting {PR_ID} with the ID of this pull request):

git fetch origin pull/{PR_ID}/head:name_of_local_branch

git checkout name_of_local_branch

Copy link
Member

@AMollis AMollis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can some of the remaining usages of HandsAggregatorSubsystem be changed to IHandsAggregatorSubsystem ? XRSubsystemHelpers may hold something that is not a HandsAggregatorSubsystem.

@shaynie shaynie requested a review from AMollis June 22, 2023 22:40
@marlenaklein-msft
Copy link
Contributor

I think there are some docs that should be updated after this, opening an issue to track work #11644.

Copy link
Contributor

@marlenaklein-msft marlenaklein-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TextToSpeechSubsystem is also in the core package, should this be moved as well?

Copy link
Member

@AMollis AMollis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@shaynie shaynie merged commit 98a2aab into microsoft:mrtk3 Jun 23, 2023
@shaynie shaynie deleted the user/shaynie/subsystemImplMoves branch June 23, 2023 16:12
@@ -3,6 +3,7 @@
"rootNamespace": "Microsoft.MixedReality.Toolkit.Speech.Windows",
"references": [
"Microsoft.MixedReality.Toolkit.Core",
"Microsoft.MixedReality.Toolkit.Input",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird. (I don't think we should move these base classes, but if they need to be moved) I'm not sure why the speech classes weren't moved into the speech package. That's where the concrete implementations already live.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these classes make sense to move (apologies for being less certain before: I though you were referring to layer 3 but were actually referring to layer 2, below).

There are three defined parts to an MRTK subsystem:

  1. The interface, largely used to keep the API surface of the subsystems and providers consistent
    1. Lives in Core
  2. The base class, largely scaffolding code linking the subsystem to an abstract provider, following Unity's established patterns
    1. Previously lived in Core, moving to Input in this PR
  3. The actual implementation, largely implementing the abstract provider from 2
    1. Lives in the corresponding packages, like Input or Speech.Windows

1 and 2 should live in the Core, imo, because devs shouldn't need to write the scaffolding themselves or have to import the Input package just to get access to the scaffolding. There's almost no variation in this level, and any needed variation could/should live in the concrete implementation.

3 is where the actual interesting differences lie, and the only level devs should need to write themselves (if they don't want to use MRTK's impl).

The existence of the interface is slightly misleading. I don't believe Unity's pattern for subsystems typically defines explicit interface types, but we used it just to maintain an API consistency across subsystems and their providers.

drusk-unity pushed a commit to drusk-unity/MixedRealityToolkit-Unity that referenced this pull request Jun 26, 2023
…#11641)

* Move input subsystem implementations to the input package.

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

Successfully merging this pull request may close these issues.

4 participants