Add API additions to assist with polling workflow #1795
Conversation
…es of API_MinorVersionsHaveNoBreakingChanges
…e of InputAction.CallbackContext
… go null when not in progress - Added tests for activeValueType
…sInputActionAssetChanges
…Frame and WasPressed/ReleasedThisFrame
… the action is being disabled to avoid setting unperformed
…ith additional verification of phase transitions in mega test
* FIX: UITK Listen button not working with Any control types * Remove jira links for ISX project issues from changelog
…posite or control magnitude
…oject wide actions are available (#1791) * CHANGE: Restructured main Input System Package settings node to be Input Actions and Settings to be a child node when Project Wide Actions are available.
* Add Reset() to default values This event is called once the component is added for the first time https://docs.unity3d.com/2022.3/Documentation/ScriptReference/MonoBehaviour.Reset.html * Check for changes on "Reset" not captured by EndChangeCheck() Without this, changes will not be detected when a user selects "Reset" in the Component Inspector. * Update CHANGELOG * Add project-wide actions define * Only reference InputSystem.Editor on the Editor
…etween just canceled and canceled+unperformed
…tionally test with the Press interaction
…athEditor` editor inside UITK `IMGUIContainer` when deleting binding (#1789) * Added try-catch block to stop exceptions from being thrown * Added Changelog entry --------- Co-authored-by: Håkan Sidenvall <hakan.sidenvall@unity3d.com> Co-authored-by: James McGill <jamesmcgill@users.noreply.github.com>
…1564) (#1793) * restore default settings * made left click work on menu button * hide menu for project input action asset * fixed icon for reset menu * added changelog * PR refactor * applied event change for postSaveAction as well --------- Co-authored-by: James McGill <jamesmcgill@users.noreply.github.com>
…th the default interaction is started
# Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md # Packages/com.unity.inputsystem/Documentation~/Actions.md
…ter merge - Also fixed dead link to default actions
…more technically correct
Co-authored-by: Dave Ruddell <94039157+vrdave-unity@users.noreply.github.com>
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Thanks for this PR @chris-massie and sorry for the delayed review and associated feedback. In general I think these additions make a lot of sense and adds value. However I have some comments and questions as can be seen in the review.
Adding review as a comment for now, approving after concluded questions/discussion aspects.
Packages/com.unity.inputsystem/InputSystem/Actions/Composites/AxisComposite.cs
Show resolved
Hide resolved
| There are two main techniques you can use to respond to Actions in your project. These are to either use **polling** or an **event-driven** approach. | ||
|
|
||
| - The **Polling** approach refers to the technique of repeatedly checking the current state of the Actions you are interested in. Typically you would do this in the Update() method of a Monobehaviour script. | ||
| - The **Polling** approach refers to the technique of repeatedly checking the current state of the Actions you are interested in. Typically you would do this in the `Update()` method of a `MonoBehaviour` script. |
There was a problem hiding this comment.
Not completely related to this PR, but maybe we should provide some additional guidance here that polling may be used when the order of actions performed is irrelevant, while event-driven callback approach is appropriate when order of actions during a frame is relevant or what do you think?
There was a problem hiding this comment.
I thought it was still frame based? Does Input System actually know if A was pressed before B if they were both pressed in the same frame?
I don't think I know enough about the details to update the documentation about that in this PR.
There was a problem hiding this comment.
When observing events yes, but not when using polling APIs. I will take a separate. note on addressing this.
| /// <seealso cref="InputControl.valueType"/> | ||
| /// <seealso cref="InputBindingComposite.valueType"/> | ||
| /// <seealso cref="activeControl"/> | ||
| public unsafe Type activeValueType |
There was a problem hiding this comment.
I would assume this type potentially changes multiple times during a frame, i.e. if using callbacks there might be multiple controls active at certain points in time. For a polling scenario I assume only the "most" or "last" actuated control would be influencing the type here or how is this intended to be interpreted? If this is the intention, maybe it should be mentioned in the doc?
There was a problem hiding this comment.
Great you covered composite behavior in the doc
There was a problem hiding this comment.
Not sure I completely understand how this is intended to be used though, would it be possible to incorporate it into the example or at least elaborate a bit on a typical use-case so I understand better?
There was a problem hiding this comment.
I added some notes to the xml-doc. Basically we have some input actions where we permit multiple bindings of different control types. In XRI, we allow either a Vector2 or float trigger for "select", but we can't call ReadValue<float> or ReadValue<Vector2> without it potentially throwing an exception. We have to wrap it in a try/catch. It would be nicer if we could query the expected type directly to know which version to call.
This value is already exposed in the CallbackContext with CallbackContext.valueType, but this property lets us get it in a polling way instead of having to be in the performed callback method.
There was a problem hiding this comment.
Totally agree or potentially in the long run avoid having non-typed actions in general. Thanks for explaining.
| /// <summary> | ||
| /// Read the current amount of actuation of the control that is driving this action. | ||
| /// </summary> | ||
| /// <returns>Returns the current level of control actuation (usually [0..1]) or -1 if |
There was a problem hiding this comment.
Based on the code below it seems to return 0f and not -1f if not supported?
There was a problem hiding this comment.
The magnitude property can be -1 even though haveMagnitude is true. The Actions_CanQueryMagnitudeFromAction_WithQuaternionControl_ReturnsInvalidMagnitude test handles this scenario where the attitude Quaternion control is actuated but doesn't have a meaningful magnitude value, so it returns -1.
To me, it seemed like returning 0 in this method was preferable to returning -1 if no control is actuated, so that's why I used that value in this method for the default.
Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs
Outdated
Show resolved
Hide resolved
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Also thanks for investing in providing code coverage via automated tests. I believe its a necessity for this rather complex evaluation functions.
jamesmcgill
left a comment
jamesmcgill
left a comment
There was a problem hiding this comment.
Thanks for this @chris-massie and also sorry for the long delay.
A lot of good changes especially the test coverage and docs.
I would ask to remove the newFindAction if you can live without it as it doesn't work quite like the others and exposes some internal behaviour.
As there is a lot of docs I would add @duckets for a quick review.
Maybe he also has some thoughts on Unperformed as a name. I feel it's somehow off, but I don't really have any better ideas.
Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs
Outdated
Show resolved
Hide resolved
|
Approved after speaking to Chris and agreeing to remove FindAction for now and perhaps revisit in a future PR |
…o much internal behavior
…th enable/disable within the same frame
|
@vrdave-unity OK to merge this one now that all checks pass or is your requested change from review still valid? If OK, would you mind approving this PR. |
Description
This adds new methods and properties to make the polling workflow easier. We plan on using these new methods in XRI.
InputAction.activeValueTypewill allow us to callReadValue<TValue>()without needing to wrap it in atry/catch InvalidOperationExceptionfor code that uses anInputActionthat allows for either a Vector2 or float binding.InputAction.GetMagnitudewill allow us to get the float value of a Vector2 action without having to recompute the expensive (sqrt) magnitude again. If you ignore processors, this lets us also use another method for getting a [0,1] float value with that Vector2 or float action.InputAction.WasCompletedThisFramewill allow us to know the frame that the action was released, but be more correct than usingWasReleasedThisFramefor the non-default interactions like Hold or Sector in XRI.Changes made
To replicate the functionality of
WasReleasedThisFramewhere it does not return true when the currently pressed action is disabled, I needed to modify the signature of someinternal/privatemethods to pass down the next phase after the targettoPhase. This lets us skip setting theuintof the frame count when the action is changing fromperformedtocanceleddue to going to thedisabledphase.Notes
We could potentially also add
WasCanceledThisFrameandWasStartedThisFrame, but I didn't do so in this branch.Pass-Through and Value type actions with the default interaction will never return
trueforWasCompletedThisFrame.Performedeven when returning back to the default value (i.e. it does not transition toCanceled). There is even a codeREVIEWcomment that questions that behavior. That seems like odd, unexpected behavior and is something that should be considered for change in a major version change.WaitingtoStarted, thenPerformedin the same frame, and back toStartedwithout firing an event. It stays inStartedand only invokesPerformedwhen the value changes while still actuated. Due to that, it's not clear how to track that the value is no longer performed, so Value types also always returnfalseforWasCompletedThisFrame. It would be difficult to let theChangePhaseOfActionInternalmethod know that it was going toCanceledfromStartedbecause of the default interaction Value type to mark it as unperformed, while not doing that for Button type actions or actions with button-like interactions like Press where it should only do so when in thePerformedphase. So for that reason, default interaction Value types never returntrueforWasCompletedThisFrame.Checklist
Before review:
Changed,Fixed,Addedsections.([case %number%](https://issuetracker.unity3d.com/issues/...)).Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.