Add configurable trackpad haptic feedback - #1461
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSteam trackpad input now supports configurable click and movement haptics. The change adds touchpad actions, coordinate and swipe mapping, DualSense touch sequencing, Steam HID gain support, and related controller and layout settings. ChangesTrackpad input and mapping
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TrackpadInput
participant TouchpadActions
participant LayoutManager
participant SteamController
participant SteamHIDAPI
TrackpadInput->>TouchpadActions: Process touchpad input
TouchpadActions->>LayoutManager: Return touch sample and haptic state
LayoutManager->>SteamController: Send trackpad haptic
SteamController->>SteamHIDAPI: Send gain-enabled haptic packet
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HandheldCompanion/Actions/MouseActions.cs`:
- Around line 284-308: The UpdateTrackpadHaptics method must also require Steam
lizard/Hybrid mode to be disabled before generating replacement haptic pulses.
Add the existing lizard-mode state check alongside the SteamController target
check, returning early when lizard mode is enabled while preserving the current
distance and haptic-step behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c48bdd1-f49e-4f3d-98ac-2ff3f173243a
📒 Files selected for processing (13)
HandheldCompanion/Actions/MouseActions.csHandheldCompanion/App.configHandheldCompanion/Controllers/Steam/GordonController.csHandheldCompanion/Controllers/Steam/NeptuneController.csHandheldCompanion/Controllers/Steam/SteamController.csHandheldCompanion/Misc/Layout.csHandheldCompanion/Misc/LayoutTemplate.csHandheldCompanion/Properties/Settings.Designer.csHandheldCompanion/Properties/Settings.settingsHandheldCompanion/Views/Pages/ControllerPage.xamlHandheldCompanion/Views/Pages/ControllerPage.xaml.cssteam-hidapi.net/Hid/HidEnums.cssteam-hidapi.net/SteamController.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12c0dc55ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| public override void SetHaptic(ButtonFlags button, bool released) |
There was a problem hiding this comment.
When a Steam trackpad click is mapped to a KeyboardActions or ButtonActions binding with haptics enabled, this MouseActions-only override never runs: UpdateTrackpadClickHaptics emits the shared click pulse, and the mapped action then calls the base IActions.SetHaptic, producing a second pulse while the global level is enabled. Move this suppression to a common action/controller path so the controller-level feedback is emitted once regardless of mapping type.
Useful? React with 👍 / 👎.
| float distance = delta.Length(); | ||
| if (distance < TrackpadHapticJitterThreshold) | ||
| return; |
There was a problem hiding this comment.
For slow or high-report-rate trackpad movement where every coordinate delta is below 128 units, each sample returns here before contributing to trackpadHapticDistance, so even sustained travel never produces movement feedback. The jitter filter needs to preserve persistent sub-threshold movement—such as by accumulating displacement before applying the threshold—rather than discarding every small sample independently.
Useful? React with 👍 / 👎.
| int strength = ManagerFactory.settingsManager.GetInt("SteamTrackpadClickHaptics"); | ||
| if (strength <= 0) | ||
| return; | ||
|
|
||
| SendTrackpadClickHaptic(motor, System.Math.Clamp(strength, 1, 3), released: !pressed); |
There was a problem hiding this comment.
On a non-Steam-Deck host with a Gordon Steam Controller or generic 0x12f0 SteamOS handheld selected, this shared base path still reads the new default value of 3 and sends High click feedback, but ControllerPage hides SteamDeckPanel whenever IDevice.GetCurrent() is not SteamDeck. Those supported controllers therefore cannot select Off or another level through the UI; either limit this behavior to actual Deck hardware or expose the setting whenever an affected Steam controller is available.
Useful? React with 👍 / 👎.
| Vector2 trackpadDelta = outVector - prevVector; | ||
| prevVector = outVector; | ||
| UpdateTrackpadHaptics(layout.flags, touched, trackpadDelta); |
There was a problem hiding this comment.
When a new touch begins exactly at the pad center, ConsumeNewTouch records the transition but the earlier zero-vector return skips the firstTouch branch, leaving prevVector at the previous touch's final position. The first small movement of the new touch is therefore treated here as a potentially large stale delta and can immediately emit a false movement pulse; initialize the baseline before returning for a touched zero vector.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HandheldCompanion/Actions/MouseActions.cs`:
- Around line 206-214: Update the early-return condition in the touch-processing
flow around isTrackpad so Vector2.Zero positions continue through delta and
haptic processing for trackpads. Return early only when outVector is zero and
the current layout is not a trackpad, preserving the first-touch trackpad
initialization behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e9780f4-2800-44b2-b4f8-b687cf1088db
📒 Files selected for processing (8)
HandheldCompanion/Actions/IActions.csHandheldCompanion/Actions/MouseActions.csHandheldCompanion/Controllers/Steam/GordonController.csHandheldCompanion/Controllers/Steam/NeptuneController.csHandheldCompanion/Controllers/Steam/SteamController.csHandheldCompanion/Views/Pages/ControllerPage.xamlHandheldCompanion/Views/Pages/ControllerPage.xaml.cssteam-hidapi.net/SteamController.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- HandheldCompanion/Controllers/Steam/GordonController.cs
- HandheldCompanion/Controllers/Steam/SteamController.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bdffff62b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (action.MouseType is MouseActionsType.Move or MouseActionsType.Scroll && | ||
| action.HapticMode == HapticMode.Off) | ||
| action.HapticMode = HapticMode.Down; |
There was a problem hiding this comment.
For every pre-schema saved layout containing a trackpad Move or Scroll action, this migration changes HapticMode.Off to Down, even when the user explicitly selected Off through the existing action settings. Because old files cannot distinguish an omitted default from an intentional Off value, upgrading silently re-enables movement haptics and overwrites the user's configuration; apply the new default only to newly created layouts or use a migration marker that preserves explicit choices.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| if (outVector == Vector2.Zero) return; |
There was a problem hiding this comment.
When this action's shift slot is not active, base.Execute replaces outVector with zero and this return leaves prevVector, trackpadHapticJitter, and trackpadHapticDistance untouched. If the finger moves while the action is masked and the shift subsequently becomes active without a new touch edge, the next sample counts all of that inactive travel as a live delta and can emit an immediate spurious movement pulse; update/reset the trackpad baseline while gated before returning.
Useful? React with 👍 / 👎.
|
For visibility, this PR is also related to #336, #357, #1007, and #1096. It addresses the missing Steam Deck trackpad movement and click haptics mentioned in those reports. The broader issues also cover separate topics such as acceleration, momentum, scrolling behavior, menu navigation, and controller testing, which are outside the scope of this PR. |
|
It looks fine overall but I see a few valid comments from the AI. Also, now that we have more devices and controllers shipping with trackpads, it'd be good to make this non Steam Deck specific so Legion Go users and DualSense/DualShock4/Steam Controller(s) owners could also benefit from it. Also, as part of another pull request discussion, I suggest we create a TouchpadActions class instead of having touchpad related stuff on MouseActions and ButtonActions. Which means we'd have a new UI element on the layout drop downs : Touchpad when targeted controller has touchpad-related Target buttons/TargetsAxis available. |
|
Retested the latest branch on a Steam Deck. Both trackpads now provide correctly routed movement and click feedback, click feedback occurs on both press and release, Off disables it, Low, Medium, and High are clearly distinct, and High is the default. Slow movement and center touches no longer produce missing or stale feedback. The solution builds locally with 0 errors. Movement feedback now uses the selected controller's normal haptic path, so it is not limited to Steam controllers. Native click feedback remains on the Steam HID path. I left the broader TouchpadActions and layout UI work to #1454 to avoid duplicating or conflicting with that implementation. All fixes are pushed in 58c4f44. |
58c4f44 to
8dc1cbd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HandheldCompanion/Views/Pages/ActionSettingsPage.xaml`:
- Around line 146-150: Localize the DualSense touchpad UI introduced in
ActionSettingsPage by replacing the hardcoded “DualSense touchpad” heading and
touchpad card headers with resource bindings, and add the corresponding resource
entries. Update MappingViewModel’s TouchpadCoordinateRangeDescription
construction to use localized resources for its generated text, preserving the
existing binding and visibility behavior.
- Around line 153-170: Add IsSnapToTickEnabled="True" to all four coordinate
Slider controls bound to TouchpadX, TouchpadY, TouchpadEndX, and TouchpadEndY,
while preserving their existing TickFrequency and bindings so fractional values
cannot be selected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ca1930-8b51-4799-926d-e3457709232e
📒 Files selected for processing (26)
HandheldCompanion/Actions/IActions.csHandheldCompanion/Actions/MouseActions.csHandheldCompanion/Actions/TouchpadActions.csHandheldCompanion/App.configHandheldCompanion/Controllers/Dummies/DummyDualSenseController.csHandheldCompanion/Controllers/SDL/DualSenseController.csHandheldCompanion/Controllers/Steam/GordonController.csHandheldCompanion/Controllers/Steam/NeptuneController.csHandheldCompanion/Controllers/Steam/SteamController.csHandheldCompanion/Inputs/ButtonFlags.csHandheldCompanion/Managers/LayoutManager.csHandheldCompanion/Misc/DS4Touch.csHandheldCompanion/Misc/Layout.csHandheldCompanion/Misc/LayoutTemplate.csHandheldCompanion/Properties/Settings.Designer.csHandheldCompanion/Properties/Settings.settingsHandheldCompanion/Targets/DualSenseTarget.csHandheldCompanion/ViewModels/Layout/Mappings/AxisMappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/ButtonMappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/TriggerMappingViewModel.csHandheldCompanion/Views/Pages/ActionSettingsPage.xamlHandheldCompanion/Views/Pages/ControllerPage.xamlHandheldCompanion/Views/Pages/ControllerPage.xaml.cssteam-hidapi.net/Hid/HidEnums.cssteam-hidapi.net/SteamController.cs
🚧 Files skipped from review as they are similar to previous changes (13)
- steam-hidapi.net/Hid/HidEnums.cs
- HandheldCompanion/Misc/Layout.cs
- HandheldCompanion/Controllers/Steam/SteamController.cs
- HandheldCompanion/Properties/Settings.settings
- HandheldCompanion/Controllers/Steam/GordonController.cs
- HandheldCompanion/Misc/LayoutTemplate.cs
- HandheldCompanion/Views/Pages/ControllerPage.xaml
- HandheldCompanion/Properties/Settings.Designer.cs
- HandheldCompanion/App.config
- HandheldCompanion/Controllers/Steam/NeptuneController.cs
- HandheldCompanion/Views/Pages/ControllerPage.xaml.cs
- HandheldCompanion/Actions/MouseActions.cs
- steam-hidapi.net/SteamController.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dc1cbd9d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ApplyTouchpadCoordinates(touchpadSample); | ||
| ProcessGyroActions(controllerState, shiftSlot, deltaMs); |
There was a problem hiding this comment.
When a DualSense profile maps gyro output to RightPad—a target exposed by DummyDualSenseController—an active coordinate touch, click, or swipe is written here and then immediately modified by ProcessGyroActions, which blends gyro output into the same RightPadX/Y axes. The emitted touch therefore drifts from its configured coordinates and swipes can be distorted or clamped; apply the selected touchpad sample after gyro processing so these exact-position actions retain priority.
Useful? React with 👍 / 👎.
| public override void Execute(ButtonFlags button, bool value, ShiftSlot shiftSlot, float delta) | ||
| { | ||
| base.Execute(button, value, shiftSlot, delta); |
There was a problem hiding this comment.
When an axis or trigger mapping targets one of these touchpad actions and per-action haptics are enabled, inherited ButtonActions.Execute(AxisLayout, ...) calls this method with the synthetic output flag (TouchpadCoordinateClick, TouchpadCoordinateTouch, or TouchpadSwipe) as the haptic button. On a Steam Deck or Gordon controller, SteamController.GetMotorForButton has no case for those flags and throws from SetHaptic, interrupting report processing as soon as the mapping activates; route haptics using a supported physical source/motor or suppress them for these synthetic targets.
Useful? React with 👍 / 👎.
|
Please see my previous message: #1461 (comment). Now that we're bringing that much parameters specific to Touchpad, it's time to migrate from MouseActions/ButtonActions to a single TouchpadActions class. |
LavX
changed the title
|
Refactored as requested. Touchpad-specific mapping and haptic handling now lives in TouchpadActions, the layout target list is capability-driven, and Quick Profiles can represent left and right pad gyro output without rewriting unsupported mappings. Retested the final build on Steam Deck, and the solution builds with 0 errors. Ready for another look. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507fa07570
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static void WriteTouch(byte[] data, int x, int y) | ||
| { | ||
| ushort touchX = InputUtils.ClampToUShort(x, 0, DS4Touch.TOUCHPAD_WIDTH - 1); | ||
| ushort touchY = InputUtils.ClampToUShort(y, 0, DS4Touch.TOUCHPAD_HEIGHT - 1); |
There was a problem hiding this comment.
When emitting any physical or synthetic touch through the DualSense target, this clamps Y to the DS4 range of 0–942 and writes it directly into the VIIPER report. The replaced implementation scaled that range to the DualSense report's 0–1080 range, so touches and swipes can now reach only about 87% of the emulated pad height and coordinates near the physical bottom are reported substantially too high; restore the target-specific Y scaling before writing the report.
Useful? React with 👍 / 👎.
| // --- Haptics --- | ||
| public HapticMode HapticMode = HapticMode.Off; | ||
| public HapticStrength HapticStrength = HapticStrength.Low; | ||
| public bool? HapticOverride = null; |
There was a problem hiding this comment.
For layouts saved before this field existed, HapticOverride deserializes as null even when a physical trackpad-click action has an explicit HapticMode or strength. The new click resolver honors an action only when this value is exactly true, while the common SetHaptic path now suppresses physical-click haptics, so upgrading silently replaces those saved Down/Up/Off or strength choices with the global setting; infer/migrate the override for legacy click mappings rather than treating every missing value as global.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
HandheldCompanion/Controllers/IController.cs (1)
448-472: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent canceled rumble tasks from writing vibration.
A queued task can start after a later
Rumblecall cancels and replacescurrentCts. It still callsSetVibrationbefore it observes cancellation atTask.Delay. That stale write can overwrite the newer haptic command and remain active because the stale task no longer ownsrumbleCts.Before
SetVibration, lockrumbleLockand return unlesscurrentCtsis stillrumbleCtsand its token is not canceled. Keep the vibration write in that protected check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Controllers/IController.cs` around lines 448 - 472, Update the rumbleTask body in IController so it locks rumbleLock before calling SetVibration(LargeMotor, SmallMotor), and only performs the write when currentCts still references rumbleCts and its token is not canceled; otherwise return without writing. Keep the existing delay, cleanup, and ownership checks unchanged.HandheldCompanion/Actions/IActions.cs (1)
180-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve legacy per-action haptics for physical clicks.
Existing saved actions default
HapticOverridetonull, soTouchpadActions.HasCustomHapticSettingsreturnsfalseandResolveClickHapticProfileuses the globalTrackpadClickHapticsvalue instead of the action’s non-zeroHapticMode/HapticStrength. WhenSetHapticnow bypasses physical-click buttons for allIActionssubclasses, those legacy settings stop applying unless the mapper re-enables the override or a migration setsHapticOverridetotruefor non-zero legacy haptics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Actions/IActions.cs` around lines 180 - 188, Update SetHaptic in IActions so physical-click buttons are bypassed only when the action does not have an explicit or legacy per-action haptic configuration. Preserve non-zero HapticMode/HapticStrength settings when HapticOverride is null, either by re-enabling the override during mapping or migrating those actions to HapticOverride=true, while retaining the physical-click bypass for actions using global haptic settings.HandheldCompanion/Managers/LayoutManager.cs (1)
715-724: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake touchpad coordinate output coexist with axis/touchpad gyro output.
ProcessButtonActions,ProcessAxisActions, andProcessGyroActionsblend intooutputState.AxisState[RightPadX/Y], butApplyTouchpadCoordinatesthen assigns those axes directly when a coordinate sample exists. If anAxisLayoutFlags.RightPadtouchpad action, axis plan action, or gyro touchpad mapping runs in the same tick, replace the assignment with blending or make the precedence explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Managers/LayoutManager.cs` around lines 715 - 724, Update ApplyTouchpadCoordinates so coordinate samples blend with, or use explicitly documented precedence against, RightPadX and RightPadY values produced by ProcessButtonActions, ProcessAxisActions, and ProcessGyroActions in the same tick. Preserve the existing coordinate conversion while preventing direct assignments from overwriting AxisLayoutFlags.RightPad touchpad, axis-plan, or gyro mappings.
🧹 Nitpick comments (6)
HandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.cs (2)
365-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree subclasses re-declare the base touchpad visibility properties verbatim.
MappingViewModeldefinesTouchpadSettingsVisibilityandTouchpadSwipeSettingsVisibilityasvirtualwith the exact logic that all three subclasses then repeat. The shared root cause is that the base implementation already covers every subclass, so the overrides add no behavior and create four copies to maintain.
HandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.cs#L365-L375: keep these as the singlevirtualimplementation; no change needed here beyond confirming it stays the source of truth.HandheldCompanion/ViewModels/Layout/Mappings/AxisMappingViewModel.cs#L380-L388: delete both overrides and inherit from the base.HandheldCompanion/ViewModels/Layout/Mappings/ButtonMappingViewModel.cs#L418-L426: delete both overrides and inherit from the base.HandheldCompanion/ViewModels/Layout/Mappings/TriggerMappingViewModel.cs#L26-L34: delete both overrides and inherit from the base.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.cs` around lines 365 - 375, Remove the redundant TouchpadSettingsVisibility and TouchpadSwipeSettingsVisibility overrides from AxisMappingViewModel (lines 380-388), ButtonMappingViewModel (lines 418-426), and TriggerMappingViewModel (lines 26-34), allowing them to inherit the shared virtual implementations from MappingViewModel. Keep MappingViewModel (lines 365-375) unchanged as the single source of truth.
343-363: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the controller lookup in these two visibility getters.
Both getters call
ControllerManager.GetDefault(true)on every evaluation, andTouchpadAxisActionTypeVisibilityalso enumeratesTouchpadActions.GetAxisTargets(controller). The mapping view models raiseOnPropertyChanged(string.Empty)on every action-type change and every target change, and the layout page holds one view model per input. Each refresh therefore repeats the manager lookup and the LINQ enumeration for every row.Cache the two results and invalidate them from the existing
VirtualManager_ControllerSelectedhandler, which already triggersActionTypeChanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.cs` around lines 343 - 363, Cache the controller lookup and computed touchpad target results used by TouchpadActionTypeVisibility and TouchpadAxisActionTypeVisibility instead of recalculating them on every getter evaluation. Invalidate those cached values in the existing VirtualManager_ControllerSelected handler alongside ActionTypeChanged, ensuring subsequent property evaluations refresh against the newly selected controller.HandheldCompanion/Managers/LayoutManager.cs (1)
874-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the gyro blending logic into a shared helper.
Lines 880-888 duplicate the blend computation from the
ActionType.Joystickcase at lines 852-860. Only the value source differs (aA.GetValue()versustouchpadAction.GetAxisValue()). Extract one private helper that takes the targetAxisLayoutFlags, the weight, and the incomingVector2. This keeps both paths consistent when the blend formula changes.♻️ Proposed helper extraction
+ private void BlendIntoAxis(AxisLayoutFlags axis, float weight, Vector2 value) + { + var xyOut = _axisXY[axis]; + var current = new Vector2(outputState.AxisState[xyOut.X], outputState.AxisState[xyOut.Y]); + + float norm = Math.Clamp(current.Length() / short.MaxValue, 0f, 1f); + var blended = current + value * (weight - norm); + + outputState.AxisState[xyOut.X] = ClampShort(blended.X); + outputState.AxisState[xyOut.Y] = ClampShort(blended.Y); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Managers/LayoutManager.cs` around lines 874 - 893, Extract the duplicated gyro blending and axis assignment logic from the ActionType.Joystick and ActionType.Touchpad branches into one private helper accepting the target AxisLayoutFlags, weight, and incoming Vector2. Update both branches to call the helper while preserving their existing value sources and touch-button handling.HandheldCompanion/ViewModels/Layout/Mappings/ButtonMappingViewModel.cs (1)
280-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd parentheses to the mixed
||/&&condition.C# binds
&&tighter than||, so the expression evaluates asJoystick || (Touchpad && Action is TouchpadActions { TargetType: Axis }). That matches the intent, but the grouping is not visible in the source. Add explicit parentheses so a later edit cannot change the meaning silently.♻️ Proposed clarification
public override Visibility Button2AxisVisibility => - ActionTypeIndex == (int)ActionType.Joystick || - ActionTypeIndex == (int)ActionType.Touchpad && - Action is TouchpadActions { TargetType: TouchpadTargetType.Axis } + ActionTypeIndex == (int)ActionType.Joystick || + (ActionTypeIndex == (int)ActionType.Touchpad && + Action is TouchpadActions { TargetType: TouchpadTargetType.Axis }) ? Visibility.Visible : Visibility.Collapsed;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/ViewModels/Layout/Mappings/ButtonMappingViewModel.cs` around lines 280 - 285, Update Button2AxisVisibility so the mixed || and && condition has explicit parentheses around the intended grouped clauses, preserving the current evaluation of the Joystick case and the Touchpad axis-action case.HandheldCompanion/ViewModels/Layout/LayoutItemPageViewModel.cs (1)
114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving this label from the enum and the resource strings.
This switch hardcodes indices and English text.
ActionTypeIndexToNameConvertermaps the same indices to localizedResources.LayoutPage_ActionType_*strings. The two lists must now be kept in sync by hand, and this one is not localized.MappingViewModel.GetActionTypeDisplayName()already performs the enum-to-resource lookup; call it instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/ViewModels/Layout/LayoutItemPageViewModel.cs` around lines 114 - 126, Replace the local actionType switch in the surrounding method with MappingViewModel.GetActionTypeDisplayName(), using the current mapping’s action type value as required by that API. Remove the hardcoded indices and English labels so the displayed name comes from the shared localized resource lookup.HandheldCompanion/ViewModels/Pages/ProfilesPageViewModel.cs (1)
3188-3203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe pruning step depends on the enum declaration order.
The loop places each desired mode at its target index. The trailing
whilethen removes items from the end of the collection. This removes the correct entries only becauseLeftPadandRightPadare declared last inMotionOutput. If a future value is inserted before them, obsolete entries can remain in the middle and valid entries can be removed from the tail.Remove obsolete entries explicitly by value before the placement loop.
♻️ Proposed order-independent pruning
MotionOutput[] desiredModes = Enum.GetValues<MotionOutput>() .Where(availableModes.Contains) .ToArray(); + for (int index = MotionOutputModes.Count - 1; index >= 0; index--) + { + if (!availableModes.Contains(MotionOutputModes[index].Value)) + MotionOutputModes.RemoveAt(index); + } + for (int index = 0; index < desiredModes.Length; index++) { MotionOutput mode = desiredModes[index]; MotionOutputViewModel? existing = MotionOutputModes.FirstOrDefault(item => item.Value == mode); if (existing is null) MotionOutputModes.Insert(index, new MotionOutputViewModel(mode)); else if (MotionOutputModes.IndexOf(existing) != index) MotionOutputModes.Move(MotionOutputModes.IndexOf(existing), index); } - - while (MotionOutputModes.Count > desiredModes.Length) - MotionOutputModes.RemoveAt(MotionOutputModes.Count - 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/ViewModels/Pages/ProfilesPageViewModel.cs` around lines 3188 - 3203, Update the MotionOutputModes synchronization logic before the placement loop to explicitly remove every existing mode whose Value is not contained in desiredModes, rather than pruning only from the tail afterward. Keep the existing insertion and move behavior in the loop, and remove the trailing count-based while removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HandheldCompanion/Properties/Settings.settings`:
- Around line 125-127: Update CustomSettingsProvider.GetPropertyValues to
migrate the legacy SteamTrackpadClickHaptics value when loading
TrackpadClickHaptics: if the new setting node is absent, read the old key, use
its value, and persist it under TrackpadClickHaptics instead of defaulting to 3.
In `@HandheldCompanion/ViewModels/Layout/Mappings/GyroMappingViewModel.cs`:
- Around line 435-438: Update the Touchpad branch in the relevant mapping
override to call OnPropertyChanged(string.Empty) immediately after
SetTouchpadTarget, matching AxisMappingViewModel, ButtonMappingViewModel, and
TriggerMappingViewModel so TargetType-dependent visibility bindings refresh.
In `@HandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.cs`:
- Around line 396-397: Update the mapping migration and display logic around
GetJoystickTargets so persisted ActionType.Joystick mappings targeting LeftPad
or RightPad are converted or relabeled as ActionType.Touchpad mappings. Ensure
these saved actions remain supported and no longer display “Unavailable on
current controller” when those axes are valid touchpad targets.
---
Outside diff comments:
In `@HandheldCompanion/Actions/IActions.cs`:
- Around line 180-188: Update SetHaptic in IActions so physical-click buttons
are bypassed only when the action does not have an explicit or legacy per-action
haptic configuration. Preserve non-zero HapticMode/HapticStrength settings when
HapticOverride is null, either by re-enabling the override during mapping or
migrating those actions to HapticOverride=true, while retaining the
physical-click bypass for actions using global haptic settings.
In `@HandheldCompanion/Controllers/IController.cs`:
- Around line 448-472: Update the rumbleTask body in IController so it locks
rumbleLock before calling SetVibration(LargeMotor, SmallMotor), and only
performs the write when currentCts still references rumbleCts and its token is
not canceled; otherwise return without writing. Keep the existing delay,
cleanup, and ownership checks unchanged.
In `@HandheldCompanion/Managers/LayoutManager.cs`:
- Around line 715-724: Update ApplyTouchpadCoordinates so coordinate samples
blend with, or use explicitly documented precedence against, RightPadX and
RightPadY values produced by ProcessButtonActions, ProcessAxisActions, and
ProcessGyroActions in the same tick. Preserve the existing coordinate conversion
while preventing direct assignments from overwriting AxisLayoutFlags.RightPad
touchpad, axis-plan, or gyro mappings.
---
Nitpick comments:
In `@HandheldCompanion/Managers/LayoutManager.cs`:
- Around line 874-893: Extract the duplicated gyro blending and axis assignment
logic from the ActionType.Joystick and ActionType.Touchpad branches into one
private helper accepting the target AxisLayoutFlags, weight, and incoming
Vector2. Update both branches to call the helper while preserving their existing
value sources and touch-button handling.
In `@HandheldCompanion/ViewModels/Layout/LayoutItemPageViewModel.cs`:
- Around line 114-126: Replace the local actionType switch in the surrounding
method with MappingViewModel.GetActionTypeDisplayName(), using the current
mapping’s action type value as required by that API. Remove the hardcoded
indices and English labels so the displayed name comes from the shared localized
resource lookup.
In `@HandheldCompanion/ViewModels/Layout/Mappings/ButtonMappingViewModel.cs`:
- Around line 280-285: Update Button2AxisVisibility so the mixed || and &&
condition has explicit parentheses around the intended grouped clauses,
preserving the current evaluation of the Joystick case and the Touchpad
axis-action case.
In `@HandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.cs`:
- Around line 365-375: Remove the redundant TouchpadSettingsVisibility and
TouchpadSwipeSettingsVisibility overrides from AxisMappingViewModel (lines
380-388), ButtonMappingViewModel (lines 418-426), and TriggerMappingViewModel
(lines 26-34), allowing them to inherit the shared virtual implementations from
MappingViewModel. Keep MappingViewModel (lines 365-375) unchanged as the single
source of truth.
- Around line 343-363: Cache the controller lookup and computed touchpad target
results used by TouchpadActionTypeVisibility and
TouchpadAxisActionTypeVisibility instead of recalculating them on every getter
evaluation. Invalidate those cached values in the existing
VirtualManager_ControllerSelected handler alongside ActionTypeChanged, ensuring
subsequent property evaluations refresh against the newly selected controller.
In `@HandheldCompanion/ViewModels/Pages/ProfilesPageViewModel.cs`:
- Around line 3188-3203: Update the MotionOutputModes synchronization logic
before the placement loop to explicitly remove every existing mode whose Value
is not contained in desiredModes, rather than pruning only from the tail
afterward. Keep the existing insertion and move behavior in the loop, and remove
the trailing count-based while removal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb138935-7390-4243-8909-837deb145199
📒 Files selected for processing (31)
HandheldCompanion/Actions/IActions.csHandheldCompanion/Actions/MouseActions.csHandheldCompanion/Actions/TouchpadActions.csHandheldCompanion/App.configHandheldCompanion/Controllers/IController.csHandheldCompanion/Controllers/Steam/GordonController.csHandheldCompanion/Controllers/Steam/NeptuneController.csHandheldCompanion/Controllers/Steam/SteamController.csHandheldCompanion/Converters/ActionTypeIndexToNameConverter.csHandheldCompanion/Converters/MotionOutputToNameConverter.csHandheldCompanion/Extensions/GlyphExtensions.csHandheldCompanion/Managers/LayoutManager.csHandheldCompanion/Misc/Layout.csHandheldCompanion/Misc/LayoutTemplate.csHandheldCompanion/Properties/Resources.Designer.csHandheldCompanion/Properties/Resources.resxHandheldCompanion/Properties/Settings.Designer.csHandheldCompanion/Properties/Settings.settingsHandheldCompanion/Utils/InputUtils.csHandheldCompanion/ViewModels/Layout/LayoutItemPageViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/AxisMappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/ButtonMappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/GyroMappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/MappingViewModel.csHandheldCompanion/ViewModels/Layout/Mappings/TriggerMappingViewModel.csHandheldCompanion/ViewModels/Pages/ProfilesPageViewModel.csHandheldCompanion/Views/Pages/ActionSettingsPage.xamlHandheldCompanion/Views/Pages/ControllerPage.xamlHandheldCompanion/Views/Pages/ControllerPage.xaml.csHandheldCompanion/Views/QuickPages/QuickProfilesPage.xamlHandheldCompanion/Views/TemplatesDictionary.xaml
💤 Files with no reviewable changes (3)
- HandheldCompanion/Actions/MouseActions.cs
- HandheldCompanion/Controllers/Steam/NeptuneController.cs
- HandheldCompanion/Controllers/Steam/GordonController.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- HandheldCompanion/App.config
- HandheldCompanion/Views/Pages/ControllerPage.xaml
| <Setting Name="TrackpadClickHaptics" Type="System.Int32" Scope="User"> | ||
| <Value Profile="(Default)">3</Value> | ||
| </Setting> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'SteamTrackpadClickHaptics|TrackpadClickHaptics' \
-g '*.cs' -g '*.settings' -g '*.config' .
fd -t f -i 'CustomSettingsProvider' . -x rg -n -C 8 \
'Load|Read|Migrate|Upgrade|SteamTrackpadClickHaptics|TrackpadClickHaptics' {}Repository: Valkirie/HandheldCompanion
Length of output: 26406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## settings provider file candidates"
fd -i 'CustomSettingsProvider' . -t f
echo
echo "## relevant Provider source"
files="$(fd -i 'CustomSettingsProvider' . -t f)"
for f in $files; do
echo "--- $f"
nl -ba "$f" | sed -n '1,210p'
done
echo
echo "## all references to old names and migration methods"
rg -n -C 3 'SteamTrackpadClickHaptics|TrackpadClickHaptics|PropertyValuesSaved|PropertyValuesLoading|ResetProperties|SettingChanging|PropertyChanged|OnSettingChanged|SettingGetting|SettingChanging|ApplicationSettingsBase' \
-g '*.cs' .Repository: Valkirie/HandheldCompanion
Length of output: 367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## relevant Provider source"
for f in ./HandheldCompanion/CustomSettingsProvider.cs; do
printf '%s\n' "--- $f"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,240p'
done
echo
echo "## all references to old names and migration methods"
rg -n -C 3 'SteamTrackpadClickHaptics|TrackpadClickHaptics|PropertyValuesSaved|PropertyValuesLoading|ResetProperties|SettingChanging|PropertyChanged|OnSettingChanged|SettingGetting|SettingChanging|ApplicationSettingsBase' \
-g '*.cs' .Repository: Valkirie/HandheldCompanion
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## CustomSettingsProvider relevant source"
awk '{printf "%6d\t%s\n", NR, $0}' HandheldCompanion/CustomSettingsProvider.cs | sed -n '1,180p'
echo
echo "## focused references in C# files"
rg -n -C 4 'SteamTrackpadClickHaptics|TrackpadClickHaptics' -g '*.cs' .
echo
echo "## focused migration/event references in C# files"
rg -n 'PropertyValuesSaved|PropertyValuesLoading|ResetProperties|SettingChanging|OnSettingChanged|ApplicationSettingsProvider' -g '*.cs' .Repository: Valkirie/HandheldCompanion
Length of output: 31081
Preserve the old trackpad haptics value during settings load.
Settings.settings and App.config now declare TrackpadClickHaptics, while no persisted setting references SteamTrackpadClickHaptics. CustomSettingsProvider.GetPropertyValues returns the default value 3 when the setting node is missing, so existing user config with SteamTrackpadClickHaptics is not migrated. Check for the old key when loading TrackpadClickHaptics, then write it under the new name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@HandheldCompanion/Properties/Settings.settings` around lines 125 - 127,
Update CustomSettingsProvider.GetPropertyValues to migrate the legacy
SteamTrackpadClickHaptics value when loading TrackpadClickHaptics: if the new
setting node is absent, read the old key, use its value, and persist it under
TrackpadClickHaptics instead of defaulting to 3.
| case ActionType.Touchpad: | ||
| if (SelectedTarget.Tag is not null) | ||
| SetTouchpadTarget(SelectedTarget.Tag); | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This touchpad case omits the property refresh used by the sibling view models.
AxisMappingViewModel (line 912), ButtonMappingViewModel (line 742) and TriggerMappingViewModel (line 357) all call OnPropertyChanged(string.Empty) after SetTouchpadTarget. This override does not. SetTouchpadTarget changes TargetType, and base visibility properties such as Button2AxisVisibility and AxisSettingsSectionVisibility depend on TargetType. Those bindings therefore do not refresh here.
Add the same refresh, or state in a comment why the gyro mapping does not need it.
🐛 Proposed fix
case ActionType.Touchpad:
if (SelectedTarget.Tag is not null)
+ {
SetTouchpadTarget(SelectedTarget.Tag);
+ OnPropertyChanged(string.Empty);
+ }
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case ActionType.Touchpad: | |
| if (SelectedTarget.Tag is not null) | |
| SetTouchpadTarget(SelectedTarget.Tag); | |
| break; | |
| case ActionType.Touchpad: | |
| if (SelectedTarget.Tag is not null) | |
| { | |
| SetTouchpadTarget(SelectedTarget.Tag); | |
| OnPropertyChanged(string.Empty); | |
| } | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@HandheldCompanion/ViewModels/Layout/Mappings/GyroMappingViewModel.cs` around
lines 435 - 438, Update the Touchpad branch in the relevant mapping override to
call OnPropertyChanged(string.Empty) immediately after SetTouchpadTarget,
matching AxisMappingViewModel, ButtonMappingViewModel, and
TriggerMappingViewModel so TargetType-dependent visibility bindings refresh.
Adds per-mapping action type support and updates the action settings UI to disable unsupported options. Also broadens button visibility handling for applicable action types.
Expose the axis-to-button threshold slider for trigger mappings and update related visibility notifications.
Round response curve axis values to one decimal place before formatting, ensuring displayed labels match the intended precision.
|
I'm working on it. I'm getting a hard time figuring out what's going on with TouchpadSettingsVisibility and the conditions to meet. I also believe some of those new options are almost dupes of the current ones. Like Coordinate click and Coordinate touch. I'll simplify some of it. |
Adds touchpad coordinate visualization and live property updates, enables coordinate targets for DualShock 4 controllers, reorganizes mapping properties, and updates touchpad labels and designer preview visibility.
Protects concurrent access to profile collections and layout data during mutations, sanitization, and serialization by introducing profile synchronization and locking layout operations.
|
My open thinking on this PR. We have LeftPadTouch, RightPadTouch, TouchpadCoordinateTouch, LeftPadClick, RightPadClick, TouchpadCoordinateClick.
|
Replace coordinate and center-click mappings with unified touchpad click/touch gestures, including finger-aware DualSense output and updated controller targets.
Stop adding touchpad axis targets to button and trigger mapping target lists, including matching logic for axis actions.
|
We're getting there... |
Add configurable left/right touchpad fingers for mapped actions and preserve simultaneous touch output across DualShock 4 and DualSense targets.
Use output touch activity to select the click finger, remove synthetic touch coordinates and redundant axis updates, and simplify click sequencing state.
Replace the removed MicrophoneMute flag with B5 in the dummy controller and DualSense target while preserving the mute button bit mapping.
Corrects Steam Deck and trackpad settings panel visibility, safely handles missing controllers, and marks trackpad touch/click flags as UI-only.
Move touchpad frame state and output application into TouchpadActions, simplifying LayoutManager processing while preserving axis accumulation, gesture prioritization, and haptic updates.
Unifies haptic handling across controllers, adds touchpad axis deadzone support, simplifies touchpad output processing, and removes the global trackpad haptics setting in favor of per-action configuration.
Remove per-action global haptic override handling and UI, assign default touchpad click and axis haptic settings, and enable Steam Controller gesture targets.
Allow touchpad click and touch actions to optionally use configured coordinates, expose the setting in the action UI, and enable coordinate-based default mappings.
2 participants
Summary
TouchpadActions.Problem
Disabling Steam lizard mode also removes the firmware-generated trackpad feedback. Touchpad behavior had also become split across mouse and button actions, which made device support and settings difficult to keep consistent.
Implementation
TouchpadActionsnow owns touchpad button targets, axis targets, coordinate and swipe handling, and touchpad haptics. Layout target lists are built from the capabilities advertised by the selected virtual controller. Existing mappings with unavailable targets are kept intact instead of being silently replaced.Movement feedback uses the selected controller's normal haptic path. Steam trackpad clicks use native Steam haptic commands for distinct strength levels and correct left and right routing. Per-mapping haptic settings can override the global click setting, and the Controller page reports when an override is active.
Quick Profiles now represents gyro output to either touchpad. These options are capability-driven while still displaying an existing unsupported mapping so loading a profile remains non-destructive.
Validation
git diff --checksuccessfully.Physical validation was performed on a Steam Deck. Other controller paths require testing on their respective hardware.
Built on #1454.
Closes #590