-
Notifications
You must be signed in to change notification settings - Fork 825
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
Support for Unity Alpha SDK #2600
Conversation
Build Succeeded 👏 Build Id: 8b1680ce-6836-4989-9ad1-814efe988508 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
/assign @markmandel Hope you don't me assigning you. You seem to have reviewed the last few unity sdk changes. |
Thanks for this - dragging in a few people who know Unity better from Slack. Quick review though - we will need some docs please 😄 Have a look at: And also https://agones.dev/site/docs/contribute/ for details on how to edit the docs, and make sure new features only show up on release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My C# is not great, but eyeballing it, things seem fine.
Just need some documentation please before merging 😄
@MaxHayman - The next release is in a week. Do you think you'll have time to add some docs so we can get this merged for the release? |
Sorry had been quite busy. Is this okay? I wasn't sure what to put for the release gate of the content as I wasn't sure if it would make it in to the next release. But would this work? diff --git a/site/content/en/docs/Guides/Client SDKs/unity.md b/site/content/en/docs/Guides/Client SDKs/unity.md
index 063eecac0..af62311c3 100644
--- a/site/content/en/docs/Guides/Client SDKs/unity.md
+++ b/site/content/en/docs/Guides/Client SDKs/unity.md
@@ -21,6 +21,16 @@ Check the [Client SDK Documentation]({{< relref "_index.md" >}}) for more detail
| Configuration | Watch | ✔️ |
| Metadata | SetAnnotation | ✔️ |
| Metadata | SetLabel | ✔️ |
+{{% feature expiryVersion="1.24.0" %}}
+| Player Tracking | GetConnectedPlayers | ❌️ |
+| Player Tracking | GetPlayerCapacity | ❌️ |
+| Player Tracking | GetPlayerCount | ❌️ |
+| Player Tracking | IsPlayerConnected | ❌️ |
+| Player Tracking | PlayerConnect | ❌️ |
+| Player Tracking | PlayerDisconnect | ❌️ |
+| Player Tracking | SetPlayerCapacity | ❌️ |
+{{% /feature %}}
+{{% feature publishVersion="1.24.0" %}}
| Player Tracking | GetConnectedPlayers | ✔️ |
| Player Tracking | GetPlayerCapacity | ✔️ |
| Player Tracking | GetPlayerCount | ✔️ |
@@ -28,6 +38,7 @@ Check the [Client SDK Documentation]({{< relref "_index.md" >}}) for more detail
| Player Tracking | PlayerConnect | ✔️ |
| Player Tracking | PlayerDisconnect | ✔️ |
| Player Tracking | SetPlayerCapacity | ✔️ |
+{{% /feature %}}
Additional methods have been added for ease of use:
@@ -119,6 +130,8 @@ configuration changes.
agones.WatchGameServer(gameServer => Debug.Log($"Server - Watch {gameServer}"));
```
+{{% feature publishVersion="1.24.0" %}}
+
## Player Tracking
{{< alpha title="Player Tracking" gate="PlayerTracking" >}}
@@ -191,6 +204,8 @@ This returns a list of the playerIDs that are currently connected to the GameSer
List<string> players = await agones.GetConnectedPlayers();
```
+{{% /feature %}}
+
{{% alert title="Warning" color="warning"%}}
The following code causes deadlock. Do not use a `Wait` method with the returned Task.
|
Build Failed 😱 Build Id: 7f21d4c8-aeb5-494a-a99e-ac40e64aca56 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
All good! We all hit busy times 👍🏻 Yeah, we might need to skip this for this release, since the above isn't in your PR, but the overall structure looks good. Only thing I can think of without it getting built - often you'll need to duplicate the entire table between But give it go on the PR, and we can preview and adjust as needed. |
Applied and wrapped it around the whole table! |
Build Succeeded 👏 Build Id: 0363e8cd-6fca-47be-a957-2405cade0a7d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this work! One small tweak, and a rendering issue I haven't seen before.
Co-authored-by: Mark Mandel <markmandel@google.com>
Co-authored-by: Mark Mandel <markmandel@google.com>
Build Failed 😱 Build Id: 0710caa5-6e63-4da1-bfb5-8ac65b7f91ed To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Had to wrangle the formatting of the alpha shortcode to handle shortcode in shortcode rendering, and also updated everything to 1.25.0.
Took me a minute to work out what the issue was the with the shortcode formatting - so I sent you a PR to your PR 😄 it's a bit of a hacky, fix, but it works. Also noted moving everything to 1.25.0. I can't update your branch to main (because you are on a org repo, it's a weird github rule - unless you can find a way to give me rights to add commits to your branch) - if you could merge my PR, rebase against main, I'm happy to approve and merge. (I thought about adding the rebase to the PR, but I figured that would end up quite confusing). Thanks for your patience on this one! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, MaxHayman 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:
Approvers can indicate their approval by writing |
Looks perfect - if you can rebase against Context: https://github.com/orgs/github-community/discussions/5634 |
New changes are detected. LGTM label has been removed. |
We got it done! |
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Currently the Unity SDK does not support the player tracking features
#1905
#1677
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer:
Using AgonesAlphaSdk instead of AgonesSdk will give you access to the new alpha features. Made some methods protected to I could avoid disrupting as much of the existing code as possible.