Skip to content

Added EvaluateMagnitude override to Vector3Composite to fix issues with callbacks #1763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Added EvaluateMagnitude override to Vector3Composite to fix issues with callbacks #1763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65a0efb
Added tests to exercise Vector2 / Vector3 cancel behaviour
Secticide Oct 2, 2023
359ebe9
Fixed issue with Vector3Composite cancel callbacks and made some mino…
Secticide Oct 2, 2023
ce8785f
Added changelog entry
Secticide Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8057,6 +8057,39 @@ public void Actions_Vector2Composite_RespectsButtonPressurePoint()
Assert.That(value.Value, Is.EqualTo(Vector2.left));
}

[Test]
[Category("Actions")]
public void Actions_Vector2Composite_WithKeyboardKeys_CancelOnRelease()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent to see you've added tests to cover this functionality which seem to have been missing.

{
var keyboard = InputSystem.AddDevice<Keyboard>();

// Set up classic WASD control.
var action = new InputAction();
action.AddCompositeBinding("Dpad")
.With("Up", "<Keyboard>/w")
.With("Down", "<Keyboard>/s")
.With("Left", "<Keyboard>/a")
.With("Right", "<Keyboard>/d");
action.Enable();

bool wasCanceled = false;
action.canceled += ctx => { wasCanceled = true; };

// Test all directions to ensure they are correctly canceled
var keys = new Key[] { Key.W, Key.A, Key.S, Key.D };
foreach (var key in keys)
{
wasCanceled = false;
InputSystem.QueueStateEvent(keyboard, new KeyboardState(key));
InputSystem.Update();

InputSystem.QueueStateEvent(keyboard, new KeyboardState());
InputSystem.Update();

Assert.That(wasCanceled, Is.EqualTo(true));
}
}

[Test]
[Category("Actions")]
public void Actions_CanCreateComposite_WithPartsBeingOutOfOrder()
Expand Down Expand Up @@ -8915,6 +8948,41 @@ public void Actions_CanCreateVector3Composite()
Is.EqualTo(new Vector3(1, -1, -1)).Using(Vector3EqualityComparer.Instance));
}

[Test]
[Category("Actions")]
public void Actions_Vector3Composite_WithKeyboardKeys_CancelOnRelease()
{
var keyboard = InputSystem.AddDevice<Keyboard>();

// Set up classic WASD control (with QE for forward / backward).
var action = new InputAction();
action.AddCompositeBinding("3DVector")
.With("Forward", "<Keyboard>/q")
.With("Backward", "<Keyboard>/e")
.With("Up", "<Keyboard>/w")
.With("Down", "<Keyboard>/s")
.With("Left", "<Keyboard>/a")
.With("Right", "<Keyboard>/d");
action.Enable();

bool wasCanceled = false;
action.canceled += ctx => { wasCanceled = true; };

// Test all directions to ensure they are correctly canceled
var keys = new Key[] { Key.Q, Key.E, Key.W, Key.A, Key.S, Key.D };
foreach (var key in keys)
{
wasCanceled = false;
InputSystem.QueueStateEvent(keyboard, new KeyboardState(key));
InputSystem.Update();

InputSystem.QueueStateEvent(keyboard, new KeyboardState());
InputSystem.Update();

Assert.That(wasCanceled, Is.EqualTo(true));
}
}

[Test]
[Category("Actions")]
public void Actions_CanSerializeAndDeserializeActionMapsWithCompositeBindings()
Expand Down
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed exiting empty input fields for actions, action maps and composites in the input action asset editor.
- Fixed an issue where selecting an Action in the Input Action Asset Editor tree-view and then pressing ESC to unselect would throw an `InvalidOperationException`.
- Fixed an issue where selecting an Action Map in the Input Action Asset Editor list and then pressing ESC to unselect would print an `NullReferenceException` to the Debug console.
- Fixed case [ISXB-251](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-251) (Action only calls started & performed callbacks when control type is set to Vector3Composite). `EvaluateMagnitude` wasn't overridden for Vector3Composite, also made some minor changes to Vector3Composite and Vector2Composite for consistency.

## [1.8.0-pre.1] - 2023-09-04

Expand Down
10 changes: 5 additions & 5 deletions Packages/com.unity.inputsystem/InputSystem/Actions/Composites/Vector2Composite.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public class Vector2Composite : InputBindingComposite<Vector2>
/// </remarks>
// ReSharper disable once MemberCanBePrivate.Global
// ReSharper disable once FieldCanBeMadeReadOnly.Global
[InputControl(layout = "Axis")] public int up = 0;
[InputControl(layout = "Axis")] public int up;

/// <summary>
/// Binding for the button represents the down (that is, <c>(0,-1)</c>) direction of the vector.
Expand All @@ -63,7 +63,7 @@ public class Vector2Composite : InputBindingComposite<Vector2>
/// </remarks>
// ReSharper disable once MemberCanBePrivate.Global
// ReSharper disable once FieldCanBeMadeReadOnly.Global
[InputControl(layout = "Axis")] public int down = 0;
[InputControl(layout = "Axis")] public int down;

/// <summary>
/// Binding for the button represents the left (that is, <c>(-1,0)</c>) direction of the vector.
Expand All @@ -73,15 +73,15 @@ public class Vector2Composite : InputBindingComposite<Vector2>
/// </remarks>
// ReSharper disable once MemberCanBePrivate.Global
// ReSharper disable once FieldCanBeMadeReadOnly.Global
[InputControl(layout = "Axis")] public int left = 0;
[InputControl(layout = "Axis")] public int left;

/// <summary>
/// Binding for the button that represents the right (that is, <c>(1,0)</c>) direction of the vector.
/// </summary>
/// <remarks>
/// This property is automatically assigned by the input system.
/// </remarks>
[InputControl(layout = "Axis")] public int right = 0;
[InputControl(layout = "Axis")] public int right;

[Obsolete("Use Mode.DigitalNormalized with 'mode' instead")]
public bool normalize = true;
Expand Down Expand Up @@ -124,7 +124,7 @@ public class Vector2Composite : InputBindingComposite<Vector2>
/// </code>
/// </example>
/// </remarks>
public Mode mode;
public Mode mode = Mode.DigitalNormalized;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a API/behaviorial change (may not be a valid patch) or is it just a case of explicit initialisation? (fine)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I answer my own question, it seems like its not a change since Mode.DigitalNormalized was assigned value zero which would be equivalent to default(Mode), hence this is better in my opinion since explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit, I'm a little sad that Vector2Composite and Vector3Composite both have a Mode enum with the same values (although they have been re-ordered for Vector2) and that the mode defaults are different.

But yes, this is just a cosmetic change for explicitness and consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that inconsistency is indeed sad, something to address when a breaking change can be done.


/// <inheritdoc />
public override Vector2 ReadValue(ref InputBindingCompositeContext context)
Expand Down
7 changes: 7 additions & 0 deletions Packages/com.unity.inputsystem/InputSystem/Actions/Composites/Vector3Composite.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,13 @@ public override Vector3 ReadValue(ref InputBindingCompositeContext context)
}
}

/// <inheritdoc />
public override float EvaluateMagnitude(ref InputBindingCompositeContext context)
{
var value = ReadValue(ref context);
return value.magnitude;
}

/// <summary>
/// Determines how a <c>Vector3</c> is synthesized from part controls.
/// </summary>
Expand Down