Differential D2947 Diff 13289 source/rlinterface/RLInterface.h

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"

				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 {			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 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`
	bool saveReplay;			bool saveReplay;
	player_id_t playerID;			player_id_t playerID;
				vladislavbelovUnsubmitted Done Inline Actions Braces 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
				};
				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 {			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<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
				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`
	{			{

	public:			public:

	std::string Step(const std::vector<Command> commands);			std::string Step(std::vector<RLGameCommand>&& commands);
				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 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…
	std::string Reset(const ScenarioConfig* scenario);			std::string Reset(ScenarioConfig&& scenario);
	std::vector<std::string> GetTemplates(const std::vector<std::string> names) const;			std::vector<std::string> GetTemplates(const std::vector<std::string>& names) const;
				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…

	void EnableHTTP(const char* server_address);			void EnableHTTP(const char* server_address);
				vladislavbelovUnsubmitted Not Done Inline Actions Why `const char` and not `std::string` or `CStr`? vladislavbelov:* Why `const char*` and not `std::string` or `CStr`?
	std::string SendGameMessage(const GameMessage msg);
	bool TryGetGameMessage(GameMessage& msg);
	void TryApplyMessage();			void TryApplyMessage();
				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 GetGameState();			bool IsGameRunning() const;
				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.
	bool IsGameRunning();

	private:			private:
				std::string SendGameMessage(const GameMessage& msg);
				bool TryGetGameMessage(GameMessage& msg);
				std::string GetGameState() const;

	mg_context* m_MgContext = nullptr;			mg_context* m_MgContext = nullptr;

				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.
	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;
				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).
	ScenarioConfig m_ScenarioConfig;			ScenarioConfig m_ScenarioConfig;
	};			};

	extern RLInterface* g_RLInterface;			extern std::unique_ptr<RLInterface> g_RLInterface;

				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…
	#endif // INCLUDED_RLINTERFACE			#endif // INCLUDED_RLINTERFACE