-
Notifications
You must be signed in to change notification settings - Fork 325
CHANGE: ISX-2322 Rely on module to manage polling frequency. #2200
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
base: develop
Are you sure you want to change the base?
Conversation
…ogies/InputSystem into isx-2322-polling-frequency
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2200 +/- ##
========================================
Coverage 68.11% 68.12%
========================================
Files 367 367
Lines 53610 53612 +2
========================================
+ Hits 36519 36521 +2
Misses 17091 17091
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Looks like asmdef condition is true for trunk which it shouldn't be, will make sure related Unity PR is on-par with trunk investigate. |
… contain required changes. Note that to test one has to weak this asmdef.
As suspected trunk had advanced hence asmdef is now a3 to exclude trunk. Note that during testing you have to modify the asmdef to be a2 when tested with the corresponding Unity branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Added a comment of something that would be relevant to have IMO.
public void Devices_PollingFrequencyIsAtLeast60HzByDefault() | ||
{ | ||
Assert.That(InputSystem.pollingFrequency, Is.GreaterThanOrEqualTo(60)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also run this test when we check XInput on Windows? I'm not sure if its ideal to do this on the package but I'd like to test that this pollingFrequency is at least affected by platforms that do expose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a setting. The KPI branch on module "sampling performance" is testing exactly that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I also added the achievable frequency to input debugger long ago. If you set this to 250 you might get what the device and system conditions support. e.g. 125
Description
Removes polling frequency as a property being managed by
InputManager
and instead delegates life-time and initial setting to be configured by platform and input-module. This requires a specific Unity branch, see internal issue ISX-2322.Also removed double-booking of pollingFrequency field from serialised state since I cannot see any reason to have it part of serialised state. Since property is now in native it won't be subject to domain reloads.
Testing status & QA
Tested on macOS by setting native polling frequency to 123 Hz with a custom local build during development.
Recommended to test on Windows with Xbox gamepad on the associated Unity branch since Windows has a higher default than 60 Hz on that branch which affects the Windows Gaming Input backend. E.g. testing with a DualSense on Windows wouldn't make much difference since DualSense is using unconstrained polling at the platform level and do not depend on polling frequency setting.
Overall Product Risks
Small.
Comments to reviewers
Note that this branch should be merged after the corresponding Unity feature branch has landed since version is otherwise unknown: update changeling, comments and asmdef condition
Checklist
Before review:
Changed
,Fixed
,Added
sections.Devices_PollingFrequencyIsAtLeast60HzByDefault
conditionally replacesDevices_PollingFrequencyIs60HzByDefault
conditionally based on Unity version being used.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: