FIX: [ISXB-1716] Add support for all existing InputControl types in InputTestFixture.Trigger #2270
Conversation
K-Tone
changed the title
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent |
![u-pr-agent[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
| if (control is IntegerControl integerControl) | ||
| { | ||
| Set(integerControl, integerControl.ReadValue() + 1); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Suggestion: Incrementing the IntegerControl value can lead to an integer overflow if the current value is int.MaxValue. This would wrap the value to int.MinValue, which might be an unexpectedly large change. To make the trigger more robust, check for int.MaxValue and decrement instead to ensure a small, predictable change. [general, importance: 6]
| if (control is IntegerControl integerControl) | |
| { | |
| Set(integerControl, integerControl.ReadValue() + 1); | |
| return; | |
| } | |
| if (control is IntegerControl integerControl) | |
| { | |
| var value = integerControl.ReadValue(); | |
| if (value == int.MaxValue) | |
| Set(integerControl, value - 1); | |
| else | |
| Set(integerControl, value + 1); | |
| return; | |
| } |
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent
There was a problem hiding this comment.
Not sure we need to do that really, there's barely any contract that Trigger should produce a minimal change
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2270 +/- ##
===========================================
+ Coverage 76.70% 76.82% +0.11%
===========================================
Files 465 476 +11
Lines 87919 88714 +795
===========================================
+ Hits 67442 68153 +711
- Misses 20477 20561 +84 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 24 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Thanks for doing this @K-Tone and I think this fills the gap called out in the ticket.
However I also believe this makes it even more clear why this is incorrectly implemented to begin with which I think is outside of this PR but wanted to mention anyway for alignment. Since Input System claims to be an extendible system, and decided to expose this test fixture, it means that the number of InputControl or InputDevice types is not fixed. Hence, IMO, implementing something like Trigger here is not scalable. It would have been much better to not have such a function in the fixture at all, and instead add such support via extension methods. Then each plugin implementing the control could provide a test assembly with an extension method providing the needed Trigger implementation for that particular type only. The same goes for Press, Release and all "test extensions" defined in this file. Something to think about for when we do v.2.0 API of this. Maybe we should even file a ticket on it or what do you think? It would be a straightforward refactor. If InputTestFixture would not be public, this would be less important, but still better design IMO, or what do you think?
Totally agreed, that's what I've been thinking about while working on the ticket. |
|
Probably not much to review here (no fix), but still CC for viz, so that you're aware this happened @Pauliusd01 |
…rol types that we know of
Co-authored-by: u-pr-agent[bot] <205906871+u-pr-agent[bot]@users.noreply.github.com>
…d in conditionally
K-Tone
force-pushed
the
anthony/isxb-1716-inputtestfixture-not-implemented
branch
from
fe296dc to
8be40a1
Compare
Description
There was a problem reported that if you invoke InputTestFixture.Trigger with anything other than a Button or an Axis, there would be a NotImplementedException thrown.
Here, we address it by logically extending the approach originally taken by the Axis implementation (change the value slightly as an effect from Trigger) to all other possible InputControl variations that we have. We only have two outliers here -- the integer control just gets its value incremented by one instead of the regular small change. Also the TouchState thing gets the next state in the list as well.
Testing status & QA
Local testing
Overall Product Risks
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: