DOCS: Warn against switching action maps from within action callbacks#2406
DOCS: Warn against switching action maps from within action callbacks#2406MorganHoarau wants to merge 1 commit intodevelopfrom
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The review identified a few areas for improvement in the documentation examples:
- Added a check for the action phase in the
OnFirecallback to prevent redundant map switches. - Included missing variable declarations to make the code snippet more self-contained.
🤖 Helpful? 👍/👎
| > void OnFire(InputAction.CallbackContext context) | ||
| > { | ||
| > // Do not switch maps here directly — defer it instead. | ||
| > m_SwitchActionMap = true; | ||
| > m_NextActionMap = "UI"; | ||
| > } |
There was a problem hiding this comment.
The OnFire callback is triggered for all phases of an action (e.g., Started, Performed, and Canceled). Without checking the context phase, the logic to switch the action map will be queued every time the button is pressed and again when it is released.
This can lead to unexpected behavior, such as the map switching back to "UI" when the user releases the "Fire" button, even if another map switch was intended in between. It is generally best practice to check if the action was performed:
void OnFire(InputAction.CallbackContext context)
{
if (!context.performed) return;
// Do not switch maps here directly — defer it instead.
m_SwitchActionMap = true;
m_NextActionMap = "UI";
}🤖 Helpful? 👍/👎
| > bool m_SwitchActionMap; | ||
| > string m_NextActionMap; |
There was a problem hiding this comment.
The variable playerInput is used on line 199 but is not declared within the snippet. To make the example more complete and self-contained for users, consider adding its declaration.
| > bool m_SwitchActionMap; | |
| > string m_NextActionMap; | |
| PlayerInput playerInput; | |
| bool m_SwitchActionMap; | |
| string m_NextActionMap; |
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2406 +/- ##
===========================================
- Coverage 77.92% 77.40% -0.52%
===========================================
Files 482 483 +1
Lines 97755 98577 +822
===========================================
+ Hits 76175 76306 +131
- Misses 21580 22271 +691
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
My assumption was wrong, no need to update doc, issue was in user code. |
1 participant
Description
Issue: The documentation for action callbacks did not warn users against enabling/disabling action maps (or calling PlayerInput.SwitchCurrentActionMap) from within a callback. Doing so while the Input System is processing events can corrupt internal state and trigger errors.
Fix: Added a WARNING callout to
RespondingToActions.md(in the action callbacks section) that explains the restriction and provides a deferred-switch pattern using a flag + Update() as the recommended workaround.https://jira.unity3d.com/browse/ISXB-1367
Testing status & QA
NA
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.