Skip to content

FIX: PlayerInput component is redrawn all the time when the notification mode is set to Invoke Unity events #2166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 15, 2025
Merged

FIX: PlayerInput component is redrawn all the time when the notification mode is set to Invoke Unity events #2166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbe216a
patch up PlayerInput editor to prevent from recreating internals all …
K-Tone Apr 10, 2025
25eb7e2
update release notes
K-Tone Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed Input Actions code generation overwriting user files when the names happened to match. [ISXB-1257](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1257)
- Fixed Input Actions code generation using locale-dependent rules when lowercasing and uppercasing strings. [ISXB-1406]
- Fixed an issue when providing JoinPlayer with a specific split screen index. [ISXB-897](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-897)
- Fixed Inspector Window being refreshed all the time when a PlayerInput component is present with Invoke Unity Events nofication mode chosen [ISXB-1448](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1448)
- Fixed an issue where an action with a name containing a slash "/" could not be found via `InputActionAsset.FindAction(string,bool)`. [ISXB-1306](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1306).

## [1.14.0] - 2025-03-20
Expand Down
26 changes: 15 additions & 11 deletions Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInputEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,17 +417,21 @@ private void OnNotificationBehaviorChange()
case PlayerNotifications.InvokeUnityEvents:
{
var playerInput = (PlayerInput)target;
if (playerInput.m_DeviceLostEvent == null)
playerInput.m_DeviceLostEvent = new PlayerInput.DeviceLostEvent();
if (playerInput.m_DeviceRegainedEvent == null)
playerInput.m_DeviceRegainedEvent = new PlayerInput.DeviceRegainedEvent();
if (playerInput.m_ControlsChangedEvent == null)
playerInput.m_ControlsChangedEvent = new PlayerInput.ControlsChangedEvent();
serializedObject.Update();

// Force action refresh.
m_ActionAssetInitialized = false;
Refresh();

bool areEventsDirty = (playerInput.m_DeviceLostEvent == null) || (playerInput.m_DeviceRegainedEvent == null) || (playerInput.m_ControlsChangedEvent == null);

playerInput.m_DeviceLostEvent ??= new PlayerInput.DeviceLostEvent();
playerInput.m_DeviceRegainedEvent ??= new PlayerInput.DeviceRegainedEvent();
playerInput.m_ControlsChangedEvent ??= new PlayerInput.ControlsChangedEvent();

if (areEventsDirty)
Copy link
Collaborator Author

@K-Tone K-Tone Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, recreating is not required in the case that we changed nothing here actually. Worse, if we set m_ActionAssetInitialized to false, it will trigger another reset the next update frame (as explained in the description section).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also for a style-aware reviewer, the null-coalescing operation looks safe in this context because none of the events derive from Unity's Object)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-iinitialization: I find it very confusing in general that the this implementation (also prior to your changes) assigns these events from the editor onto the playerInput instance. It seems the method that includes this code (OnNotificationBehaviorChange) is only intended to be executed whenever there is a reconfiguration of what notification mechanism to use. So from what I can digest from a quick glance at this it seems like you have identified something that is not behaving like its intended if it leads to a loop of updates to the SO.

Re-style: Agreed (UnityEventBase), but it may be less convenient to debug (depending on the debugger). I do not have a strong opinion about this though so less code might be nicer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I find this confusing too. Just acknowledging that the traffic in this file is fairly light, seems it's ok to target for lower impact that minimises the risk of regression in this fragile code corner. Good question what is lower risk though. Blaming this file, I can see that there was this change that probably caused the refresh issue: 3660631. I don't know, maybe we could only set actionsWereChanged when they were really changed and not when we were in fact trying to initialize the thing for the first (and consecutive times). That's maybe more risk to be honest. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fix looks fine (unless I am overlooking something). I think it makes sense to just manually check that we do not regress 3660631 as part of QA/testing this fix.

{
serializedObject.Update();

// Force action refresh.
m_ActionAssetInitialized = false;
Refresh();
}
break;
}
}
Expand Down