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 <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 | |||||
{ | |||||
NONCOPYABLE(Interface); | |||||
public: | |||||
Interface(const char* server_address); | |||||
struct mg_context; | /** | ||||
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(); | |||||
class RLInterface | private: | ||||
{ | static void* MgCallback(mg_event event, struct mg_connection *conn, const struct mg_request_info *request_info); | ||||
public: | /** | ||||
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` | |||||
* Process commands, update the simulation by one turn. | |||||
* @return the gamestate after processing commands. | |||||
*/ | |||||
std::string Step(std::vector<GameCommand>&& commands); | |||||
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… | |||||
/** | |||||
* 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); | /** | ||||
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! | |||||
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. | |||||
*/ | |||||
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… | |||||
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.