[UrealSDK] Creating requests should work in all versions of UE4 #1944

domgreen · 2021-01-06T11:49:15Z

What type of PR is this?
/kind hotfix

What this PR does / Why we need it:

We are replacing the usage if TSharedRef<IHttpRequest> with FHttpRequestRef to allow it to work across versions.

It seems that this basically a typedef under the hood to make this work nicely for the current versions.

Which issue(s) this PR fixes:
Closes #1940

Special notes for your reviewer:

Have tested with 4.25.1
Need to have someone test with 4.26 prior to merging.

agones-bot · 2021-01-06T12:40:15Z

Build Failed 😱

Build Id: 605939e3-5c9f-4e62-9b3b-270f0dd44006

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

domgreen · 2021-01-06T14:02:38Z

^ had a look at build log seems like the tests failed ... nothing jumped out at me apart from website failing to build:

includes/website.mk:72: recipe for target 'site-test' failed
make[1]: Leaving directory '/workspace/build'
includes/website.mk:65: recipe for target 'hugo-test' failed

aLekSer

Change seems reasonable to me.

domgreen · 2021-01-06T18:03:47Z

Thanks @aLekSer will wait for confirmation of it working wtih 4.26 before asking for a merge

comerford · 2021-01-07T01:55:08Z

good news and bad news - the good news is that the version of the plugin from Dom compiled just fine, the bad news is that so did the previous version. This is either due to the fact that we use a customised version of the engine or it is because we only have limited usage of the plugin so far (the end game state change is not yet wired up for example). Seems like I am a poor tester for this one for now.

domgreen · 2021-01-07T10:05:58Z

Thanks for trying @comerford 👍

As @gamedevix is the original reporter might be best to see if they can see it being fixed with this fork.

domgreen · 2021-01-07T16:45:21Z

As mentioned on #1940 by @Gamedevix , this is due to our return type still being:

TSharedRef<IHttpRequest> UAgonesComponent::BuildAgonesRequest

This explains why it compiles for me and not in 4.26 will muse over it ... ideas welcome 👍

comerford · 2021-01-07T17:41:22Z

@domgreen Does specifying the ESPMode explicitly rather than using auto break older versions?

TSharedRef<IHttpRequest, ESPMode::ThreadSafe> Request = Http->CreateRequest();

domgreen · 2021-01-07T19:31:54Z

So the issue is that in 4.25.1 its:

TSharedRef<IHttpRequest, ESPMode::Fast> Request = Http->CreateRequest();

Meaning that the signature of CreateRequest seems to have changed ... some more digging required 😄

markmandel

Just kicking this to "request changes" so it doesn't get accidentally merged until we can work out a good fix 👍

domgreen · 2021-01-23T14:40:58Z

@Gamedevix @comerford @mbc-audiovisuales would you mind checking this wiht 4.26

agones-bot · 2021-01-23T14:56:42Z

Build Succeeded 👏

Build Id: 7be2a2b2-8f83-4054-9393-d9de6897fa07

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.12.0-ae0ff73
image: gcr.io/agones-images/agones-sdk:1.12.0-ae0ff73
image: gcr.io/agones-images/agones-ping:1.12.0-ae0ff73
Linux C++ SDK (build): agonessdk-1.12.0-ae0ff73-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.12.0-ae0ff73.zip

A preview of the website (the last 30 builds are retained):

https://ae0ff73-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1944/head:pr_1944 && git checkout pr_1944
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.12.0-ae0ff73

highlyunavailable · 2021-02-23T18:17:00Z

This is a required change for me to use Agones with 4.26 - it compiles fine on 4.26.1 and I had to manually patch the Agones SDK with these changes to get it to compile. What is blocking a merge of this? I can help test it on 4.25 and 4.26 if needed.

markmandel · 2021-02-23T18:33:59Z

@domgreen to confirm, but I'm pretty sure that testing on a variety of versions is the blocking issue.

If we can confirm the fix works across versions, I'm good to merge 👍

highlyunavailable · 2021-02-23T18:51:18Z

If we just need a variety of versions I can provide that, given the matrix you need tested. I have 4.25.3 and 4.26.1 are immediately available, but I can try to get more if needed.

Apologies if you already know this as well, but the other method (rather than using auto for the variable type) is to use #IF by doing the following:

#include "Runtime/Launch/Resources/Version.h"

And then wrap the signature or variable assignments in the code:

#if ENGINE_MAJOR_VERSION == 4 && ENGINE_MINOR_VERSION >= 26 && ENGINE_PATCH_VERSION >= 0
    FHttpRequestRef BuildAgonesRequest(FString Path = "", const FHttpVerb Verb = FHttpVerb::Post, const FString Content = "{}");
#else
    TSharedRef<IHttpRequest> BuildAgonesRequest(FString Path = "", const FHttpVerb Verb = FHttpVerb::Post, const FString Content = "{}");
#endif

It's a little messier looking but it removes the need for auto if that's also a blocker and this is the supported Unreal Way to do changes across versions like this and is used in multiple portions of the engine.

domgreen · 2021-02-23T19:02:37Z

@markmandel yeah just waiting on confirmation that it works on 4.26.1 I have it working with 4.25.4 and UDN says this should work with all versions just haven't had confirmation till now👍

@highlyunavailable thank you so much for testing and recommendations

domgreen · 2021-02-23T19:06:18Z

@markmandel i think with this confirmation we are good to merge 👯

highlyunavailable · 2021-02-23T19:08:38Z

yeah just waiting on confirmation that it works on 4.26.1 I have it working with 4.25.4 and UDN says this should work with all versions just haven't had confirmation till now👍

Yep it absolutely does. If you ever need Unreal testing feel free to @ me, I'm an Unreal jockey at the moment and happy to wrangle the CPP.

markmandel · 2021-02-23T19:13:50Z

Sweet. I'll hit that approve button!

google-oss-robot · 2021-02-23T19:14:06Z

New changes are detected. LGTM label has been removed.

google-oss-robot · 2021-02-23T19:14:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, domgreen, 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:

~~OWNERS~~ [aLekSer,markmandel]

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

google-oss-robot · 2021-02-23T19:14:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, domgreen, 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:

~~OWNERS~~ [aLekSer,markmandel]

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

agones-bot · 2021-02-23T19:35:45Z

Build Succeeded 👏

Build Id: 70286345-d156-4efb-bac3-f017e2dd3f14

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.13.0-79a8de3
image: gcr.io/agones-images/agones-sdk:1.13.0-79a8de3
image: gcr.io/agones-images/agones-ping:1.13.0-79a8de3
Linux C++ SDK (build): agonessdk-1.13.0-79a8de3-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.13.0-79a8de3.zip

A preview of the website (the last 30 builds are retained):

https://79a8de3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1944/head:pr_1944 && git checkout pr_1944
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.13.0-79a8de3

google-cla bot added the cla: yes label Jan 6, 2021

google-oss-robot added the size/XS label Jan 6, 2021

google-oss-robot requested review from EricFortin and pooneh-m January 6, 2021 11:49

domgreen mentioned this pull request Jan 6, 2021

Unable to COMPILE after added agones plugin in UE4.26 #1940

Closed

aLekSer approved these changes Jan 6, 2021

View reviewed changes

google-oss-robot assigned aLekSer Jan 6, 2021

google-oss-robot added lgtm approved labels Jan 6, 2021

markmandel requested changes Jan 15, 2021

View reviewed changes

google-oss-robot assigned markmandel Jan 15, 2021

google-oss-robot removed the lgtm label Jan 15, 2021

domgreen added 2 commits January 23, 2021 14:30

Using auto for createrequest

efc2e28

FHttpRequestRef Allows backward compat

ae0ff73

google-oss-robot added size/M and removed size/XS labels Jan 23, 2021

domgreen changed the title ~~Using auto for createrequest~~ [UrealSDK] Creating requests should work in all versions of UE4 Jan 23, 2021

markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jan 26, 2021

Base automatically changed from master to main February 4, 2021 00:09

highlyunavailable mentioned this pull request Feb 23, 2021

Add WebSocket capability to WatchGameServer REST API #1999

Merged

markmandel approved these changes Feb 23, 2021

View reviewed changes

google-oss-robot added the lgtm label Feb 23, 2021

Merge branch 'main' into ue4.26_httprequest

79a8de3

google-oss-robot removed the lgtm label Feb 23, 2021

markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs. and removed feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) labels Feb 23, 2021

markmandel added this to the 1.13.0 milestone Feb 23, 2021

markmandel merged commit da4d592 into googleforgames:main Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UrealSDK] Creating requests should work in all versions of UE4 #1944

[UrealSDK] Creating requests should work in all versions of UE4 #1944

domgreen commented Jan 6, 2021 •

edited

Loading

agones-bot commented Jan 6, 2021

domgreen commented Jan 6, 2021

aLekSer left a comment

domgreen commented Jan 6, 2021

comerford commented Jan 7, 2021

domgreen commented Jan 7, 2021

domgreen commented Jan 7, 2021

comerford commented Jan 7, 2021

domgreen commented Jan 7, 2021

markmandel left a comment

domgreen commented Jan 23, 2021

agones-bot commented Jan 23, 2021

highlyunavailable commented Feb 23, 2021

markmandel commented Feb 23, 2021

highlyunavailable commented Feb 23, 2021

domgreen commented Feb 23, 2021

domgreen commented Feb 23, 2021

highlyunavailable commented Feb 23, 2021

markmandel commented Feb 23, 2021

google-oss-robot commented Feb 23, 2021

google-oss-robot commented Feb 23, 2021

google-oss-robot commented Feb 23, 2021

agones-bot commented Feb 23, 2021

[UrealSDK] Creating requests should work in all versions of UE4 #1944

[UrealSDK] Creating requests should work in all versions of UE4 #1944

Conversation

domgreen commented Jan 6, 2021 • edited Loading

agones-bot commented Jan 6, 2021

domgreen commented Jan 6, 2021

aLekSer left a comment

Choose a reason for hiding this comment

domgreen commented Jan 6, 2021

comerford commented Jan 7, 2021

domgreen commented Jan 7, 2021

domgreen commented Jan 7, 2021

comerford commented Jan 7, 2021

domgreen commented Jan 7, 2021

markmandel left a comment

Choose a reason for hiding this comment

domgreen commented Jan 23, 2021

agones-bot commented Jan 23, 2021

highlyunavailable commented Feb 23, 2021

markmandel commented Feb 23, 2021

highlyunavailable commented Feb 23, 2021

domgreen commented Feb 23, 2021

domgreen commented Feb 23, 2021

highlyunavailable commented Feb 23, 2021

markmandel commented Feb 23, 2021

google-oss-robot commented Feb 23, 2021

google-oss-robot commented Feb 23, 2021

google-oss-robot commented Feb 23, 2021

agones-bot commented Feb 23, 2021

domgreen commented Jan 6, 2021 •

edited

Loading