-
Notifications
You must be signed in to change notification settings - Fork 813
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 plugin WatchGameServer implementation #2064
Unreal plugin WatchGameServer implementation #2064
Conversation
Build Succeeded 👏 Build Id: b1c71a75-68a9-4c4c-955c-03e1e7878907 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.
Overall looks great really like thjat its being done with websockets 👍 this has been something on my backlog to look at for a while (but the long polling approach) so this is fantastic.
Main comments are around splitting the keep alive of the socket connection and the Health
method feel that these are two seperate conecrns so should be dealt with seperartly.
I would also suggest turning the WatchGameServer
to pass in a delegate that is called when the gs is updated. This can then tidy up the above and also only set up the ws if peopel explicitly want to watch for state changes.
OOI - which versions of unreal have you tested this with? It compiles on 4.25.4
👍
@domgreen: 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. |
Noticed in the build script your on
We should look at getting a test on this at |
Thanks for the comments! I'll look into making some of the changes you suggested. BTW re 4.26: this exact code is used in a live game on 4.26 so it definitely compiles there too, but I'll see if I can scrounge up some drive space to do a test build of it on 4.26 binary. |
99286bd
to
e3fe572
Compare
Build Succeeded 👏 Build Id: 132749aa-ae87-4be7-89c6-5c099cb799dd 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 Failed 😱 Build Id: 0ac550ec-57fd-4480-850f-e58f0f780d30 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
1 similar comment
Build Failed 😱 Build Id: 0ac550ec-57fd-4480-850f-e58f0f780d30 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
@domgreen Changes made as requested, plus a few more! Let me know if that meets with your approval. I originally used a multicast delegate so I could bind/unbind from watching, but if we're copying the method of the Go SDK, you can only bind and never unbind from watching. It doesn't matter though, since in practice I bind a watcher once and then never touch it again. Also, here's a build log from a fresh 4.26 project to confirm that it builds there:
|
Build Succeeded 👏 Build Id: 7376d97d-8841-4558-9711-16369bf5afd1 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:
|
e3fe572
to
cf973be
Compare
Build Succeeded 👏 Build Id: 2612a7b9-ad3a-467b-8aa3-5e7433fc0fa6 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 fantastic! Great stuff, happy for this to go in (and I'll start using it soon 👍 )
Have 2 small nits but not a blocker.
@domgreen: 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. |
cf973be
to
75779ca
Compare
Build Succeeded 👏 Build Id: bfc8a10f-c4aa-4a4f-b463-07bd86173e49 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 @pooneh-m |
The only question I have is - should this also update https://agones.dev/site/docs/guides/client-sdks/unreal/#sdk-functionality ? (Probably with appropriate https://agones.dev/site/docs/contribute/#at-the-page-level wrappers) |
I can update my branch to show it's implemted then merge straight after? |
Or cherry pick commits over update the cross to a tick and merge all together 😉 |
Actually, should I remove this part too?
Or flag it as Also, how do I flag a table row as versioned? |
That's a good catch, I missed that. Ah yes - wrap it in a A single table row tends to be a pain :/ easy way - copy the entire table into one expiring
|
Build Succeeded 👏 Build Id: 972f164f-cd3d-4f7b-b99b-bb86a3d2dd00 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:
|
210c98f
to
bda08b9
Compare
Alright, all updated. We should be good to go @markmandel ! @domgreen : I actually set the entire table to expire after 1.15 is released since the client would be feature-complete at that point and there wouldn't be a need to have a table of features that are implemented. Also if you need a table generator in the future, I highly recommend the one I linked - I use it extensively, even for stuff like making fixed width data tables to paste into code blocks in chats. |
Build Succeeded 👏 Build Id: 681396a9-c1cd-424b-8b2d-52e897ef53ab 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: 72886643-aeea-4872-b5ab-a542155961d6 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:
|
@highlyunavailable I think we might want to keep the table in, I'm currently going through documenting the other SDKs to show what they have implemented. Keeping this consistent even if full of ticks might be a good things. @markmandel WDYT? Code all looks good thought 🎉 🥳 |
Ahhh if that's the plan then I shall re-add and let you tweak as necessary. Table has been re-added |
bda08b9
to
566ea32
Compare
Build Succeeded 👏 Build Id: 37d5a1e0-2516-4dff-8d80-80d31d0dc7bd 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:
|
LGTM! @domgreen any issues on your end? |
@markmandel all good 👍 Exciting to be feature complete. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: domgreen, highlyunavailable, markmandel 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 |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: aa864128-a466-4a81-a536-85d45b1fd640 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:
|
What type of PR is this?
/kind feature
What this PR does / Why we need it:
This PR adds the WatchGameServer functionality to the Unreal plugin. It allows Unreal-based games to react immediately to Agones state changes instead of polling.
Also corrects some docs around the WatchGameServer websocket endpoint.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Build log from Unreal for CI purposes.
Build log: