Skip to content

fix: spawning new player does not remove previous #1779

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.

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

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Mar 7, 2022

Spawning a new player object with SpawnAsPlayerObject does not set the IsPlayerObject property of the previous player object to false allowing for multiple player objects which shouldn't be the case.
MTT-1570

Changelog

com.unity.netcode.gameobjects

-Fixed: issue when spawning new player if an already existing player exists it does not remove IsPlayer from the previous player.

Testing and Documentation

  • Includes integration tests.

This will un-mark any NetworkObjects currently marked as a player if a new player instance is spawned for an already connected player.
Handles both client and server sides.
This is the test to validate this fix.
Copy link
Contributor

@mattwalsh-unity mattwalsh-unity left a comment

Choose a reason for hiding this comment

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

Would like some more comments in the test and have the test named more self-describingly

[UnityTest]
public IEnumerator SpawnAnotherPlayerObject()
{
var originalPlayer = m_PlayerNetworkObjects[1][1];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to rationalize to myself that I should know why I am looking this up by [1][1]. Worth a comment to explain.

}

[UnityTest]
public IEnumerator SpawnAnotherPlayerObject()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would title this "TestReplacementPlayerObjectCleanup" or similar. "SpawnAnotherPlayerObject", if I'm coming in cold, just tells me we are testing spawning yet another PlayerObject - a title that specifically conveys that you are checking that old player objects are de-player-ified will help future generations

updating name for better clarity
removed hard-coded indices values with relative properties
adding comments for clarity
updated comments slightly.
minor typo adjustment
@NoelStephensUnity NoelStephensUnity enabled auto-merge (squash) March 7, 2022 21:25
@NoelStephensUnity NoelStephensUnity merged commit 419df98 into develop Mar 8, 2022
@NoelStephensUnity NoelStephensUnity deleted the fix/mtt-1610-spawning-new-player-doesnt-remove-previous branch March 8, 2022 01:19
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