-
Notifications
You must be signed in to change notification settings - Fork 318
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.
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
NEW: Add Android backButtonLeavesApp to New Input #1914
NEW: Add Android backButtonLeavesApp to New Input #1914
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manugildev Thanks for submitting this PR. Would you mind filling in the PR template to aid the review process for this new functionality and API surface being proposed? Instructions for filling in the PR template are available in the PR itself but also here: https://github.com/Unity-Technologies/InputSystem/blob/develop/.github/pull_request_template.md
Clearly describing the purpose of the PR and motivating the changes made makes it simpler to understand and review.
In addition som initial questions:
- This is adding a new type of device, AndroidDevice that communicates with the native backend/run-time via IOCTL command GCC. However I do not see any Unity version dependent #ifdef checks present. Is this IOCTL command available back to Unity 2019 which the Input System package is still compliant with? If not, please guard the availability of this feature under appropriate compile-time version checks if intended to just return default if runtime is not implementing the IOCTL command and document from which Unity version this is expected to work, or guard the API under a certain UNITY version to control availability.
- Noted that CHANGELOG.md do not contain an addition under ADD section describing the addition, would it be possible to get that added?
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/GetCustomCommand.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/GetCustomCommand.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/AndroidDevice.cs
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/AndroidSupport.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/GetCustomCommand.cs
Outdated
Show resolved
Hide resolved
P.S you probably need to some tests to validate SetCustomCommand/GetCustomCommand works fine |
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/InputSettingsAndroid.cs
Show resolved
Hide resolved
971e6e3
to
ab07662
Compare
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/AndroidDevice.cs
Show resolved
Hide resolved
a28f01d
to
04e1db1
Compare
37c9174
to
6f93df2
Compare
…nLeavesApp on device
6f93df2
to
2dc6312
Compare
public class AndroidDevice : InputDevice | ||
{ | ||
[InputControl(synthetic = true)] | ||
public ButtonControl button { get; protected set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this? I see it's a public API, how would user use it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to get rid of this ButtonControl. InputDevices need to have AT LEAST one control. If not they throw an exception. In our case, this controls is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manugildev Could you provide some additional context regarding what exception/trace you did get?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekcoh I have tried pretty much everything but I couldn't find a way to have no ButtonControl
. This is the exception I am hitting (code).
Could not create a device for 'AndroidDevice (Android)' (exception: System.InvalidOperationException: Control '/AndroidDevice' with layout 'AndroidDevice' has no size set and has no children to compute size from
at UnityEngine.InputSystem.Layouts.InputDeviceBuilder.ComputeStateLayout (UnityEngine.InputSystem.InputControl control)
at UnityEngine.InputSystem.Layouts.InputDeviceBuilder.InstantiateLayout (UnityEngine.InputSystem.Layouts.InputControlLayout layout, UnityEngine.InputSystem.Utilities.InternedString variants, UnityEngine.InputSystem.Utilities.InternedString name, UnityEngine.InputSystem.InputControl parent)
at UnityEngine.InputSystem.Layouts.InputDeviceBuilder.Setup (UnityEngine.InputSystem.Utilities.InternedString layout, UnityEngine.InputSystem.Utilities.InternedString variants, UnityEngine.InputSystem.Layouts.InputDeviceDescription deviceDescription)
There is in fact a test that check that a DeviceWithNoControls fails to be created: Controls_CanTurnControlPathIntoHumanReadableText_EvenIfLayoutCannotBeFoundOrHasErrors
Add/change deprecation message to Input.bindings backButtonLeavesApp once ready to land. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left another round of review comments. Generally I struggle a bit to see how this fits API-wise. It seems like we are transforming a cross-platform interface into a OS-specific here and introducing what I assume is supposed to be a non-removable device representing the system.
Thanks for updating migration doc but I also think there is some guidance needed to explain how this boolean flag is expected to be used for the preview render feature. At the moment I do not fully understand how I should reason about this setting or what I need to do differently depending on how this flag is set. Happy to follow-up via DM or have a chat and maybe a demo that helps clarify this.
Packages/com.unity.inputsystem/InputSystem/Devices/Commands/GetCustomCommand.cs
Show resolved
Hide resolved
public class AndroidDevice : InputDevice | ||
{ | ||
[InputControl(synthetic = true)] | ||
public ButtonControl button { get; protected set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manugildev Could you provide some additional context regarding what exception/trace you did get?
/// Project-wide input settings for the Android platform. | ||
/// </summary> | ||
[Serializable] | ||
public class AndroidSettings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be something Android specific when we advertise it as cross-platform on 3 operating systems for the UnityEngine.Input counterpart? https://docs.unity3d.com/ScriptReference/Input-backButtonLeavesApp.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekcoh backButtonLeavesApp
is a legacy input setting. It's only living on Android land now, it is needed for predictive back gesture
to work (which is something we have implemented for Android 14 as per Google's request).
Even if we have the code for Windows
platforms still living in unity/unity, there are no supported device that has a physical back button anymore. They only had them in Windows Phones and early Windows RT tablets, none of which are supported anymore. This has been confirmed by the windows-platform team.
So it makes sense that moving into the new Input System, we only added in the only device that still has a back button functionality, Android.
Packages/com.unity.inputsystem/InputSystem/Plugins/Android/InputSettingsAndroid.cs
Show resolved
Hide resolved
6b5b8fb
to
fb47d36
Compare
(force push the only contains auto formatter changes) |
fb47d36
to
98add9b
Compare
@ekcoh I did some new changes based on the code review. Let me know if this is ready so I can proceed with landing the |
Sorry for providing this feedback at this stage, but I would suggest we do not land this PR for the following reasons:
Please let me know or DM me if there is some value you see with this additional API in addition to what already exists in Unity.Input? |
@ekcoh Hello I stumbled upon this thread as I was searching for the Input System's equivalent to It is very confusing to say "No corresponding API yet." in the Input System doc without providing a workaround to handle back button on Android, especially considering back button behavior is required by Google Play Store. If Unity is not supporting |
@seeyam-q thanks for bringing this up! Actually, last friday we merged a change to make We are expecting these changes to land in |
Description
Add
backButtonLeavesApp
support for New Input System.Changes made
This PR creates a new
AndroidDevice
that is registered during initialization. ThisAndroidDevice
represents a handheld device, and can be use to store device specific input functions that are not part of other input devices such as the keyboard, gamepad or sensors.We also added
SetCustomCommand
andGetCustomCommand
used to be able to set and get respectively a payload between the InputSystem package and runtime. This is then used to set and getbackButtonLeavesApp
to the native runtime. This two new commands are general purpose commands, and can be used for any other input device.backButtonLeavesApp
, is just a boolean flag needed for Predictive Back Gesture (PBG) support to work on Android. This allows users to preview the destination or other result of a back gesture before fully completing it, allowing them to decide whether to continue or stay in the current view.Notes
This new API can be used as such:
Checklist
Before review:
Changed
,Fixed
,Added
sections.([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
.