Page MenuHomeWildfire Games

Address concerns in rP23917
ClosedPublic

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 added inline comments.Sep 7 2020, 2:20 AM
source/main.cpp
657

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.Sep 7 2020, 2:20 AM
irishninja marked an inline comment as done.
  • Formatting fixes, code cleanup
Vulcan added a comment.Sep 7 2020, 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.Sep 7 2020, 7:35 AM
source/ps/GameSetup/GameSetup.h
28 ↗(On Diff #13445)

Add newline between the two

irishninja updated this revision to Diff 13446.Sep 7 2020, 4:44 PM
  • Add newline (and reorder)
Vulcan added a comment.Sep 7 2020, 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

It should be solved.

142

What's happening if the val is incorrect integer?

143

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

source/rlinterface/RLInterface.h
39

Braces on new line.

68–69

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

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

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

142

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

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

wraitii updated this revision to Diff 13761.Nov 6 2020, 4:12 PM
wraitii added a subscriber: wraitii.

Minor tweaks + add comments (I didn't know if you were available initially).

I think this looks good but I'll ping Vladislav as well.

wraitii added inline comments.Nov 6 2020, 4:14 PM
source/rlinterface/RLInterface.cpp
142

atoi will return 0 if it can't be converted, which ought to be fairly safe. I think we have to trust mongoose here.

source/rlinterface/RLInterface.h
21

I've added this back because it makes it possible to put the mongoose callback in the class, thus cleaning up the private/public interface split. I don't think it's a huge issue since the header included in two c++ files only.

Vulcan added a comment.Nov 6 2020, 4:16 PM

Build failure - The Moirai have given mortals hearts that can endure.

builderr-debug-gcc6.txt
g++: internal compiler error: Segmentation fault (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
make[1]: *** [obj/gui_Debug/L10n.o] Error 4
make: *** [gui] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3410/display/redirect

Vulcan added a comment.Nov 6 2020, 4:18 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/1758/display/redirect

wraitii added inline comments.Nov 6 2020, 4:19 PM
source/rlinterface/RLInterface.h
63

Shouldn't this just be done in the constructor? It seems enabling the RL interface without actually enabling the HTTP will just prevent you from playing.

84

Your call, but this might be better named ProcessPendingMessage or PollMessage perhaps( to parallel the SDL convention of SDL_PollEvent).

irishninja marked 4 inline comments as done.Nov 10 2020, 3:27 AM
irishninja added inline comments.
source/rlinterface/RLInterface.h
63

Yeah, I am happy to do that. I was following the convention from Profiler2 though it isn't really the best example to follow given that it doesn't rely on EnableHTTP to function.

84

I was calling it TryApplyMessage to (loosely) follow the convention from buffered channels in boost (ie, try_pop) and make it obvious that it was non-blocking. Technically, it should probably return a bool to follow the convention more closely but the return value isn't needed anywhere so it seemed unnecessary.

I'll let you change the EnableHTTP thing, but as said from my end this looks good.

source/rlinterface/RLInterface.h
84

I see. I definitely didn't see that parallel :P

IMO it's not really that fitting as a parallel, and it being obviously non-blocking isn't such a necessity (it could very well be blocking and still be entirely functional, from a "no human input" POV).

Though as I said, I'm not sure my suggestions are much better.

Stan added a comment.Nov 10 2020, 10:18 AM

Just a question, but it seems we use std::string everywhere. Shouldn't we use CStr instead?

source/rlinterface/RLInterface.h
57

This was not adressed. See NONCOPYABLE(className) in source/lib/code_annotation.h:217

irishninja updated this revision to Diff 13837.Nov 11 2020, 3:17 AM
irishninja marked 4 inline comments as done.
  • Make non-copyable; start http server in ctor

Just updated the revision to address some of the remaining comments. Let me know if there are any remaining issues/comments/etc!

Stan - I am using std::string everywhere is because it is referring to the game state which is returned by ScriptInterface.StringifyJSON(&state, false) as a std::string. Eventually these are all converted to c strings but I put that off until later so I can use convenience methods like string.empty(). Happy to change it if it isn't ideal or I am missing something :)

source/rlinterface/RLInterface.cpp
47

My thinking had been that game messages do not need to be used after they are "sent" so I could pass them as an r-value. This should help avoid any additional copying and make it more clear in the code that the message has been "consumed" (so it shouldn't be edited afterwards, etc).

Does that make sense?

48

Adding m_GameMessage = std::move(msg); should fix this here, right? I was thinking that it should help since the variable is moved to the data member and will no longer be freed after it goes out of scope (ie, when the variable frame is popped off the stack). Is this correct or am I missing something?

source/rlinterface/RLInterface.h
84

So, the reason why blocking vs non-blocking mattered was when running w/ a GUI since you may want to pan around and view things while the AI is playing the game. I believe it is a bit choppy and problematic if it was blocking when running with a GUI.

Not to make any argument for the naming - I am happy to defer to whatever the consensus is - but I figured I would mention the significance here :) (Technically, I think I ran into this awhile back when I actually was using boost channels for communicating between the threads. :) )

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

builderr-debug-macos.txt
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/graphics/tests/test_Camera.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/graphics/tests/test_Camera.h:168:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CVector3D(-101.0f, -101.0f, 101.0f),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/simulation2/tests/test_SerializeTemplates.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/simulation2/tests/test_SerializeTemplates.h:39:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        3, 0, 1, 4, 1, 5
                        ^~~~~~~~~~~~~~~~
                        {               }
1 warning generated.
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
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.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/libgraphics.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
In file included from ../../../source/graphics/tests/test_Camera.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/graphics/tests/test_Camera.h:168:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CVector3D(-101.0f, -101.0f, 101.0f),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/simulation2/tests/test_SerializeTemplates.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/simulation2/tests/test_SerializeTemplates.h:39:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        3, 0, 1, 4, 1, 5
                        ^~~~~~~~~~~~~~~~
                        {               }
1 warning generated.

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

wraitii added inline comments.Nov 11 2020, 8:33 AM
source/rlinterface/RLInterface.cpp
47

I think vlad's comment in the context that you kept a reference to that r-value below, which didn't help, but since you've fixed that, I think your reasoning holds up.

source/rlinterface/RLInterface.h
84

Yes I understand that actually, my comment was more that, while it is useful, it isn't required for this to "work". Being blocking would degrade the experience, but isn't a complete deal-breaker. Ergo I'd say it's not a fundamental property of this function that it is non-blocking, just a nice bonus.

Anyways :p

Stan added inline comments.Nov 11 2020, 11:03 AM
source/rlinterface/RLInterface.cpp
50

Do you need 6 threads? AFAIK you're not using a browser?

289

Early return;

292

Invert, early return, to remove indent.

293

Could be a helper function? ApplyMessageMaybe?

391

Maybe g_Game != nullptr? No strong feelings.

irishninja marked 5 inline comments as done.Nov 12 2020, 4:48 AM
irishninja updated this revision to Diff 13864.Nov 12 2020, 4:48 AM
  • Refactor TryApplyMessage; decrease thread count

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/1842/display/redirect

source/main.cpp
483

g_RLInterface = std::make_unique<RL::Interface>(server_address.c_str());, since we have C++17.

source/rlinterface/RLInterface.cpp
40

No need to assign nullptr to a global object.

143–146

Instead of duplicating allocations you might create a string with enough space:

const int contentSize = std::atoi(val);
std::string content(' ', contentSize);
mg_read(conn, content.data(), content.size());
scenario.content = std::move(content);
167–169

The same here.

207–209

And here. It seems a code duplication, might be unified.

395

Might be const float.

irishninja updated this revision to Diff 15223.Jan 13 2021, 2:33 AM

Fix minor C++ issues around type and unique_ptr creation

Thanks for the feedback! I have made a few of the changes and left a question about the last remaining ones.

source/rlinterface/RLInterface.cpp
143–146

Makes sense. However, it looks like content.data() is const and can't be used to set the contents of the string using mg_read. Am I missing something?

irishninja updated this revision to Diff 15279.Jan 14 2021, 2:34 AM

Applying the changes on top of the latest version of 0 AD to hopefully fix the build error.

Build has FAILED

builderr-debug-macos.txt
../../../source/rlinterface/RLInterface.cpp:303:5: error: use of undeclared identifier 'EndGame'
                                EndGame();
                                ^
../../../source/rlinterface/RLInterface.cpp:307:15: error: no viable conversion from 'shared_ptr<ScriptContext>' to 'JSContext *'
                        JSContext* cx = scriptInterface.GetContext();
                                   ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../source/rlinterface/RLInterface.cpp:308:4: error: unknown type name 'JSAutoRequest'
                        JSAutoRequest rq(cx);
                        ^
../../../source/rlinterface/RLInterface.cpp:353:16: error: no viable conversion from 'shared_ptr<ScriptContext>' to 'JSContext *'
                                JSContext* cx = scriptInterface.GetContext();
                                           ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../source/rlinterface/RLInterface.cpp:354:5: error: unknown type name 'JSAuto

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2855/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/macos-differential/2855/display/redirectconsole

Build has FAILED

builderr-debug-gcc7.txt
../../../source/rlinterface/RLInterface.cpp: In member function 'void RL::Interface::ApplyMessage(const RL::GameMessage&)':
../../../source/rlinterface/RLInterface.cpp:303:5: error: 'EndGame' was not declared in this scope
     EndGame();
     ^~~~~~~
../../../source/rlinterface/RLInterface.cpp:303:5: note: suggested alternative: 'g_Game'
     EndGame();
     ^~~~~~~
     g_Game
../../../source/rlinterface/RLInterface.cpp:307:47: error: cannot convert 'std::shared_ptr<ScriptContext>' to 'JSContext*' in initialization
    JSContext* cx = scriptInterface.GetContext();
                                               ^
../../../source/rlinterface/RLInterface.cpp:308:4: error: 'JSAutoRequest' was not declared in this scope
    JSAutoRequest rq(cx);
    ^~~~~~~~~~~~~
../../../source/rlinterface/RLInterface.cpp:308:4: note: suggested alternative: 'JSAutoRealm'
    JSAutoRequest rq(cx);
    ^~~~~~~~~~~~~
    JSAutoRealm
../../../source/rlinterface/RLInterface.cpp:326:47:

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4514/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4514/display/redirectconsole

wraitii updated this revision to Diff 15329.Jan 15 2021, 9:35 AM

This should compile. Will run a docker noPCH build to make sure there's no issues.

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libscriptinterface_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ..

See https://jenkins.wildfiregames.com/job/macos-differential/2871/display/redirect for more details.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 15 2021, 5:30 PM
This revision was automatically updated to reflect the committed changes.