NEW: Pen.isSupported, Mouse.isSupported and Touchscreen.isPressureSupported [ISX-2046, ISX-2079] - #2476
NEW: Pen.isSupported, Mouse.isSupported and Touchscreen.isPressureSupported [ISX-2046, ISX-2079]#2476ekcoh wants to merge 2 commits into
Conversation
a022e21 to
790862f
Compare
|
Added @K-Tone and @MorganHoarau for visibility. Not sure branch/PR is done yet. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/review |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Harness Review
The capability-query implementation and its command/cache coverage are coherent, but the current editor-version gate can expose engine-dependent code before the required engine support is available.
Reviewed commit 790862f
🤖 Helpful? 👍/👎
| }, | ||
| { | ||
| "name": "Unity", | ||
| "expression": "6000.7.0a6", |
There was a problem hiding this comment.
This gate must use the first Editor version that actually contains the engine endpoint. As written, an ordinary 6000.7.0a6 (or newer) build without the pending engine change enables both assemblies, which then reference the engine-only NativeInputCapabilities/CapabilityState symbols and fail compilation. The PR description confirms that engine change has not landed and that this version needs updating; set the threshold to the shipped engine version before merging.
🤖 Helpful? 👍/👎
There was a problem hiding this comment.
Correct, and it is a genuine blocker rather than a nit. Cannot be resolved yet, since the version this should point at does not exist until the engine side lands (unity PR #120240, same ticket).
What I have done instead of leaving it implicit: added a ////TODO at the top of InputSystem/Runtime/Devices/Commands/InputCapabilitySupport.cs spelling out that the expression is a local development Editor version and must be set to the shipping version before merge. It went there because asmdef files are JSON and cannot carry a comment of their own, which is part of why this is easy to miss.
The PR description also calls it out, and the dependency direction is stated at the top: this branch must not merge before the engine change.
Worth keeping this thread open until the version is pinned, rather than resolving it on the strength of a TODO.
0b9fb1c to
f61a89b
Compare
ekcoh
changed the title
…ported [ISX-2046] Legacy UnityEngine.Input conflates "is X supported on this platform" with "is an X present right now", and the Input System had no equivalent for the first question at all. Input.stylusTouchSupported reports true on any iPad new enough to pair an Apple Pencil, whether or not one is paired. Input.mousePresent is genuine detection on Windows and the consoles but a hardcoded true on macOS and Linux. These three properties answer the capability question. Presence keeps its existing answer, Device.current != null. The queries go to the engine's system endpoint, which is addressed by a reserved device id that is never registered, so it never appears in the device list and no device can answer a question about the platform. That endpoint lands separately in the engine repository under the same ticket. Touch pressure is answered at platform scope rather than per touchscreen because that is the scope at which the answer exists: every platform sources it from a device model or an OS API property rather than by enumerating digitizers. A per-instance property can be added later, preferring a per-device answer over this one, without changing what is here. The commands and the InputCapabilitySupport tristate are internal rather than public, unlike the rest of the Commands folder. They target the system endpoint, so there is no custom device for a user to answer them for, and keeping them internal avoids freezing a capability model that the low level input API is expected to revisit. The engine answers with a tristate whose Unknown is zero, so an unimplemented query reads as "we do not know" rather than a confident false; the public properties collapse anything other than Supported to false. Both the properties and the plumbing are gated on UNITY_INPUTSYSTEM_SUPPORTS_CAPABILITY_QUERIES, so the API is absent rather than present-and-always-false on engine versions that cannot answer. The version expression is currently the local development engine and must be updated to whichever version the engine side actually ships in. ExecuteGlobalCommand is replaced by ExecuteSystemCommand. The former had no callers, having been orphaned when UseWindowsGamingInputCommand was removed by ISXB-927, and its premise was wrong: it addressed device id 0 on the assumption that the engine routes such commands by FourCC alone, but InputDeviceIOCTL resolves the id against the device registry and 0 is the invalid-device sentinel, so nothing sent there could ever be answered. Capabilities cannot change while the process runs, so each is queried at most once. The cache lives on InputManager rather than in a static, so a domain reload or a test installing a different runtime discards it without needing an explicit reset hook. Tests cover each state including Unknown, that the properties answer with no device added, that a query is never delivered to a real device, that repeated reads issue one command, that nothing answering reports false rather than throwing, and that the enum values, the FourCC codes and the payload size all agree with the engine's own constants.
f61a89b to
95d60ca
Compare
There was a problem hiding this comment.
💡 Harness Review
I examined the capability command marshalling, per-manager cache lifecycle, version-gated public accessors, tests, and the updated migration guidance; I found no new actionable issues beyond the previously reported findings.
Reviewed commit 95d60ca
🤖 Helpful? 👍/👎
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2476 +/- ##
========================================
Coverage 78.95% 78.95%
========================================
Files 767 767
Lines 140780 140821 +41
========================================
+ Hits 111153 111186 +33
- Misses 29627 29635 +8 Flags with carried forward coverage won't be shown. Click here to find out more.
|
| [`Pen.isSupported`](xref:UnityEngine.InputSystem.Pen) and [`Touchscreen.isPressureSupported`](xref:UnityEngine.InputSystem.Touchscreen). | ||
| These never change while the application runs, so read them once and decide whether to offer device-specific | ||
| functionality. | ||
| - **Is a device available to read from right now?** Use `Device.current != null && Device.current.enabled`. |
There was a problem hiding this comment.
I can see this causing a lot of trial and error for users, especially the enabled part. I would suggest adding a new Device.isAvailable to do that check, which would go well with isSupported from a semantic perspective.
There was a problem hiding this comment.
Agreed, and why I mentioned this in the PR description, there called device.IsUsable(), that is doable as an extension method which scales better than adding strongly coupled API - since it goes for all device types. However, its a simple addition and not load bearing but if you think I should fold that in and increase the API surface we commit to I am happy to, regardless, the parity it solved without it IMO.
There was a problem hiding this comment.
Might also be - as you say - root class is sufficient. Leaving this open until you have a chance to respond @MorganHoarau
There was a problem hiding this comment.
Make sense, not a fan of "usable" as a term as I don't think I've ever seen any Unity API use it. I'm not sure to understand why an extension method scales better than adding a new property on Device though. In both case we are exposing a new API. But this could be move as a separate ticket to improve the clarity around the topic.
However, I fear this would never be tackled on the side, so I would still add it here. It is a small addition and I don't see the concept going away anytime soon.
There was a problem hiding this comment.
I am not a fan of "usable" either, and not a fan of "available" either since I would assume its available as soon as its connected, but this also gates on device being enabled.
I take the extension method thing back since I kind of ignored the fact that current design uses OO so having it on the base makes sense in this case instead of doing constrained generics.
I intererpret your reply as YES - add it? (Ignoring the open naming headache)
Review feedback: the docs had accumulated the reasoning behind the change
rather than what a reader needs at the call site.
- InputManager.ExecuteSystemCommand: drop the remarks block. Its first
paragraph was history about the ExecuteGlobalCommand it replaced, and the
second restated what the three call sites just below already show.
- Query{Pen,Mouse,TouchPressure}SupportedCommand: drop the presence clause
that duplicated the summary directly above it, and the platform-scope
rationale that the engine's PlatformSystemCapabilities.h already owns.
Kept the routing note, which is what stops these being sent through
InputDevice.ExecuteCommand like every sibling in the folder, and the
FourCC-must-match constraint, which is hand-mirrored across two repos.
- InputCapabilitySupport: drop the TODO about the unpinned version
expression. The merge gate is the open review thread on the asmdef, which
is enforced; a comment is not.
- Mouse/Pen/Touchscreen.isSupported: trim the remarks to what a caller
needs. The legacy Input Manager comparison belongs in the migration
documentation, which the remaining text now points at for the availability
check. Kept what false means, the concrete undetermined case, and the
main-thread contract.
- InputManager.ExecuteSystemCommand: send through m_Runtime rather than
InputRuntime.s_Instance, matching every other DeviceCommand call site and
staying correct when a test installs its own runtime.
- CHANGELOG: drop the feature explanation and the code snippet, reference
the migration documentation instead, and cite ISX-2079 alongside ISX-2046
since Mouse.isSupported is that ticket's deliverable.
- corresponding-old-new-api.md: qualify the property names in the migration
table, link the legacy properties, and unpin a Unity 5.5 docs URL.
MorganHoarau
left a comment
MorganHoarau
left a comment
There was a problem hiding this comment.
I still think enduser docs can be improve. I won't go over each for now, but I've provided more details for one + recommeded enduser-docs skill.
The doc might also mutate further if you end up adding the new isUsable/isAvailable API.
I would personally add it, but better ask other reviewer their opinion too
| [`Input.GetMouseButtonUp`](https://docs.unity3d.com/ScriptReference/Input.GetMouseButtonUp.html)<br/>Example: `Input.GetMouseButtonUp(0)`|Use [`wasReleasedThisFrame`](xref:UnityEngine.InputSystem.Controls.ButtonControl) on the corresponding mouse button.<br/>Example: `InputSystem.Mouse.current.leftButton.wasReleasedThisFrame` | ||
| [`Input.mousePosition`](https://docs.unity3d.com/ScriptReference/Input-mousePosition.html)|Use [`Mouse.current.position.ReadValue()`](xref:UnityEngine.InputSystem.Mouse)<br/>Example: `Vector2 position = Mouse.current.position.ReadValue();`<br/> **Note:** Mouse simulation from touch isn't implemented yet. | ||
| [`Input.mousePresent`](https://docs.unity3d.com/ScriptReference/Input-mousePresent.html)|No corresponding API yet. | ||
| [`Input.mousePresent`](https://docs.unity3d.com/ScriptReference/Input-mousePresent.html)|Use [`Mouse.isSupported`](xref:UnityEngine.InputSystem.Mouse) to check whether the platform supports mouse input at all.<br/>Example: `if (Mouse.isSupported) ShowMouseSettings();`<br/>**Note:** Not a drop-in replacement; see [Device capability and device availability](#device-capability-and-device-availability) above. Input System does not currently deliver mouse input on iOS, iPadOS or visionOS, so `Mouse.isSupported` is `false` there even though the platform itself supports indirect mice. Requires a recent Editor version. |
There was a problem hiding this comment.
"Not a drop-in replacement;"
This doesn't sound good to me. Make sure to add @suearkinunity so she can review
| /// deciding whether to offer functionality wants the same behaviour in both cases. It never | ||
| /// means the Editor was unable to ask, since the property only exists where it can. | ||
| /// | ||
| /// Whether a mouse is available to read from right now is a separate question, and needs both |
There was a problem hiding this comment.
This does not read good for me as and enduser doc. As an enduser, I don't have time to read and just want access to the relevant information in a minimum effort. For instance, I would expect something like "To know if a mouse is available, use and .".
I would recommend running enduser-docs skill (located in the unity/unity repo's .claude/skills on this branch.
There was a problem hiding this comment.
Trying to be more specific about what is bothering me:
- The current formulation sounds like a meta commentary. It tells the reader that a distinction exists rather than just making the distinction.
- It buries the actionable content.
- There are casual filler (i.e. "read from right now").
2 participants
Description
Legacy
UnityEngine.Inputconflates "is X supported on this platform" with "is an X present right now", and Input System has no equivalent for the first question.Input.stylusTouchSupportedreports true on any iPad new enough to pair an Apple Pencil, paired or not.Input.mousePresentis a hardcodedtrueon Windows, macOS, Linux and WebGL, and genuine detection only on iOS, Android, UWP and the consoles. Windows looks like detection but is not:TouchPhaseEmulation::GetMousePresent()isreturn true;.This adds the capability half.
Pen.isSupportedMouse.isSupportedTouchscreen.isPressureSupportedFor ISX-2046 that is two of its three requested mappings:
stylusTouchSupported→Pen.isSupportedandtouchPressureSupported→Touchscreen.isPressureSupported. The third,multiTouchEnabled→Touchscreen.multiTouchEnabled, is not here. The ticket notes it is a setting rather than a hardware capability, and suppressing additional touches globally is behavioural work rather than a query, so it wants its own change. The migration doc points users atprimaryTouchmeanwhile, with the two behavioural differences spelled out.Mouse.isSupportedis the short-term scope of ISX-2079, whose remaining half is a real presence primitive rather than a capability one.The queries are answered by a system endpoint in the engine: a reserved device id that is never registered, so it never appears in
InputSystem.devicesand no real device can answer a question about the platform. Internally, three internal command structs and an internalInputCapabilitySupporttristate, dispatched through a newInputManager.ExecuteSystemCommand, queried once and cached.Presence is a separate question, and it needs both halves. ISX-2079 is explicit that
Mouse.current != nullis the wrong replacement formousePresent: it tracks the device object's lifecycle, and several platforms register aMouseorPenunconditionally, so it is non-null with no hardware attached. The pattern is:The pair is asymmetric on purpose.
current != nullgoes false when a platform reports removal;enablednever reflects a disconnect, it reflects whether the device is switched on. Neither alone is a presence check. Where a platform registers unconditionally and never reports removal this still over-reports, which is what the follow-up issues listed on the engine PR cover. Adevice.IsUsable()extension could hide the pair later without changing the semantics.Important
Depends on an engine change that has not landed yet (PR #120240, same ticket). Until it does,
UNITY_INPUTSYSTEM_SUPPORTS_CAPABILITY_QUERIESnever fires and everything here compiles out, so red or empty CI on this PR is expected and is not a defect. Draft is up to open the design discussion, not to merge first.Testing status & QA
Verified on macOS M1 against a locally built
6000.7.0a6Editor carrying the engine endpoint, which is the only configuration where this code compiles at all.Unknownfor each property; the properties answering with no device added; a query never being delivered to a real device; repeated reads issuing exactly one command; nothing answering reportingfalserather than throwing.CapabilityState, the FourCC codes againstNativeInputCapabilities, and the payload size against the single byte the engine's validation accepts. Nothing generates these mirrors, so these are what catch the two repos drifting apart.Devices_category, sinceInputManager,Pen,MouseandTouchscreenare all touched. 392 tests, 383 passing, 0 failures, 9 pre-existing not-run.For QA: nothing to verify until the engine PR lands. After that, the interesting cases are the platforms where the answer is not obviously true. macOS should report
Pen.isSupported == true, which contradicts legacystylusTouchSupported, andisPressureSupported == false. Windows also reportsisPressureSupported == false, even though Win32 can supply pressure: Unity'sProcessPointerTouchtestsTOUCH_MASK_PRESSUREagainsttouchFlagsrather thantouchMask, so the branch never runs and every touch reports a constant1.0f. That is a separately tracked engine bug, and the answer deliberately reports what a caller would actually receive rather than what the OS could provide.Overall Product Risks
InputManagergains three cached fields and one method;Pen,MouseandTouchscreengain one static property each. The one shared-code change is replacing the unusedExecuteGlobalCommandwithExecuteSystemCommand(see below). Editor-branch risk is limited to theversionDefinesexpression being wrong, which would make the API silently absent rather than misbehave.Comments to reviewers
ExecuteGlobalCommandis replaced byExecuteSystemCommand. The old helper had zero callers, orphaned when ISXB-927 deletedUseWindowsGamingInputCommand, and its premise was wrong: it addressed device id0on the comment that the engine routes such commands by FourCC alone, butInputDeviceIOCTLresolves the id against the device registry and0is the invalid-device sentinel, so nothing sent there could ever have been answered. The engine still declares the matching'UWGI'code that nothing handles; being cleaned up separately.The commands and
InputCapabilitySupportare internal, unlike everything else inDevices/Commands/. They target the system endpoint, so there is no custom device a user could answer them for, and it avoids freezing a capability model the low level input API is expected to revisit. This is the decision I'd most like challenged, since it is a visible deviation from the folder's convention. Note that ISX-2046 asks for the managed interop structs to live inInputModulerather than the package, and they do:CapabilityStateand the payload are inUnityEngineInternal.Input. The package-side command structs here are what ISX needs in order to issue a command at all, and are a separate thing from those.Three command structs rather than one, because
IInputDeviceCommandInfocouples a struct to one static FourCC and a single struct taking the code as a parameter would maketypeStatica lie. Costs about ten duplicated lines each.The Device Simulator force-false is deliberately not replicated. ISX-2079 asks for parity with legacy
mousePresentincluding its!simulateTouchEnabledclause. That clause was a workaround for legacy having no device concept, where lying about the capability was the only lever available. ISX solves the same problem structurally:DeviceSimulator/InputSystemPlugin.cs:61disables the nativeMouseandPenwhile simulating and re-enables them afterwards, soenabledis the signal that moves. A platform capability should not know about an Editor simulation. Flagging it because it is a stated requirement on that ticket which this consciously does not meet.Touch pressure is platform-scoped, not per touchscreen. That is the scope at which the answer exists: iOS asks the device model, Windows reads a WM_POINTER property, Android asks the platform. None enumerate digitizers, so a per-device channel would deliver system-scoped truth dressed as per-instance precision. If a platform ever gains touchscreens that genuinely differ, a per-device command can be added and preferred over this one, additively.
The API is version-gated rather than always-present, so users get a compile error on older Editors instead of a silent
false. The expression is currently6000.7.0a6, the local development Editor, and must be updated to whichever version the engine change actually ships in before this merges.Open questions: internal vs public commands; whether the properties should be instance rather than static (they are static because the answer is platform-scoped); whether
Touchscreen.multiTouchEnabledshould be folded in here or tracked separately; whether the manual section blocks merge.Checklist
Before review:
Addedsection, with a before/after migration snippet.Devices_Thing_Behaviournaming.isSupportedandcurrent != null. Deliberately deferred: the presence half of that guidance misfires today on macOS and iOS, which register pens unconditionally, and those are separately tracked engine bugs. Worth deciding whether this blocks merge.During merge:
NEW:.