FIX: uum 138143 button press points for stickcontrol and vector2 add functionality#2411
Conversation
…ld used for stick controls and Vector2. Test
…at a user declared as an interaction.
|
The diff contains approximately 325,668 tokens (~977,005 characters), which exceeds the maximum limit of 150,000 tokens. Please consider splitting this PR into smaller, focused changes. 🤖 Helpful? 👍/👎 |
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2411 +/- ##
===========================================
+ Coverage 78.13% 78.91% +0.78%
===========================================
Files 483 756 +273
Lines 98770 139229 +40459
===========================================
+ Hits 77169 109867 +32698
- Misses 21601 29362 +7761 Flags with carried forward coverage won't be shown. Click here to find out more.
|
Darren-Kelly-Unity
changed the title
…-stickcontrol-and-vector2-add-functionality # Conflicts: # Packages/com.unity.inputsystem/InputSystem/Runtime/Controls/InputControlExtensions.cs
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
@Darren-Kelly-Unity Would it be possible to get rid of all the whitespace diffs on this PR? It would be much easier to review if this was corrected.
…-stickcontrol-and-vector2-add-functionality
…-stickcontrol-and-vector2-add-functionality
… it was implicitly implemented.
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Reviewed, I had quite a lot of questions and some remaining from last review so leaving this as comment-only for now.
| /// <summary> | ||
| /// Effective press threshold: <see cref="pressPoint"/> when set, otherwise the global default. | ||
| /// </summary> | ||
| float pressPointOrDefault { get; } |
There was a problem hiding this comment.
Why do we need two? Would it be better to have one, e.g. pressPoint that returns default if not having a layout-configured press threshold? Since both go through an interface anyway I wouldn't expect it to be an optimisation?
There was a problem hiding this comment.
This is just following the format that was already there for now, not sure about removing one as it was publicly available before.
There was a problem hiding this comment.
If it was public we cannot remove it now (unless it fits a scheme to deprecate), but I am afraid two variants creates confusion around the interface contract that a user should implement.
There was a problem hiding this comment.
But if the new interface only implemented the previous implicit function:
pressPointOrDefault wouldn't this still work since it takes the same value if available and otherwise fall back?
There was a problem hiding this comment.
Since if (control is IActuationPressPoint actuation) is the new condition also covering Button?
There was a problem hiding this comment.
I can try that and see if it works, you may be right yes!
…ctor2Control.cs Co-authored-by: Håkan Sidenvall <hakan.sidenvall@unity3d.com>
…ctor2Control.cs Co-authored-by: Håkan Sidenvall <hakan.sidenvall@unity3d.com>
…ves." This reverts commit bce3776.
…es test coverage shown." This reverts commit 1243bc4.
…t improves test coverage shown."" This reverts commit db81e01.
…see if it improves test coverage shown.""" This reverts commit 8ff8746.
…-stickcontrol-and-vector2-add-functionality
…-stickcontrol-and-vector2-add-functionality
…-stickcontrol-and-vector2-add-functionality
| |[`InputAction.IsPressed()`](xref:UnityEngine.InputSystem.InputAction.IsPressed)|True if the level of [actuation](xref:UnityEngine.InputSystem.InputControl.EvaluateMagnitude) on the action has crossed the applicable press point and did not yet fall to or below the [release threshold](xref:UnityEngine.InputSystem.InputSettings.buttonReleaseThreshold).| | ||
| |[`InputAction.WasPressedThisFrame()`](xref:UnityEngine.InputSystem.InputAction.WasPressedThisFrame)|True if the level of [actuation](xref:UnityEngine.InputSystem.InputControl.EvaluateMagnitude) on the action has, at any point during the current frame, reached or gone above the applicable press point.| | ||
| |[`InputAction.WasReleasedThisFrame()`](xref:UnityEngine.InputSystem.InputAction.WasReleasedThisFrame)|True if the level of [actuation](xref:UnityEngine.InputSystem.InputControl.EvaluateMagnitude) on the action has, at any point during the current frame, gone from being at or above the applicable press point to at or below the [release threshold](xref:UnityEngine.InputSystem.InputSettings.buttonReleaseThreshold).| | ||
| The applicable press point is chosen from, in order: a positive [`pressPoint`](xref:UnityEngine.InputSystem.Controls.ButtonControl.pressPoint) on the driving control when it is a [ButtonControl](xref:UnityEngine.InputSystem.Controls.ButtonControl), or a positive [`pressPoint`](xref:UnityEngine.InputSystem.Controls.Vector2Control.pressPoint) when it is a [Vector2Control](xref:UnityEngine.InputSystem.Controls.Vector2Control) or [StickControl](xref:UnityEngine.InputSystem.Controls.StickControl); otherwise, if the binding lists one or more [Press](xref:UnityEngine.InputSystem.Interactions.PressInteraction) interactions, the `pressPoint` from the first in the binding's interaction order whose `pressPoint` is greater than zero (any interactions before it that are not `Press`, or `Press` with the default `pressPoint` of zero, are skipped for this step); otherwise [defaultButtonPressPoint](xref:UnityEngine.InputSystem.InputSettings.defaultButtonPressPoint). On [composites](xref:input-system-action-bindings#composite-bindings), interaction parameters are taken from the composite binding. |
There was a problem hiding this comment.
Looks like the generated preview put that line in the array.
Make sure to add doc to review the doc changes
| /// | ||
| /// Finally, note that custom button press points of controls (see <see cref="ButtonControl.pressPoint"/>) | ||
| /// are respected and will take precedence over <see cref="InputSettings.defaultButtonPressPoint"/>. | ||
| /// Press threshold (based on <see cref="InputControl.EvaluateMagnitude()"/> for the driving control): for a |
There was a problem hiding this comment.
1st copy-paste from line 1157. This is a maintenance burden.
I'd recommend going for only defining for IsPressed() and then just a oneline:
/// The press threshold is resolved as described in remarks.
This is what you did for WasPressedThisDynamicUpdate and WasReleasedThisDynamicUpdate.
Note that the block (starting by Also note that because ...) above is also duplicated and would benefit from the same treatment.
| /// | ||
| /// Finally, note that custom button press points of controls (see <see cref="ButtonControl.pressPoint"/>) | ||
| /// are respected and will take precedence over <see cref="InputSettings.defaultButtonPressPoint"/>. | ||
| /// Press threshold (based on <see cref="InputControl.EvaluateMagnitude()"/> for the driving control): for a |
There was a problem hiding this comment.
2nd copy-paste from line 1157. Same as above.
| /// | ||
| /// Finally, note that custom button press points of controls (see <see cref="ButtonControl.pressPoint"/>) | ||
| /// are respected and will take precedence over <see cref="InputSettings.defaultButtonPressPoint"/>. | ||
| /// Press threshold (based on <see cref="InputControl.EvaluateMagnitude()"/> for the driving control): for a |
There was a problem hiding this comment.
Very dense, even with the formatting in doc. I had to write it down myself and split it in a bullet point to understand. I don't know if it's possible to have a numbered list in xml doc? Would be a question for doc team.
I asked Claude, supposedly this is the way in xml doc:
/// The press threshold is resolved in priority order:
/// <list type="number">
/// <item>A positive <see cref="ButtonControl.pressPoint"/> or <see cref="Vector2Control.pressPoint"/>
/// on the driving control.</item>
/// <item>The <see cref="PressInteraction.pressPoint"/> from the first
/// <see cref="PressInteraction"/> on the binding with a value greater than zero.
/// For composites, interactions are read from the composite binding.</item>
/// <item><see cref="InputSettings.defaultButtonPressPoint"/>.</item>
/// </list>However I am doubtful that our in house documentation parser is able to work with it because it only support a subsets of tag and rules.
| /// <see cref="UnityEngine.InputSystem.InputAction.IsPressed"/> when this control drives the action. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// By default, this property is set to -1. If the value of the property is negative, |
There was a problem hiding this comment.
nit
| /// By default, this property is set to -1. If the value of the property is negative, | |
| /// By default, this field is set to -1. If the value of the property is negative, |
|
The tickets mentions a deadzone issue: "Deadzone(0.3) WasPerformedThisFrame triggers incorrectly at Magnitude 0.000". Have you tried to repro that issue? I could not find tests for it. |
4 participants
Purpose of this PR
Fixes UUM-138143 —
InputAction.IsPressed()/ in-progress style checks could fire at the wrong effective magnitude for Vector2 / stick bindings when developers setpressPointon the control or onPressInteraction, instead of honoring those thresholds consistently.Introduces
IActuationPressPoint, implements it onButtonControl,Vector2Control, and (viaVector2Control)StickControl, and threadspressPoint/pressPointOrDefaultthroughInputAction,InputActionState, andInputControlExtensionsso press-style polling matches the documented interaction defaults. AddsActuationPressPointTestsand refreshesRespondingToActions.md.Release Notes
InputAction.IsPressed()(and related press / in-progress behavior) for actions bound toVector2ControlandStickControlnow respects per-controlpressPointand bindingPressInteraction.pressPoint, aligning withButtonControland fixing incorrect early triggers when rewriting default interaction thresholds.Functional Testing status
ActuationPressPointTestsand updates inCoreTests_Actions/CoreTests_Controlscover press-point actuation paths.IsPressedbefore processed magnitude reaches the configured press threshold for vector / stick cases.Performance Testing Status
I have added performance testing for the hot path methods that Hakan flagged and I don't see any major issues when comparing. There is a slight increase in cost of performance of about 0.1ms for 1000 iterations on the GetActuationPressThreshold() method that was asked to be checked.
No new per-frame allocations identified in scope.
Overall Product Risks
Technical Risk: 2 — Touches
InputActionStateand widely used press APIs; edge cases around magnitude, deadzones, and interactions need regression confidence.Halo Effect: 2 — Any title using
IsPressed/WasPressedThisFrameon Vector2 / stick actions may see timing changes until bindings are tuned; behavior is intended to match documentedpressPointsemantics.Class diagram
Git range analyzed:
origin/develop...HEAD(4 commits).Mermaid class diagram (GitHub)
classDiagram direction TB class IActuationPressPoint { <<interface>> +pressPoint +pressPointOrDefault } class ButtonControl { +pressPoint +pressPointOrDefault } class Vector2Control { +pressPoint +pressPointOrDefault } class StickControl { } class InputAction { +IsPressed() +WasPressedThisFrame() +WasReleasedThisFrame() } class InputActionState { } class InputControlExtensions { <<static>> } IActuationPressPoint <|.. ButtonControl IActuationPressPoint <|.. Vector2Control Vector2Control <|-- StickControl InputAction --> InputActionState : uses InputControlExtensions ..> IActuationPressPoint : pressPointOrDefaultDocumentation Impact
Changes analyzed:
origin/develop...HEAD(4 commits).User-facing: New public
IActuationPressPoint;InputActionpress-helper remarks/behavior;ButtonControl,Vector2Control,StickControldocs aroundpressPoint;RespondingToActions.mdupdated for polling vspressPoint.Documentation updates in this PR
RespondingToActions.md— ClarifiesIsPressed/ frame helpers vs interactions andpressPoint; review wording and cross-refs after merge.Follow-up (optional)
IActuationPressPointcallout in a control reference page, add a short subsection and link from Responding to Actions.Jira (fetched via MCP): Bug, In Progress, TBD priority, assignee Darren Kelly, label Input-Actions. Ticket summary: InputAction.IsPressed() and IsInProgress() triggers on wrong values when rewriting the default values of Vector2 interactions.