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

[K-Tone]

Description

A customer noted that if you change the notification mode on a PlayerInput component, that forces refresh of the inspector window every frame (can be observed by watching at the blinking dot at the left top corner).

The reason this happens is PlayerInputEditor component has lazy initilization logic in OnInspectorGUI

            if (EditorGUI.EndChangeCheck() || !m_ActionAssetInitialized || CheckIfActionAssetChanged())
            {
                OnActionAssetChange();
                actionsWereChanged = true;
            }

When we reach this block, m_ActionAssetInitialized is always false -- since it's false by default, and it will keep being reset every frame, as you'll see in a bit.

So, we fall inside, set actionsWereChanged, and dive into OnActionAssetChange where we do all sorts of things, but most importantly, we set m_ActionAssetInitialized (unconditionally, in the beginning of the function).

However, if we keep reading OnInspectorGUI further, we see this block

            if (EditorGUI.EndChangeCheck() || actionsWereChanged || !m_NotificationBehaviorInitialized)
                OnNotificationBehaviorChange();

Recall that actionsWereChanged is set in the first block and hasn't been changed yet, so we fall into the if block and call OnNotificationBehaviorChange where we have this wonderful final block:

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();

You can see that we're resetting m_ActionAssetInitialized and updating the serializedObject as well. This is precisely what triggers the inspector window update, since the next update we will surely be following exactly the same steps.

The patch I'm applying aims at minimal impact by not resetting m_ActionAssetInitialized when none of the device events need to be recreated any more. This looks safe.

Testing status & QA

Local testing, waiting on a QA.

Overall Product Risks

• Complexity: Low
• Halo Effect: Low

Checklist

Before review:

• Changelog entry added.
  • Explains the change in Changed, Fixed, Added sections.
  • For API change contains an example snippet and/or migration example.
  • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
  • Jira port for the next release set as "Resolved".
• Tests added/changed, if applicable.
  • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
  • Performance tests.
  • Integration tests.
• Docs for new/changed API's.
  • Xmldoc cross references are set correctly.
  • Added explanation how the API works.
  • Usage code examples added.
  • The manual is updated, if needed.

During merge:

• Commit message for squash-merge is prefixed with one of the list:
  • NEW: ___.
  • FIX: ___.
  • DOCS: ___.
  • CHANGE: ___.
  • RELEASE: 1.1.0-preview.3.

After merge:

• Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

[K-Tone]

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).

[K-Tone]

(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)

[ekcoh]

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.

[K-Tone]

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?

[ekcoh]

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.

[ekcoh]

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.