Differential D2947 Diff 15223 source/rlinterface/RLInterface.h

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"
				wraitiiUnsubmitted Not Done Inline Actions 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. 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
				{
				vladislavbelovUnsubmitted Not Done Inline Actions `extern 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…
				irishninjaAuthorUnsubmitted Done Inline Actions I moved this to `source/ps/GameSetup/GameSetup.h` irishninja: I moved this to `source/ps/GameSetup/GameSetup.h`
				struct ScenarioConfig
				{
				vladislavbelovUnsubmitted Not Done Inline Actions `ScenarioConfig` is related only to RL. Maybe use namespace instead? vladislavbelov: `ScenarioConfig` is related only to RL. Maybe use namespace instead?
				vladislavbelovUnsubmitted Done Inline Actions Braces 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
				vladislavbelovUnsubmitted Done Inline Actions No need to have all enums in the one line. vladislavbelov: No need to have all enums in the one line.
				vladislavbelovUnsubmitted Done Inline Actions `GameMessageType` 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
				{
				vladislavbelovUnsubmitted Done Inline Actions Braces 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:			/**
				vladislavbelovUnsubmitted Not Done Inline Actions Are objects of this class supposed to be copied? vladislavbelov: Are objects of this class supposed to be copied?
				StanUnsubmitted Done Inline Actions This 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);

				wraitiiUnsubmitted Not Done Inline Actions 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. wraitii: Shouldn't this just be done in the constructor? It seems enabling the RL interface without…
				irishninjaAuthorUnsubmitted Done Inline Actions 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. 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);			/**
				vladislavbelovUnsubmitted Not Done Inline Actions This `Step` in which space? Simulation, real time or something else? vladislavbelov: This `Step` in which space? Simulation, real time or something else?
				irishninjaAuthorUnsubmitted Done Inline Actions This 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).
				vladislavbelovUnsubmitted Not Done Inline Actions What does this method do? vladislavbelov: What does this method do?
				irishninjaAuthorUnsubmitted Done Inline Actions This 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…
				vladislavbelovUnsubmitted Not Done Inline Actions It's unclear where does it get the message from. vladislavbelov: It's unclear where does it get the message from.
				vladislavbelovUnsubmitted Not Done Inline Actions Why `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…
				vladislavbelovUnsubmitted Not Done Inline Actions Why `const char` and not `std::string` or `CStr`? vladislavbelov:* Why `const char*` and not `std::string` or `CStr`?
				vladislavbelovUnsubmitted Not Done Inline Actions I'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.
				vladislavbelovUnsubmitted Not Done Inline Actions Is it lock for all methods? vladislavbelov: Is it lock for all methods?
				irishninjaAuthorUnsubmitted Done Inline Actions Yeah, 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).
				vladislavbelovUnsubmitted Not Done Inline Actions Should it be public? vladislavbelov: Should it be public?
				irishninjaAuthorUnsubmitted Done Inline Actions Nope. Fixed this locally and will update soon. irishninja: Nope. Fixed this locally and will update soon.
				vladislavbelovUnsubmitted Not Done Inline Actions Should it be public? vladislavbelov: Should it be public?
				irishninjaAuthorUnsubmitted Done Inline Actions Nope. 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.
				*/
				wraitiiUnsubmitted Not Done Inline Actions Your 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…
				irishninjaAuthorUnsubmitted Not Done Inline Actions 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. irishninja: I was calling it `TryApplyMessage` to (loosely) follow the convention from buffered channels in…
				wraitiiUnsubmitted Not Done Inline Actions 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. wraitii: I see. I definitely didn't see that parallel :P IMO it's not really that fitting as a…
				irishninjaAuthorUnsubmitted Done Inline Actions 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. :) ) irishninja: So, the reason why blocking vs non-blocking mattered was when running w/ a GUI since you may…
				wraitiiUnsubmitted Not Done Inline Actions 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 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