-
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
Unity SDK - Watch GameServer Functionality #1234
Unity SDK - Watch GameServer Functionality #1234
Conversation
/assign @dzlier-gcp @pooneh-m Please review 😄 |
Build Succeeded 👏 Build Id: f097105f-503f-4b35-86c1-b4d4d1088079 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:
|
protected override bool ReceiveData(byte[] data, int dataLength) | ||
{ | ||
string json = Encoding.UTF8.GetString(data); | ||
var dictionary = (Dictionary<string, object>) Json.Deserialize(json); |
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.
can the data contain invalid json?
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.
If it did, it would be Exceptional.
🤣
{ | ||
string json = Encoding.UTF8.GetString(data); | ||
var dictionary = (Dictionary<string, object>) Json.Deserialize(json); | ||
var gameServer = new GameServer(dictionary["result"] as Dictionary<string, object>); |
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.
Check to verify "result" key exists in the dictionary?
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.
If it doesn't, you will end up with an empty non-populated GameServer.
Should we throw an Exception if it didn't work as expected?
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.
if result is not in the dictionary, an exception will be thrown:
[System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.] at System.Collections.Generic.Dictionary.get_Item(TKey key)
If you want a non-populated GS, you should handle this with if dictionary.Contains("result")
...
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.
Hmnn. Actually I don't mind if the exception occurs. It's probably a better indicator that something went horribly wrong, rather than giving a non-populated GS.
So I'm going to go with "works as intended" 👍 (unless you have an objection?)
Appreciate the explanations!
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.
The exception is hard to debug without more details. However, if it is intended, LGTM.
echoBytes = Encoding.UTF8.GetBytes($"Reserve({recvTexts[1]}) {ok}"); | ||
} | ||
else | ||
{ | ||
echoBytes = Encoding.UTF8.GetBytes($"ERROR: Invalid Reserve command, must use 1 argument"); | ||
} | ||
break; | ||
case "Watch": | ||
agones.WatchGameServer(gameServer => { Debug.Log($"Server - Watch {gameServer}"); }); |
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.
I think you can drop parenthesis x => {y;} --> x => y
5c6c28e
to
35f9017
Compare
Build Succeeded 👏 Build Id: 2147d491-729b-4b9a-9c7f-c8a7a6509db2 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:
|
Just noting that I have seen this - will review before EOW |
{ | ||
string json = Encoding.UTF8.GetString(data); | ||
var dictionary = (Dictionary<string, object>) Json.Deserialize(json); | ||
var gameServer = new GameServer(dictionary["result"] as Dictionary<string, object>); |
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.
if result is not in the dictionary, an exception will be thrown:
[System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.] at System.Collections.Generic.Dictionary.get_Item(TKey key)
If you want a non-populated GS, you should handle this with if dictionary.Contains("result")
...
configuration changes. | ||
|
||
```csharp | ||
agones.WatchGameServer(gameServer => { Debug.Log($"Server - Watch {gameServer}"); }); |
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.
x => {y;} to x => y here as well?
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.
Doh! Nice pickup. Will change!
Able to use the `DownloadHandlerScript` to intercept streaming HTTP data from the /watch/gameserver REST endpoint. This finishes the feature work for Unity on googleforgames#927
35f9017
to
222abf0
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
Build Succeeded 👏 Build Id: a37eade6-805f-409d-baee-4426243e3a98 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:
|
I somehow missed this was Approved. Getting it merged! |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 9b135153-5fc6-48ae-8736-c0ecc2b11446 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:
|
Able to use the `DownloadHandlerScript` to intercept streaming HTTP data from the /watch/gameserver REST endpoint. This finishes the feature work for Unity on googleforgames#927
Able to use the
DownloadHandlerScript
to intercept streaming HTTP data from the /watch/gameserver REST endpoint.This finishes the feature work for Unity on #927