Changeset View
Standalone View
source/rlinterface/RLInterface.h
| Show All 12 Lines | |||||
| * | * | ||||
| * 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" | ||||
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 { | 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? | |||||
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` | |||||
| bool saveReplay; | bool saveReplay; | ||||
| player_id_t playerID; | player_id_t playerID; | ||||
Done Inline ActionsBraces on new line. vladislavbelov: Braces on new line. | |||||
| std::string content; | std::string content; | ||||
| }; | }; | ||||
| struct Command { | struct RLGameCommand | ||||
| { | |||||
| int playerID; | int playerID; | ||||
| std::string json_cmd; | std::string json_cmd; | ||||
| }; | }; | ||||
| enum GameMessageType { Reset, Commands }; | enum class GameMessageType { Reset, Commands }; | ||||
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 { | struct GameMessage { | ||||
Done Inline ActionsBraces on new line. vladislavbelov: Braces on new line. | |||||
| GameMessageType type; | GameMessageType type; | ||||
| std::vector<Command> commands; | std::vector<RLGameCommand> commands; | ||||
| }; | }; | ||||
| extern void EndGame(); | extern void EndGame(); | ||||
| struct mg_context; | struct mg_context; | ||||
| const static std::string EMPTY_STATE; | |||||
| /** | |||||
| * 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. | |||||
| */ | |||||
| class RLInterface | class RLInterface | ||||
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` | |||||
| { | { | ||||
| public: | public: | ||||
| std::string Step(const std::vector<Command> commands); | std::string Step(const std::vector<RLGameCommand>& commands); | ||||
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). | |||||
| std::string Reset(const ScenarioConfig* scenario); | std::string Reset(const ScenarioConfig* scenario); | ||||
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… | |||||
| std::vector<std::string> GetTemplates(const std::vector<std::string> names) const; | std::vector<std::string> GetTemplates(const std::vector<std::string>& names) const; | ||||
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… | |||||
| void EnableHTTP(const char* server_address); | void EnableHTTP(const char* server_address); | ||||
Not Done Inline ActionsWhy const char* and not std::string or CStr? vladislavbelov: Why `const char*` and not `std::string` or `CStr`? | |||||
| std::string SendGameMessage(const GameMessage msg); | std::string SendGameMessage(const GameMessage& msg); | ||||
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! | |||||
| bool TryGetGameMessage(GameMessage& msg); | bool TryGetGameMessage(GameMessage& msg); | ||||
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. | |||||
| void TryApplyMessage(); | void TryApplyMessage(); | ||||
| std::string GetGameState(); | std::string GetGameState() const; | ||||
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. | |||||
| bool IsGameRunning(); | bool IsGameRunning() const; | ||||
| private: | private: | ||||
| mg_context* m_MgContext = nullptr; | mg_context* m_MgContext = nullptr; | ||||
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. | |||||
| const GameMessage* m_GameMessage = nullptr; | const GameMessage* m_GameMessage = nullptr; | ||||
| std::string m_GameState; | std::string m_GameState; | ||||
| bool m_NeedsGameState = false; | bool m_NeedsGameState = false; | ||||
| mutable std::mutex m_lock; | mutable std::mutex m_Lock; | ||||
| std::mutex m_msgLock; | std::mutex m_MsgLock; | ||||
| std::condition_variable m_msgApplied; | std::condition_variable m_MsgApplied; | ||||
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). | |||||
| ScenarioConfig m_ScenarioConfig; | ScenarioConfig m_ScenarioConfig; | ||||
| }; | }; | ||||
| extern RLInterface* g_RLInterface; | extern RLInterface* g_RLInterface; | ||||
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… | |||||
| #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.