Skip to content

Conversation

Rene-Damm
Copy link
Contributor

@Rene-Damm Rene-Damm commented Oct 27, 2021

Fixes 1306361 (FogBugz).

Description

When setting up a multiplayer UI using MultiplayerEventEvent, pointer input is confined to the playerRoot of each player. However, navigation input -- itself much more relevant to multiplayer input compared to pointer input -- is allowed to escape a playerRoot and thus makes it possible for one player to move onto the UI of another player.

Changes made

The navigation mechanism built into uGUI's Selectable is not overridable or extensible. We thus cannot limit it to parts of the UI. Discussing this with @pcosgrave.

This PR attempts a workaround by relying on the CanvasGroup mechanism that allows toggling the interactable property off for an entire hierarchy and thus all Selectable objects in the hierarchy.

Notes

CanvasGroup is not a fast mechanism. The underlying implementation relies on BroadcastMessage to reach Selectable objects. Might be good enough as a stopgap but likely, we want a more lightweight mechanism to get Selectable to only pick certain UI objects.

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.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • 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.

@Rene-Damm Rene-Damm requested review from ekcoh and pcosgrave October 27, 2021 14:09
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

{
m_CanvasGroup = m_PlayerRoot.GetComponent<CanvasGroup>();
if (m_CanvasGroup == null)
m_CanvasGroup = m_PlayerRoot.AddComponent<CanvasGroup>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this add free some side-effects? I assume CanvasGroup has other implications? Reading through the docs I guess this could work just fine if "part of solution" for local multiplayer UI and having two "systems" isn't a viable solution in itself to create two navigation trees that are disjoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a range of implications and side-effects -- most importantly, it has pretty poor performance. This is more a workaround than a proper solution. Unfortunately, with the tools we have available ATM, it doesn't seem like there's a better solution for the moment. Overall, think this is part of some extensions to uGUI that are needed to support this use case properly. Most of MultiplayerEventSystem is a workaround.

{
var index = s_MultiplayerEventSystems.IndexOfReference(this);
if (index != -1)
s_MultiplayerEventSystems.EraseAtWithCapacity(ref s_MultiplayerEventSystemCount, index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case CanvasGroup have been added by OnEnable(), should OnDisable() remove it in that case for symmetry? The user didn't intentionally added it.

@Rene-Damm Rene-Damm merged commit da6732a into develop Nov 29, 2021
@Rene-Damm Rene-Damm deleted the fix-multiplayer-ui-navigation branch November 29, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants