Skip to content

Conversation

AlexTyrer
Copy link
Contributor

@AlexTyrer AlexTyrer commented Aug 23, 2024

Description

The InputDeviceMatcher.WithManufacturer() can perform a regex match even for simple patterns (eg "Sony.+Entertainment" and "PLAYSTATION(R)3 Controller" are both treated as Regexes) which is surprisingly costly on initialization.

This PR addresses that by adding an API to perform a simple string keyword match on the device manufacturer string,

Changes made

The new WithManufacturerContains(string noRegexMatch) API performs a String.Contains() check instead looking for the simple pattern in the manufacturer string.

Changed the DualShockSupport for DualShock4Gamepad and DualShock3Gamepad to avoid regex matching.

These changes save 25ms in DualShockSupport.Initialize during player start-up (26ms -> 1ms)

Testing

Local testing using SONY controllers and Quest VR devices.

Risk

Previously a controller that reported the manufacturer as "SONY Entertainment" would match - with this change a hypothetical device that reported the manufacturer as "Entertainment Not SONY" would also match. I think this is a risk worth taking.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

…(string) - improve DualShockSupport initialization performance

o WithManufacturerContains() provides for simple keyword (no regex)  matching in the manufacturer string
o Changes DualShock4Gamepad / DualShock3Gamepad matchers to avoid regex creation on initialization

These changes save 25ms in DualShockSupport.Initialize during player start-up (26ms -> 1ms)
… noRegexMatch), added chanelog entry (ISX-1411)
@AlexTyrer AlexTyrer requested a review from lyndon-unity August 23, 2024 14:19
Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Generally I approve the change and the savings.
I've asked a couple of questions though as it looks like a slightly smaller change could give the same result.

Might also be worth adding Lewis Hammond to this PR from the PlayStation team

/// <seealso cref="InputDeviceDescription.manufacturer"/>
public InputDeviceMatcher WithManufacturerContains(string noRegExPattern)
{
return With(kManufacturerContainsKey, noRegExPattern, supportRegex:false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that supportRegex:false is passed, do you reallly need kManufacturerContainsKey.
It seems you could just use kManufacturerKey since the object type will remain a string

(but I may have missed something)

Copy link
Contributor Author

@AlexTyrer AlexTyrer Aug 23, 2024

Choose a reason for hiding this comment

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

The different key is needed to take the different path through MatchPercentage().

kManufacturerKey -> MatchSingleProperty -> string.Compare(...) == 0 // string/regex needs exactly match
kManufacturerContainsKey -> MatchSinglePropertyContains -> string.Contains(...) // string just needs to be in there somewhere

.WithInterface("HID")
.WithManufacturer("Sony.+Entertainment")
.WithProduct("PLAYSTATION(R)3 Controller"));
.WithManufacturerContains("Sony")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering it this could be just WithManufacturer("Sony", supportRegex:false) ?

There is a small chance this matches wider now - E.g. Entertainment Sony would match now, but I don't think this is likely to be any false positives.

Copy link
Contributor Author

@AlexTyrer AlexTyrer Aug 23, 2024

Choose a reason for hiding this comment

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

I believe it needs to be an exact match as the code does:

string.Compare(...) == 0

... so we would need to provide the entire manufacturer string - and there seem to be two flavours of Sony manufacturer string:

  • "Sony Interactive Entertainment"
  • "Sony Computer Entertainment"

... hence the regex in the original code.

Just checked this.

.WithManufacturer("Sony", supportRegex:false) // test
... and some of the tests fail.

This is the reason why I added the 'Contains' thing - an explicit non-regex pattern that just has to be in the manufacturer string.

@AlexTyrer AlexTyrer changed the title ADD: Added InputDeviceMatcher.WithManufacturerContains(string noRegexMatch) to improve DualShockSupport initialization performance NEW: Added InputDeviceMatcher.WithManufacturerContains(string noRegexMatch) to improve DualShockSupport initialization performance Aug 23, 2024
@AlexTyrer AlexTyrer merged commit 9c476d1 into develop Aug 27, 2024
@AlexTyrer AlexTyrer deleted the isx-1411-input-device-matcher-manufacturer-contains branch August 27, 2024 10:34
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

Successfully merging this pull request may close these issues.

2 participants