FIX: Make sure we respect the default button press point for triggers, same for Touchscreen's tap delay time and tap radius squared #2186

K-Tone · 2025-05-21T10:10:02Z

Description

This is an attempt to fix the problem where in Editor we would really apply the default input settings correctly. Like, we would read them, but we wouldn't correctly update the derived static properties so whenever that happens, we have issues like the default button press point being incorrect.

Looking at code, I believe it's not just the default button press point that was incorrect, I think all of these folks were incorrect too (though the bug report won't mention them):

            Touchscreen.s_TapDelayTime = settings.multiTapDelayTime;
            Touchscreen.s_TapRadiusSquared = settings.tapRadius * settings.tapRadius;
            // Extra clamp here as we can't tell what we're getting from serialized data.
            ButtonControl.s_GlobalDefaultButtonPressPoint = Mathf.Clamp(settings.defaultButtonPressPoint, ButtonControl.kMinButtonPressPoint, float.MaxValue);
            ButtonControl.s_GlobalDefaultButtonReleaseThreshold = settings.buttonReleaseThreshold;

Testing status & QA

Local testing, pending a QA pass.

Overall Product Risks

Complexity: Low
Halo Effect: Low

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

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.

K-Tone · 2025-05-21T10:12:34Z

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

                Object.DestroyImmediate(m_Settings);
-            m_Settings = state.settings;
+
+            settings = state.settings;


So the theory here is that using the property instead of a direct setter makes sure we update all the derived data (static caches) -- that happens in ApplySettings that is ignored in here.

Ah, good catch, since the property setter triggers the apply settings.

codecov-github-com · 2025-05-21T10:20:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

@@           Coverage Diff            @@
##           develop    #2186   +/-   ##
========================================
  Coverage    65.43%   65.43%           
========================================
  Files          367      367           
  Lines        53505    53505           
========================================
  Hits         35013    35013           
  Misses       18492    18492

Flag	Coverage Δ
linux_2021.3_project	`70.41% <100.00%> (ø)`
linux_2022.3_project	`65.27% <100.00%> (ø)`
linux_6000.0_project	`65.34% <100.00%> (ø)`
linux_6000.1_project	`65.34% <100.00%> (ø)`
linux_trunk_project	`65.34% <100.00%> (ø)`
mac_2021.3_pkg	`5.41% <0.00%> (ø)`
mac_2021.3_project	`70.42% <100.00%> (ø)`
mac_2022.3_pkg	`5.19% <0.00%> (ø)`
mac_2022.3_project	`65.29% <100.00%> (ø)`
mac_6000.0_pkg	`5.20% <0.00%> (ø)`
mac_6000.0_project	`65.37% <100.00%> (ø)`
mac_6000.1_pkg	`5.20% <0.00%> (ø)`
mac_6000.1_project	`65.37% <100.00%> (ø)`
mac_trunk_pkg	`5.20% <0.00%> (ø)`
mac_trunk_project	`65.36% <100.00%> (ø)`
win_2021.3_pkg	`5.42% <0.00%> (ø)`
win_2021.3_project	`70.49% <100.00%> (ø)`
win_2022.3_pkg	`5.20% <0.00%> (ø)`
win_2022.3_project	`65.37% <100.00%> (ø)`
win_6000.0_pkg	`5.20% <0.00%> (ø)`
win_6000.0_project	`65.44% <100.00%> (+<0.01%)`	⬆️
win_6000.1_pkg	`5.20% <0.00%> (ø)`
win_6000.1_project	`65.44% <100.00%> (-0.01%)`	⬇️
win_trunk_pkg	`5.20% <0.00%> (ø)`
win_trunk_project	`65.44% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../com.unity.inputsystem/InputSystem/InputManager.cs	`88.70% <100.00%> (ø)`
...s/com.unity.inputsystem/InputSystem/InputSystem.cs	`82.98% <ø> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

K-Tone · 2025-05-22T09:39:52Z

Just made this PR ready for review because the CI redness on iOS and tvOS looks caused by xcodebuild timing out while waiting for some update or whatever. Not related to this change.

ekcoh

Approving this fix but it would be great if we could add the missing test coverage for this, which seems unrelated (as you say) to the specific setting but relates to the restoration of settings after e.g. a domain reload. I wonder if we could get it covered by exiting and then re-entering playmode from a test.....

ekcoh · 2025-05-22T10:58:45Z

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

                Object.DestroyImmediate(m_Settings);
-            m_Settings = state.settings;
+
+            settings = state.settings;


Ah, good catch, since the property setter triggers the apply settings.

ekcoh · 2025-05-22T10:59:23Z

Packages/com.unity.inputsystem/InputSystem/InputSystem.cs

            }

-            // Create temporary settings. In the tests, this is all we need. But outside of tests,d
+            // Create temporary settings. In the tests, this is all we need. But outside of tests,


Always good to fix a typo.

Pauliusd01 · 2025-05-28T09:35:14Z

Did not forget this one, will try to review it today/tomorrow

Pauliusd01

LGTM, only one note though. Should these UTR version changes go in with this PR? I think you added them to test if it solves your CI problems? I wouldn't mind but just checking

K-Tone · 2025-06-02T08:25:38Z

In the end, I think I would send the UTR change with this PR because otherwise it's very painful to get a green automation run on this PR otherwise

K-Tone · 2025-06-02T08:27:52Z

I didn't make a test here in the end, I think I just don't know how to make a meaningful one without making a new test project with the updated defaults

Packages/com.unity.inputsystem/CHANGELOG.md

… a bit of derived state that is invalid otherwise

…nd tvOS instabilities (per the Slack advice)

K-Tone commented May 21, 2025

View reviewed changes

K-Tone changed the title ~~make sure we actually apply the loaded settings, since there is quite…~~ FIX: Make sure we respect the default button press point for triggers, same for Touchscreen's tap delay time and tap radius squared May 21, 2025

K-Tone requested review from Pauliusd01 and ekcoh May 21, 2025 12:28

K-Tone marked this pull request as ready for review May 22, 2025 09:39

ekcoh approved these changes May 22, 2025

View reviewed changes

Pauliusd01 approved these changes May 29, 2025

View reviewed changes

K-Tone force-pushed the anthony/ixsb-1152-invalid-default-button-press-point branch from 885241b to 4ddce95 Compare June 2, 2025 08:24

K-Tone commented Jun 2, 2025

View reviewed changes

Packages/com.unity.inputsystem/CHANGELOG.md Outdated Show resolved Hide resolved

K-Tone and others added 6 commits June 3, 2025 14:02

make sure we actually apply the loaded settings, since there is quite…

6d0a58d

… a bit of derived state that is invalid otherwise

update the changelog

8281faa

fix up the typo in comments

7716176

format code

1e2bdb9

force usage of an older utr release in an attempt to get rid of iOS a…

e4920cb

…nd tvOS instabilities (per the Slack advice)

Update Packages/com.unity.inputsystem/CHANGELOG.md

5735715

K-Tone force-pushed the anthony/ixsb-1152-invalid-default-button-press-point branch from f0ab047 to 5735715 Compare June 3, 2025 11:02

K-Tone merged commit 757e7b5 into develop Jun 3, 2025
112 checks passed

K-Tone deleted the anthony/ixsb-1152-invalid-default-button-press-point branch June 3, 2025 15:23

FIX: Make sure we respect the default button press point for triggers, same for Touchscreen's tap delay time and tap radius squared #2186

FIX: Make sure we respect the default button press point for triggers, same for Touchscreen's tap delay time and tap radius squared #2186

Uh oh!

Conversation

K-Tone commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

Uh oh!

K-Tone May 21, 2025

Choose a reason for hiding this comment

Uh oh!

ekcoh May 22, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-github-com bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

K-Tone commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

ekcoh May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ekcoh May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Pauliusd01 commented May 28, 2025

Uh oh!

Pauliusd01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K-Tone commented Jun 2, 2025

Uh oh!

K-Tone commented Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

K-Tone commented May 21, 2025 •

edited

Loading

codecov-github-com bot commented May 21, 2025 •

edited

Loading

K-Tone commented May 22, 2025 •

edited

Loading

Pauliusd01 left a comment •

edited

Loading