Page MenuHomeWildfire Games

Address concerns in rP23917
Needs ReviewPublic

Authored by irishninja on Aug 10 2020, 6:44 PM.

Details

Summary

This revision is addressing concerns raised by @vladislavbelov from rP23917.

There might be a better name for RLGameCommand but it should at least avoid collisions.

Also, I added a comment explaining the RL Interface at a high level. Let me know if you would like comments for individual methods, more details in the comment, etc.

Test Plan

The existing sample should still work and it should still compile.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
irishninja requested review of this revision.Aug 10 2020, 6:49 PM
Stan added a subscriber: Stan.Aug 10 2020, 7:34 PM
Stan added inline comments.
source/rlinterface/RLInterface.cpp
347

Const & ?

378

const ?

irishninja updated this revision to Diff 13168.Aug 10 2020, 7:44 PM

Address some of Stan's feedback

  • Add extra const's
  • Use reference in for loop
irishninja updated this revision to Diff 13170.Aug 11 2020, 5:16 AM
  • Add more const's
irishninja updated this revision to Diff 13177.Aug 12 2020, 5:19 AM
  • Merge RunRLServer logic into main loop

I just took another pass at it and removed the duplicate logic from RunRLServer. (I believe @vladislavbelov mentioned this over IRC.) I still need to test it when running the GUI but thought I would push it anyway to get feedback. As this reuses more code, it does still include mod installation but I am open to changing it, if preferred!

irishninja updated this revision to Diff 13178.Aug 12 2020, 2:25 PM
  • Small refactor of game loop when nonvisual
source/main.cpp
658

It's better to add braces to the while.

683

Using pure pointers isn't safe, are you sure that it can't be replaced by std::unique_ptr for example?

source/rlinterface/RLInterface.cpp
35–36

Wrong order.

48–49

Do you know what's going on here?

@Itms it's also the question for you.

250

I suppose Enable != Start. Enable doesn't mean that it starts something.

266–267

The brace should be on the new line.

292–293

The brace should be on the new line.

source/rlinterface/RLInterface.h
26–31

ScenarioConfig is related only to RL. Maybe use namespace instead?

41–42

No need to have all enums in the one line.

41–42

GameMessageType is also related only to RL.

54–63

Are objects of this class supposed to be copied?

66–67

This Step in which space? Simulation, real time or something else?

66–67

Why const ScenarioConfig* and not const ScenarioConfig& here? Especially in case you formally "move" it to the method.

71

Why const char* and not std::string or CStr?

71–72

Should it be public?

71–72

Should it be public?

72–73

What does this method do?

80

I'd like to add some empty lines to separate members with a similar context.

85–87

Is it lock for all methods?

irishninja marked 5 inline comments as done.Aug 22 2020, 8:30 PM

I have made most of the changes locally and will be updating the revision soon! I also added a couple inline responses to some of the questions.

source/rlinterface/RLInterface.cpp
48–49

I am setting m_GameMessage to reference msg (which is on the stack). This doesn't result in any issues since m_GameMessage only needs to be available until m_MsgApplied.wait(msgLock) unblocks which certainly is within the lifetime of the previous stack frame (either Step or Reset).

I will probably update this to use an rvalue ref (and use std::move) to make ownership more obvious... Thoughts?

250

This was following the convention used in CProfiler2 but I don't have a preference myself either way. Let me know if you would like it to be changed to something like StartHTTP

source/rlinterface/RLInterface.h
66–67

This is stepping the simulation (stepping the game engine is exposed via the HTTP endpoint).

71–72

Nope. Thanks for the catch!

71–72

Nope. Fixed this locally and will update soon.

72–73

This applies any existing message (ie, instance of GameMessage) which has been received by the RL interface to the current game (or resets the game).

85–87

Yeah, at least it is for all methods used by the HTTP endpoint (ie, Step, Reset, GetTemplates).

irishninja updated this revision to Diff 13289.EditedTue, Aug 25, 6:50 PM

Switch to smart pointer for g_RLInterface and use rvalue refs to make ownership more clear in SendGameMessage. Clean up RLInterface

irishninja updated this revision to Diff 13397.Fri, Sep 4, 4:47 PM
  • add missing "new"

@vladislavbelov, @Itms - I still need to add a namespace or prefix the more general names (such as GameMessageType) but otherwise this should be ready for review! Let me know if there are any other questions/comments!

irishninja updated this revision to Diff 13400.Fri, Sep 4, 5:49 PM
  • Add RL namespace
vladislavbelov added inline comments.Sun, Sep 6, 12:30 PM
source/main.cpp
661

What's about the visual case?

source/rlinterface/RLInterface.cpp
40

You don't need to add RL for each method, you can just wrap the whole code in namespace RL.

48–49

Yes, and that should be avoided. We shouldn't store pointers/references to local variables as class members until we explicitly guarantee the lifetime.

80–81

Additional variables isn't needed here, it doesn't give more information about what's going on here and the lines are pretty short.

source/rlinterface/RLInterface.h
29

Braces on new line.

vladislavbelov added inline comments.Sun, Sep 6, 12:41 PM
source/rlinterface/RLInterface.h
27

extern void EndGame(); shouldn't be in the header. It should be in the implementation on in the GameSetup header.

irishninja marked 3 inline comments as done.Mon, Sep 7, 2:20 AM

Thanks for the review! I am pushing an updated version now.

source/main.cpp
661

In the visual case, the game messages are applied in the Frame function similar to when the game simulation step is called when running w/o --rl-interface (should be line 397)

source/rlinterface/RLInterface.cpp
40

Yeah, that would've been a good idea to make it cleaner. Updated now.

source/rlinterface/RLInterface.h
27

I moved this to source/ps/GameSetup/GameSetup.h

irishninja updated this revision to Diff 13445.Mon, Sep 7, 2:20 AM
irishninja marked an inline comment as done.
  • Formatting fixes, code cleanup
Vulcan added a comment.Mon, Sep 7, 2:28 AM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1519/display/redirect

Stan added inline comments.Mon, Sep 7, 7:35 AM
source/ps/GameSetup/GameSetup.h
28

Add newline between the two

irishninja updated this revision to Diff 13446.Mon, Sep 7, 4:44 PM
  • Add newline (and reorder)
Vulcan added a comment.Mon, Sep 7, 4:57 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1520/display/redirect

source/rlinterface/RLInterface.cpp
48–49

It should be solved.

140–141

What's happening if the val is incorrect integer?

141–142

Incorrect type of std::unique_ptr<char>, it should be std::unique_ptr<char[]>.

source/rlinterface/RLInterface.h
42–49

Braces on new line.

72–73

It's unclear where does it get the message from.

irishninja updated this revision to Diff 13484.Sat, Sep 12, 5:46 PM
irishninja marked 2 inline comments as done.
  • Update SendGameMessage to rvalue ref. Fix char types
irishninja added inline comments.Sat, Sep 12, 5:47 PM
source/rlinterface/RLInterface.cpp
48–49

My bad. I forgot to make it an rvalue ref here, too.

140–141

Hmm... I was assuming that it was a properly formed HTTP request. Would you like me to be more pessimistic about malformed HTTP requests?

So far, I haven't worried too much about it since "Content-Length" is often handled by http request libraries and the RL interface requires both a flag to be set and then only accepts requests from localhost (unless you change the address to 0.0.0.0). I also figured that there wasn't much motivation for malicious agents to attack the game via the RL interface as it is likely being used for development of an AI rather than a hosting the game engine as a server of sorts. (In that context, I had actually imagined there would still be an orchestrator of sorts collecting and applying the commands as all actions at a given step are part of a single call to Step - and it likely wouldn't make sense for both players to control the stepping of the game arbitrarily.)

Regardless, I can certainly update it if preferred. Let me know what you think!

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1545/display/redirect

source/rlinterface/RLInterface.cpp
47

r-value doesn't make things easier here.

48–49

The issue is still here. You're storying reference to a local variable.

  • Move game message ownership correctly

Just updated the code again to move the ownership of the message in SendGameMessage to the member variable. Hopefully this addresses the outstanding issue.

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1546/display/redirect