Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Unity SDK example #395

Closed
wants to merge 3 commits into from
Closed

Added Unity SDK example #395

wants to merge 3 commits into from

Conversation

GabeBigBoxVR
Copy link
Contributor

Added Unity SDK Example Client

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 2aa9f4eb-b6e7-4cb5-8a98-1d28b278105b

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

(experimental) To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/395/head:pr_395 && git checkout pr_395
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.6.0-123c9f6

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two quick things

  1. Should this be in SDKs?
  2. We'll need the Apache licence header at the top before merging.

@GabeBigBoxVR
Copy link
Contributor Author

@markmandel Done

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a proper look - looks good, just a few questions.

sdks/csharp/AgonesSdkClient.cs Outdated Show resolved Hide resolved
sdks/csharp/AgonesSdkClient.cs Outdated Show resolved Hide resolved
// See the License for the specific language governing permissions and
// limitations under the License.

using Newtonsoft.Json;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you don't use Unity's native JSON support?

If we did that, we wouldn't have any external dependencies, and it would be easy to install.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we don't use Unity's native JSON parser since it has a lot of problems with complex JSON. It had a ton of bugs and we don't use it because of the issues it can introduce.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: f3697730-358f-436f-b511-3da24394412a

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

(experimental) To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/395/head:pr_395 && git checkout pr_395
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.6.0-896c131

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question about Newtonsoft.Json made me realise - we need some documentation for this 😄

So please add a README.md that discusses how to install, and use, and what dependencies are needed and where to get them from.

I would probably be happier with no external dependencies, just because Unity's marketplace is not a very good package management system - but will defer to your opinion on this, as you are the one who knows this area better than I do.

Will also need an entry in the list of SDKs here: https://github.com/GoogleCloudPlatform/agones/blob/master/sdks/README.md

👍

private const string emptyPayload = "{}";
private static byte[] emptyPayloadBytes;
private WaitForSeconds wait = new WaitForSeconds(0.5f);
private float updateInterval = 0.5f;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three fun(?) questions about this bit here:

  1. Should updateInterval be public, so that people can adjust this via the Component editor?
  2. Since we have the capability to grab the GameServer configuration from the SDK, should we do that, and then use that to start this Health ping? (then the updateInterval would not need to be public)
  3. If we are going to set an opinionated health loop - shouldn't we also provide a callback mechanism so that users can provide their own base logic for health pinging? (and maybe also a way to disable the opinionated health ping?). Right now, there is no other choice on health pinging other than what is provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering out-of-order

2: I would say yes, but note that Unity Developers really like being able to modify things directly in the Editor. (Estimating based on personal observation only) it seems like upwards of 1/3 of all the Asset Packages are just custom Editors that provide interfaces for things that engineers can do with code.

If we expect anyone using this package to already be familiar with Agones, Kubernetes, etc, then we can just grab the GameServer configuration.

However, if we want this to be accessible to newbie coders or even designers at some point, then I would suggest a long-term goal of providing some kind of Editor Plugin. This is an area I've spent a lot of time on, so I'd be able to help with that.

1: As above, yes (public) if we want it to be accessible to people less familiar with the console. Another option instead of making this public would just be to add the [SerializeField] tag to the UpdateInterval property below, that way it shows up in Editor and modifying it calls the set method accordingly.

3: This could be as simple as making the "Health" method virtual, and allowing the user to subclass AgonesSdkClient if they want different Health checking. As of now, the option to disable Health Checking appears to be LaunchOptions.AgonesEnabled, because I don't see that being used anywhere else, but if there are circumstances where you want Agones enabled but not Health checking, then yeah it could be a different flag.

@markmandel
Copy link
Member

@dzlier-gcp You got some cycles to take a look, I'm sure your Unity is better than mine 😄

@markmandel markmandel added kind/feature New features for Agones area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Oct 24, 2018
/// <summary>
/// JSON payload object
/// </summary>
internal class KeyValueMessage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would using the existing KeyValuePair<string, string> class work for this? I believe it works with JsonUtility, but please double-check. Would save you defining a custom key value class though.

private const string emptyPayload = "{}";
private static byte[] emptyPayloadBytes;
private WaitForSeconds wait = new WaitForSeconds(0.5f);
private float updateInterval = 0.5f;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering out-of-order

2: I would say yes, but note that Unity Developers really like being able to modify things directly in the Editor. (Estimating based on personal observation only) it seems like upwards of 1/3 of all the Asset Packages are just custom Editors that provide interfaces for things that engineers can do with code.

If we expect anyone using this package to already be familiar with Agones, Kubernetes, etc, then we can just grab the GameServer configuration.

However, if we want this to be accessible to newbie coders or even designers at some point, then I would suggest a long-term goal of providing some kind of Editor Plugin. This is an area I've spent a lot of time on, so I'd be able to help with that.

1: As above, yes (public) if we want it to be accessible to people less familiar with the console. Another option instead of making this public would just be to add the [SerializeField] tag to the UpdateInterval property below, that way it shows up in Editor and modifying it calls the set method accordingly.

3: This could be as simple as making the "Health" method virtual, and allowing the user to subclass AgonesSdkClient if they want different Health checking. As of now, the option to disable Health Checking appears to be LaunchOptions.AgonesEnabled, because I don't see that being used anywhere else, but if there are circumstances where you want Agones enabled but not Health checking, then yeah it could be a different flag.

/// </summary>
private void Start()
{
AgonesSdkClient.emptyPayloadBytes = Encoding.UTF8.GetBytes(AgonesSdkClient.emptyPayload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - I don't think we use class modifiers on static fields within the class itself, but maybe double-check the style guide to be sure.

}

/// <summary>
/// Unity Start
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is neither necessary nor strictly accurate, as Start is called when the GameObject a MonoBehaviour is attached to is activated for the first time in game (not when Unity starts).

{
AgonesSdkClient.emptyPayloadBytes = Encoding.UTF8.GetBytes(AgonesSdkClient.emptyPayload);

if (LaunchOptions.AgonesEnabled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's LaunchOptions?

// See the License for the specific language governing permissions and
// limitations under the License.

using Newtonsoft.Json;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have several times found myself importing Newtonsoft into a Unity program, only to realize that Unity's built-in JsonUtility.ToJson and JsonUtility.FromJson<T> do provide the functionality I need. This obviates the need to install a separate NuGet package, which is not very well documented/supported in Unity, I find.

From what I can see JsonConvert being used for here, it looks like JsonUtility would cover this, but let me know if it doesn't work for some reason.

if (LaunchOptions.AgonesEnabled)
{
this.StartCoroutine(this.UpdateLoop());
MainContext.Instance.InstanceBinder.Bind<AgonesSdkClient>(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reference to MainContext or InstanceBinder anywhere either (in Unity documentation, or C# at large), where do these come from?

string payload = JsonConvert.SerializeObject(msg);

UnityWebRequest request = UnityWebRequest.Put(uri, payload);
request.uploadHandler = new UploadHandlerRaw(Encoding.UTF8.GetBytes(payload));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant to UnityWebRequest.Put above, as that function sets the uploadHandlerRaw with the provided data (I would move the Encoding.UTF8.GetByes to the Put param though).

private static void EmptyPost(string uri)
{
UnityWebRequest request = UnityWebRequest.Post(uri, AgonesSdkClient.emptyPayload);
request.uploadHandler = new UploadHandlerRaw(AgonesSdkClient.emptyPayloadBytes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

/// </summary>
public void Ready()
{
AgonesSdkClient.EmptyPost("http://localhost:59358/ready");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this port static among all usecases? The target URL should maybe have Editor-visible components.

@GabeBigBoxVR GabeBigBoxVR mentioned this pull request Oct 31, 2018
@markmandel markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Nov 21, 2018
@markmandel markmandel removed feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) labels Nov 28, 2018
@lefnire
Copy link

lefnire commented Apr 20, 2019

I see this was marked merge-after-release but then decided against, and OP closed. Is this PR not forthcoming?

@markmandel
Copy link
Member

afaik, nobody is working on in it currently. Would you be interested in taking it up? 😄

@lefnire
Copy link

lefnire commented Apr 21, 2019

Was evaluating the repo for a project and this ticket's the kicker. I'm not proficient enough to contribute, if that changes I'll let you know.

@dav-otero
Copy link

I am doing some Agones PoCs in Unity. Is this the only official work done on this side? Is there any ETA for an official Unity SDK?

I actually have been doing some fast work and created a simple Agones SDK for Unity. In order to try out Agones in Unity I needed to create the SDK using a simple Rest Client we also use for other stuff in the game server. It just works. But obviouly this should not be the "official" sdk coming from Agones.

Tbh, probably the way to go is to have a standard C# client and let the Unity games hook the SDK calls in their game loops. This c# should not have any dependency apart from the C# version. The reason of not having Unity-related stuff is because the dev paradigm is changing in the Unity ecosystem with the introduction of DoTS... so going fro Monobehaviours, Coroutines and the like might not be the best "standard" way to build the sdk client.

Instead, I would go for a C# client using http async .NET calls to consume the sidecar REST api. Then is up to the game server when to call Ready, Health and the rest of the calls.

@dav-otero
Copy link

In fact, based on what I see for other SDK implementations (for example unreal), there not a hard need to have json serialization, that it will tie us to a third party library if going with standard C#.

https://github.com/GoogleCloudPlatform/agones/blob/master/sdks/unreal/Agones/Source/Agones/AgonesHook.cpp

The only call that requires json serialization would be GameServer, that is not even implemented in the Unreal SDK.

@lefnire
Copy link

lefnire commented Apr 30, 2019

It just works. But obviouly this should not be the "official" sdk coming from Agones.

@dav-otero do you have a public repo/branch for this nonetheless? for any of use interested in Unity on Agones, something to work with would be a boon!

@markmandel
Copy link
Member

Sounds like you two have a great idea of what should be done. We currently don't have anyone focused on this at the moment - so would love to have contributions in this area! 👍

@lefnire
Copy link

lefnire commented Apr 30, 2019

@markmandel I'm just wondering what happened with this thread here. It looked like @GabeBigBoxVR added full support, submitted a PR which was getting reviewed and ready for merge. Then the PR got closed, and merge tag removed - both silently. Was something agreed on offline that this PR is a no-go?

@markmandel
Copy link
Member

@lefnire there were a lot of review comments from @dzlier-gcp about issues with the PR - please scroll up and review - maybe you can fix them, because they were never finalised.

Then OP closed the PR - I'm not going to guess at another party's motivations. That being said, I'm not sure what you are getting at with "silently" - all the discussion was made on this PR and is available right here.

The merge-after-release tag is a marker to ensure that PR's don't get merged between RC and full release - not an indication of a PR is ready for merging. Please review our release process for full understanding: https://github.com/GoogleCloudPlatform/agones/blob/master/docs/governance/release_process.md

@GabeBigBoxVR
Copy link
Contributor Author

@lefnire I closed the pull request because we decided not to use Agones and I didn't have enough time to complete it. If someone wants to take the code and finish it, i'm happy to share it.

@dav-otero
Copy link

@lefnire Sorry, I don't have a public repo. The way I did it is by using a third party rest client. The way I see it after thinking about it, it is that it should use the UnityWebRequest in order to perform the Http calls. I was convinced that the best way to go is to build up a generic c# client, using System.Net HttpClient, so that Unity games can just use it. But I found out that System.Net is not safe to use for all platforms. For example, HttpClient will not work for WebGL, as WebGL does not support threading. So basically stuff on System.Net namespace is not safe to use for crossplatform. My point: the SDK should be Unity focused so that it can work in all Unity supported platforms.

@dav-otero
Copy link

The rest client I am using is this one: https://github.com/proyecto26/RestClient

@gambit-monkey
Copy link

gambit-monkey commented May 14, 2019

@markmandel So basically there is no official Unity Agones support coming anytime soon I take it? I was a little mislead as I saw on here https://cloud.google.com/solutions/gaming/ a section about Kubernetes and Agones along with OpenMatch/Unity so spun up Kubernetes and Agones through the GCP marketplace and now have no way to utilize it with Unity. I have been using Gamelift and the AWS gaming services up to this point, but wanted to give the Google stuff a go. However I guess that is not doable unless we want to put development hours into it. You all have the Firebase stuff for Unity just not the gaming stuff.

@markmandel
Copy link
Member

@markmandel So basically there is no official Unity Agones support coming anytime soon I take it?

Please review the top level 1.0 ticket - you'll see that a Unity SDK is coming before the 1.0 release.

I was a little mislead as I saw on here https://cloud.google.com/solutions/gaming/ a section about Kubernetes and Agones along with OpenMatch/Unity so spun up Kubernetes and Agones through the GCP marketplace and now have no way to utilize it with Unity.

My apologies that you were misled. If you could provide exactly where the site specified that Agones worked with Unity, I'll happily get it corrected.

I have been using Gamelift and the AWS gaming services up to this point, but wanted to give the Google stuff a go. However I guess that is not doable unless we want to put development hours into it. You all have the Firebase stuff for Unity just not the gaming stuff.

Agones is a free, and open source project. We are always interested in having a variety of developers with a variety of backgrounds collaborate in this space to reach this common goal. I would welcome your contribution if you decide to help us develop the Unity SDK for Agones.

@whisper0077 whisper0077 mentioned this pull request Jun 8, 2019
@TimoSchmechel TimoSchmechel mentioned this pull request Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants