From fcbec8c202f52aabd329dca74270d3bae58e4ec8 Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Mon, 21 Jun 2021 15:53:33 -0400 Subject: [PATCH 1/8] Fixed removal of player's prefab on disconnect --- .../Source/Components/NetBindComponent.cpp | 6 +++++ .../ServerToClientConnectionData.cpp | 3 +++ .../EntityReplicationManager.h | 4 +++- .../NetworkEntity/NetworkEntityManager.cpp | 22 +++++-------------- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp b/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp index 352bc92d1c..b81af9232e 100644 --- a/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp +++ b/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp @@ -623,6 +623,12 @@ namespace Multiplayer void NetBindComponent::HandleMarkedDirty() { + if (m_needsToBeStopped) + { + // Entity is about to deleted, it's not safe to proceed + return; + } + m_dirtiedEvent.Signal(); if (NetworkRoleHasController(GetNetEntityRole())) { diff --git a/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp b/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp index d2440d28f4..45ac9cd850 100644 --- a/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp +++ b/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp @@ -11,6 +11,7 @@ */ #include +#include namespace Multiplayer { @@ -44,6 +45,8 @@ namespace Multiplayer ServerToClientConnectionData::~ServerToClientConnectionData() { + AZ::Interface::Get()->GetNetworkEntityManager()->MarkForRemoval(m_controlledEntity); + m_entityReplicationManager.Clear(false); m_controlledEntityRemovedHandler.Disconnect(); } diff --git a/Gems/Multiplayer/Code/Source/NetworkEntity/EntityReplication/EntityReplicationManager.h b/Gems/Multiplayer/Code/Source/NetworkEntity/EntityReplication/EntityReplicationManager.h index 6172f30e8a..0db027d5f9 100644 --- a/Gems/Multiplayer/Code/Source/NetworkEntity/EntityReplication/EntityReplicationManager.h +++ b/Gems/Multiplayer/Code/Source/NetworkEntity/EntityReplication/EntityReplicationManager.h @@ -39,7 +39,9 @@ namespace Multiplayer { class IEntityDomain; class EntityReplicator; - + + //! @class EntityReplicationManager + //! @brief Handles replication of relevant entities for one connection. class EntityReplicationManager final { public: diff --git a/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityManager.cpp b/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityManager.cpp index 3405abdc57..190dc20db1 100644 --- a/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityManager.cpp +++ b/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityManager.cpp @@ -282,15 +282,6 @@ namespace Multiplayer { //RewindableObjectState::ClearRewoundEntities(); - // Keystone has refactored these API's, rewrite required - //AZ::SliceComponent* rootSlice = nullptr; - //{ - // AzFramework::EntityContextId gameContextId = AzFramework::EntityContextId::CreateNull(); - // AzFramework::GameEntityContextRequestBus::BroadcastResult(gameContextId, &AzFramework::GameEntityContextRequests::GetGameEntityContextId); - // AzFramework::EntityContextRequestBus::BroadcastResult(rootSlice, &AzFramework::EntityContextRequests::GetRootSlice); - // AZ_Assert(rootSlice != nullptr, "Root slice returned was NULL"); - //} - AZStd::vector removeList; removeList.swap(m_removeList); for (NetEntityId entityId : removeList) @@ -304,13 +295,12 @@ namespace Multiplayer AZ_Assert(netBindComponent != nullptr, "NetBindComponent not found on networked entity"); netBindComponent->StopEntity(); - // Delete Entity, method depends on how it was loaded - // Try slice removal first, then force delete - //AZ::Entity* rawEntity = removeEntity.GetEntity(); - //if (!rootSlice->RemoveEntity(rawEntity)) - //{ - // delete rawEntity; - //} + // At the moment, we spawn one entity at a time and avoid Prefab API calls and never get a spawn ticket, + // so this is the right way for now. Once we support prefabs we can use AzFramework::SpawnableEntitiesContainer + // Additionally, prefabs spawning is async! Whereas we currently create entities immediately, see: + // @NetworkEntityManager::CreateEntitiesImmediate + AzFramework::GameEntityContextRequestBus::Broadcast( + &AzFramework::GameEntityContextRequestBus::Events::DestroyGameEntity, netBindComponent->GetEntityId()); } m_networkEntityTracker.erase(entityId); From fd021f065a4f160ad022b2fb6d88b4c977fd8b7b Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Tue, 22 Jun 2021 10:17:28 -0400 Subject: [PATCH 2/8] Fixes to codegen to avoid nullptr access during disconnects --- .../Code/Source/AutoGen/AutoComponent_Source.jinja | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Gems/Multiplayer/Code/Source/AutoGen/AutoComponent_Source.jinja b/Gems/Multiplayer/Code/Source/AutoGen/AutoComponent_Source.jinja index 92ad524711..56fa769517 100644 --- a/Gems/Multiplayer/Code/Source/AutoGen/AutoComponent_Source.jinja +++ b/Gems/Multiplayer/Code/Source/AutoGen/AutoComponent_Source.jinja @@ -1673,12 +1673,18 @@ namespace {{ Component.attrib['Namespace'] }} void {{ ComponentBaseName }}::ActivateController(Multiplayer::EntityIsMigrating entityIsMigrating) { - m_controller.get()->Activate(entityIsMigrating); + if (m_controller) + { + m_controller->Activate(entityIsMigrating); + } } void {{ ComponentBaseName }}::DeactivateController(Multiplayer::EntityIsMigrating entityIsMigrating) { - m_controller.get()->Deactivate(entityIsMigrating); + if (m_controller) + { + m_controller->Deactivate(entityIsMigrating); + } } void {{ ComponentBaseName }}::NetworkAttach(Multiplayer::NetBindComponent* netBindComponent, Multiplayer::ReplicationRecord& currentEntityRecord, Multiplayer::ReplicationRecord& predictableEntityRecord) From 0f05791bd8228eaca24dfb10edfbd05a165bbaa1 Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Mon, 21 Jun 2021 20:28:43 -0400 Subject: [PATCH 3/8] Corrects ReplicationSet to be ordered so that logic in EntityReplicationManager::UpdateWindow is correct --- .../Multiplayer/ReplicationWindows/IReplicationWindow.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gems/Multiplayer/Code/Include/Multiplayer/ReplicationWindows/IReplicationWindow.h b/Gems/Multiplayer/Code/Include/Multiplayer/ReplicationWindows/IReplicationWindow.h index 5d90fc286b..8e51c30b2c 100644 --- a/Gems/Multiplayer/Code/Include/Multiplayer/ReplicationWindows/IReplicationWindow.h +++ b/Gems/Multiplayer/Code/Include/Multiplayer/ReplicationWindows/IReplicationWindow.h @@ -14,7 +14,7 @@ #include #include -#include +#include namespace Multiplayer { @@ -24,7 +24,7 @@ namespace Multiplayer NetEntityRole m_netEntityRole = NetEntityRole::InvalidRole; float m_priority = 0.0f; }; - using ReplicationSet = AZStd::unordered_map; + using ReplicationSet = AZStd::map; class IReplicationWindow { From 35364c94038cdbfec2de5a02bc04c77bfb415c4b Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Tue, 22 Jun 2021 23:13:23 -0400 Subject: [PATCH 4/8] Fixing a nullptr crash in NetworkTime --- Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp b/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp index f94a8c59d0..9a8a2c1bc1 100644 --- a/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp +++ b/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp @@ -128,9 +128,11 @@ namespace Multiplayer for (NetworkEntityHandle entityHandle : m_rewoundEntities) { - NetBindComponent* netBindComponent = entityHandle.GetNetBindComponent(); + if (NetBindComponent* netBindComponent = entityHandle.GetNetBindComponent()) + { netBindComponent->NotifySyncRewindState(); } + } m_rewoundEntities.clear(); } } From e5aa56d565d19210ec40d183b965011c5fcdf65e Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Wed, 23 Jun 2021 09:38:43 -0400 Subject: [PATCH 5/8] Added a runtime switch to turn on/off removal of player spawnable on disconnect --- .../Source/ConnectionData/ServerToClientConnectionData.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp b/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp index 45ac9cd850..ca6de98911 100644 --- a/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp +++ b/Gems/Multiplayer/Code/Source/ConnectionData/ServerToClientConnectionData.cpp @@ -19,6 +19,7 @@ namespace Multiplayer AZ_CVAR(uint32_t, sv_ClientMaxRemoteEntitiesPendingCreationCount, AZStd::numeric_limits::max(), nullptr, AZ::ConsoleFunctorFlags::DontReplicate, "Maximum number of entities that we have sent to the client, but have not had a confirmation back from the client"); AZ_CVAR(uint32_t, sv_ClientMaxRemoteEntitiesPendingCreationCountPostInit, AZStd::numeric_limits::max(), nullptr, AZ::ConsoleFunctorFlags::DontReplicate, "Maximum number of entities that we will send to clients after gameplay has begun"); AZ_CVAR(AZ::TimeMs, sv_ClientEntityReplicatorPendingRemovalTimeMs, AZ::TimeMs{ 10000 }, nullptr, AZ::ConsoleFunctorFlags::DontReplicate, "How long should wait prior to removing an entity for the client through a change in the replication window, entity deletes are still immediate"); + AZ_CVAR(bool, sv_removeDefaultPlayerSpawnableOnDisconnect, true, nullptr, AZ::ConsoleFunctorFlags::DontReplicate, "Whether to remove player's default spawnable when a player disconnects"); ServerToClientConnectionData::ServerToClientConnectionData ( @@ -45,7 +46,10 @@ namespace Multiplayer ServerToClientConnectionData::~ServerToClientConnectionData() { - AZ::Interface::Get()->GetNetworkEntityManager()->MarkForRemoval(m_controlledEntity); + if (sv_removeDefaultPlayerSpawnableOnDisconnect) + { + AZ::Interface::Get()->GetNetworkEntityManager()->MarkForRemoval(m_controlledEntity); + } m_entityReplicationManager.Clear(false); m_controlledEntityRemovedHandler.Disconnect(); From b9dec79743c0076ece27998c1330aa63c53751c4 Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Wed, 23 Jun 2021 11:04:25 -0400 Subject: [PATCH 6/8] Clean up --- .../Multiplayer/Code/Source/Components/NetBindComponent.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp b/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp index b81af9232e..352bc92d1c 100644 --- a/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp +++ b/Gems/Multiplayer/Code/Source/Components/NetBindComponent.cpp @@ -623,12 +623,6 @@ namespace Multiplayer void NetBindComponent::HandleMarkedDirty() { - if (m_needsToBeStopped) - { - // Entity is about to deleted, it's not safe to proceed - return; - } - m_dirtiedEvent.Signal(); if (NetworkRoleHasController(GetNetEntityRole())) { From 93a34033a98f957805d7220c6f9c294695cc6f94 Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Wed, 23 Jun 2021 13:21:54 -0400 Subject: [PATCH 7/8] Correcting tabs --- Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp b/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp index 9a8a2c1bc1..5189ad9544 100644 --- a/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp +++ b/Gems/Multiplayer/Code/Source/NetworkTime/NetworkTime.cpp @@ -130,8 +130,8 @@ namespace Multiplayer { if (NetBindComponent* netBindComponent = entityHandle.GetNetBindComponent()) { - netBindComponent->NotifySyncRewindState(); - } + netBindComponent->NotifySyncRewindState(); + } } m_rewoundEntities.clear(); } From 5a061e409d799e96118f8d19fac047531254d645 Mon Sep 17 00:00:00 2001 From: AMZN-Olex <5432499+AMZN-Olex@users.noreply.github.com> Date: Wed, 23 Jun 2021 18:00:13 -0400 Subject: [PATCH 8/8] Fixed EntityDelete messages --- .../Code/Source/NetworkEntity/NetworkEntityUpdateMessage.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityUpdateMessage.cpp b/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityUpdateMessage.cpp index 3fe5a497cb..a9db87ec2c 100644 --- a/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityUpdateMessage.cpp +++ b/Gems/Multiplayer/Code/Source/NetworkEntity/NetworkEntityUpdateMessage.cpp @@ -64,10 +64,11 @@ namespace Multiplayer NetworkEntityUpdateMessage::NetworkEntityUpdateMessage(NetEntityId entityId, bool wasMigrated, bool takeOwnership) : m_entityId(entityId) + , m_isDelete(true) , m_wasMigrated(wasMigrated) , m_takeOwnership(takeOwnership) { - ; + // this is a delete entity message c-tor } NetworkEntityUpdateMessage& NetworkEntityUpdateMessage::operator =(NetworkEntityUpdateMessage&& rhs)