Feature/bifrost/playercontroller rework (#1271)

* Initial change to route PlayerController spawning on non-auth server through the client path, in order to get it calling BeginPlay with the correct authority.  This change likely needs more work, but committing/pushing now to share it.

* Cleaning up the PlayerController change a bit

* Fix problem with PlayerController not replicating unless the server that spawned it has authority over it

* In tandem with the incoming engine branch change to disable creation of the PlayerState on non-auth servers, we no longer need to cleanup the player state here

* Name out parameter correctly

* Update changelog

* Report failure to create the SpatialNetConnection

* Now it even compiles

* Fix bitwise operator

* Update SpatialGDK/Source/SpatialGDK/Private/EngineClasses/SpatialNetDriver.cpp

Co-Authored-By: Giray Ozil <giray@improbable.io>

* Addressing some PR comments

* Addressing PR comments

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Bug fixes:
 - Fixed an issue that could cause multiple Channels to be created for an Actor.
+- PlayerControllers on non-auth servers now have BeginPlay called with correct authority.
 
 ## [`0.6.0`] - 2019-07-31
 

SpatialGDK/Source/SpatialGDK/Private/EngineClasses/SpatialNetDriver.cpp

@@ -1408,12 +1408,13 @@ USpatialNetConnection * USpatialNetDriver::GetSpatialOSNetConnection() const
 	}
 }
 
-USpatialNetConnection* USpatialNetDriver::AcceptNewPlayer(const FURL& InUrl, FUniqueNetIdRepl UniqueId, FName OnlinePlatformName, bool bExistingPlayer)
+bool USpatialNetDriver::CreateSpatialNetConnection(const FURL& InUrl, const FUniqueNetIdRepl& UniqueId, const FName& OnlinePlatformName, USpatialNetConnection** OutConn)
 {
-	bool bOk = true;
+	check(*OutConn == nullptr);
+	*OutConn = NewObject<USpatialNetConnection>(GetTransientPackage(), NetConnectionClass);
+	check(*OutConn != nullptr);
 
-	USpatialNetConnection* SpatialConnection = NewObject<USpatialNetConnection>(GetTransientPackage(), NetConnectionClass);
-	check(SpatialConnection);
+	USpatialNetConnection* SpatialConnection = *OutConn;
 
 	// We create a "dummy" connection that corresponds to this player. This connection won't transmit any data.
 	// We may not need to keep it in the future, but for now it looks like path of least resistance is to have one UPlayer (UConnection) per player.
@@ -1447,71 +1448,77 @@ USpatialNetConnection* USpatialNetDriver::AcceptNewPlayer(const FURL& InUrl, FUn
 
 	FString ErrorMsg;
 	AGameModeBase* GameMode = GetWorld()->GetAuthGameMode();
-	if (GameMode)
-	{
-		GameMode->PreLogin(Tmp, SpatialConnection->LowLevelGetRemoteAddress(), SpatialConnection->PlayerId, ErrorMsg);
-	}
+	check(GameMode);
+
+	GameMode->PreLogin(Tmp, SpatialConnection->LowLevelGetRemoteAddress(), SpatialConnection->PlayerId, ErrorMsg);
 
 	if (!ErrorMsg.IsEmpty())
 	{
 		UE_LOG(LogSpatialOSNetDriver, Error, TEXT("PreLogin failure: %s"), *ErrorMsg);
-		bOk = false;
+		// TODO: Destroy connection. UNR-584
+		return false;
 	}
 
-	if (bOk)
-	{
-		FString LevelName = GetWorld()->GetCurrentLevel()->GetOutermost()->GetName();
-		SpatialConnection->SetClientWorldPackageName(GetWorld()->GetCurrentLevel()->GetOutermost()->GetFName());
+	SpatialConnection->SetClientWorldPackageName(GetWorld()->GetCurrentLevel()->GetOutermost()->GetFName());
 
-		FString GameName;
-		FString RedirectURL;
-		if (GameMode)
-		{
-			GameName = GameMode->GetClass()->GetPathName();
-			GameMode->GameWelcomePlayer(SpatialConnection, RedirectURL);
-		}
+	FString RedirectURL;
+	GameMode->GameWelcomePlayer(SpatialConnection, RedirectURL);
 
-		if (!bExistingPlayer)
-		{
-			SpatialConnection->PlayerController = World->SpawnPlayActor(SpatialConnection, ROLE_AutonomousProxy, InUrl, SpatialConnection->PlayerId, ErrorMsg);
-		}
-		else
-		{
-			// Most of this is taken from "World->SpawnPlayActor", excluding the logic to spawn a pawn which happens during
-			// GameMode->PostLogin(...).
-			APlayerController* NewPlayerController = GameMode->SpawnPlayerController(ROLE_AutonomousProxy, UrlString);
-
-			// Destroy the player state (as we'll be replacing it anyway).
-			NewPlayerController->CleanupPlayerState();
-
-			// Possess the newly-spawned player.
-			NewPlayerController->NetPlayerIndex = 0;
-			NewPlayerController->Role = ROLE_Authority;
-			NewPlayerController->SetReplicates(true);
-			NewPlayerController->SetAutonomousProxy(true);
-			NewPlayerController->SetPlayer(SpatialConnection);
-			// We explicitly don't call GameMode->PostLogin(NewPlayerController) here, to avoid the engine restarting the player.
-			// TODO: Should we call AGameSession::PostLogin? - UNR:583
-			// TODO: Should we trigger to blueprints that a player has "joined" via GameMode->K2_PostLogin(Connection)? - UNR:583
-
-			SpatialConnection->PlayerController = NewPlayerController;
-		}
+	return true;
+}
 
-		if (SpatialConnection->PlayerController == NULL)
-		{
-			// Failed to connect.
-			UE_LOG(LogSpatialOSNetDriver, Error, TEXT("Join failure: %s"), *ErrorMsg);
-			SpatialConnection->FlushNet(true);
-			bOk = false;
-		}
+void USpatialNetDriver::AcceptNewPlayer(const FURL& InUrl, const FUniqueNetIdRepl& UniqueId, const FName& OnlinePlatformName)
+{
+	USpatialNetConnection* SpatialConnection = nullptr;
+
+	if (!CreateSpatialNetConnection(InUrl, UniqueId, OnlinePlatformName, &SpatialConnection))
+	{
+		UE_LOG(LogSpatialOSNetDriver, Error, TEXT("Failed to create SpatialNetConnection!"));
+		return;
+	}
+
+	FString ErrorMsg;
+	SpatialConnection->PlayerController = GetWorld()->SpawnPlayActor(SpatialConnection, ROLE_AutonomousProxy, InUrl, SpatialConnection->PlayerId, ErrorMsg);
+
+	if (SpatialConnection->PlayerController == nullptr)
+	{
+		// Failed to connect.
+		UE_LOG(LogSpatialOSNetDriver, Error, TEXT("Join failure: %s"), *ErrorMsg);
+		SpatialConnection->FlushNet(true);
 	}
+}
+
+// This function is called for server workers who received the PC over the wire
+void USpatialNetDriver::PostSpawnPlayerController(APlayerController* PlayerController, const FString& WorkerAttribute)
+{
+	check(PlayerController != nullptr);
+	checkf(!WorkerAttribute.IsEmpty(), TEXT("A player controller entity must have an owner worker attribute."));
 
-	if (!bOk)
+	PlayerController->SetFlags(GetFlags() | RF_Transient);
+
+	FString URLString = FURL().ToString();
+	URLString += TEXT("?workerAttribute=") + WorkerAttribute;
+
+	// We create a connection here so that any code that searches for owning connection, etc on the server
+	// resolves ownership correctly
+	USpatialNetConnection* OwnershipConnection = nullptr;
+	if (!CreateSpatialNetConnection(FURL(nullptr, *URLString, TRAVEL_Absolute), FUniqueNetIdRepl(), FName(), &OwnershipConnection))
 	{
-		// TODO: Destroy connection. UNR:584
+		UE_LOG(LogSpatialOSNetDriver, Error, TEXT("Failed to create SpatialNetConnection!"));
+		return;
 	}
 
-	return bOk ? SpatialConnection : nullptr;
+	OwnershipConnection->PlayerController = PlayerController;
+
+	PlayerController->NetPlayerIndex = 0;
+	// We need to lie about our authority briefly here so that SetReplicates will succeed.
+	// In the case this is being called after receiving an actor over the wire, our authority is intended to be ROLE_SimulatedProxy.
+	// (It will get set immediately after this call in SpatialReceiver::CreateActor)
+	ENetRole OriginalRole = PlayerController->Role;
+	PlayerController->Role = ROLE_Authority;
+	PlayerController->SetReplicates(true);
+	PlayerController->Role = OriginalRole;
+	PlayerController->SetPlayer(OwnershipConnection);
 }
 
 bool USpatialNetDriver::Exec(UWorld* InWorld, const TCHAR* Cmd, FOutputDevice& Ar)

SpatialGDK/Source/SpatialGDK/Private/Interop/SpatialPlayerSpawner.cpp

@@ -55,7 +55,7 @@ void USpatialPlayerSpawner::ReceivePlayerSpawnRequest(Schema_Object* Payload, co
 			URLString += TEXT("?simulatedPlayer=1");
 		}
 
-		NetDriver->AcceptNewPlayer(FURL(nullptr, *URLString, TRAVEL_Absolute), UniqueId, OnlinePlatformName, false);
+		NetDriver->AcceptNewPlayer(FURL(nullptr, *URLString, TRAVEL_Absolute), UniqueId, OnlinePlatformName);
 	}
 
 	// Send a successful response if the player has been accepted, either from this request or one in the past.

SpatialGDK/Source/SpatialGDK/Private/Interop/SpatialReceiver.cpp

@@ -589,6 +589,10 @@ void USpatialReceiver::ReceiveActor(Worker_EntityId EntityId)
 		// Taken from PostNetInit
 		if (NetDriver->GetWorld()->HasBegunPlay() && !EntityActor->HasActorBegunPlay())
 		{
+			// Whenever we receive an actor over the wire, the expectation is that it is not in an authoritative
+			// state.  This is because it should already have had authoritative BeginPlay() called.  If we have
+			// authority here, we are calling BeginPlay() with authority on this actor a 2nd time, which is always incorrect.
+			check(!EntityActor->HasAuthority());
 			EntityActor->DispatchBeginPlay();
 		}
 
@@ -822,29 +826,18 @@ AActor* USpatialReceiver::CreateActor(UnrealMetadata* UnrealMetadataComp, SpawnD
 		return nullptr;
 	}
 
+	const bool bIsServer = NetDriver->IsServer();
+
 	// Initial Singleton Actor replication is handled with GlobalStateManager::LinkExistingSingletonActors
-	if (NetDriver->IsServer() && ActorClass->HasAnySpatialClassFlags(SPATIALCLASS_Singleton))
+	if (bIsServer && ActorClass->HasAnySpatialClassFlags(SPATIALCLASS_Singleton))
 	{
 		return FindSingletonActor(ActorClass);
 	}
 
-	// If we're checking out a player controller, spawn it via "USpatialNetDriver::AcceptNewPlayer"
-	if (NetDriver->IsServer() && ActorClass->IsChildOf(APlayerController::StaticClass()))
-	{
-		checkf(!UnrealMetadataComp->OwnerWorkerAttribute.IsEmpty(), TEXT("A player controller entity must have an owner worker attribute."));
-
-		FString URLString = FURL().ToString();
-		URLString += TEXT("?workerAttribute=") + UnrealMetadataComp->OwnerWorkerAttribute;
-
-		// TODO: Once we can checkout PlayerController and PlayerState atomically, we can grab the UniqueId and online subsystem type from PlayerState. UNR-933
-		UNetConnection* Connection = NetDriver->AcceptNewPlayer(FURL(nullptr, *URLString, TRAVEL_Absolute), FUniqueNetIdRepl(), FName(), true);
-		check(Connection);
-
-		return Connection->PlayerController;
-	}
-
 	UE_LOG(LogSpatialReceiver, Verbose, TEXT("Spawning a %s whilst checking out an entity."), *ActorClass->GetFullName());
 
+	const bool bCreatingPlayerController = ActorClass->IsChildOf(APlayerController::StaticClass());
+
 	FActorSpawnParameters SpawnInfo;
 	SpawnInfo.SpawnCollisionHandlingOverride = ESpawnActorCollisionHandlingMethod::AlwaysSpawn;
 	SpawnInfo.bRemoteOwned = true;
@@ -855,6 +848,11 @@ AActor* USpatialReceiver::CreateActor(UnrealMetadata* UnrealMetadataComp, SpawnD
 	AActor* NewActor = NetDriver->GetWorld()->SpawnActorAbsolute(ActorClass, FTransform(SpawnDataComp->Rotation, SpawnLocation), SpawnInfo);
 	check(NewActor);
 
+	if (bIsServer && bCreatingPlayerController)
+	{
+		NetDriver->PostSpawnPlayerController(Cast<APlayerController>(NewActor), UnrealMetadataComp->OwnerWorkerAttribute);
+	}
+
 	// Imitate the behavior in UPackageMapClient::SerializeNewActor.
 	const float Epsilon = 0.001f;
 	if (!SpawnDataComp->Velocity.Equals(FVector::ZeroVector, Epsilon))

SpatialGDK/Source/SpatialGDK/Public/EngineClasses/SpatialNetDriver.h

@@ -90,7 +90,8 @@ class SPATIALGDK_API USpatialNetDriver : public UIpNetDriver
 	void OnAcceptingPlayersChanged(bool bAcceptingPlayers);
 
 	// Used by USpatialSpawner (when new players join the game) and USpatialInteropPipelineBlock (when player controllers are migrated).
-	USpatialNetConnection* AcceptNewPlayer(const FURL& InUrl, FUniqueNetIdRepl UniqueId, FName OnlinePlatformName, bool bExistingPlayer);
+	void AcceptNewPlayer(const FURL& InUrl, const FUniqueNetIdRepl& UniqueId, const FName& OnlinePlatformName);
+	void PostSpawnPlayerController(APlayerController* PlayerController, const FString& WorkerAttribute);
 
 	void AddActorChannel(Worker_EntityId EntityId, USpatialActorChannel* Channel);
 	void RemoveActorChannel(Worker_EntityId EntityId);
@@ -217,6 +218,7 @@ class SPATIALGDK_API USpatialNetDriver : public UIpNetDriver
 #endif
 
 	void ProcessRPC(AActor* Actor, UObject* SubObject, UFunction* Function, void* Parameters);
+	bool CreateSpatialNetConnection(const FURL& InUrl, const FUniqueNetIdRepl& UniqueId, const FName& OnlinePlatformName, USpatialNetConnection** OutConn);
 
 	friend USpatialNetConnection;
 	friend USpatialWorkerConnection;