-
Notifications
You must be signed in to change notification settings - Fork 817
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
Extend Agones Unreal SDK #1303
Extend Agones Unreal SDK #1303
Conversation
Build Succeeded 👏 Build Id: 3a518848-5b9c-468d-aebf-d467055ac907 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:
|
Build Succeeded 👏 Build Id: 59b1ab67-c0e6-4fba-a9b3-c549dc669858 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 can take a look through, but @devjgm you are our usual C++ person (I believe Unreal is different???) do you have time to do a review? |
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 had a quick look - barely means anything to me 😄 @padhansell - since it looks like you are doing a review (thanks!) please let us know when you think this is good to go, and I'll approve. 👍 |
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.
@WVerlaek probably worth also compiling for the Editor, and testing in Shipping
configuration if you haven't already, just to be double sure that nothing is getting #ifdef
d out and causing compilation issues. Your changes shouldn't introduce anything, but generally worth checking for sanity 😄
Otherwise looks good to me @markmandel!
, *SidecarAddress | ||
, (Settings->bHealthPingEnabled ? TEXT("True") : TEXT("False")) | ||
, Settings->HealthPingSeconds | ||
, (Settings->bDebugLogEnabled ? TEXT("True") : TEXT("False"))); | ||
, (Settings->bDebugLogEnabled ? TEXT("True") : TEXT("False")) |
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.
@WVerlaek we discussed this yesterday but adding here for posterity!
In a subsequent PR we should probably ditch bDebugLogEnabled
and instead just make use of the log levels in Unreal's logging system, which would:
a) Allow developers to change the logging behaviour through .ini configuration files, and on-the-fly through console commands. As an example, through the in-game console (or remotely using the "messaging" system), you could run the command log LogAgonesHook error
to suppress any logs with a severity less than Error
b) Make the plugin more consistent with the rest of the engine
c) Remove some code
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.
Agree, standardize on the engine and how UE4 enginers' work would be fantastic in a follow-up.
Mentioned in the past that blueprint support would be fantastic.
@padhansell: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thanks for taking a look! Not sure how easy it would be to create a make target or example for the unreal plugin, might potentially be quite a large undertaking. I've been testing the plugin using an existing unreal engine project, a new sample project would need to be created as well (which could be quite a lot to check in to the repo?) |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Build Succeeded 👏 Build Id: 5bbe9ea9-7ef1-46fc-9e24-5b66baeabb74 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:
|
Build Succeeded 👏 Build Id: 1eed3ca9-5c7f-4162-a6cd-910b8de7b667 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:
|
Have checked that compiling for Editor and |
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.
🚢
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, padhansell, WVerlaek 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 |
If there is a way to maybe do a skeleton project? I.e. something like "create a new project, and then copy these files into this place" ? That being said, the Unity sample is 62M, which is fine - how big are we talking? |
* Add SetLabel SetAnnotation GetGameServer and request retries * Update Unreal sdk settings in docs * Fix docs list markdown * Add const and improve error message
Co-authored-by: Mark Mandel <markmandel@google.com>
SetLabel
,SetAnnotation
, andGetGameServer
.Tested by:
TESTS=ready,health,gameserver,setlabel,setannotation make run-sdk-conformance-local
Example
SetLabel
requests based off data received byGetGameServer
(output from SDK conformance test):