FIX: Project wide actions enabling actions overrides PlayerInput (ISXB-920) #2144
Conversation
There was a problem hiding this comment.
Changes/fix looks good to me @ritamerkl. I think auto-enabling action maps for Project-wide input actions might cause more issues than it solves generally. For this fix, would it be possible to add a test case to make sure we have fixed the bug and avoid future regression?
There was a problem hiding this comment.
Seems like PWA are enabled whether they're being used or not, is it expected?
https://github.com/user-attachments/assets/3e177cdf-3d46-4dc6-9f13-684a6d1cbe98
In the video I first show off this PR, PWA actions are enabled in the debugger alongside the player input actions I'm using and then show the same thing on current develop
|
I changed the approach to fix to duplicate the action asset always, since we do not want to operate directly on it, but on a copy for PlayerInput. This is the case for project wide action assets as well as for assets that are used for the UIInputModule or elsewhere. This should fix the issues you mentioned @Pauliusd01. |
…the original that might be used elsewhere
|
Hold back with testing / reviews for now, WIP |
… actions enabling calls
| Assert.That(actions["UI/Navigate"].controls, Is.Empty); | ||
| Assert.That(actions["UI/Point"].controls, Is.EquivalentTo(new[] { mouse.position })); | ||
| Assert.That(actions["UI/Click"].controls, Is.EquivalentTo(new[] { mouse.leftButton })); | ||
| Assert.That(player.actions.FindActionMap("Gameplay").enabled, Is.True); |
There was a problem hiding this comment.
Maybe insert a new line after this one and add a comment why there is a split now?
| @@ -391,12 +392,13 @@ public void PlayerInput_CanAssignActionsToPlayer() | |||
| var actions = InputActionAsset.FromJson(kActions); | |||
| playerInput.actions = actions; | |||
|
|
|||
| Assert.That(playerInput.actions, Is.SameAs(actions)); | |||
| Assert.That(playerInput.actions, !Is.SameAs(actions)); // Should have created a copy. | |||
There was a problem hiding this comment.
This is dangerous since it would call Equals or any overload, what you really want to check here is ReferenceEquals to make sure its a different instance (some types compare equal even if they are different instances)
There was a problem hiding this comment.
outdated with new commits, replaced with (playerInput.actions.actionMaps[0].name, Is.EqualTo(actions.actionMaps[0].name)
| @@ -391,12 +392,13 @@ public void PlayerInput_CanAssignActionsToPlayer() | |||
| var actions = InputActionAsset.FromJson(kActions); | |||
| playerInput.actions = actions; | |||
|
|
|||
| Assert.That(playerInput.actions, Is.SameAs(actions)); | |||
| Assert.That(playerInput.actions, !Is.SameAs(actions)); // Should have created a copy. | |||
| Assert.That(playerInput.actions.actionMaps.Count, !Is.SameAs(actions.actionMaps.Count)); | |||
There was a problem hiding this comment.
I'm confused around this assert, what is it proving?
There was a problem hiding this comment.
outdated with new commits, replaced with (playerInput.actions.actionMaps[0].name, Is.EqualTo(actions.actionMaps[0].name)
| } | ||
|
|
||
| [Test] | ||
| [Category("PlayerInput")] | ||
| public void PlayerInput_AssigningNewActionsToPlayer_DisablesExistingActions() | ||
| public void PlayerInput_AssigningNewActionsToPlayer_LeavingOriginalAssetsUntouched() |
There was a problem hiding this comment.
Related to this, what happens when the original asset is modified after being cloned? Is the PlayerInput updated to reflect or just fixed to what it was when instantiated? I am not sure if it intends to monitor that or not...
There was a problem hiding this comment.
If the original changes it wouldn't reflect in the copies of the PlayerInput actions component. They could only be changed through the playerInput.actions property for a single PlayerInput.
Thinking about it I am less certain if it wouldn't introduce more regression seeing this comment:
// Make sure that no cloning of actions happened on the prefab.
// https://fogbugz.unity3d.com/f/cases/1319756/
Thus I wonder if that is no problem for the other PlayerInput instances, since it will be cloned if more than one player jumps in.
| for (var binding = 0; binding < oldActions.actionMaps[actionMap].bindings.Count; binding++) | ||
| m_Actions.actionMaps[actionMap].ApplyBindingOverride(binding, oldActions.actionMaps[actionMap].bindings[binding]); | ||
| } | ||
| // duplicate action asset to not operate on the original(as it might be used outside - eg project wide action asset or UIInputModule) |
There was a problem hiding this comment.
IMO this is much cleaner to not let it depend on some condition whether there is a clone or not.
ritamerkl
force-pushed
the
fix-project-wide-actions-override-playerinput
branch
from
2281ca6 to
b05b5e8
Compare
| @@ -342,8 +342,14 @@ public InputActionAsset actions | |||
| UninitializeActions(); | |||
| } | |||
|
|
|||
| var didChange = m_Actions != null; | |||
There was a problem hiding this comment.
I guess this is related to your question around comparing InputActionAssets? It's not going to be cheap to compare, but a comparator would be useful. Additionally that comparator could compute a hash to make the comparison cheaper. However, this could also be fine since it will very likely be different if it's another instance. Otherwise the project itself is oddly designed.
There was a problem hiding this comment.
Yes, all this is around avoiding copying the asset multiple times, which works as far as I tested and for the cases I could come up with, but like you said ideally it would be great to be able to compare an copy of asset to it's original to determine that it's a clone of the asset.
| @@ -1430,6 +1429,18 @@ private void InitializeActions() | |||
| m_ActionsInitialized = true; | |||
| } | |||
|
|
|||
| private void CopyActionAsset() | |||
There was a problem hiding this comment.
Maybe rename this CopyActionAssetAndApplyBindingOverrides() since it attempts to preserve binding overrides? I wonder how it actually works though since the new asset might be completely different.... (not new on this PR)
There was a problem hiding this comment.
addressed in ce0a9a5
|
Still seeing some odd things with project wide actions getting duplicated. |
Yes, that is intended. The project wide actions will be enabled, if the PlayerInput uses the same action asset like the project wide actions, the PlayerInput clone of the asset will be enabled at the same time. Also, I noticed that the actions asset gets the "cloned" tag as well in your version Yes, that is due to the fact that the PlayerInput asset is duplicated for the first player now too. But this will reset to the original asset exiting the play mode. |
Description
This PR fixes this issue: ISXB-920, where the root cause is that the project wide actions enable all action maps and therefore override the PlayerInput settings.
Major changes:
Testing status & QA
Testing with the client project.
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
To QA: please also test if using the project wide actions works like before, and if the PlayerInput with using different action maps (and in best case also multiple action maps enabled at once) works as expected.
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: