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

Improvements to subsystem query API #11333

Merged
merged 14 commits into from
Jan 18, 2023
Merged

Conversation

Zee2
Copy link
Contributor

@Zee2 Zee2 commented Dec 28, 2022

Overview

These are some significant improvements to the subsystem query API that were originally part of the new_hand_menu branch/PR. I still have a few last things to add to that before merging, but I wanted to get these improvements in ASAP.

Changes

  • Adds handy conversion operator to HandJointPose to automatically cast it to a Pose
  • Less confusing doccomments on Handedness
  • Removed confusing doccomment remark on XRSubsystemUtilities
  • Refactors XRSubsystemHelpers to internally reuse scratchpad List<T>s, cached by generic type
    • This does mean that the list reference will be reused/squashed for subsequent calls, so calling clients shouldn't hold on to a reference to the list for longer than necessary. But for the vast majority of instances where people are trying to get a reference to a single subsystem, this is a huge improvement (i.e., just trying to access the hands API)

@Zee2
Copy link
Contributor Author

Zee2 commented Jan 4, 2023

Profiling results here. Takeaways: the auto-scratchpad-list-system is equally performant to maintaining your own reusable list yourself. The .HandsAggregator property is very very cheap (0.14ms for 10,000 calls per frame). Allocing every query is very inefficient, as expected. The .running check is free.

Overall, the auto-scratch-list is 2x efficient over the naive call, and the direct helper property is an 86.7x speedup over the naive call.

// Just checking .running property
// 0.00ms, GC = 0
bool test = true;
for (int i = 0; i < 10000; i++)
{
    test &= hands.running;
}

// Direct helper property, cached
// 0.14ms, GC = 0
for (int i = 0; i < 10000; i++)
{
    hands = XRSubsystemHelpers.HandsAggregator;
}

// Auto-scratch-list
// 6ms, GC = 0
for (int i = 0; i < 10000; i++)
{
    hands = XRSubsystemHelpers.GetFirstRunningSubsystem<HandsAggregatorSubsystem>();
}

// Using Unity API directly, no reused list
// 8-12ms, GC = 20,000 (1.0 MB)
for (int i = 0; i < 10000; i++)
{
    var list = new List<HandsAggregatorSubsystem>();
    SubsystemManager.GetSubsystems(list);
    if (list.Count > 0)
    {
        hands = list[0];
    }
}

// Using Unity API directly, reusing list
// 6ish ms, GC = 0
for (int i = 0; i < 10000; i++)
{
    SubsystemManager.GetSubsystems(reusableList);
    if (reusableList.Count > 0)
    {
        hands = reusableList[0];
    }
}

@@ -110,7 +107,7 @@ public Vector3 PokePosition
{
get
{
if (HandSubsystem.TryGetJoint(TrackedHandJoint.IndexTip,
if (XRSubsystemHelpers.HandsAggregator.TryGetJoint(TrackedHandJoint.IndexTip,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a null check on the aggregator here and in GrabPosition below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do in another PR so that the change won't reset the approval :D

Copy link

@david-c-kline david-c-kline left a comment

Choose a reason for hiding this comment

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

Overall looks good, there are a couple of questions and it would be good to resolve the null check issues that @keveleigh pointed out.

@Zee2 Zee2 merged commit 1da947f into microsoft:mrtk3 Jan 18, 2023
Zee2 added a commit that referenced this pull request Jan 24, 2023
* Some missing nullchecks from the previous PR

* Fixing deprecated usage + warnings
drusk-unity pushed a commit to drusk-unity/MixedRealityToolkit-Unity that referenced this pull request Jun 26, 2023
* Cherrypicks from new_hand_menu

* Removed confusing doccomment remark

* Deprecating HandsUtils.Subsystem

* Etc

* Obsoleting some un-flaggy extension methods, fixing `ToXRNode` behavior

* Refactors

* More cleanup

* Fixing doccomment

* SolverHandler optimization

* oops

Co-authored-by: Kurtis <kurtie@microsoft.com>
drusk-unity pushed a commit to drusk-unity/MixedRealityToolkit-Unity that referenced this pull request Jun 26, 2023
…rosoft#11382)

* Some missing nullchecks from the previous PR

* Fixing deprecated usage + warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants