-
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
Unreal SDK add Allocate + Reserve and changes to the plugin settings #1345
Conversation
Build Failed 😱 Build Id: 49b25e8c-f5e4-4121-a062-83f588fcd2b2 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
- Request Retry Limit. Maximum number of times a failed request to the Agones sidecar is retried. Health requests are not retried. (default: `30`) | ||
- Send Ready at Startup. Automatically send a Ready request when the server starts. Disable this to manually control when the game server should be marked as ready. (default: `true`) |
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.
Does there need to be more docs around how to use the the supplied methods, such as Allocate
, Ready
, SetLabel
etc?
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 tend to agree, it might be worth adding a paragraph on how these mehtods / structs are available to be set from within UE4Editor?
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.
Might also be worth adding a paragraph around using this locally ... eg. how we can turn off the healthcheck/ready checks when just launching via UE4Editor that makes it easier for most GameDevs to iterate fast before pushing into our clusters.
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.
Have added some docs on how to use the sdk methods, please lmk if it makes any sense 😄
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.
For local dev (or for running outside an agones environment) I think the plugin could use some more work to support disabling executing the sdk methods / auto health checks. Was thinking about an additional config value bAgonesEnabled
which can be set to default
, forceEnabled
, or forceDisabled
, where default
will disable calling out to the agones sidecar when running locally inside the unreal editor - but that's not something for this pr
@domgreen, @padhansell - would either of you like to a gander and see if this is good to go please? |
(hit rebuild on the CI. Failing on Flaky test) |
Build Succeeded 👏 Build Id: 5a07eb6e-25e6-4c63-a61f-8616325b453a 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.
Looks good, slight nit on happy path indentation and would add a slight bit more to the docs.
else | ||
{ | ||
UE_LOG(LogAgonesHook, Error, TEXT("Failed to send reserve request, error serializing duration (%d)"), Seconds); | ||
} |
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.
dunno the style for cpp well but would prefer to return early in the conditional with this one
eg. if the duration doesnt parse return and log ... if it does then normal flow no ident sendrequest
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.
don't believe unreal has a specific guideline for this but agree it looks better, will change
- Request Retry Limit. Maximum number of times a failed request to the Agones sidecar is retried. Health requests are not retried. (default: `30`) | ||
- Send Ready at Startup. Automatically send a Ready request when the server starts. Disable this to manually control when the game server should be marked as ready. (default: `true`) |
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 tend to agree, it might be worth adding a paragraph on how these mehtods / structs are available to be set from within UE4Editor?
- Request Retry Limit. Maximum number of times a failed request to the Agones sidecar is retried. Health requests are not retried. (default: `30`) | ||
- Send Ready at Startup. Automatically send a Ready request when the server starts. Disable this to manually control when the game server should be marked as ready. (default: `true`) |
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.
Might also be worth adding a paragraph around using this locally ... eg. how we can turn off the healthcheck/ready checks when just launching via UE4Editor that makes it easier for most GameDevs to iterate fast before pushing into our clusters.
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.
All looks good to me 👍
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.
Let's do this!
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: domgreen, 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 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: domgreen, 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 |
Build Succeeded 👏 Build Id: f8dee534-1b97-42da-9a16-0203a61c6d1a 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:
|
…oogleforgames#1345) * Add Allocate,Reserve, remove debug log setting, add auto startup setting * Return early * Add usage docs Co-authored-by: Mark Mandel <mark.mandel@gmail.com>
Adds the
Allocate
andReserve
methods to the Unreal plugin, and an option to configure whetherReady
should be sent automatically at startup.Also removed the debug logging option in the settings following up from #1303 (comment) to be more consistent with UnrealEngine logging behaviour (and changed the default verbosity to Verbose)