FIX: HID 8-bit hat switch crashes InputDeviceBuilder (UUM-143659)#2424
FIX: HID 8-bit hat switch crashes InputDeviceBuilder (UUM-143659)#2424LeoUnity wants to merge 4 commits into
Conversation
Adds Devices_CanCreateGenericHID_FromGamepadWithEightBitHatSwitch to HIDTests, modelled on the ESP32-BLE-Gamepad descriptor from the ticket: an HID gamepad whose report descriptor declares a hat switch with Report Size 8 (instead of the standard 4 bits). Currently fails on develop: InputDeviceBuilder.InsertControlBitRangeNode goes into infinite recursion while constructing the control tree; InputManager catches the resulting IndexOutOfRangeException and logs "Could not create a device for ...", so the device never appears. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nge tree (UUM-143659)
For an HID hat switch, the HID layer adds four directional sub-controls
(hat/up, hat/right, hat/down, hat/left) that are meant to overlap the
hat itself and only differ in their DiscreteButton value ranges. Those
sub-controls were added with WithBitOffset() but no WithByteOffset(),
so the layout system's auto-allocator placed each one in a fresh byte.
For a 4-bit hat that's harmless: sizeInBits=4 is bit-addressing and all
four pack into the parent's byte via the bitfield path. For an 8-bit
hat (sizeInBits=8 is byte-addressing) each sub-control consumed its
own byte, pushing them past the device's HID-derived report size. The
dangling controls then sent InputDeviceBuilder.InsertControlBitRangeNode
into infinite recursion because no partition could ever contain them.
Fixes:
1. HID.cs: add WithByteOffset(0) to each of the four hat sub-controls
so they're anchored to the hat parent's byte and overlap it after
FinalizeControlHierarchy bakes in the parent's byteOffset.
2. InputDeviceBuilder.cs: skip synthetic controls when building the
control bit-range tree. Synthetic controls (e.g. DpadControl's x/y,
StickControl's up/down/left/right) get their state block aliased to
a sibling/parent in FinishSetup -- but FinishSetup runs *after*
FinalizeControlHierarchy, so the tree was being built with the
pre-FinishSetup offsets, which could also fall outside the device's
bit range. Their bits are already covered by the non-synthetic
control they alias, so they don't need their own tree entry.
Either fix alone is not sufficient: the HID fix resolves the
non-synthetic hat sub-controls; the synthetic skip resolves
DpadAxis hat/x and hat/y, which suffer from the same overrun
via a different mechanism.
Adds a CHANGELOG entry. Test:
Devices_CanCreateGenericHID_FromGamepadWithEightBitHatSwitch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Fix is legit IMO as smokes out two bugs in the HID parser / InputDeviceBuilder.
|
@LeoUnity Looks legit, needs a formatter run. |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The proposed fix for the HID device layout crash is solid, but excluding synthetic controls from the bit-range tree introduces a critical regression in button press tracking and cache invalidation. Reverting this exclusion while keeping the underlying layout fixes will ensure the engine remains stable.
🤖 Helpful? 👍/👎
| // Construct control bit range tree. Skip synthetic controls (e.g. DpadControl's | ||
| // x/y axes, StickControl's up/down/left/right) — they don't have their own state | ||
| // bits (FinishSetup will alias their state block to a non-synthetic sibling/parent), | ||
| // so any state-change detection on their bits is covered by that sibling/parent | ||
| // already being in the tree. Including them was UUM-143659: synthetic controls | ||
| // can carry pre-FinishSetup offsets that fall outside the device's bit range, | ||
| // sending InsertControlBitRangeNode into infinite recursion. | ||
| if (control != m_Device && !control.synthetic) |
There was a problem hiding this comment.
It looks like skipping synthetic controls from the bit-range tree here will break cache invalidation and button state tracking (wasPressedThisFrame / wasReleasedThisFrame) for ALL synthetic controls (e.g., Keyboard.anyKey, Mouse.press, Gamepad.leftStick.up) across the engine.
During a state update, InputDevice.WriteChangedControlStatesInternal uses this tree to determine which controls overlap the changed memory so it can call MarkAsStale() and add buttons to m_UpdatedButtons. If synthetic controls are omitted, their cached values will never be invalidated when the underlying bits change, and their frame-press states won't update. MarkAsStale on the parent control does not automatically cascade to its children during the hot-path state update.
The good news is this skip shouldn't be necessary to fix the crash! As you correctly noted, the recursion on hat/x was caused by it inheriting a pre-FinishSetup offset from hat/right that fell outside the device's size. However, because your fix in HID.cs correctly anchors hat/right to a valid byte offset (0), hat/x (which uses useStateFrom = "right") will now also inherit that valid offset. This means hat/x will no longer fall out of bounds, preventing the infinite recursion naturally without needing to exclude synthetic controls.
| // Construct control bit range tree. Skip synthetic controls (e.g. DpadControl's | |
| // x/y axes, StickControl's up/down/left/right) — they don't have their own state | |
| // bits (FinishSetup will alias their state block to a non-synthetic sibling/parent), | |
| // so any state-change detection on their bits is covered by that sibling/parent | |
| // already being in the tree. Including them was UUM-143659: synthetic controls | |
| // can carry pre-FinishSetup offsets that fall outside the device's bit range, | |
| // sending InsertControlBitRangeNode into infinite recursion. | |
| if (control != m_Device && !control.synthetic) | |
| // Construct control bit range tree | |
| if (control != m_Device) |
📚 Additional Context
Packages/com.unity.inputsystem/InputSystem/Runtime/Devices/InputDeviceBuilder.csPackages/com.unity.inputsystem/InputSystem/Runtime/Controls/InputControl.csPackages/com.unity.inputsystem/InputSystem/Runtime/Controls/DpadControl.csPackages/com.unity.inputsystem/InputSystem/Runtime/Controls/StickControl.csPackages/com.unity.inputsystem/InputSystem/Runtime/Devices/InputDevice.csPackages/com.unity.inputsystem/InputSystem/Runtime/InputManager.csPackages/com.unity.inputsystem/InputSystem/Runtime/Plugins/HID/HID.cs
🤖 Helpful? 👍/👎 by bug_hunter
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2424 +/- ##
===========================================
+ Coverage 78.13% 78.89% +0.76%
===========================================
Files 483 762 +279
Lines 98770 139282 +40512
===========================================
+ Hits 77169 109885 +32716
- Misses 21601 29397 +7796 Flags with carried forward coverage won't be shown. Click here to find out more.
|
…icient
The HID.cs WithByteOffset(0) change makes hat/right and hat/up land at
the hat's own byte. The synthetic hat/x and hat/y inherit those offsets
via useStateFrom, so the pre-FinishSetup tree-build no longer sees
out-of-range synthetics. The InputDeviceBuilder synthetic-skip is no
longer needed to fix UUM-143659.
Keeping that skip broke APIVerificationTests.API_PrecompiledLayoutsAreUpToDate
for Keyboard/Mouse/Touchscreen: removing synthetic controls from the
bit-range tree changed the serialized layout of every device, so the
checked-in Fast{Keyboard,Mouse,Touchscreen}.cs no longer match what the
builder produces.
Verified locally:
- Devices_CanCreateGenericHID_FromGamepadWithEightBitHatSwitch still
passes with the HID-only fix.
- API_PrecompiledLayoutsAreUpToDate passes for all three devices.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI triage (post-revert, commit b463697)After the synthetic-skip revert the test regression is gone —
This PR only modifies the 4 hat sub-control |
2 participants
Summary
Fixes UUM-143659 — connecting an HID gamepad whose report descriptor declares a hat switch with
Report Sizeof 8 bits (e.g. ESP32-BLE-Gamepad) madeInputDeviceBuilder.InsertControlBitRangeNodeinfinite-loop, raisingIndexOutOfRangeException; the device was logged asCould not create a device for ...and never appeared.Root cause
Two independent overrun bugs converge in
DpadControlconstruction for an HID hat:HID.cs:849-906) addshat/up/right/down/leftwithWithBitOffset()but noWithByteOffset(). The layout system's auto-allocator puts each sub-control in a fresh byte. For 4-bit hats they pack into one byte via the bitfield path (sizeInBits % 8 != 0); for 8-bit hats each claims its own byte, pushing them past the device's HID-derived report size.InputDeviceBuilder.cs) was processing the syntheticDpadAxischildren (hat/x,hat/y) of the Dpad.DpadAxisControl.FinishSetupaliases their state block to a sibling — butFinishSetupruns afterFinalizeControlHierarchy, so the tree saw their pre-FinishSetup offsets, which could also fall outside the device.Either dangling control sent
InsertControlBitRangeNodeinto endless recursion on the recurse-right branch (no partition could ever contain the control), eventuallyIndexOutOfRangeException.Fixes
HID.cs— anchor each of the four hat sub-controls withWithByteOffset(0)so they overlap the hat's byte afterFinalizeControlHierarchyRecursivebakes in the parent'sbyteOffset.InputDeviceBuilder.cs— skip synthetic controls inFinalizeControlHierarchyRecursivewhen inserting into the bit-range tree; their bits are already represented by the non-synthetic control they alias.Both changes are needed: the HID fix handles non-synthetic
hat/up/right/down/left; the synthetic-skip fix handleshat/x/hat/y.Test plan
Devices_CanCreateGenericHID_FromGamepadWithEightBitHatSwitch— fails ondevelop, passes with the fix.Devices+HID Devicestest categories in CI — the existing 4-bit-hat test (Devices_CanCreateGenericHID_FromDeviceWithBinaryReportDescriptor) and other Dpad-using tests are the most likely regression candidates.Notes
🤖 Generated with Claude Code