Changeset View
Standalone View
source/rlinterface/RLInterface.h
| /* Copyright (C) 2020 Wildfire Games. | /* Copyright (C) 2020 Wildfire Games. | ||||
| * This file is part of 0 A.D. | * This file is part of 0 A.D. | ||||
| * | * | ||||
| * 0 A.D. is free software: you can redistribute it and/or modify | * 0 A.D. is free software: you can redistribute it and/or modify | ||||
| * it under the terms of the GNU General Public License as published by | * it under the terms of the GNU General Public License as published by | ||||
| * the Free Software Foundation, either version 2 of the License, or | * the Free Software Foundation, either version 2 of the License, or | ||||
| * (at your option) any later version. | * (at your option) any later version. | ||||
| * | * | ||||
| * 0 A.D. is distributed in the hope that it will be useful, | * 0 A.D. is distributed in the hope that it will be useful, | ||||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||
| * GNU General Public License for more details. | * GNU General Public License for more details. | ||||
| * | * | ||||
| * You should have received a copy of the GNU General Public License | * You should have received a copy of the GNU General Public License | ||||
| * along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. | * along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. | ||||
| */ | */ | ||||
| #ifndef INCLUDED_RLINTERFACE | #ifndef INCLUDED_RLINTERFACE | ||||
| #define INCLUDED_RLINTERFACE | #define INCLUDED_RLINTERFACE | ||||
| #include "simulation2/helpers/Player.h" | #include "simulation2/helpers/Player.h" | ||||
| #include "third_party/mongoose/mongoose.h" | |||||
wraitii: I've added this back because it makes it possible to put the mongoose callback in the class… | |||||
| #include <condition_variable> | #include <condition_variable> | ||||
| #include <mutex> | #include <mutex> | ||||
| #include <string> | |||||
| #include <vector> | #include <vector> | ||||
| struct ScenarioConfig { | namespace RL | ||||
| { | |||||
Not Done Inline Actionsextern void EndGame(); shouldn't be in the header. It should be in the implementation on in the GameSetup header. vladislavbelov: `extern void EndGame();` shouldn't be in the header. It should be in the implementation on in… | |||||
Done Inline ActionsI moved this to source/ps/GameSetup/GameSetup.h irishninja: I moved this to `source/ps/GameSetup/GameSetup.h` | |||||
| struct ScenarioConfig | |||||
| { | |||||
Not Done Inline ActionsScenarioConfig is related only to RL. Maybe use namespace instead? vladislavbelov: `ScenarioConfig` is related only to RL. Maybe use namespace instead? | |||||
Done Inline ActionsBraces on new line. vladislavbelov: Braces on new line. | |||||
| bool saveReplay; | bool saveReplay; | ||||
| player_id_t playerID; | player_id_t playerID; | ||||
| std::string content; | std::string content; | ||||
| }; | }; | ||||
| struct Command { | |||||
| struct GameCommand | |||||
| { | |||||
| int playerID; | int playerID; | ||||
| std::string json_cmd; | std::string json_cmd; | ||||
| }; | }; | ||||
| enum GameMessageType { Reset, Commands }; | enum class GameMessageType | ||||
Done Inline ActionsNo need to have all enums in the one line. vladislavbelov: No need to have all enums in the one line. | |||||
Done Inline ActionsGameMessageType is also related only to RL. vladislavbelov: `GameMessageType` is also related only to RL. | |||||
| struct GameMessage { | { | ||||
| None, | |||||
| Reset, | |||||
| Commands, | |||||
| }; | |||||
| /** | |||||
| * Holds messages from the RL client to the game. | |||||
| */ | |||||
| struct GameMessage | |||||
| { | |||||
Done Inline ActionsBraces on new line. vladislavbelov: Braces on new line. | |||||
| GameMessageType type; | GameMessageType type; | ||||
| std::vector<Command> commands; | std::vector<GameCommand> commands; | ||||
| }; | }; | ||||
| extern void EndGame(); | /** | ||||
| * Implements an interface providing fundamental capabilities required for reinforcement | |||||
| * learning (over HTTP). | |||||
| * | |||||
| * This consists of enabling an external script to configure the scenario (via Reset) and | |||||
| * then step the game engine manually and apply player actions (via Step). The interface | |||||
| * also supports querying unit templates to provide information about max health and other | |||||
| * potentially relevant game state information. | |||||
| * | |||||
| * See source/tools/rlclient/ for the external client code. | |||||
| * | |||||
| * The HTTP server is threaded. | |||||
| * Flow of data (with the interface active): | |||||
| * 0. The game/main thread calls TryApplyMessage() | |||||
| * - If no messages are pending, GOTO 0 (the simulation is not advanced). | |||||
| * 1. TryApplyMessage locks m_MsgLock, pulls the message, processes it, advances the simulation, and sets m_GameState. | |||||
| * 2. TryApplyMessage notifies the RL thread that it can carry on and unlocks m_MsgLock. The main thread carries on frame rendering and goes back to 0. | |||||
| * 3. The RL thread locks m_MsgLock, reads m_GameState, unlocks m_MsgLock, and sends the gamestate as HTTP Response to the RL client. | |||||
| * 4. The client processes the response and ultimately sends a new HTTP message to the RL Interface. | |||||
| * 5. The RL thread locks m_MsgLock, pushes the message, and starts waiting on the game/main thread to notify it (step 2). | |||||
| * - GOTO 0. | |||||
| */ | |||||
| class Interface | |||||
Not Done Inline ActionsAre objects of this class supposed to be copied? vladislavbelov: Are objects of this class supposed to be copied? | |||||
Done Inline ActionsThis was not adressed. See NONCOPYABLE(className) in source/lib/code_annotation.h:217 Stan: This was not adressed. See NONCOPYABLE(className) in `source/lib/code_annotation.h:217` | |||||
| { | |||||
| NONCOPYABLE(Interface); | |||||
| public: | |||||
| Interface(const char* server_address); | |||||
| struct mg_context; | /** | ||||
Not Done Inline ActionsThis Step in which space? Simulation, real time or something else? vladislavbelov: This `Step` in which space? Simulation, real time or something else? | |||||
Done Inline ActionsThis is stepping the simulation (stepping the game engine is exposed via the HTTP endpoint). irishninja: This is stepping the simulation (stepping the game engine is exposed via the HTTP endpoint). | |||||
Not Done Inline ActionsWhat does this method do? vladislavbelov: What does this method do? | |||||
Done Inline ActionsThis applies any existing message (ie, instance of GameMessage) which has been received by the RL interface to the current game (or resets the game). irishninja: This applies any existing message (ie, instance of `GameMessage`) which has been received by… | |||||
Not Done Inline ActionsIt's unclear where does it get the message from. vladislavbelov: It's unclear where does it get the message from. | |||||
Not Done Inline ActionsWhy const ScenarioConfig* and not const ScenarioConfig& here? Especially in case you formally "move" it to the method. vladislavbelov: Why `const ScenarioConfig*` and not `const ScenarioConfig&` here? Especially in case you… | |||||
Not Done Inline ActionsWhy const char* and not std::string or CStr? vladislavbelov: Why `const char*` and not `std::string` or `CStr`? | |||||
Not Done Inline ActionsI'd like to add some empty lines to separate members with a similar context. vladislavbelov: I'd like to add some empty lines to separate members with a similar context. | |||||
Not Done Inline ActionsIs it lock for all methods? vladislavbelov: Is it lock for all methods? | |||||
Done Inline ActionsYeah, at least it is for all methods used by the HTTP endpoint (ie, Step, Reset, GetTemplates). irishninja: Yeah, at least it is for all methods used by the HTTP endpoint (ie, Step, Reset, GetTemplates). | |||||
Not Done Inline ActionsShould it be public? vladislavbelov: Should it be public? | |||||
Done Inline ActionsNope. Fixed this locally and will update soon. irishninja: Nope. Fixed this locally and will update soon. | |||||
Not Done Inline ActionsShould it be public? vladislavbelov: Should it be public? | |||||
Done Inline ActionsNope. Thanks for the catch! irishninja: Nope. Thanks for the catch! | |||||
Not Done Inline ActionsShouldn'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. wraitii: Shouldn't this just be done in the constructor? It seems enabling the RL interface without… | |||||
Done Inline ActionsYeah, 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. irishninja: Yeah, I am happy to do that. I was following the convention from Profiler2 though it isn't… | |||||
| const static std::string EMPTY_STATE; | * Non-blocking call to process any pending messages from the RL client. | ||||
| * Updates m_GameState to the gamestate after messages have been processed. | |||||
| */ | |||||
| void TryApplyMessage(); | |||||
Not Done Inline ActionsYour call, but this might be better named ProcessPendingMessage or PollMessage perhaps( to parallel the SDL convention of SDL_PollEvent). wraitii: Your call, but this might be better named `ProcessPendingMessage` or `PollMessage` perhaps( to… | |||||
Not Done Inline ActionsI 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. irishninja: I was calling it `TryApplyMessage` to (loosely) follow the convention from buffered channels in… | |||||
Not Done Inline ActionsI 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. wraitii: I see. I definitely didn't see that parallel :P
IMO it's not really that fitting as a… | |||||
Done Inline ActionsSo, 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. :) ) irishninja: So, the reason why blocking vs non-blocking mattered was when running w/ a GUI since you may… | |||||
Not Done Inline ActionsYes 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 wraitii: Yes I understand that actually, my comment was more that, while it is useful, it isn't required… | |||||
| class RLInterface | private: | ||||
| { | static void* MgCallback(mg_event event, struct mg_connection *conn, const struct mg_request_info *request_info); | ||||
| public: | /** | ||||
| * Process commands, update the simulation by one turn. | |||||
| * @return the gamestate after processing commands. | |||||
| */ | |||||
| std::string Step(std::vector<GameCommand>&& commands); | |||||
| /** | |||||
| * Reset the game state according to scenario, cleaning up existing games if required. | |||||
| * @return the gamestate after resetting. | |||||
| */ | |||||
| std::string Reset(ScenarioConfig&& scenario); | |||||
| /** | |||||
| * @return template data for all templates of @param names. | |||||
| */ | |||||
| std::vector<std::string> GetTemplates(const std::vector<std::string>& names) const; | |||||
| std::string Step(const std::vector<Command> commands); | /** | ||||
| std::string Reset(const ScenarioConfig* scenario); | * @return true if a game is currently running. | ||||
| std::vector<std::string> GetTemplates(const std::vector<std::string> names) const; | */ | ||||
| bool IsGameRunning() const; | |||||
| void EnableHTTP(const char* server_address); | /** | ||||
| std::string SendGameMessage(const GameMessage msg); | * Internal helper. Move @param msg into m_GameMessage, wait until it has been processed by the main thread, | ||||
| * and @return the gamestate after that message is processed. | |||||
| * It is invalid to call this if m_GameMessage is not currently empty. | |||||
| */ | |||||
| std::string SendGameMessage(GameMessage&& msg); | |||||
| /** | |||||
| * Internal helper. | |||||
| * @return true if m_GameMessage is not empty, and updates @param msg, false otherwise (msg is then unchanged). | |||||
| */ | |||||
| bool TryGetGameMessage(GameMessage& msg); | bool TryGetGameMessage(GameMessage& msg); | ||||
| void TryApplyMessage(); | |||||
| std::string GetGameState(); | /** | ||||
| bool IsGameRunning(); | * Process any pending messages from the RL client. | ||||
| * Updates m_GameState to the gamestate after messages have been processed. | |||||
| */ | |||||
| void ApplyMessage(const GameMessage& msg); | |||||
| /** | |||||
| * @return the full gamestate as a JSON strong. | |||||
| * This uses the AI representation since it is readily available in the JS Engine. | |||||
| */ | |||||
| std::string GetGameState() const; | |||||
| private: | private: | ||||
| mg_context* m_MgContext = nullptr; | GameMessage m_GameMessage; | ||||
| const GameMessage* m_GameMessage = nullptr; | ScenarioConfig m_ScenarioConfig; | ||||
| std::string m_GameState; | std::string m_GameState; | ||||
| bool m_NeedsGameState = false; | bool m_NeedsGameState = false; | ||||
| mutable std::mutex m_lock; | |||||
| std::mutex m_msgLock; | mutable std::mutex m_Lock; | ||||
| std::condition_variable m_msgApplied; | std::mutex m_MsgLock; | ||||
| ScenarioConfig m_ScenarioConfig; | std::condition_variable m_MsgApplied; | ||||
| }; | }; | ||||
| extern RLInterface* g_RLInterface; | } | ||||
| extern std::unique_ptr<RL::Interface> g_RLInterface; | |||||
| #endif // INCLUDED_RLINTERFACE | #endif // INCLUDED_RLINTERFACE | ||||
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.