fix: bug in tanks sample

[Markario]

Small bug in the Tanks sample that prevents local player from clearing the ready menu if the network players are already ready.

[uweeby]

It seems to work as it when I tested it. I build a standalone and ran the editor as host. Joining the standalone as client i tried hitting ready on both the host first and then the client first. In both instances I didnt get stuck anywhere. Did I test it wrong?

[Markario]

For me it's when a third player hits the ready button. I forgot to specify that above.

[Markario]

GameReadyCheck will hide the menus and enable movement because the network players are ready (and I'm guessing that at that point the localPlayer isn't in that list yet, still wrapping my head around this library) and ShowReadyMenu will then negate that change immediately because the local player is not ready.

[paulpach]

@Markario thanks for this PR, looks like you are on to something.
Could you provide step by step instructions on how to reproduce the bug?

[uweeby]

I tried with 3 clients and regardless of the order they start in it seems to work.

[Markario]

Interesting 🤔

Previously I was testing on Unity 2019.4.20f1 with mirrorNG 63.5 on Windows 10. Tested again with Unity 2020.2.4f1 and Mirage 66.2.1 on Mac. Same behavior.

• Loaded the tanks sample
• built and ran 2 apps
• hit play in editor, started host mode (in editor), entered name, hit enter
• started client mode in 1st app, entered name, hit enter
• started client mode in 2nd app, entered name, hit enter, menu doesn't go away

With some logging the behavior I see is:

StartPanel is hidden before the 3rd player has clicked ready

Mirage/Assets/Mirage/Samples~/Tanks/Scripts/TankGameManager.cs

Lines 77 to 79 in 8efe7ce

	//Update Local GUI:
	StartPanel.SetActive(false);
	HealthTextLabel.SetActive(true);

Then shown again because they are not ready

Mirage/Assets/Mirage/Samples~/Tanks/Scripts/TankGameManager.cs

Lines 59 to 61 in 8efe7ce

	StartPanel.SetActive(true);
	}

After hitting ready, the menu is not hidden because GameReadyCheck() will not be called again (skipped when IsGameReady=true)

Mirage/Assets/Mirage/Samples~/Tanks/Scripts/TankGameManager.cs

Line 66 in 8efe7ce

if (!IsGameReady)

Perhaps it is the case that TankGameManager logic is correct, but instead there is an error in which SpawnedObjects are added to players which is checked in

Mirage/Assets/Mirage/Samples~/Tanks/Scripts/TankGameManager.cs

Line 101 in 8efe7ce

foreach (Tank tank in players)

I'm still poking around this library so I have no idea about timing of server<->client syncing in NetworkManager.ClientObjectManager.SpawnedObjects but it seems that the local player's Tank is not in the resulting players list and that is why I made the change in this PR.

[Markario]

Looking at this again, maybe it's not the intention to allow the order of events I was testing with. Just kept testing with what felt natural to me, but it appears to work smoothly if I start host mode and both client modes before clicking ready on any of them.

[uweeby]

Ooo a late join scenario. I can see how it would break now. If this was a real game it would make more sense to either block new connections after the game starts or put them in some kind of queue. Since it's a developer example I don't think late joiners are an issue. So I'm ok with the change.

[Markario]

Yeah, I wasn't really sure how the sample was intended to be used, but it makes sense that this behavior would be explicitly handled in a real implementation so patching it or not is fine with me. I was testing the sample in the way I eventually intend to implement it myself so I was a bit tunnel visioned.

[uweeby]

I will leave it up to you. If you want to resolve the conflict on the commit so it passes we can merge it. Otherwise if you want to close it that's fine too .

[github-actions]

🎉 This PR is included in version 67.2.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀