Skip to content
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

Add dynamic configuration of the sidecar http port to the unreal sdk. #1131

Merged
merged 1 commit into from
Oct 22, 2019

Conversation

roberthbailey
Copy link
Member

@roberthbailey roberthbailey commented Oct 22, 2019

So I've been banging my head against UE4 today. The new code certainly does the right thing when the environment variable isn't set. But I can't convince my system to set the environment so that UE4 will actually see any values when the game is run.

It turns out that cross compilation only works on Windows, so I can't compile a game to Linux to try and run it in a docker container (which is how I've tested setting environment variables for the other SDKs). And no matter what tricks I try, it always returns empty string when the example game I'm using is run on my mac.

So... this might work. Based on the documentation, it should work, but without being able to actually test it I can't be sure if it does.

Part of #851.

@roberthbailey
Copy link
Member Author

/cc @YannickLange

@google-oss-robot
Copy link

@roberthbailey: GitHub didn't allow me to request PR reviews from the following users: YannickLange.

Note that only googleforgames members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @YannickLange

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.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 764bf9f5-193e-46c8-bd93-f4b16d40dca9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7a60670a-ec10-4176-a26e-c9771c5846e9

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1131/head:pr_1131 && git checkout pr_1131
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-913499b

@@ -34,9 +34,6 @@ Available settings:
{{% feature expiryVersion="1.1.0" %}}
- Agones Sidecar IP. (default: `http://localhost:59358`)
{{% /feature %}}
{{% feature publishVersion="1.1.0" %}}
- Agones Sidecar IP. (default: `http://localhost:${AGONES_SDK_HTTP_PORT}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I've removed this as a setting in the unreal editor. The code no longer uses the setting (and the setting no longer exists) so it should not be documented.

I've previously changed it to reference the environment variable (even though I hadn't updated the code) but as I looked at the code I realized it didn't make sense as a setting.

@pooneh-m
Copy link
Contributor

Please also get a second approval from someone familiar with unreal SDK. The change for setting default port looks good to me.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a1c19cdb-60a9-430e-b71d-1b48b5443ce0

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1131/head:pr_1131 && git checkout pr_1131
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-38c7061

@roberthbailey
Copy link
Member Author

I figured out why I couldn't get this to work yesterday.

FGenericPlatformMisc::GetEnvironmentVariable(...) always returns an empty string (return FString();) rather than reading from the environment. Changing it to FPlatformMisc::GetEnvironmentVariable(...) actually reads the environment and the plugin now does the right thing.

I've been able to test locally that I connect to different ports by setting different environment variables in a sample game so I'm much more confident about this change now.

@markmandel markmandel added this to the 1.1.0 milestone Oct 22, 2019
@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, pooneh-m, roberthbailey

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:
  • OWNERS [markmandel,pooneh-m,roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 0133d178-7279-4836-b3d1-9691cd86a6e2

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1131/head:pr_1131 && git checkout pr_1131
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-30350be

@markmandel markmandel merged commit 6c179c3 into googleforgames:master Oct 22, 2019
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Oct 22, 2019
@roberthbailey roberthbailey deleted the unreal-sdk branch August 24, 2020 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones lgtm size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants