FIX: reject non-finite InputSystem.pollingFrequency (ISX-2837) - #2474
Open
ekcoh wants to merge 3 commits into
Open
FIX: reject non-finite InputSystem.pollingFrequency (ISX-2837)#2474ekcoh wants to merge 3 commits into
ekcoh wants to merge 3 commits into
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Harness Review
The setter now rejects every non-finite float before it can mutate either stored or native runtime polling state, while preserving the existing rejection of non-positive values. I examined the platform branches, runtime propagation, test coverage, and changelog entry.
Reviewed commit 5912b84
🤖 Helpful? 👍/👎
Collaborator
|
It's still reading back as NaN for me (with the warning thrown which is correct) so maybe It's my scene that's wrong. I'll send you the project via dm with instructions |
…ISX-2837) Follow-up to the ISX-2837 guard. No behaviour change. The original comment justified rejecting +Infinity as the same caching bug as NaN. It is not. The native backend accepts +Infinity as "poll continuously" and zero as "disable polling", so rejecting either here is a deliberate narrowing of a supported native mode and the reason should say so. - Explain the +Infinity and zero rejections on their own terms, and drop the ////REVIEW asking whether zero should turn off polling - that question is answered in the comment now. - Add the accepted range and an <exception> tag to the public InputSystem.pollingFrequency XMLDoc, which had neither. - Note on the test's +Infinity assertion that it deliberately diverges from the native SetPollingFrequency_ShouldAcceptInfinityAsContinuousPolling. - Reword the changelog to cover both non-finite values and both consequences.
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2474 +/- ##
=========================================
Coverage 78.95% 78.95%
=========================================
Files 767 767
Lines 140780 140912 +132
=========================================
+ Hits 111153 111261 +108
- Misses 29627 29651 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Rejects non-finite values in
InputSystem.pollingFrequency. The setter previously guarded only withvalue <= 0;NaNcomparesfalseagainst everything, so it slipped past (as did+Infinity), got cached, and was returned on every subsequent read — permanently leaving the property atNaNand disabling background polling. The setter now rejectsNaN/±Infinity(and non-positive values) with anArgumentException.Fixes ISX-2837. Found by QA while validating unity/unity PR #120254. The engine-side counterpart (native
SetAsynchronousInputPollingFrequency) is hardened separately in UUM-149921 / UUM-149922; this PR closes the package-side half.Testing status & QA
Devices_CannotSetPollingFrequencyToNonFiniteValue(CoreTests_Devices) assertingNaN,+Infinity,-Infinity,0, and negative all throwArgumentException, and that a rejected assignment leaves the previous value untouched.InputSystem.pollingFrequency = float.NaN;now throws instead of the property reading backNaN.Overall Product Risks
Comments to reviewers
NaN/+Infinitynow throwArgumentException(previously silently accepted;-Infinitywas already caught by<= 0). No valid input changes behavior.float.IsFiniteintentionally avoided (unavailable on netstandard2.0); usingIsNaN/IsInfinity, matching existing usage inInputDeviceDebuggerWindow.Checklist
Before review:
Changed,Fixed,Addedsections.Devices_CannotSetPollingFrequencyToNonFiniteValue).During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.