move Netspeak Unreal SDK into Agones Unreal SDK #1739

domgreen · 2020-08-05T11:49:11Z

What type of PR is this?

/kind breaking

What this PR does / Why we need it:

This PR updates the Unreal SDK to the one at https://github.com/netspeakgames/UnrealAgonesSDK

has support for C++ and Blueprints
more typical of a Unreal Plugin
has more coverage of API (see README.md)
- missing watch
has Alpha APIs for PlayerConnect etc.
has some more documentation of how to get up and running with Unreal

Which issue(s) this PR fixes:
Closes #1683

Special notes for your reviewer:
👋 hi, thanks for the great project.

agones-bot · 2020-08-05T12:03:02Z

Build Failed 😱

Build Id: 1e4ca539-e5b7-46ff-9963-80de5a4be5b9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

domgreen · 2020-08-05T12:09:41Z

ERROR
ERROR: build step 27 "gcr.io/cloud-builders/gsutil" failed: step exited with non-zero status: 1
Step #23 - "sdk-conformance": includes/sdk.mk:145: recipe for target 'run-sdk-conformance-no-build' failed
Step #23 - "sdk-conformance": includes/sdk.mk:88: recipe for target 'run-sdk-command' failed
Step #23 - "sdk-conformance": make[2]: *** [run-sdk-conformance-no-build] Terminated
Step #23 - "sdk-conformance": make[2]: *** [run-sdk-command] Terminated
Step #23 - "sdk-conformance": make[1]: *** [run-sdk-conformance-test] Terminated
Step #23 - "sdk-conformance": make[1]: *** [run-sdk-conformance-test] Terminated
Step #23 - "sdk-conformance": includes/sdk.mk:152: recipe for target 'run-sdk-conformance-test' failed
Step #23 - "sdk-conformance": includes/sdk.mk:152: recipe for target 'run-sdk-conformance-test' failed
Step #23 - "sdk-conformance": make: *** [run-sdk-conformance-test-rest] Terminated
Step #23 - "sdk-conformance": make: *** [run-sdk-conformance-test-rust] Terminated
Step #23 - "sdk-conformance": includes/sdk.mk:174: recipe for target 'run-sdk-conformance-test-rest' failed
Step #23 - "sdk-conformance": includes/sdk.mk:168: recipe for target 'run-sdk-conformance-test-rust' failed

Seems like failing conformance tests.

markmandel

Thanks for sending this through, excited to see this moving forward.

markmandel · 2020-08-06T20:22:46Z

sdks/unreal/Agones/Source/Agones/Agones.Build.cs

-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
 using UnrealBuildTool;


All files will need to have the Google LLC Apache licence at the top please, before we can accept them.

markmandel · 2020-08-06T20:23:02Z

sdks/unreal/Agones/Source/Agones/Classes/Classes.h

@@ -0,0 +1,361 @@
+#pragma once


Apache licence please

markmandel · 2020-08-06T20:24:17Z

sdks/unreal/Agones/Source/Agones/Private/Agones.cpp

-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
+// Copyright Epic Games, Inc. All Rights Reserved.


Unfortunately I don't think we can't accept this with the current copyright 😞 not sure if that is going to be an issue?

This was due to taking the short cut of generating via UE4 can rewrite exactly the same by hand.

markmandel · 2020-08-06T20:25:38Z

sdks/unreal/Agones/README.md


-## Developer Information
+## Getting Started


These user docs should really live in: https://agones.dev/site/docs/guides/client-sdks/unreal/

There is a guide on how to document upcoming features so that they can be hidden until the next release here:
https://agones.dev/site/docs/contribute/#documentation-for-upcoming-features

markmandel · 2020-08-06T20:27:49Z

If someone from @drichardson @dotcom @WVerlaek @robbieheywood (or other Unreal users) can give this a review, that would also be awesome, just to make sure there are no concerns or issues 👍

agones-bot · 2020-08-06T20:48:53Z

Build Succeeded 👏

Build Id: 97fe111c-aea0-4a4d-8af4-7addb50ebe3b

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.8.0-87d68f3
image: gcr.io/agones-images/agones-sdk:1.8.0-87d68f3
image: gcr.io/agones-images/agones-ping:1.8.0-87d68f3
Linux C++ SDK (build): agonessdk-1.8.0-87d68f3-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.8.0-87d68f3.zip

A preview of the website (the last 30 builds are retained):

https://87d68f3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1739/head:pr_1739 && git checkout pr_1739
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-87d68f3

dotcom · 2020-08-06T21:20:35Z

I tried this SDK (with UE version=4.24.3-0+++Release-4.24 source build) and it was very easy to use. niceee.

By the way, about WatchGameserver,
this seems to me that in Unreal, we need to use a FSocket because of getting the stream's progress.

Fortunately, IHttpRequest has a Delegate called FHttpRequestProgressDelegate, which allows us to monitor whether or not the Stream has made progress. Of course, what the above is doing is simply calling CheckProgressDelegate(); every time with Tick() to see if there is a change in the Bytes received.
However, this seems to cause a memory leak in theory. This is because it doesn't discard any information about past Gameserver changes. (you can look FCurlHttpRequest::ReceiveResponseBodyCallback)

A good reason to use FSocket is to be able to discard the past data. (Of course, assuming we use libcurl, It would be nice if we could add a method to FCurlHttpRequest to clean the buffer. But I can't think of how to do it well.)

That's just my opinion. Please point out to me if I'm wrong.

markmandel · 2020-08-06T21:43:58Z

@dotcom thanks for taking it for a spin!!

WVerlaek

Nice work @domgreen, got a few comments but overall looking good

WVerlaek · 2020-08-07T10:03:03Z

sdks/unreal/Agones/Source/Agones/Classes/Classes.h

+{
+	GENERATED_BODY()
+
+	UPROPERTY()


Should this also be UPROPERTY(BlueprintReadOnly)?

Yep, just did a check and I only set it in C++ it is passed in as a argument in the func call, will check the rest of the file as well.

WVerlaek · 2020-08-07T10:11:14Z

sdks/unreal/Agones/Source/Agones/Private/AgonesComponent.cpp

+	}
+
+	// TODO(dom) - look at JSON encoding in UE4.
+	Json = Json.Replace(TEXT("playerId"), TEXT("playerID"));


Any way this can be done differently?

This frustrated me for days ... I would love to know if there is a way to fix this, seems that JSON encoding isnt great in UE4. Do you have any suggestions?

Feel your pain.. Not sure either, does renaming the PlayerId field of FAgonesPlayer to PlayerID do anything?

nope, I did try that ... could have just been holding it wrong but have had a few ppl look at it internally and nothing obvious came up. If you could see if the i8e team knows a work around that would be fantastic.

WVerlaek · 2020-08-07T10:12:14Z

sdks/unreal/Agones/Source/Agones/Private/AgonesComponent.cpp

+	}
+
+	// TODO(dom) - look at JSON encoding in UE4.
+	Json = Json.Replace(TEXT("playerId"), TEXT("playerID"));


WVerlaek · 2020-08-07T10:18:53Z

sdks/unreal/Agones/Source/Agones/Public/AgonesComponent.h

+ * See - https://agones.dev/ for more information.
+ */
+UCLASS(ClassGroup = (Custom), meta = (BlueprintSpawnableComponent))
+class AGONES_API UAgonesComponent final : public UActorComponent


Previously Agones plugin settings could be configured in a config file (by having UCLASS(config = Game, defaultconfig) as class properties), is that still possible or do these settings need to be set at runtime now?

It is possible to do as mentioned we could add the UCLASS config section ... would require some more thinking of where we would want the config for the plugin to live. Can add it onto my back log to investigate.

I think that would be good, also to ease the migration from the old plugin to this one. It also makes sense for things like auto connect and health checks to live in a config file as they are picked up immediately once the Agones plugin is created, you'd have to be careful changing these values at runtime at the right time

have added port, rate of health check and disableConnect to the config ... this is also added to the docs

Nice, can you add defaultconfig as well (from #1352)?

mind blown, didnt even know this existed 😂

mind blown, didnt even know this existed

✨✨✨ THE POWER OF OPEN SOURCE AT WORK ✨✨✨

WVerlaek · 2020-08-07T10:24:48Z

sdks/unreal/Agones/Source/Agones/Private/AgonesComponent.cpp

+TSharedRef<IHttpRequest> UAgonesComponent::BuildAgonesRequest(const FString Path, const FHttpVerb Verb, const FString Content)
+{
+	FHttpModule* Http = &FHttpModule::Get();
+	TSharedRef<IHttpRequest> Request = Http->CreateRequest();


Any particular reason automatic request retries have been removed? That's probably fine if you have autoConnect turned on but in the case you have it disabled and the game itself calls Ready at some point it would be helpful if that request will retry until e.g. the agones sidecar is ready

We do have helper functions that do this on startup that can be used to poll the endpoint till the sidecar is healthy, it will then call ready and return the game-server as part of a delegate that you can do with what you need.

Main reasons for not using the auto request retries was I've not seen it heavily used in other plugins and now that we have the fail call back its much easier to allow the game play programmers to deal with the inability to make the call themselves via delegates, there may be edge cases where they would prefer a failed call to do something else rather than keep retrying.

Ah right instead of calling Ready you'd then use Connect to poll until ready, I think that works. Might be worth adding a small section to the docs calling this out as part of migrating from the old plugin to this one

This is more inline with what I have seen in the other Agones plugins from what I remember. Will add a section to the docs.

Ahhh, it's already in the docs :D saying that there are additional utility methods that have been added to the plugin that can be used for both health and connect 🥇

domgreen · 2020-08-07T11:08:17Z

Nice work @domgreen, got a few comments but overall looking good

Thanks for the review @WVerlaek will make some changes and push back ... miss you 😸

…o netspeak_sdk

agones-bot · 2020-08-07T12:35:56Z

Build Succeeded 👏

Build Id: 9ae8b0ef-b7bf-47ed-a671-7fbb1a4d1168

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.8.0-d4562b8
image: gcr.io/agones-images/agones-sdk:1.8.0-d4562b8
image: gcr.io/agones-images/agones-ping:1.8.0-d4562b8
Linux C++ SDK (build): agonessdk-1.8.0-d4562b8-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.8.0-d4562b8.zip

A preview of the website (the last 30 builds are retained):

https://d4562b8-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1739/head:pr_1739 && git checkout pr_1739
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-d4562b8

domgreen · 2020-08-07T12:44:30Z

@WVerlaek its not letting me re-request review from you but if you have time that would be lovely. 🅿G💣

agones-bot · 2020-08-07T12:46:56Z

Build Succeeded 👏

Build Id: 3cc09f34-7138-4840-98e6-04fee69d5944

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.8.0-aac2629
image: gcr.io/agones-images/agones-sdk:1.8.0-aac2629
image: gcr.io/agones-images/agones-ping:1.8.0-aac2629
Linux C++ SDK (build): agonessdk-1.8.0-aac2629-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.8.0-aac2629.zip

A preview of the website (the last 30 builds are retained):

https://aac2629-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1739/head:pr_1739 && git checkout pr_1739
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-aac2629

WVerlaek · 2020-08-07T16:49:53Z

Small nit to add defaultconfig as well but other than that LGTM!

markmandel

One small thing on the docs, but otherwise, also looks good to me.

Looks like we'll be able to squeeze this in before RC next Tuesday, which is awesome!

markmandel · 2020-08-07T17:18:01Z

site/content/en/docs/Guides/Client SDKs/unreal.md

@@ -18,7 +18,7 @@ Check the [Client SDK Documentation]({{< relref "_index.md" >}}) for more detail

 Download the source from the [Releases Page](https://github.com/googleforgames/agones/releases)
 or {{< ghlink href="sdks/unreal" >}}directly from GitHub{{< /ghlink >}}.
-
+{{% feature expiryVersion="1.8.0" %}}


site/content/en/docs/Guides/Client SDKs/unreal.md

agones-bot · 2020-08-07T19:09:18Z

Build Succeeded 👏

Build Id: 07240103-f8ad-46ca-a04f-7094200568be

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.8.0-d1239ba
image: gcr.io/agones-images/agones-sdk:1.8.0-d1239ba
image: gcr.io/agones-images/agones-ping:1.8.0-d1239ba
Linux C++ SDK (build): agonessdk-1.8.0-d1239ba-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.8.0-d1239ba.zip

A preview of the website (the last 30 builds are retained):

https://d1239ba-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1739/head:pr_1739 && git checkout pr_1739
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-d1239ba

markmandel · 2020-08-07T20:55:35Z

@WVerlaek if you could please confirm you got the defaultconfig thingo you were looking for, I am very happy to approve and merge this PR! 😀

WVerlaek · 2020-08-07T21:15:25Z

Yup thanks for adding, LGTM

markmandel

🤸🤸🤸🤸🤸

google-oss-robot · 2020-08-07T21:16:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: domgreen, markmandel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markmandel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* moving Netspeak Agones SDK into main repo SDK Co-authored-by: Mark Mandel <markmandel@google.com>

domgreen added 2 commits August 5, 2020 11:30

moving Netspeak Agones SDK into main repo SDK

d6d0845

netspeakgames/UnrealAgonesSDK#1

28114ec

google-oss-robot requested review from markmandel and roberthbailey August 5, 2020 11:49

google-oss-robot added the size/XXL label Aug 5, 2020

googlebot added the cla: yes label Aug 5, 2020

Merge branch 'master' into netspeak_sdk

87d68f3

markmandel reviewed Aug 6, 2020

View reviewed changes

WVerlaek reviewed Aug 7, 2020

View reviewed changes

domgreen added 3 commits August 7, 2020 13:15

review comments

62d4217

Merge branch 'netspeak_sdk' of https://github.com/domgreen/agones int…

d4562b8

…o netspeak_sdk

Merge branch 'master' into netspeak_sdk

aac2629

domgreen requested a review from markmandel August 7, 2020 12:41

markmandel reviewed Aug 7, 2020

View reviewed changes

docs tweaks, defaultconfig

d1239ba

markmandel approved these changes Aug 7, 2020

View reviewed changes

google-oss-robot assigned markmandel Aug 7, 2020

google-oss-robot added the lgtm label Aug 7, 2020

google-oss-robot added the approved label Aug 7, 2020

markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/breaking Breaking change kind/feature New features for Agones labels Aug 7, 2020

markmandel added this to the 1.8.0 milestone Aug 7, 2020

markmandel merged commit 8e215d3 into googleforgames:master Aug 7, 2020

domgreen deleted the netspeak_sdk branch August 8, 2020 08:15

ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020

move Netspeak Unreal SDK into Agones Unreal SDK (googleforgames#1739)

19b7cfc

* moving Netspeak Agones SDK into main repo SDK Co-authored-by: Mark Mandel <markmandel@google.com>

move Netspeak Unreal SDK into Agones Unreal SDK #1739

move Netspeak Unreal SDK into Agones Unreal SDK #1739

Conversation

domgreen commented Aug 5, 2020

agones-bot commented Aug 5, 2020

domgreen commented Aug 5, 2020

markmandel left a 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

Choose a reason for hiding this comment

markmandel commented Aug 6, 2020

agones-bot commented Aug 6, 2020

dotcom commented Aug 6, 2020 • edited Loading

markmandel commented Aug 6, 2020

WVerlaek left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domgreen commented Aug 7, 2020

agones-bot commented Aug 7, 2020

domgreen commented Aug 7, 2020

agones-bot commented Aug 7, 2020

WVerlaek commented Aug 7, 2020

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Aug 7, 2020

markmandel commented Aug 7, 2020

WVerlaek commented Aug 7, 2020

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Aug 7, 2020

dotcom commented Aug 6, 2020 •

edited

Loading