FIX: PlayerInput does not work with C# wrappers (ISXB-1535) #2188
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2188 +/- ##
===========================================
- Coverage 65.45% 65.45% -0.01%
===========================================
Files 367 367
Lines 53505 53508 +3
===========================================
Hits 35024 35024
- Misses 18481 18484 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug related to PlayerInput support with C# wrappers by removing the automatic copying of action assets and adding logic to correctly disable project-wide actions when a PlayerInput is active.
- Removed asset copying logic from PlayerInput properties and Awake.
- Added handling to disable project-wide actions via InputSystem and updated corresponding tests.
- Revised tests to assert reference equality and proper enabling/disabling of action maps.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs | Removed asset copying in setter and Awake, added disabling of project-wide actions when enabled. |
| Packages/com.unity.inputsystem/InputSystem/InputSystem.cs | Introduced a flag to control enabling of actions after entering Play Mode with updated logic in OnPlayModeChange. |
| Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs | Updated and added tests to validate new behavior in asset assignment and action map states. |
| Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs | Added tests to verify that project-wide action maps are correctly disabled based on PlayerInput’s default action map. |
Comments suppressed due to low confidence (3)
Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs:342
- Removing the asset copying functionality here changes how action assets are managed. Adding a comment to explain this change will help maintainers understand why modifications to the original asset are now allowed.
var didChange = m_Actions != null;
Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs:1810
- The removal of the Awake method that copied the action asset should be documented, as it significantly alters asset management and may affect users expecting a copied instance.
private void Awake()
Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs:412
- Please confirm that the updated assertions correctly reflect the intended behavior, ensuring that only the appropriate action map is enabled while others remain disabled.
Assert.That(actions2.actionMaps[0].enabled, Is.False);
jfreire-unity
changed the title
jfreire-unity
force-pushed
the
isxb-1535-playerinput-does-not-work-w-csharp-wrappers
branch
2 times, most recently
from
35296c7 to
1b40c51
Compare
jfreire-unity
force-pushed
the
isxb-1535-playerinput-does-not-work-w-csharp-wrappers
branch
from
1b40c51 to
2eb009a
Compare
There was a problem hiding this comment.
I would suggest to test this with the ticket where the original PR was raised and now reverted.
This does not look like a solid solution in long term. This area is fragile and the fix is more a workaround. Since the optimal solution is probably not feasible to implement straight away -that's fine-, but I still wonder if there isn't an option without reverting the copy. I remember we talked about some other variants too.
| /// </remarks> | ||
| internal static bool m_EnableActionsAfterEnterPlayMode = true; | ||
|
|
||
| internal static void AllowEnableActionsAfterEnterPlayMode(bool enabled) |
There was a problem hiding this comment.
I find the naming of the method a bit odd in combination with the parameter. In this case it is hard to understand immediately if "enabled" means the actions or allowing to enable them.
I would suggest rephrasing the method name or calling the parameter something like "bool allowed".
There was a problem hiding this comment.
Thanks, I'll revisit the naming. I did forget to look at the naming again as I did a first iteration of something a bit simpler but that it didn't work.
I'll try to see if I can avoid introducing this and just move EnableActions() to a better place that doesn't require them.
| m_Actions = value; | ||
|
|
||
| if (didChange || m_Enabled) | ||
| // copy action asset for the first player so that the original asset stays untouched | ||
| CopyActionAssetAndApplyBindingOverrides(); |
There was a problem hiding this comment.
I am still concerned with removing the copy for the first player.
In the long run this potentially leads to conflicts again as soon as action assets are accessed and operated from somewhere out of the player input.
There was a problem hiding this comment.
leads to conflicts again
Can you specify which conflicts have occurred before? I'm not aware of any other bugs/conlfits that would require a copy for the first player.
There was a problem hiding this comment.
What I am referring to is that the project wide actions was the first occurrence, since the asset can be operated freely from anywhere else without further restrictions I think it's not unlikely to encounter similar problems in the future (even if we can not directly think of one now).
There was a problem hiding this comment.
I believe project-wide actions didn't consider this use case during the design phase. In the end, PWA it's just another asset with a "small" difference that has all actions enabled. The normal behavior of an asset is to come with actions disabled. But this is not really relevant to the discussion of doing copies, which I want to explore further.
Making a copy during assignment (playerInput.asset = myAsset) is problematic for two reasons:
- It breaks expected behavior in C# - as a developer, I assume reference types maintain references rather than creating copies on assignment.
- It introduces inconsistency with our existing code, like
InputSystemUIInputModule.actionAssetwhich maintains a reference, and doesn't do a copy.
I feel we get into a slippery slope of inconsistencies if we go down the path of doing copies for every asset assignment.
I'm fine with going with a different direction, but I would like to remove the copy behavior on assignment regardless of what we agree on doing.
There was a problem hiding this comment.
I agree on that, assigning with a copy under the hood and the reference is indeed confusing.
Since this happens for all other players (except for the first), the most consistent way to do it would probably to not do it at all, or changing the behaviour to make the copy more visible.
To get a consistent behaviour I think wee need to decide for one way. What I am thinking now is:
- defining a "rootAsset" which can be assigned and then just make the player copy available. (breaking change)
- not copying the asset but outsourcing the runtime data to a metafile for instance (bigger change, but maybe not necessarily breaking change?)
I think the third option is the solution you proposed, which rolls back to the previous inconsistent state. I think that's valid with the argument to not break anything, though I believe it's not ideal in terms of consistent behaviour.
| if (m_Actions != null && m_Actions == InputSystem.actions) | ||
| { | ||
| InputSystem.AllowEnableActionsAfterEnterPlayMode(false); | ||
| InputSystem.actions.Disable(); |
There was a problem hiding this comment.
This will have an effect on the PlayerInput asset too, is disabling of all actions intended at this spot?
The bug mentioned is fixed with this PR, as well as the one this PR targets. The other solutions I previously proposed for maintaining the copy expose yet another public API to users. This exposes no other API to the users. Ideally, I would even prefer that this code would not exist, but I'm not seeing another way atm. I think if we communicate clearly that PlayerInput actions disable project-wide actions at "startup", it's still better than maintaining the copy behavior and introducing a flag to users. Project-wide actions behavior is to have all action maps enabled so that it's easier for users to get any kind of Input. |
I am not sure what you are referring to, the setter will create a copy once the PlayerInput action asset is set in runtime (which basically means set by code (which is consistent with the copy after assigning it in the component). To refer to the asset users would need to use the PlayerInput actions instead of the asset, which I think is feasible, since for multiplayer this logic would be wrong anyway- it would only affect the first user. It changes the way to access the PlayerInput asset via code, that is true, but I think the way to access the real asset for the first player, neglecting that all other players may reference to a copy is just wrong in the first place.
That might apply to the two cases, I agree. Thinking about future regressions/upcoming bugs I am not very sure if that's the best way.
Now we did not really fix one of the root causes, so my gut feeling is that there are more bugs to come.
|
|
I understand the effort to not introduce new API or change behavior significantly though, and support that. |
|
Regarding
Is my understanding correct that it essentially says that one cannot use PlayerInput and Project-wide Input Action Assets (in other places) at the same time right? Since PlayerInput acts as "manager of enabled action maps"? |
| /// <remarks> | ||
| /// This property is set to <c>true</c> by default. | ||
| /// </remarks> | ||
| internal static bool m_EnableActionsAfterEnterPlayMode = true; |
There was a problem hiding this comment.
If this is kept, it should be private.
|
|
||
| [Category(TestCategory)] | ||
| [Test] | ||
| // Test that will make sure that a PlayerInput component using project wide actions will: |
There was a problem hiding this comment.
Thanks for adding an inline description of the test, but maybe put it inside Description(string) attribute instead?
| @@ -55,7 +55,7 @@ public void PlayerInput_CanInstantiatePlayer() | |||
|
|
|||
| Assert.That(player, Is.Not.Null); | |||
| Assert.That(player.playerIndex, Is.EqualTo(0)); | |||
| Assert.That(player.actions.actionMaps.Count, Is.EqualTo(prefabPlayerInput.actions.actionMaps.Count)); | |||
| Assert.That(player.actions, Is.SameAs(prefabPlayerInput.actions)); | |||
There was a problem hiding this comment.
Not sure if this maps to Object.ReferenceEquals, might be good to double check so its not by value
There was a problem hiding this comment.
I looked into the docs, and it checks two references are the same 👍
Technically, you can. Since you can operate on both assets at runtime without restrictions. Project-wide actions asset is just a normal asset, that comes with all actions enabled by default, right? This default behavior of PWA is what clashes with PlayerInput initialization behavior since it tries to enable just a single action map (such as `PlayerInput.defaultActionMap) which is already enabled. But doesn't disable ALL the other ones. It is still possible that |
Description
This PR attempts to fix a PlayerInput issue when working with code generated from InputActionAssets (vulgarly called a "C# Wrapper).
In that attempt, it reverts #2144 since it introduces a regression resulting in ISXB-1535.
The fundamental problem of #2144 is caused by project-wide actions having ALL action maps enabled by default, which can create a problem if the user has the same binding in both action maps of project-wide actions assigned to
PlayerInput. In the first time a control with the same binding or more than one action map is actuated, all of its actions in different action maps will beperformed❌ See ISXB-920 for more.Also, if a user tries to call
InputSystem.actions.Disable()on Start/Awake/OnEnable in their scripts to disable action maps manually, it will not work. The actions will still be enabled withInputSystem.EnableActions()in Play-mode (this doesn't happen in Standalone builds).Below there's a sketch of the events code flow order in the Play-mode (last step doesn't occur in the standalone build, so the problem wouldn't exist).
flowchart LR A["InitializeInEditor(or inPlayer)<br/><i>calls EnableActions()</i>"] B["PlayerInput.OnEnable<br/><i>calls SwitchCurrentActionMaps()</i>"] C["User Scripts<br/><i>Start(), OnEnable(), Awake(), etc </i>"] D["InputSystem.OnPlayModeChange(EnteredPlayMode)<br/><i>calls EnableActions()</i>"] A --> B --> C --> D style D stroke:#ffff00,stroke-width:2pxIn #2144 a copy of the asset for a PlayerInput component is ALWAYS made, regardless if you're in single player or local multiplayer. The copied asset has all its action maps disabled by default. This allows the PlayerInput to enable the ones it needs to and fixes the problem.
However, C# wrappers have "self-contained" assets (see e.g. DefaultInputActions.asset). So if a copy of the self-contained asset is made and assigned to
playerInput.asset, using the default wrapper functionality will not work, since it acts on the self-contained asset ❌ See test introduced to make sure we fix the problem reported by the user:PlayerInput_ActionFromCodeGeneratedActionIsTheSameBeingReferenced.Instead of creating a copy of the asset, I propose a solution where the
PlayerInputcomponent using project-wide actions will disable all its action maps at startup. This allowsPlayerInputto then select the appropriate action map to be enabled (either a default action map or some other one the user enables in their scripts).This somehow needs to happen because the intended default behavior of ProjectWideActions is to have all actions enabled by default, which will cause problems with
PlayerInputas it only selects the one that should be enabled.Testing status & QA
Tested both projects of reported bugs, ISXB-1535 and ISXB-920.
Added automated testing to avoid future regressions.
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
Alternative: If this is interpreted as big breaking change, then we could instead allow for the user to be able to disable action maps in Start/Awake/OnEnable.
If this PR is accepted, I will add public documentation for it with regards to PlayerInput and project-wide actions sections
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.After merge: