Skip to content

Fix/migration cleanup lost changes#417

Merged
mattwalsh-unity merged 4 commits intodevelopfrom
fix/migration-cleanup-lost-changes
Jan 8, 2021
Merged

Fix/migration cleanup lost changes#417
mattwalsh-unity merged 4 commits intodevelopfrom
fix/migration-cleanup-lost-changes

Conversation

@LukeStampfli
Copy link
Copy Markdown
Contributor

This PR adds features and bug fixes which were removed by accident when merging our internal changes into the new develop branch.
Albin already approved this PR in our internal repo. @mattwalsh-unity since you made the commits which migrated those, could you confirm that these (especially the soft sync object part) weren't removed on purpose?

Comment thread com.unity.multiplayer.mlapi/Runtime/Serialization/BitWriter.cs
objectsToDestroy.Add(pair.Value);
}

for (int i = 0; i < objectsToDestroy.Count; i++)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth a comment to say why we batch the objects before then destroying them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@TwoTenPvP maybe you recall?

}

// Clean up the diffed scene objects. I.E scene objects that have been destroyed
if (SpawnManager.pendingSoftSyncObjects.Count > 0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this be hoisted into a function inside SpawnManager? There are other functions like this e.g. DestroySceneObjects

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, just pushed a commit to refactor this a bit

Copy link
Copy Markdown
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.

Some organization comments

@mattwalsh-unity
Copy link
Copy Markdown
Contributor

Confirmed these commits were not removed on purpose, but on looking at them seems like they could be organized a bit first

@mattwalsh-unity mattwalsh-unity merged commit 7881f75 into develop Jan 8, 2021
@mattwalsh-unity mattwalsh-unity deleted the fix/migration-cleanup-lost-changes branch January 8, 2021 21:04
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