UNR-925: exposing connection events #637

padhansell · 2019-02-05T10:28:07Z

Description

This PR exposes connection events to the user through the USpatialNetDriver (OnConnected, OnDisconnected, OnConnectionFailure). It also makes the USpatialWorkerConnection private in order to better encapsulate it and prevent people from relying on its availability - after discussing with @joshuahuburn and @improbable-valentyn, we'd like to move ownership of this in to the NetDriver down the line anyway.
Also note that the existing delegates have been converted to events so that they cannot be invoked outside of the NetDriver

Tests

This was tested manually in the editor by going through these cases:

starting the game
stopping the game
killing the runtime while the game is running
attempting to connect when the runtime is not up

Documentation

In-code comments in USpatialNetDriver

Release note

Feature: Exposed SpatialOS connection events in USpatialNetDriver

Vatyx · 2019-02-05T12:03:14Z

SpatialGDK/Source/SpatialGDK/Private/EngineClasses/SpatialGameInstance.cpp

@@ -67,7 +67,8 @@ bool USpatialGameInstance::HasSpatialNetDriver() const

 void USpatialGameInstance::CreateNewSpatialWorkerConnection()
 {
-	SpatialConnection = NewObject<USpatialWorkerConnection>();
+	SpatialConnection = NewObject<USpatialWorkerConnection>(this);


Do we want to make the GameInstance the outer?

I always explicitly add outers to highlight who is supposed to be owning the object. Seems like that's not the case elsewhere in the codebase though so happy to change

Vatyx · 2019-02-05T12:04:27Z

SpatialGDK/Source/SpatialGDK/Private/EngineClasses/SpatialGameInstance.cpp

+
+void USpatialGameInstance::Shutdown()
+{
+	auto World = GetWorld();


Suggested change

auto World = GetWorld();

UWorld* World = GetWorld();

Vatyx · 2019-02-05T12:04:39Z

SpatialGDK/Source/SpatialGDK/Private/EngineClasses/SpatialGameInstance.cpp

+void USpatialGameInstance::Shutdown()
+{
+	auto World = GetWorld();
+	if (World && SpatialConnection->IsConnected())


Suggested change

if (World && SpatialConnection->IsConnected())

if (World != nullptr && SpatialConnection->IsConnected())

m-samiec · 2019-02-05T12:59:09Z

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp

@@ -2,6 +2,7 @@

 #include "Interop/Connection/SpatialWorkerConnection.h"

+#include "EngineClasses//SpatialNetDriver.h"


Suggested change

#include "EngineClasses//SpatialNetDriver.h"

#include "EngineClasses/SpatialNetDriver.h"

padhansell · 2019-02-06T12:22:52Z

@Vatyx after talking with @m-samiec, i've decided to leave this logic as is for now and have raised https://improbableio.atlassian.net/browse/UNR-962 for the next bit of work

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp

padhansell · 2019-02-07T11:47:02Z

@Vatyx @m-samiec can you take another look at this plz

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp

joshuahuburn · 2019-02-11T11:07:46Z

SpatialGDK/Source/SpatialGDK/Private/EngineClasses/SpatialGameInstance.cpp

@@ -67,7 +67,8 @@ bool USpatialGameInstance::HasSpatialNetDriver() const

 void USpatialGameInstance::CreateNewSpatialWorkerConnection()
 {
-	SpatialConnection = NewObject<USpatialWorkerConnection>();
+	SpatialConnection = NewObject<USpatialWorkerConnection>(this);
+	SpatialConnection->Init(GetWorld());


Isn't this going to be null at the point of initilisation for StartPlayInEditorGameInstance and StartGameInstance?

You mean World will be?

From what I've seen, we always have a World at this point

Vatyx · 2019-02-11T11:23:11Z

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp

+	bIsConnected = false;
+
+	Worker_OpList* OpList = Worker_Connection_GetOpList(WorkerConnection, 0);
+	for (int i = 0; i < static_cast<int>(OpList->op_count); i++)


The other place we iterate over the OpList, we use a size_t as the type for i. Could we do the same here just to be consistent?

Vatyx · 2019-02-11T11:23:53Z

SpatialGDK/Source/SpatialGDK/Public/EngineClasses/SpatialGameInstance.h

+	// TODO: Move SpatialConnection ownership to NetDriver
+	friend class USpatialNetDriver;
+	UPROPERTY()
+	USpatialWorkerConnection* SpatialConnection;


This is going to break server travel in the deployments right? I'm not sure we want to do this at the moment.

Yeah if ownership of SpatialConnection was moved to the NetDriver that would break ServerTravel I think - how to support that would have to be part of a bigger discussion. For now, this shouldn't break anything

DW-Sebastien · 2019-02-11T11:46:43Z

Hey @padhansell, I wanted to use this, but I hit an issue (might be related to the comment from @joshuahuburn above):

I don't see how to access these new events in the following way;

void UCorvusGameInstance::OnPostStart()
{
	USpatialNetDriver* spatialNetDriver = Cast<USpatialNetDriver>(GetWorld()->GetNetDriver());
	if (spatialNetDriver)
	{
		spatialNetDriver->OnConnected.AddUObject(this, &UCorvusGameInstance::OnSpatialOsConnected);
	}
}

But my problem is that the NetDriver is nullptr on the client at this stage (I call OnPostStart() at the end of StartPlayInEditorGameInstance and StartGameInstance.

On the server the NetDriver is set during StartGameInstance, and on the client I can wait until LoadCompleted where it is set... but all this is a bit cumbersome.

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp

Co-Authored-By: padhansell <[email protected]>

…github.com/spatialos/UnrealGDK into feature/UNR-925-exposing-connection-events

joshuahuburn

Looks good, could you also please add a release note

padhansell · 2019-02-11T14:39:09Z

Hey @padhansell, I wanted to use this, but I hit an issue (might be related to the comment from @joshuahuburn above):

I don't see how to access these new events in the following way;
void UCorvusGameInstance::OnPostStart()
{
	USpatialNetDriver* spatialNetDriver = Cast<USpatialNetDriver>(GetWorld()->GetNetDriver());
	if (spatialNetDriver)
	{
		spatialNetDriver->OnConnected.AddUObject(this, &UCorvusGameInstance::OnSpatialOsConnected);
	}
}
But my problem is that the NetDriver is nullptr on the client at this stage (I call OnPostStart() at the end of StartPlayInEditorGameInstance and StartGameInstance.

On the server the NetDriver is set during StartGameInstance, and on the client I can wait until LoadCompleted where it is set... but all this is a bit cumbersome.

Thanks for flagging this up @DW-Sebastien. You're right that it's not an ideal flow - once connection handling has been refactored, we'll be able to expose this same functionality through core engine delegates/virtual calls and avoid this altogether

SpatialGDK/Source/SpatialGDK/Public/Interop/SpatialReceiver.h

Co-Authored-By: padhansell <[email protected]>

* Moving events to NetDriver * Adding disconnection callback * Tidying up * Adding comments * Adding additional check for when Spatial networking is disabled * Addressing review comments * Adding getter for NetDriver * Adding TODO comment * Adding missing declaration * Review feedback Co-Authored-By: padhansell <[email protected]> * Changing iterator type * Remove whitespace Co-Authored-By: padhansell <[email protected]> * Update SpatialReceiver.h * Including World * Changing World access * Adding GEngine check

padhansell added 3 commits February 4, 2019 10:26

Moving events to NetDriver

c788744

Adding disconnection callback

edd6050

Tidying up

a70a457

padhansell self-assigned this Feb 5, 2019

padhansell requested review from joshuahuburn and m-samiec February 5, 2019 10:28

improbable-prow-robot added the size/L Denotes a PR that changes 150-299 lines, ignoring generated files. label Feb 5, 2019

Adding comments

8406ff0

padhansell added size/S Denotes a PR that changes 15-39 lines, ignoring generated files. and removed size/L Denotes a PR that changes 150-299 lines, ignoring generated files. labels Feb 5, 2019

Vatyx reviewed Feb 5, 2019

View reviewed changes

improbable-prow-robot added size/L Denotes a PR that changes 150-299 lines, ignoring generated files. and removed size/S Denotes a PR that changes 15-39 lines, ignoring generated files. labels Feb 5, 2019

m-samiec reviewed Feb 5, 2019

View reviewed changes

padhansell added 2 commits February 6, 2019 12:10

Adding additional check for when Spatial networking is disabled

139770e

Addressing review comments

4a9a503

m-samiec reviewed Feb 6, 2019

View reviewed changes

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp Outdated Show resolved Hide resolved

padhansell added 2 commits February 6, 2019 15:01

Adding getter for NetDriver

1147190

Adding TODO comment

237d2df

DW-Sebastien reviewed Feb 8, 2019

View reviewed changes

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp Show resolved Hide resolved

Adding missing declaration

d8808aa

joshuahuburn reviewed Feb 11, 2019

View reviewed changes

Vatyx reviewed Feb 11, 2019

View reviewed changes

m-samiec reviewed Feb 11, 2019

View reviewed changes

SpatialGDK/Source/SpatialGDK/Private/Interop/Connection/SpatialWorkerConnection.cpp Outdated Show resolved Hide resolved

m-samiec approved these changes Feb 11, 2019

View reviewed changes

m-samiec and others added 3 commits February 11, 2019 14:02

Review feedback

412ddec

Co-Authored-By: padhansell <[email protected]>

Changing iterator type

dd84773

Merge branch 'feature/UNR-925-exposing-connection-events' of https://…

45891c4

…github.com/spatialos/UnrealGDK into feature/UNR-925-exposing-connection-events

joshuahuburn approved these changes Feb 11, 2019

View reviewed changes

improbable-valy reviewed Feb 11, 2019

View reviewed changes

SpatialGDK/Source/SpatialGDK/Public/Interop/SpatialReceiver.h Outdated Show resolved Hide resolved

improbable-valy approved these changes Feb 11, 2019

View reviewed changes

improbable-valy and others added 7 commits February 11, 2019 15:52

Remove whitespace

4b28ade

Co-Authored-By: padhansell <[email protected]>

Update SpatialReceiver.h

4d31867

Merge branch 'master' into feature/UNR-925-exposing-connection-events

4ffc7e1

Including World

33cbe3e

Changing World access

2ed0970

Adding GEngine check

466af30

Merge branch 'master' into feature/UNR-925-exposing-connection-events

c1058a8

m-samiec merged commit 233cf40 into master Feb 12, 2019

m-samiec deleted the feature/UNR-925-exposing-connection-events branch February 12, 2019 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UNR-925: exposing connection events #637

UNR-925: exposing connection events #637

padhansell commented Feb 5, 2019 •

edited

Loading

Vatyx Feb 5, 2019

padhansell Feb 5, 2019

Vatyx Feb 5, 2019

Vatyx Feb 5, 2019

m-samiec Feb 5, 2019

padhansell commented Feb 6, 2019

padhansell commented Feb 7, 2019

joshuahuburn Feb 11, 2019

padhansell Feb 11, 2019

padhansell Feb 11, 2019

Vatyx Feb 11, 2019

Vatyx Feb 11, 2019

padhansell Feb 11, 2019

DW-Sebastien commented Feb 11, 2019 •

edited

Loading

joshuahuburn left a comment

padhansell commented Feb 11, 2019

	if (World && SpatialConnection->IsConnected())
	if (World != nullptr && SpatialConnection->IsConnected())

		@@ -2,6 +2,7 @@

		#include "Interop/Connection/SpatialWorkerConnection.h"

		#include "EngineClasses//SpatialNetDriver.h"

	#include "EngineClasses//SpatialNetDriver.h"
	#include "EngineClasses/SpatialNetDriver.h"

UNR-925: exposing connection events #637

UNR-925: exposing connection events #637

Conversation

padhansell commented Feb 5, 2019 • edited Loading

Description

Tests

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padhansell commented Feb 6, 2019

padhansell commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DW-Sebastien commented Feb 11, 2019 • edited Loading

joshuahuburn left a comment

Choose a reason for hiding this comment

padhansell commented Feb 11, 2019

padhansell commented Feb 5, 2019 •

edited

Loading

DW-Sebastien commented Feb 11, 2019 •

edited

Loading