Implementation of GameServer() for Unity #1169

markmandel · 2019-11-04T05:03:13Z

Refactored SendRequestAsync to return a AsyncResult which contains both a success bool, and the returned body text from the http call.
Used the swagger.json to generate the basic structure of the GameServer object, and then hand modified as needed.
Had to include third_party/MiniJSON because Unity doesn't natively support Dictionary, which means we can't get Label or Annotation data. Then had to manually import the MiniJSON data into GameServer and related model objects.
Used a set of symlinks to share the Agones Unity SDK code with the unity-simple example. Couldn't think of a better way.
Minor version update on the example project as that is the earliest version I could get in 2018.4.x
Added GameServer command to the simple-unity example as well.

Work on #927

agones-bot · 2019-11-04T05:17:38Z

Build Failed 😱

Build Id: ac3ac819-075e-4233-bed6-926555391acd

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

roberthbailey · 2019-11-04T05:41:21Z

/assign @pooneh-m

(since you have a lot more c# experience than I do)

roberthbailey · 2019-11-04T05:41:57Z

/assign @dzlier-gcp

Also Dane...

markmandel · 2019-11-04T19:40:21Z

Weird this flaked - looking at the logs, it completed:

time="2019-11-04 05:17:12.715" level=info msg="checking final crash state" gs=udp-serverv658t state=Unhealthy test=TestGameServerRestartBeforeReadyCrash
time="2019-11-04 05:17:12.715" level=info msg="Unhealthy! We are done!" test=TestGameServerRestartBeforeReadyCrash
--- FAIL: TestGameServerRestartBeforeReadyCrash (269.62s)
    gameserver_test.go:303: 
        	Error Trace:	gameserver_test.go:303
        	Error:      	Could not make GameServer Ready: %v
        	Test:       	TestGameServerRestartBeforeReadyCrash
        	Messages:   	timed out waiting for the condition
FAIL

Restarting the e2e test.

agones-bot · 2019-11-04T19:56:32Z

Build Succeeded 👏

Build Id: fe276dcb-f870-49d5-88f5-71ddb76b75ed

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1169/head:pr_1169 && git checkout pr_1169
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.2.0-1712048

pooneh-m · 2019-11-05T17:47:50Z

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

@@ -40,6 +40,15 @@ async void SomeMethod()
 }
 ```

+To get the details on the [backing `GameServer`]({{< relref "_index.md#gameserver" >}}) call `GameServer()`.
+
+Will return `null` if something there is an error in retrieving the `GameServer` record.


"something" -> remove?

🤦‍♂️ thank you!

pooneh-m · 2019-11-05T17:52:04Z

examples/unity-simple/Assets/Scripts/UdpEchoServer.cs

@@ -73,6 +73,14 @@ async void Update()

                        echoBytes = Encoding.UTF8.GetBytes($"Allocate {ok}");
                        break;
+
+                    case "GameServer":
+                        var gameserver = await agones.GameServer();


[future nit.] the C# style guide recommends suffixing async methods with Async. e.g. GameServerAsync.

You should add "Async" as the suffix of every async method name you write.

That would be a good thing. But we already have the rest of the SDK being async, and it wasn't written that way in the first place 😞 we probably should at least be consistent?

pooneh-m · 2019-11-05T18:01:53Z

sdks/unity/AgonesSdk.cs

@@ -78,7 +81,23 @@ private void OnApplicationQuit()
        /// </returns>
        public async Task<bool> Ready()
        {
-            return await SendRequestAsync("/ready", "{}");
+            return await SendRequestAsync("/ready", "{}").ContinueWith(task => task.Result.ok);


Why do you need this? -> .ContinueWith(task => task.Result.ok)

We are now no longer returning a Task<bool> but a Task<AsyncResult> from SendRequestAsync, since we needed more information from it.

pooneh-m · 2019-11-06T00:13:23Z

sdks/unity/model/GameServer.cs

+        /// <summary>
+        /// Initializes a new instance of the <see cref="GameServer" /> class.
+        /// </summary>
+        public GameServer(IReadOnlyDictionary<string, object> data)


null check for data?

pooneh-m · 2019-11-06T00:22:03Z

sdks/unity/model/GameServer.cs

+        public GameServer(IReadOnlyDictionary<string, object> data)
+        {
+            this.ObjectMeta = new GameServerObjectMeta((Dictionary<string,object>) data["object_meta"]);
+            this.Spec = new GameServerSpec((Dictionary<string,object>) data["spec"]);


Why this requires force conversion (throw exception if not converted) while status requires soft conversion with as (null value if not converted)? If intended can you add a comment to document?

pooneh-m · 2019-11-06T00:29:22Z

sdks/unity/model/SpecHealth.cs

+        public SpecHealth(IReadOnlyDictionary<string, object> data)
+        {
+            this.Disabled = data.TryGetValue("disabled", out var disabled) && bool.Parse((string) disabled);
+            this.PeriodSeconds = (Int64) data["period_seconds"];


Can we use Int64.Parse for these as well?

This is an actual Int64 stored in the map, so there is no string to parse.

pooneh-m · 2019-11-06T00:32:19Z

sdks/unity/AgonesSdk.cs

+                return null;
+            }
+
+            var data = Json.Deserialize(result.json) as Dictionary<string, object>;


Can we use a JSON deserializer, such as newtonsoft that accepts the predefined objects here with json names, instead of converting them to a dictionary and find the values?

I wish. We can't use NuGet, so we can't pull in newtonsoft. There is a Unity specific version of it, but it's huge, and seems like a massive dependency. And we can't force Users to pull things down from the asset store.

MiniJSON is lightweight, and does what we need, even though we have to do more work.

We can't use NuGet

Why not?

It's not something Unity supports. Also they have older / kinda hacked version of Mono running in there. It's a whole thing.

agones-bot · 2019-11-06T19:41:16Z

Build Succeeded 👏

Build Id: a4e5fff8-1d52-4b4f-bedc-6f02d58a862f

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1169/head:pr_1169 && git checkout pr_1169
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.2.0-f842952

markmandel · 2019-11-07T02:59:37Z

Should be ready for re-review now.

agones-bot · 2019-11-07T03:12:39Z

Build Succeeded 👏

Build Id: 69612b1f-f353-4d71-8485-cf7bb280710d

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1169/head:pr_1169 && git checkout pr_1169
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.2.0-ad78040

- Refactored `SendRequestAsync` to return a `AsyncResult` which contains both a success bool, and the returned body text from the http call. - Used the swagger.json to generate the basic structure of the `GameServer` object, and then hand modified as needed. - Had to include third_party/MiniJSON because Unity doesn't natively support `Dictionary`, which means we can't get Label or Annotation data. Then had to manually import the MiniJSON data into GameServer and related model objects. - Used a set of symlinks to share the Agones Unity SDK code with the unity-simple example. Couldn't think of a better way. - Minor version update on the example project as that is the earliest version I could get in 2018.4.x - Added `GameServer` command to the simple-unity example as well. Work on googleforgames#927

agones-bot · 2019-11-07T18:33:32Z

Build Succeeded 👏

Build Id: dabbb04f-25b5-46c7-b9cd-caf6876c4b10

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

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

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

https://529c33f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1169/head:pr_1169 && git checkout pr_1169
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.2.0-529c33f

markmandel · 2019-11-07T18:37:27Z

Thanks for the approval @pooneh-m - would love to get @dzlier-gcp to also look, just because of their Unity experience as well.

google-oss-robot · 2019-11-07T21:08:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dzlier-gcp, markmandel, pooneh-m

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~~ [dzlier-gcp,markmandel,pooneh-m]

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

- Refactored `SendRequestAsync` to return a `AsyncResult` which contains both a success bool, and the returned body text from the http call. - Used the swagger.json to generate the basic structure of the `GameServer` object, and then hand modified as needed. - Had to include third_party/MiniJSON because Unity doesn't natively support `Dictionary`, which means we can't get Label or Annotation data. Then had to manually import the MiniJSON data into GameServer and related model objects. - Used a set of symlinks to share the Agones Unity SDK code with the unity-simple example. Couldn't think of a better way. - Minor version update on the example project as that is the earliest version I could get in 2018.4.x - Added `GameServer` command to the simple-unity example as well. Work on googleforgames#927

markmandel added kind/feature New features for Agones area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Nov 4, 2019

markmandel requested review from dzlier-gcp and pooneh-m November 4, 2019 05:03

google-oss-robot added the size/XXL label Nov 4, 2019

google-oss-robot requested a review from cyriltovena November 4, 2019 05:03

google-oss-robot added the approved label Nov 4, 2019

google-oss-robot assigned pooneh-m Nov 4, 2019

roberthbailey requested review from dzlier-gcp and removed request for dzlier-gcp November 4, 2019 05:41

google-oss-robot assigned dzlier-gcp Nov 4, 2019

pooneh-m reviewed Nov 6, 2019

View reviewed changes

markmandel force-pushed the feature/unity-sdk-gameserver branch from f842952 to ad78040 Compare November 7, 2019 02:59

pooneh-m approved these changes Nov 7, 2019

View reviewed changes

google-oss-robot added the lgtm label Nov 7, 2019

markmandel force-pushed the feature/unity-sdk-gameserver branch from ad78040 to 529c33f Compare November 7, 2019 18:19

google-oss-robot removed the lgtm label Nov 7, 2019

dzlier-gcp approved these changes Nov 7, 2019

View reviewed changes

google-oss-robot added the lgtm label Nov 7, 2019

markmandel merged commit 7c50719 into googleforgames:master Nov 7, 2019

markmandel deleted the feature/unity-sdk-gameserver branch November 7, 2019 21:09

roberthbailey added this to the 1.2.0 milestone Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of GameServer() for Unity #1169

Implementation of GameServer() for Unity #1169

markmandel commented Nov 4, 2019

agones-bot commented Nov 4, 2019

roberthbailey commented Nov 4, 2019

roberthbailey commented Nov 4, 2019

markmandel commented Nov 4, 2019

agones-bot commented Nov 4, 2019

pooneh-m Nov 5, 2019

markmandel Nov 7, 2019

pooneh-m Nov 5, 2019

markmandel Nov 6, 2019

pooneh-m Nov 5, 2019 •

edited

Loading

markmandel Nov 6, 2019

pooneh-m Nov 6, 2019 •

edited

Loading

pooneh-m Nov 6, 2019

pooneh-m Nov 6, 2019

markmandel Nov 7, 2019

pooneh-m Nov 6, 2019 •

edited

Loading

markmandel Nov 6, 2019

pooneh-m Nov 6, 2019 •

edited

Loading

markmandel Nov 6, 2019

agones-bot commented Nov 6, 2019

markmandel commented Nov 7, 2019

agones-bot commented Nov 7, 2019

agones-bot commented Nov 7, 2019

markmandel commented Nov 7, 2019

google-oss-robot commented Nov 7, 2019

Implementation of GameServer() for Unity #1169

Implementation of GameServer() for Unity #1169

Conversation

markmandel commented Nov 4, 2019

agones-bot commented Nov 4, 2019

roberthbailey commented Nov 4, 2019

roberthbailey commented Nov 4, 2019

markmandel commented Nov 4, 2019

agones-bot commented Nov 4, 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

pooneh-m Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pooneh-m Nov 6, 2019 • edited Loading

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

pooneh-m Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pooneh-m Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Nov 6, 2019

markmandel commented Nov 7, 2019

agones-bot commented Nov 7, 2019

agones-bot commented Nov 7, 2019

markmandel commented Nov 7, 2019

google-oss-robot commented Nov 7, 2019

pooneh-m Nov 5, 2019 •

edited

Loading

pooneh-m Nov 6, 2019 •

edited

Loading

pooneh-m Nov 6, 2019 •

edited

Loading

pooneh-m Nov 6, 2019 •

edited

Loading