From fc5dfd2873279d5003f8a02187a71b05a44892fa Mon Sep 17 00:00:00 2001
From: Loek Le Blansch <loek@pipeframe.xyz>
Date: Sat, 16 Nov 2024 16:46:18 +0100
Subject: final touchups 2 for #26

---
 src/crepe/ComponentManager.h      | 45 ++++++++++++++++++++++++++++-----------
 src/crepe/api/BehaviorScript.h    | 37 ++++++++++++++++++--------------
 src/crepe/api/BehaviorScript.hpp  |  2 +-
 src/crepe/api/LoopManager.cpp     |  2 --
 src/crepe/api/LoopManager.h       | 26 ++++++++++++++++------
 src/crepe/api/Scene.h             |  8 ++++---
 src/crepe/api/Script.h            | 44 +++++++++++++++++++++-----------------
 src/crepe/api/Script.hpp          |  3 +--
 src/crepe/system/ParticleSystem.h |  2 --
 src/crepe/system/ScriptSystem.cpp |  5 ++---
 10 files changed, 106 insertions(+), 68 deletions(-)

diff --git a/src/crepe/ComponentManager.h b/src/crepe/ComponentManager.h
index 99d45d9..2107453 100644
--- a/src/crepe/ComponentManager.h
+++ b/src/crepe/ComponentManager.h
@@ -1,6 +1,5 @@
 #pragma once
 
-#include <forward_list>
 #include <memory>
 #include <typeindex>
 #include <unordered_map>
@@ -20,11 +19,40 @@ class GameObject;
  * This class manages all components. It provides methods to add, delete and get components.
  */
 class ComponentManager {
+	// TODO: This relation should be removed! I (loek) believe that the scene manager should
+	// create/destroy components because the GameObject's are stored in concrete Scene classes,
+	// which will in turn call GameObject's destructor, which will in turn call
+	// ComponentManager::delete_components_by_id or something. This is a pretty major change, so
+	// here is a comment and temporary fix instead :tada:
+	friend class SceneManager;
+
 public:
 	ComponentManager(); // dbg_trace
 	~ComponentManager(); // dbg_trace
 
+	/**
+	 * \brief Create a new game object using the component manager
+	 *
+	 * \param name Metadata::name (required)
+	 * \param tag Metadata::tag (optional, empty by default)
+	 * \param position Transform::position (optional, origin by default)
+	 * \param rotation Transform::rotation (optional, 0 by default)
+	 * \param scale Transform::scale (optional, 1 by default)
+	 *
+	 * \returns GameObject interface
+	 *
+	 * \note This method automatically assigns a new entity ID
+	 */
+	GameObject new_object(const std::string & name, const std::string & tag = "",
+						  const Vector2 & position = {0, 0}, double rotation = 0,
+						  double scale = 1);
+
 protected:
+	/**
+	 * GameObject is used as an interface to add/remove components, and the game programmer is
+	 * supposed to use it instead of interfacing with the component manager directly.
+	 */
+	friend class GameObject;
 	/**
 	 * \brief Add a component to the ComponentManager
 	 * 
@@ -39,11 +67,6 @@ protected:
 	 */
 	template <typename T, typename... Args>
 	T & add_component(game_object_id_t id, Args &&... args);
-	//! GameObject is used as an interface to add components instead of the
-	// component manager directly
-	friend class GameObject;
-
-public:
 	/**
 	 * \brief Delete all components of a specific type and id
 	 * 
@@ -77,6 +100,8 @@ public:
 	 * This method deletes all components.
 	 */
 	void delete_all_components();
+
+public:
 	/**
 	 * \brief Get all components of a specific type and id
 	 * 
@@ -99,11 +124,6 @@ public:
 	template <typename T>
 	std::vector<std::reference_wrapper<T>> get_components_by_type() const;
 
-	// TODO: doxygen
-	GameObject new_object(const std::string & name, const std::string & tag = "",
-						  const Vector2 & position = {0, 0}, double rotation = 0,
-						  double scale = 0);
-
 private:
 	/**
 	 * \brief The components
@@ -118,9 +138,8 @@ private:
 	std::unordered_map<std::type_index, std::vector<std::vector<std::unique_ptr<Component>>>>
 		components;
 
-	//! ID of next GameObject
+	//! ID of next GameObject allocated by \c ComponentManager::new_object
 	game_object_id_t next_id = 0;
-	std::forward_list<std::unique_ptr<GameObject>> objects;
 };
 
 } // namespace crepe
diff --git a/src/crepe/api/BehaviorScript.h b/src/crepe/api/BehaviorScript.h
index f7d484a..9d85d4c 100644
--- a/src/crepe/api/BehaviorScript.h
+++ b/src/crepe/api/BehaviorScript.h
@@ -15,12 +15,21 @@ class Script;
 /**
  * \brief Script component
  *
- * This class acts as a (component) wrapper around an instance of (a class
- * derivatived from) \c Script. \c BehaviorScript is the only ECS component
- * that stores member function implementations as data.
+ * This class acts as a (component) wrapper around an instance of (a class derivatived from) \c
+ * Script. \c BehaviorScript is the only ECS component that stores member function
+ * implementations as data.
  */
 class BehaviorScript : public Component {
 protected:
+	/**
+	 * \param id Parent \c GameObject id
+	 * \param component_manager Reference to component manager (passed through to \c Script
+	 * instance)
+	 *
+	 * \note Calls to this constructor (should) always pass through \c GameObject::add_component,
+	 * which has an exception for this specific component type. This was done so the user does
+	 * not have to pass references used within \c Script to each \c BehaviorScript instance.
+	 */
 	BehaviorScript(game_object_id_t id, ComponentManager & component_manager);
 	//! Only ComponentManager is allowed to instantiate BehaviorScript
 	friend class ComponentManager;
@@ -37,27 +46,23 @@ public:
 	BehaviorScript & set_script();
 
 protected:
-	//! ScriptSystem needs direct access to the script instance
-	friend class ScriptSystem;
-	//! Flag to indicate if script->init() has been called already
-	bool initialized = false;
 	//! Script instance
 	std::unique_ptr<Script> script = nullptr;
-	//! Reference to component manager
+	//! ScriptSystem needs direct access to the script instance
+	friend class ScriptSystem;
+
+protected:
+	//! Reference to component manager (passed to Script)
 	ComponentManager & component_manager;
-	//! Script accesses the component manager directly via its parent
-	// (BehaviorScript) reference
-	friend class Script;
 };
 
 /**
  * \brief Add a BehaviorScript component to this game object
  *
- * The \c BehaviorScript class is the only exception to the ECS harmony, and
- * requires a reference to the component manager passed to its constructor in
- * order to function normally. This is because the \c BehaviorScript (and \c
- * Script) classes are the only component-related classes that store
- * implemented member functions as data.
+ * The \c BehaviorScript class is the only exception to the ECS harmony, and requires a
+ * reference to the component manager passed to its constructor in order to function normally.
+ * This is because the \c BehaviorScript (and \c Script) classes are the only component-related
+ * classes that store implemented member functions as data.
  */
 template <>
 BehaviorScript & GameObject::add_component<BehaviorScript>();
diff --git a/src/crepe/api/BehaviorScript.hpp b/src/crepe/api/BehaviorScript.hpp
index eec26da..d80321d 100644
--- a/src/crepe/api/BehaviorScript.hpp
+++ b/src/crepe/api/BehaviorScript.hpp
@@ -14,7 +14,7 @@ BehaviorScript & BehaviorScript::set_script() {
 	dbg_trace();
 	static_assert(std::is_base_of<Script, T>::value);
 	Script * s = new T();
-	s->parent_ref = this;
+	s->game_object_id = this->game_object_id;
 	s->component_manager_ref = &this->component_manager;
 	this->script = std::unique_ptr<Script>(s);
 	return *this;
diff --git a/src/crepe/api/LoopManager.cpp b/src/crepe/api/LoopManager.cpp
index 6303e2b..a64366f 100644
--- a/src/crepe/api/LoopManager.cpp
+++ b/src/crepe/api/LoopManager.cpp
@@ -22,8 +22,6 @@ LoopManager::LoopManager() {
 	this->load_system<ScriptSystem>();
 }
 
-ComponentManager & LoopManager::get_component_manager() { return this->component_manager; }
-
 void LoopManager::process_input() {
 	SDLContext::get_instance().handle_events(this->game_running);
 }
diff --git a/src/crepe/api/LoopManager.h b/src/crepe/api/LoopManager.h
index 288dca2..f6904be 100644
--- a/src/crepe/api/LoopManager.h
+++ b/src/crepe/api/LoopManager.h
@@ -68,18 +68,32 @@ private:
 
 	bool game_running = false;
 
-protected:
-	ComponentManager & get_component_manager();
-	template <class T>
-	T & get_system();
-
 private:
+	//! Component manager instance
 	ComponentManager component_manager{};
-	std::unordered_map<std::type_index, std::unique_ptr<System>> systems;
 
 private:
+	/**
+	 * \brief Collection of System instances
+	 *
+	 * This map holds System instances indexed by the system's class typeid. It is filled in the
+	 * constructor of \c LoopManager using LoopManager::load_system.
+	 */
+	std::unordered_map<std::type_index, std::unique_ptr<System>> systems;
+	/**
+	 * \brief Initialize a system
+	 * \tparam T System type (must be derivative of \c System)
+	 */
 	template <class T>
 	void load_system();
+	/**
+	 * \brief Retrieve a reference to ECS system
+	 * \tparam T System type
+	 * \returns Reference to system instance
+	 * \throws std::runtime_error if the System is not initialized
+	 */
+	template <class T>
+	T & get_system();
 };
 
 } // namespace crepe
diff --git a/src/crepe/api/Scene.h b/src/crepe/api/Scene.h
index 334f306..422fffb 100644
--- a/src/crepe/api/Scene.h
+++ b/src/crepe/api/Scene.h
@@ -4,16 +4,18 @@
 
 namespace crepe {
 
+class SceneManager;
 class ComponentManager;
 
 class Scene {
-public:
+protected:
 	Scene(ComponentManager & mgr, const std::string & name);
+	friend class SceneManager;
+public:
 	virtual ~Scene() = default;
 
-	virtual void load_scene() = 0;
-
 public:
+	virtual void load_scene() = 0;
 	const std::string name;
 
 protected:
diff --git a/src/crepe/api/Script.h b/src/crepe/api/Script.h
index 12ec7f1..1435581 100644
--- a/src/crepe/api/Script.h
+++ b/src/crepe/api/Script.h
@@ -2,6 +2,8 @@
 
 #include <vector>
 
+#include "../types.h"
+
 namespace crepe {
 
 class ScriptSystem;
@@ -11,32 +13,31 @@ class ComponentManager;
 /**
  * \brief Script interface
  *
- * This class is used as a base class for user-defined scripts that can be
- * added to game objects using the \c BehaviorScript component.
+ * This class is used as a base class for user-defined scripts that can be added to game
+ * objects using the \c BehaviorScript component.
+ *
+ * \note Additional *events* (like Unity's OnDisable and OnEnable) should be implemented as
+ * member or lambda methods in derivative user script classes and registered in \c init().
  */
 class Script {
-	//! ScriptSystem calls \c update()
-	friend class crepe::ScriptSystem;
-
 protected:
 	/**
 	 * \brief Script initialization function
 	 *
 	 * This function is called during the ScriptSystem::update() routine *before*
-	 * Script::update() if it (a) has not yet been called and (b) the \c
-	 * BehaviorScript component holding this script instance is active.
+	 * Script::update() if it (a) has not yet been called and (b) the \c BehaviorScript component
+	 * holding this script instance is active.
 	 */
 	virtual void init() {}
 	/**
 	 * \brief Script update function
 	 *
-	 * This function is called during the ScriptSystem::update() routine if the
-	 * \c BehaviorScript component holding this script instance is active.
+	 * This function is called during the ScriptSystem::update() routine if the \c BehaviorScript
+	 * component holding this script instance is active.
 	 */
 	virtual void update() {}
-	// NOTE: additional *events* (like unity's OnDisable and OnEnable) should be implemented as
-	// member methods in derivative user script classes and registered in init(), otherwise this
-	// class will balloon in size with each added event.
+	//! ScriptSystem calls \c init() and \c update()
+	friend class crepe::ScriptSystem;
 
 protected:
 	/**
@@ -46,8 +47,7 @@ protected:
 	 *
 	 * \returns Reference to component
 	 *
-	 * \throws nullptr if this game object does not have a component matching
-	 * type \c T
+	 * \throws nullptr if this game object does not have a component matching type \c T
 	 */
 	template <typename T>
 	T & get_component() const;
@@ -63,18 +63,22 @@ protected:
 	std::vector<std::reference_wrapper<T>> get_components() const;
 
 protected:
-	// NOTE: Script must have a constructor without arguments so the game
-	// programmer doesn't need to manually add `using Script::Script` to their
-	// concrete script class.
+	// NOTE: Script must have a constructor without arguments so the game programmer doesn't need
+	// to manually add `using Script::Script` to their concrete script class.
 	Script() = default;
 	//! Only \c BehaviorScript instantiates Script
 	friend class BehaviorScript;
 
 private:
-	// These references are set by BehaviorScript immediately after calling the
-	// constructor of Script.
-	BehaviorScript * parent_ref = nullptr;
+	// These references are set by BehaviorScript immediately after calling the constructor of
+	// Script.
+	game_object_id_t game_object_id = -1;
 	ComponentManager * component_manager_ref = nullptr;
+	// TODO: use OptionalRef instead of pointer
+
+private:
+	//! Flag to indicate if \c init() has been called already
+	bool initialized = false;
 };
 
 } // namespace crepe
diff --git a/src/crepe/api/Script.hpp b/src/crepe/api/Script.hpp
index c4e07e8..a064a90 100644
--- a/src/crepe/api/Script.hpp
+++ b/src/crepe/api/Script.hpp
@@ -20,10 +20,9 @@ T & Script::get_component() const {
 
 template <typename T>
 std::vector<std::reference_wrapper<T>> Script::get_components() const {
-	auto & parent = *this->parent_ref;
 	auto & mgr = *this->component_manager_ref;
 
-	return mgr.get_components_by_id<T>(parent.game_object_id);
+	return mgr.get_components_by_id<T>(this->game_object_id);
 }
 
 } // namespace crepe
diff --git a/src/crepe/system/ParticleSystem.h b/src/crepe/system/ParticleSystem.h
index 0a2dde1..c647284 100644
--- a/src/crepe/system/ParticleSystem.h
+++ b/src/crepe/system/ParticleSystem.h
@@ -4,8 +4,6 @@
 
 #include "System.h"
 
-#include "System.h"
-
 namespace crepe {
 
 class ParticleEmitter;
diff --git a/src/crepe/system/ScriptSystem.cpp b/src/crepe/system/ScriptSystem.cpp
index f4a826b..c4d724c 100644
--- a/src/crepe/system/ScriptSystem.cpp
+++ b/src/crepe/system/ScriptSystem.cpp
@@ -18,10 +18,9 @@ void ScriptSystem::update() {
 
 	for (auto & script_ref : scripts) {
 		Script & script = script_ref.get();
-		BehaviorScript & component = *script.parent_ref;
-		if (!component.initialized) {
+		if (!script.initialized) {
 			script.init();
-			component.initialized = true;
+			script.initialized = true;
 		}
 		script.update();
 	}
-- 
cgit v1.2.3