Now it should be cleaned up! Let me know if there are any other issues! I still would like to add richer error handling but this can wait for a later revision!

Rename m_GameState and remove unused evaluate test

I updated the revision but must have made a mistake bc it included other file changes... I will try to fix it...

Rename m_GameState and remove unused evaluate test

Thanks, @wraitii. Bummer about not being able to use optional. I updated it!

• Remove std::optional for OSX support
• Fixed string initialization

• Refactor reading HTTP request content logic

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

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

Fix minor C++ issues around type and unique_ptr creation

• Refactor TryApplyMessage; decrease thread count

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

• Make non-copyable; start http server in ctor

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

• Move game message ownership correctly

• Update SendGameMessage to rvalue ref. Fix char types

• Add newline (and reorder)

• Formatting fixes, code cleanup

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

• Add RL namespace

@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!

• add missing "new"

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

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.

• Small refactor of game loop when nonvisual

I removed the mod installation from RunRLServer then realized it was pretty easy to decrease the code duplication from that method (as @vladislavbelov suggested over irc, iirc). Removing the code duplication resulted in keeping the mod installation logic. I am happy to revert if that is preferred; let me know what you think!

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!

• Merge RunRLServer logic into main loop

Yeah, that makes sense. It is probably also unnecessary for the containerized case since you can just mount the files into the container and the environment can still be spun up with a single command.

Makes sense. I admit that I had been thinking that there was no need to change most of the initial startup for the game when running the RL interface so I just left it in.

I have addressed the concerns in D2947. Let me know what you think!

• Add more const's

Address some of Stan's feedback

• Add extra const's
• Use reference in for loop

@Itms, Yeah, I will check those issues out and make a new revision!

Adding the missing include fixes the issue for me. Good catch, @Nescio about the unused includes - those were left over from an earlier version that I forgot to remove :/

Fixed minor style issues

Fixed seg fault due to user exiting the game from the UI when the script is using it, too.

Style fixes mostly from the last round of feedback. I am still digging into the seg fault from earlier. I had a couple questions about some of the earlier comments that I made inline and will update the code after I have a clearer idea of the approach to take!

Updated the code following the review from Itms. I still need to check out the seg fault but everything else should be addressed!

Remove regex, boost parsing. Still running into linking errors so need more testing but wanted to push these updates regardless to get feedback and see how it does in jenkins. (I suspect that the linking issues are specific to my machine...)

After rebasing the changes, I have been seeing the following linking error:

Fix issues in main.cpp and rebased (using the github mirror).

I have made most of the changes. To my knowledge, the only remaining issues are:

• Update GetGameState to use GetEntitiesWithInterface (as described in the earlier comment). I asked a question about this in the earlier comment.
• Use json_spirit to make it less std::string heavy.

Removed sdl linking from premake (only removed the include last time!)

GetTemplates is now const and removed unused include

Miscellaneous code cleanup. Still need to address the GetGameState comments and the use of jsonspirit rather than strings by @wraitii.

Made the miscellaneous code fixes. Pushing them momentarily!

Revert extern_libs5 file.

Fixed various linting/style issues. Rebased to the latest version of 0 AD.

I have made most of the changes and will be updating the revision soon (arcanist is hanging a bit after the rebase). I still need to update the GetGameState function and make it a little less string heavy as mentioned by @wraitii but most other changes should have been applied.

Removed some grpc includes that I missed

Remove old build option, reorder python args, and minor update to the python readme. Python arguments for reset were reordered from reset(config, player_id=1, save_replay=False) to reset(config, save_replay=False, player_id=1) as I suspect it will be more common for users to want to save the replay rather than change the player's ID.

Remove empty requirements.txt file

Updated the diff to use mongoose instead of gRPC and cleaned up python API.

Thanks, @wraitii ! I am happy to have a review (and someone else who shares my excitement for this! :)

Fixed remaining call to to_proto (from old ScenarioConfig logic).

Remove programmatic creation of scenario config/game setup JSON and updated the example.

Added --with-rlinterface to the script for building libraries on OSX (only built fiber if flag exists). Removed a condition which is always false (checking if on windows with the --with-rlinterface flag since it throws an unsupported error message earlier if so).

Added support for OSX (required updates to script for building libraries and premake).

This adds a couple dependencies when building with the --with-rlinterface (passed to ./update-workspaces.sh):

• GRPC (C++ and Python)
  • Protobuf
  • Python support requires grpcio-tools package
• boost fiber
• boost system

Added error when building on unsupported platform (windows). (@Stan mentioned in IRC that windows support was not necessary for merging.)

Updated python dependencies and fixed bug when built w/o the RL interface.

Updated installation/testing instructions in README for the python client and removed the user option for the RL interface address as it must be set on start and may be confusing for users.

Added tests for actions and configuring scenarios. I also removed the extra example since it didn't contain anything new as compared to the better one ;)

As an update, I am currently working on adding tests for the actions and scenario config (using pytest). I have tests for all the actions in zero_ad/actions.py and a number of the different scenario settings. I am currently working on testing random map usage. After finishing the tests, I am planning on training a simple agent as an example demonstrating how it can be used to expose an OpenAI Gym API to 0 AD :) (I will probably do this in an external repo and then just post a link to it.)

Miscellaneous code cleanup per Stan's feedback.

Updates:

• Define ScenarioConfig in Python. Added basic helpers for defining victory conditions as well as more generic setters (set_map_setting, set_game_setting).
• Refactored RL Server main loop to own method
• Changed default build to not include RL interface (uses --with-rlinterface)

Added more actions (construct, gather, train, attack-walk) and an example script showing a lot of these basic actions.

• Added --without-rlinterface compile flag
• Removed generated files
• Added config option for the rpc server address (both to the default.cfg and to the options.json used in-game)

Cleaned up the updates to the SaveReplayMetadata function (mentioned in the prior reviews). I also made the argument for the fn const since it is read-only.

I will be updating the diff with the most recent fixes. Dumb question: when I update the diff, does it add the changes to the existing diff or overwrite it with the most recent diff? I usually use git and GitHub and am still getting used to Phabricator...

Updates:

• Removed duplicate functions for getReplayMetadata to more directly call the fn from the GuiInterface.
  • Updated VisualReplay::SaveReplayMetadata to call GuiInterface directly from the C++. After the code change, I removed the (now unused) ScriptInterface argument from the method signature.
  • Replaced the single invocation of getReplayMetadata with Engine.GuiInterfaceCall("GetReplayMetadata")
• Added the registration of GetInitAttributes which should have been included in the original diff.
• Added myself to the credits

Thanks for the feedback! I added some responses inline and will be updating the diff (hopefully addressing all the remaining issues)!

As @Stan said, there is more discussion on the PR on GitHub. The goal of this contribution is to provide the fundamental underlying capabilities for exploring RL/ML agents in 0 AD (such as an OpenAI gym environment which can actually be implemented on top of this quite easily). I am intentionally not adding any action/observation spaces or reward as expected by OpenAI gym as these are not obvious and there are many different candidates for each of these (this is discussed more on the GitHub thread).

Thanks for the feedback! I can move it to be behind a feature flag and remove the generated files.

Changed NULL to nullptr

I added GetReplayMetadata to the GuiInterface component to reduce code duplication introduced by the original diff.

Thanks for the feedback - I have added my responses inline!