ActiveObjectMgr fixes (#13560)

src/activeobjectmgr.h

@@ -20,7 +20,11 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #pragma once
 
 #include <map>
+#include <memory>
+#include <vector>
+#include "debug.h"
 #include "irrlichttypes.h"
+#include "util/basic_macros.h"
 
 class TestClientActiveObjectMgr;
 class TestServerActiveObjectMgr;
@@ -32,14 +36,40 @@ class ActiveObjectMgr
 	friend class ::TestServerActiveObjectMgr;
 
 public:
+	ActiveObjectMgr() = default;
+	DISABLE_CLASS_COPY(ActiveObjectMgr);
+
+	virtual ~ActiveObjectMgr()
+	{
+		SANITY_CHECK(m_active_objects.empty());
+		// Note: Do not call clear() here. The derived class is already half
+		// destructed.
+	}
+
 	virtual void step(float dtime, const std::function<void(T *)> &f) = 0;
-	virtual bool registerObject(T *obj) = 0;
+	virtual bool registerObject(std::unique_ptr<T> obj) = 0;
 	virtual void removeObject(u16 id) = 0;
 
+	void clear()
+	{
+		while (!m_active_objects.empty())
+			removeObject(m_active_objects.begin()->first);
+	}
+
 	T *getActiveObject(u16 id)
 	{
-		auto n = m_active_objects.find(id);
-		return (n != m_active_objects.end() ? n->second : nullptr);
+		auto it = m_active_objects.find(id);
+		return it != m_active_objects.end() ? it->second.get() : nullptr;
+	}
+
+	std::vector<u16> getAllIds() const
+	{
+		std::vector<u16> ids;
+		ids.reserve(m_active_objects.size());
+		for (auto &it : m_active_objects) {
+			ids.push_back(it.first);
+		}
+		return ids;
 	}
 
 protected:
@@ -61,5 +91,8 @@ class ActiveObjectMgr
 		return id != 0 && m_active_objects.find(id) == m_active_objects.end();
 	}
 
-	std::map<u16, T *> m_active_objects; // ordered to fix #10985 
+	// ordered to fix #10985
+	// Note: ActiveObjects can access the ActiveObjectMgr. Only erase objects using
+	// removeObject()!
+	std::map<u16, std::unique_ptr<T>> m_active_objects;
 };

src/client/activeobjectmgr.cpp

@@ -25,28 +25,33 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 namespace client
 {
 
-void ActiveObjectMgr::clear()
+ActiveObjectMgr::~ActiveObjectMgr()
 {
-	// delete active objects
-	for (auto &active_object : m_active_objects) {
-		delete active_object.second;
-		// Object must be marked as gone when children try to detach
-		active_object.second = nullptr;
+	if (!m_active_objects.empty()) {
+		warningstream << "client::ActiveObjectMgr::~ActiveObjectMgr(): not cleared."
+				<< std::endl;
+		clear();
 	}
-	m_active_objects.clear();
 }
 
 void ActiveObjectMgr::step(
 		float dtime, const std::function<void(ClientActiveObject *)> &f)
 {
 	g_profiler->avg("ActiveObjectMgr: CAO count [#]", m_active_objects.size());
-	for (auto &ao_it : m_active_objects) {
-		f(ao_it.second);
+
+	// Same as in server activeobjectmgr.
+	std::vector<u16> ids = getAllIds();
+
+	for (u16 id : ids) {
+		auto it = m_active_objects.find(id);
+		if (it == m_active_objects.end())
+			continue; // obj was removed
+		f(it->second.get());
 	}
 }
 
 // clang-format off
-bool ActiveObjectMgr::registerObject(ClientActiveObject *obj)
+bool ActiveObjectMgr::registerObject(std::unique_ptr<ClientActiveObject> obj)
 {
 	assert(obj); // Pre-condition
 	if (obj->getId() == 0) {
@@ -55,7 +60,6 @@ bool ActiveObjectMgr::registerObject(ClientActiveObject *obj)
 			infostream << "Client::ActiveObjectMgr::registerObject(): "
 					<< "no free id available" << std::endl;
 
-			delete obj;
 			return false;
 		}
 		obj->setId(new_id);
@@ -64,30 +68,29 @@ bool ActiveObjectMgr::registerObject(ClientActiveObject *obj)
 	if (!isFreeId(obj->getId())) {
 		infostream << "Client::ActiveObjectMgr::registerObject(): "
 				<< "id is not free (" << obj->getId() << ")" << std::endl;
-		delete obj;
 		return false;
 	}
 	infostream << "Client::ActiveObjectMgr::registerObject(): "
 			<< "added (id=" << obj->getId() << ")" << std::endl;
-	m_active_objects[obj->getId()] = obj;
+	m_active_objects[obj->getId()] = std::move(obj);
 	return true;
 }
 
 void ActiveObjectMgr::removeObject(u16 id)
 {
 	verbosestream << "Client::ActiveObjectMgr::removeObject(): "
 			<< "id=" << id << std::endl;
-	ClientActiveObject *obj = getActiveObject(id);
-	if (!obj) {
+	auto it = m_active_objects.find(id);
+	if (it == m_active_objects.end()) {
 		infostream << "Client::ActiveObjectMgr::removeObject(): "
 				<< "id=" << id << " not found" << std::endl;
 		return;
 	}
 
-	m_active_objects.erase(id);
+	std::unique_ptr<ClientActiveObject> obj = std::move(it->second);
+	m_active_objects.erase(it);
 
 	obj->removeFromScene(true);
-	delete obj;
 }
 
 // clang-format on
@@ -96,7 +99,7 @@ void ActiveObjectMgr::getActiveObjects(const v3f &origin, f32 max_d,
 {
 	f32 max_d2 = max_d * max_d;
 	for (auto &ao_it : m_active_objects) {
-		ClientActiveObject *obj = ao_it.second;
+		ClientActiveObject *obj = ao_it.second.get();
 
 		f32 d2 = (obj->getPosition() - origin).getLengthSQ();
 
@@ -114,7 +117,7 @@ std::vector<DistanceSortedActiveObject> ActiveObjectMgr::getActiveSelectableObje
 	v3f dir = shootline.getVector().normalize();
 
 	for (auto &ao_it : m_active_objects) {
-		ClientActiveObject *obj = ao_it.second;
+		ClientActiveObject *obj = ao_it.second.get();
 
 		aabb3f selection_box;
 		if (!obj->getSelectionBox(&selection_box))

src/client/activeobjectmgr.h

@@ -26,13 +26,14 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 namespace client
 {
-class ActiveObjectMgr : public ::ActiveObjectMgr<ClientActiveObject>
+class ActiveObjectMgr final : public ::ActiveObjectMgr<ClientActiveObject>
 {
 public:
-	void clear();
+	~ActiveObjectMgr() override;
+
 	void step(float dtime,
 			const std::function<void(ClientActiveObject *)> &f) override;
-	bool registerObject(ClientActiveObject *obj) override;
+	bool registerObject(std::unique_ptr<ClientActiveObject> obj) override;
 	void removeObject(u16 id) override;
 
 	void getActiveObjects(const v3f &origin, f32 max_d,

src/client/clientenvironment.cpp

@@ -364,26 +364,26 @@ GenericCAO* ClientEnvironment::getGenericCAO(u16 id)
 	return NULL;
 }
 
-u16 ClientEnvironment::addActiveObject(ClientActiveObject *object)
+u16 ClientEnvironment::addActiveObject(std::unique_ptr<ClientActiveObject> object)
 {
+	auto obj = object.get();
 	// Register object. If failed return zero id
-	if (!m_ao_manager.registerObject(object))
+	if (!m_ao_manager.registerObject(std::move(object)))
 		return 0;
 
-	object->addToScene(m_texturesource, m_client->getSceneManager());
+	obj->addToScene(m_texturesource, m_client->getSceneManager());
 
 	// Update lighting immediately
-	object->updateLight(getDayNightRatio());
-	return object->getId();
+	obj->updateLight(getDayNightRatio());
+	return obj->getId();
 }
 
 void ClientEnvironment::addActiveObject(u16 id, u8 type,
 	const std::string &init_data)
 {
-	ClientActiveObject* obj =
+	std::unique_ptr<ClientActiveObject> obj =
 		ClientActiveObject::create((ActiveObjectType) type, m_client, this);
-	if(obj == NULL)
-	{
+	if (!obj) {
 		infostream<<"ClientEnvironment::addActiveObject(): "
 			<<"id="<<id<<" type="<<type<<": Couldn't create object"
 			<<std::endl;
@@ -392,12 +392,9 @@ void ClientEnvironment::addActiveObject(u16 id, u8 type,
 
 	obj->setId(id);
 
-	try
-	{
+	try {
 		obj->initialize(init_data);
-	}
-	catch(SerializationError &e)
-	{
+	} catch(SerializationError &e) {
 		errorstream<<"ClientEnvironment::addActiveObject():"
 			<<" id="<<id<<" type="<<type
 			<<": SerializationError in initialize(): "
@@ -406,12 +403,12 @@ void ClientEnvironment::addActiveObject(u16 id, u8 type,
 			<<std::endl;
 	}
 
-	u16 new_id = addActiveObject(obj);
+	u16 new_id = addActiveObject(std::move(obj));
 	// Object initialized:
-	if ((obj = getActiveObject(new_id))) {
+	if (ClientActiveObject *obj2 = getActiveObject(new_id)) {
 		// Final step is to update all children which are already known
 		// Data provided by AO_CMD_SPAWN_INFANT
-		const auto &children = obj->getAttachmentChildIds();
+		const auto &children = obj2->getAttachmentChildIds();
 		for (auto c_id : children) {
 			if (auto *o = getActiveObject(c_id))
 				o->updateAttachments();

src/client/clientenvironment.h

@@ -101,7 +101,7 @@ class ClientEnvironment : public Environment
 		Returns the id of the object.
 		Returns 0 if not added and thus deleted.
 	*/
-	u16 addActiveObject(ClientActiveObject *object);
+	u16 addActiveObject(std::unique_ptr<ClientActiveObject> object);
 
 	void addActiveObject(u16 id, u8 type, const std::string &init_data);
 	void removeActiveObject(u16 id);

src/client/clientobject.cpp

@@ -38,7 +38,7 @@ ClientActiveObject::~ClientActiveObject()
 	removeFromScene(true);
 }
 
-ClientActiveObject* ClientActiveObject::create(ActiveObjectType type,
+std::unique_ptr<ClientActiveObject> ClientActiveObject::create(ActiveObjectType type,
 		Client *client, ClientEnvironment *env)
 {
 	// Find factory function
@@ -47,11 +47,11 @@ ClientActiveObject* ClientActiveObject::create(ActiveObjectType type,
 		// If factory is not found, just return.
 		warningstream << "ClientActiveObject: No factory for type="
 				<< (int)type << std::endl;
-		return NULL;
+		return nullptr;
 	}
 
 	Factory f = n->second;
-	ClientActiveObject *object = (*f)(client, env);
+	std::unique_ptr<ClientActiveObject> object = (*f)(client, env);
 	return object;
 }
 

src/client/clientobject.h

@@ -21,6 +21,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #include "irrlichttypes_extrabloated.h"
 #include "activeobject.h"
+#include <memory>
 #include <unordered_map>
 #include <unordered_set>
 
@@ -78,16 +79,16 @@ class ClientActiveObject : public ActiveObject
 	virtual void initialize(const std::string &data) {}
 
 	// Create a certain type of ClientActiveObject
-	static ClientActiveObject *create(ActiveObjectType type, Client *client,
-		ClientEnvironment *env);
+	static std::unique_ptr<ClientActiveObject> create(ActiveObjectType type,
+			Client *client, ClientEnvironment *env);
 
 	// If returns true, punch will not be sent to the server
 	virtual bool directReportPunch(v3f dir, const ItemStack *punchitem = nullptr,
 		float time_from_last_punch = 1000000) { return false; }
 
 protected:
 	// Used for creating objects based on type
-	typedef ClientActiveObject *(*Factory)(Client *client, ClientEnvironment *env);
+	typedef std::unique_ptr<ClientActiveObject> (*Factory)(Client *client, ClientEnvironment *env);
 	static void registerType(u16 type, Factory f);
 	Client *m_client;
 	ClientEnvironment *m_env;

src/client/content_cao.cpp

@@ -199,7 +199,7 @@ class TestCAO : public ClientActiveObject
 		return ACTIVEOBJECT_TYPE_TEST;
 	}
 
-	static ClientActiveObject* create(Client *client, ClientEnvironment *env);
+	static std::unique_ptr<ClientActiveObject> create(Client *client, ClientEnvironment *env);
 
 	void addToScene(ITextureSource *tsrc, scene::ISceneManager *smgr);
 	void removeFromScene(bool permanent);
@@ -227,9 +227,9 @@ TestCAO::TestCAO(Client *client, ClientEnvironment *env):
 	ClientActiveObject::registerType(getType(), create);
 }
 
-ClientActiveObject* TestCAO::create(Client *client, ClientEnvironment *env)
+std::unique_ptr<ClientActiveObject> TestCAO::create(Client *client, ClientEnvironment *env)
 {
-	return new TestCAO(client, env);
+	return std::make_unique<TestCAO>(client, env);
 }
 
 void TestCAO::addToScene(ITextureSource *tsrc, scene::ISceneManager *smgr)
@@ -326,7 +326,7 @@ void TestCAO::processMessage(const std::string &data)
 GenericCAO::GenericCAO(Client *client, ClientEnvironment *env):
 		ClientActiveObject(0, client, env)
 {
-	if (client == NULL) {
+	if (!client) {
 		ClientActiveObject::registerType(getType(), create);
 	} else {
 		m_client = client;

src/client/content_cao.h

@@ -26,6 +26,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "itemgroup.h"
 #include "constants.h"
 #include <cassert>
+#include <memory>
 
 class Camera;
 class Client;
@@ -139,9 +140,9 @@ class GenericCAO : public ClientActiveObject
 
 	~GenericCAO();
 
-	static ClientActiveObject* create(Client *client, ClientEnvironment *env)
+	static std::unique_ptr<ClientActiveObject> create(Client *client, ClientEnvironment *env)
 	{
-		return new GenericCAO(client, env);
+		return std::make_unique<GenericCAO>(client, env);
 	}
 
 	inline ActiveObjectType getType() const override

src/script/lua_api/l_env.cpp

@@ -611,10 +611,12 @@ int ModApiEnv::l_add_entity(lua_State *L)
 	const char *name = luaL_checkstring(L, 2);
 	std::string staticdata = readParam<std::string>(L, 3, "");
 
-	ServerActiveObject *obj = new LuaEntitySAO(env, pos, name, staticdata);
-	int objectid = env->addActiveObject(obj);
+	std::unique_ptr<ServerActiveObject> obj_u =
+			std::make_unique<LuaEntitySAO>(env, pos, name, staticdata);
+	auto obj = obj_u.get();
+	int objectid = env->addActiveObject(std::move(obj_u));
 	// If failed to add, return nothing (reads as nil)
-	if(objectid == 0)
+	if (objectid == 0)
 		return 0;
 
 	// If already deleted (can happen in on_activate), return nil