FIX: ISXB-704 scroll wheel support windows raw input value #1946

benoitalain · 2024-06-04T15:09:48Z

Description

Fixes ISXB-704. Based on recently landed trunk PR.

Details of what was changed and the thought process can be found in this Google Document:
https://docs.google.com/document/d/1XCC6GqaWzKf1M1jm-UmKRn6sIlmvF7rvF1C8Ws5Yxa4/edit?usp=sharing

This PR introduces a new Settings option for the InputSystem package. See screenshot:

When the "Keep Platform Specific Input Range" option is selected, InputSystem actions that result from a Scroll Wheel hardware input will be expressed in the OS native range when retrieved with the InputAction.CallbackContext.ReadValue<Vector2>() method. On Windows, the values should range between -120 to 120, whereas on other platforms it will remain between -1 and 1 in general.

The new setting's default option is "Uniform Across All Platforms". When that option is selected, InputSystem actions that result from a Scroll Wheel will always be expressed in the -1 to 1 range, regardless of platform.

Changes made

Please write down a short description of what changes were made.

Notes

Please write down any additional notes, remove the section if not applicable.

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.
- FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
- FogBugz is marked as "Resolved" with next release version correctly set.
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.

unity-cla-assistant · 2024-06-04T15:09:54Z

All committers have signed the CLA.

ekcoh

Commenting for now until aligned on potential graceful version handling.

ekcoh · 2024-06-04T17:09:03Z

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

+            get => NativeInputSystem.normalizeScrollWheelDelta;
+            set => NativeInputSystem.normalizeScrollWheelDelta = value;
+#else
+            get; set;


Not sure I see what role these getter setters play when earlier Unity version? Wouldn't it make sense to remove the property completely instead?

(or would have expected hard coded behavior, but the answer is - it depends)

ekcoh · 2024-06-04T17:10:24Z

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

+        public float scrollWheelDeltaPerTick
+        {
+#if UNITY_6000_0_OR_NEWER
+            get { return NativeInputSystem.GetScrollWheelDeltaPerTick(); }


Recommend being consistent with getter below and use the short-hand version with =>

ekcoh · 2024-06-04T17:13:26Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

-            var scrollDelta = ctx.ReadValue<Vector2>();
-            if (scrollDelta.sqrMagnitude < k_SmallestReportedMovementSqrDist)
+            // ISXB-704: convert input value to uniform ticks before sending them to UI.
+            var scrollTicks = ctx.ReadValue<Vector2>() / InputSystem.scrollWheelDeltaPerTick;


Is InputSystem.scrollWheelDeltaPerTick guaranteed to never be zero (to make sure we never can run into division by zero)

I added an Assert on the native side in the trunk PR, is that enough? I'm not a fan of adding more asserts on all the stages where we propagate that value, do you recommend adding one here too? Only here?

If trunk version guarantees that its never zero I believe its fine to not check it in package. Agree that its better to define the API contract on lower level.

ekcoh · 2024-06-04T17:14:04Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

-            if (scrollDelta.sqrMagnitude < k_SmallestReportedMovementSqrDist)
+            // ISXB-704: convert input value to uniform ticks before sending them to UI.
+            var scrollTicks = ctx.ReadValue<Vector2>() / InputSystem.scrollWheelDeltaPerTick;
+            if (scrollTicks.sqrMagnitude < k_SmallestReportedMovementSqrDist)


Nice to avoid the sqr root

ekcoh · 2024-06-04T17:22:40Z

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

@@ -281,6 +281,25 @@ private void OnFocusChanged(bool focus)
        public Vector2 screenSize => new Vector2(Screen.width, Screen.height);
        public ScreenOrientation screenOrientation => Screen.orientation;

+        public bool normalizeScrollWheelDelta
+        {
+#if UNITY_6000_0_OR_NEWER


I guess we run into issues with UNITY 6 revision prior to trunk change landing but maybe ok if/when that revision is current or is there opportunity for a even more specific define in the asmdef to detect the exact expected version once the trunk change lands maybe?

Not sure what the general solution to this problem is. There are other places in the Input System package code where we have a similar problem, so if we find a solution for this we should apply it everywhere (at once) probably.

What I was after above was whether UNITY_6000_0_OR_NEWER was specific enough or whether to define a custom asmdef symbol based on version value to define a symbol more specifically reflecting the change as introduced into UNITY_6000 version. There are other examples here https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Unity.InputSystem.asmdef but I noticed they very not specific on a "sub-version" level so probably ok.

ekcoh · 2024-06-04T17:25:04Z

Assets/Tests/InputSystem/Plugins/InputForUITests.cs

+    [Category(kTestCategory)]
+    [TestCase(1.0f)]
+    [TestCase(120.0f)]
+    public void UIActionScroll_ReceivesNormalizedScrollWheelDelta(float scrollWheelDeltaPerTick)


Thanks for adding these tests to cover scaling.

ekcoh · 2024-06-04T17:25:34Z

Looks like a branch update and CI rerun might be in order.

ekcoh

Approving this now, I saw some minor comments remain but ok with me to leave them as-is if you consider it to be preferable to have it as it currently is.

ekcoh · 2024-06-11T07:04:14Z

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

@@ -281,6 +281,25 @@ private void OnFocusChanged(bool focus)
        public Vector2 screenSize => new Vector2(Screen.width, Screen.height);
        public ScreenOrientation screenOrientation => Screen.orientation;

+        public bool normalizeScrollWheelDelta
+        {
+#if UNITY_6000_0_OR_NEWER


What I was after above was whether UNITY_6000_0_OR_NEWER was specific enough or whether to define a custom asmdef symbol based on version value to define a symbol more specifically reflecting the change as introduced into UNITY_6000 version. There are other examples here https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Unity.InputSystem.asmdef but I noticed they very not specific on a "sub-version" level so probably ok.

ekcoh · 2024-06-11T07:05:40Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

-            var scrollDelta = ctx.ReadValue<Vector2>();
-            if (scrollDelta.sqrMagnitude < k_SmallestReportedMovementSqrDist)
+            // ISXB-704: convert input value to uniform ticks before sending them to UI.
+            var scrollTicks = ctx.ReadValue<Vector2>() / InputSystem.scrollWheelDeltaPerTick;


If trunk version guarantees that its never zero I believe its fine to not check it in package. Agree that its better to define the API contract on lower level.

duckets

Approved with amendments to tooltip and API docs

Pauliusd01

Couple of concerns from me:

I did not need these changes to fix the issue, simply opening the Editor version that has your latest changes is enough to fix it
The ifdefs for the code are not granular enough and I still get errors about missing Editor code on 6.0.6f1 Editor, should that be changed?

Also, please take a look at the slack discussion about this change in our input channel

benoitalain · 2024-06-25T15:34:43Z

Couple of concerns from me:

I did not need these changes to fix the issue, simply opening the Editor version that has your latest changes is enough to fix it

The ifdefs for the code are not granular enough and I still get errors about missing Editor code on 6.0.6f1 Editor, should that be changed?

Also, please take a look at the slack discussion about this change in our input channel

You say you only need the Editor changes, are you sure there's no copy of my package fixes that survived in your Library folder? Otherwise I can't see how the InputSystemUIInputModule fixes could come from the Editor only... makes no sense :-(

About the ifdefs not being granular enough, I'll comment in the slack discussion but I'm not sure there's a clean solution.

Pauliusd01 · 2024-06-26T05:45:49Z

Couple of concerns from me:

I did not need these changes to fix the issue, simply opening the Editor version that has your latest changes is enough to fix it

The ifdefs for the code are not granular enough and I still get errors about missing Editor code on 6.0.6f1 Editor, should that be changed?

Also, please take a look at the slack discussion about this change in our input channel

You say you only need the Editor changes, are you sure there's no copy of my package fixes that survived in your Library folder? Otherwise I can't see how the InputSystemUIInputModule fixes could come from the Editor only... makes no sense :-(

It's the user project from ISXB-704 with no library and 1.8.2 input system

benoitalain · 2024-07-02T14:50:24Z

Couple of concerns from me:

I did not need these changes to fix the issue, simply opening the Editor version that has your latest changes is enough to fix it

The ifdefs for the code are not granular enough and I still get errors about missing Editor code on 6.0.6f1 Editor, should that be changed?

Also, please take a look at the slack discussion about this change in our input channel

You say you only need the Editor changes, are you sure there's no copy of my package fixes that survived in your Library folder? Otherwise I can't see how the InputSystemUIInputModule fixes could come from the Editor only... makes no sense :-(

It's the user project from ISXB-704 with no library and 1.8.2 input system

Hi! I've updated the PR description to emphasize the changes that this PR contains. It's not observable scroll range fixes in any of the UI systems, but just the option to allow InputSystem actions returning values in the -120 to 120 range.

…om:Unity-Technologies/InputSystem into isxb-704-scrollwheelwindowsrawvaluesupport # Conflicts: # Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

Pauliusd01

Thanks for the extra info, lgtm. Do check the CI failures though, there's more than the usual culprit ones

benoitalain mentioned this pull request Jun 4, 2024

FIX: ISXB-766, ISXB-808, ISXB-704 scroll wheel factor bugs #1940

Closed

4 tasks

ekcoh reviewed Jun 4, 2024

View reviewed changes

benoitalain added 5 commits June 5, 2024 11:35

Fixed ISXB-766, ISXB-808, ISXB-704, conditional to trunk PR #49932.

30b1383

Updated for latest trunk PR changes (5f81a20).

a9f9001

Reverted fix for ISXB-808 and partly ISXB-766. Kept ISBX-704 fix.

3f6fb44

Added scroll wheel normalization tests for UI and InputForUI.

a9df122

Recommended changes from @ekcoh. Added more #if UNITY_6000_0_OR_NEWER.

8bb47fd

benoitalain force-pushed the isxb-704-scrollwheelwindowsrawvaluesupport branch from 50f03a1 to 8bb47fd Compare June 5, 2024 15:35

Added missing #if UNITY_6000_0_OR_NEWER in InputForUITests

3bb8c4a

benoitalain requested a review from duckets June 6, 2024 14:17

Added changes to CHANGELOG.md

8d81713

ekcoh approved these changes Jun 11, 2024

View reviewed changes

Merge branch 'develop' into isxb-704-scrollwheelwindowsrawvaluesupport

0084249

benoitalain requested a review from Pauliusd01 June 21, 2024 13:45

duckets added 3 commits June 21, 2024 15:24

Improved Scroll Wheel Delta UI tooltip text

4c2f553

Improved API docs for ScrollDeltaBehavior

a824b02

fixed bad xml tag

0ff0f79

duckets approved these changes Jun 21, 2024

View reviewed changes

Pauliusd01 requested changes Jun 25, 2024

View reviewed changes

Pauliusd01 self-requested a review July 2, 2024 14:30

benoitalain added 2 commits July 2, 2024 15:49

Replaced UNITY_6000_0_OR_NEWER by asmdef versionDefines >= 6000.0.9

8251776

Merge branch 'isxb-704-scrollwheelwindowsrawvaluesupport' of github.c…

40a5cbc

…om:Unity-Technologies/InputSystem into isxb-704-scrollwheelwindowsrawvaluesupport # Conflicts: # Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs

Pauliusd01 approved these changes Jul 3, 2024

View reviewed changes

Merge branch 'develop' into isxb-704-scrollwheelwindowsrawvaluesupport

b66e70c

benoitalain added 2 commits July 3, 2024 15:22

Fixed missed #if replacement in UITests.

cf9452e

formatting

7d83c63

benoitalain merged commit 78e4e0e into develop Jul 4, 2024
77 of 79 checks passed

benoitalain deleted the isxb-704-scrollwheelwindowsrawvaluesupport branch July 4, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: ISXB-704 scroll wheel support windows raw input value #1946

FIX: ISXB-704 scroll wheel support windows raw input value #1946

benoitalain commented Jun 4, 2024 •

edited

Loading

unity-cla-assistant commented Jun 4, 2024 •

edited

Loading

ekcoh left a comment

ekcoh Jun 4, 2024

ekcoh Jun 4, 2024

ekcoh Jun 4, 2024

ekcoh Jun 4, 2024

benoitalain Jun 5, 2024

ekcoh Jun 11, 2024

ekcoh Jun 4, 2024

ekcoh Jun 4, 2024

benoitalain Jun 5, 2024

ekcoh Jun 11, 2024

ekcoh Jun 4, 2024

ekcoh commented Jun 4, 2024

ekcoh left a comment

ekcoh Jun 11, 2024

ekcoh Jun 11, 2024

duckets left a comment

Pauliusd01 left a comment •

edited

Loading

benoitalain commented Jun 25, 2024

Pauliusd01 commented Jun 26, 2024

benoitalain commented Jul 2, 2024

Pauliusd01 left a comment

FIX: ISXB-704 scroll wheel support windows raw input value #1946

FIX: ISXB-704 scroll wheel support windows raw input value #1946

Conversation

benoitalain commented Jun 4, 2024 • edited Loading

Description

Changes made

Notes

Checklist

unity-cla-assistant commented Jun 4, 2024 • edited Loading

ekcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekcoh commented Jun 4, 2024

ekcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duckets left a comment

Choose a reason for hiding this comment

Pauliusd01 left a comment • edited Loading

Choose a reason for hiding this comment

benoitalain commented Jun 25, 2024

Pauliusd01 commented Jun 26, 2024

benoitalain commented Jul 2, 2024

Pauliusd01 left a comment

Choose a reason for hiding this comment

benoitalain commented Jun 4, 2024 •

edited

Loading

unity-cla-assistant commented Jun 4, 2024 •

edited

Loading

Pauliusd01 left a comment •

edited

Loading