From 11ec75c2ad0ebb747b8dc3ea27e2e98d98a05380 Mon Sep 17 00:00:00 2001 From: DS Date: Mon, 9 Oct 2023 17:13:04 +0200 Subject: [PATCH] ActiveObjectMgr fixes (#13560) --- src/activeobjectmgr.h | 41 ++++++++++-- src/client/activeobjectmgr.cpp | 41 ++++++------ src/client/activeobjectmgr.h | 7 +- src/client/clientenvironment.cpp | 29 ++++----- src/client/clientenvironment.h | 2 +- src/client/clientobject.cpp | 6 +- src/client/clientobject.h | 7 +- src/client/content_cao.cpp | 8 +-- src/client/content_cao.h | 5 +- src/script/lua_api/l_env.cpp | 8 ++- src/server/activeobjectmgr.cpp | 72 +++++++++++++-------- src/server/activeobjectmgr.h | 9 ++- src/server/serveractiveobject.h | 4 -- src/serverenvironment.cpp | 43 ++++++------ src/serverenvironment.h | 8 ++- src/unittest/test_clientactiveobjectmgr.cpp | 26 +++++--- src/unittest/test_serveractiveobjectmgr.cpp | 43 ++++++------ 17 files changed, 205 insertions(+), 154 deletions(-) diff --git a/src/activeobjectmgr.h b/src/activeobjectmgr.h index d838c04ca4b4..711c9243f711 100644 --- a/src/activeobjectmgr.h +++ b/src/activeobjectmgr.h @@ -20,7 +20,11 @@ with this program; if not, write to the Free Software Foundation, Inc., #pragma once #include +#include +#include +#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 &f) = 0; - virtual bool registerObject(T *obj) = 0; + virtual bool registerObject(std::unique_ptr 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 getAllIds() const + { + std::vector 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 m_active_objects; // ordered to fix #10985 + // ordered to fix #10985 + // Note: ActiveObjects can access the ActiveObjectMgr. Only erase objects using + // removeObject()! + std::map> m_active_objects; }; diff --git a/src/client/activeobjectmgr.cpp b/src/client/activeobjectmgr.cpp index 19e75439f258..ab7b06f7d3c9 100644 --- a/src/client/activeobjectmgr.cpp +++ b/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 &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 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 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,12 +68,11 @@ 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; } @@ -77,17 +80,17 @@ 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 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 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)) diff --git a/src/client/activeobjectmgr.h b/src/client/activeobjectmgr.h index a4243c644d7e..05931387f327 100644 --- a/src/client/activeobjectmgr.h +++ b/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 +class ActiveObjectMgr final : public ::ActiveObjectMgr { public: - void clear(); + ~ActiveObjectMgr() override; + void step(float dtime, const std::function &f) override; - bool registerObject(ClientActiveObject *obj) override; + bool registerObject(std::unique_ptr obj) override; void removeObject(u16 id) override; void getActiveObjects(const v3f &origin, f32 max_d, diff --git a/src/client/clientenvironment.cpp b/src/client/clientenvironment.cpp index 633a2896c69a..66242aa4f9a6 100644 --- a/src/client/clientenvironment.cpp +++ b/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 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 obj = ClientActiveObject::create((ActiveObjectType) type, m_client, this); - if(obj == NULL) - { + if (!obj) { infostream<<"ClientEnvironment::addActiveObject(): " <<"id="<setId(id); - try - { + try { obj->initialize(init_data); - } - catch(SerializationError &e) - { + } catch(SerializationError &e) { errorstream<<"ClientEnvironment::addActiveObject():" <<" id="<getAttachmentChildIds(); + const auto &children = obj2->getAttachmentChildIds(); for (auto c_id : children) { if (auto *o = getActiveObject(c_id)) o->updateAttachments(); diff --git a/src/client/clientenvironment.h b/src/client/clientenvironment.h index f5d46deb5dab..fbf08aecd56f 100644 --- a/src/client/clientenvironment.h +++ b/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 object); void addActiveObject(u16 id, u8 type, const std::string &init_data); void removeActiveObject(u16 id); diff --git a/src/client/clientobject.cpp b/src/client/clientobject.cpp index f4b69201ba9a..13131dd4ca88 100644 --- a/src/client/clientobject.cpp +++ b/src/client/clientobject.cpp @@ -38,7 +38,7 @@ ClientActiveObject::~ClientActiveObject() removeFromScene(true); } -ClientActiveObject* ClientActiveObject::create(ActiveObjectType type, +std::unique_ptr 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 object = (*f)(client, env); return object; } diff --git a/src/client/clientobject.h b/src/client/clientobject.h index af7a5eb9c093..f63681313762 100644 --- a/src/client/clientobject.h +++ b/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 #include #include @@ -78,8 +79,8 @@ 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 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, @@ -87,7 +88,7 @@ class ClientActiveObject : public ActiveObject protected: // Used for creating objects based on type - typedef ClientActiveObject *(*Factory)(Client *client, ClientEnvironment *env); + typedef std::unique_ptr (*Factory)(Client *client, ClientEnvironment *env); static void registerType(u16 type, Factory f); Client *m_client; ClientEnvironment *m_env; diff --git a/src/client/content_cao.cpp b/src/client/content_cao.cpp index ad316209a3c4..a22e68f6ed97 100644 --- a/src/client/content_cao.cpp +++ b/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 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 TestCAO::create(Client *client, ClientEnvironment *env) { - return new TestCAO(client, env); + return std::make_unique(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; diff --git a/src/client/content_cao.h b/src/client/content_cao.h index 62d090a6ff11..b090cdc088a7 100644 --- a/src/client/content_cao.h +++ b/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 +#include class Camera; class Client; @@ -139,9 +140,9 @@ class GenericCAO : public ClientActiveObject ~GenericCAO(); - static ClientActiveObject* create(Client *client, ClientEnvironment *env) + static std::unique_ptr create(Client *client, ClientEnvironment *env) { - return new GenericCAO(client, env); + return std::make_unique(client, env); } inline ActiveObjectType getType() const override diff --git a/src/script/lua_api/l_env.cpp b/src/script/lua_api/l_env.cpp index cd22d930bed0..33d79031672b 100644 --- a/src/script/lua_api/l_env.cpp +++ b/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(L, 3, ""); - ServerActiveObject *obj = new LuaEntitySAO(env, pos, name, staticdata); - int objectid = env->addActiveObject(obj); + std::unique_ptr obj_u = + std::make_unique(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 diff --git a/src/server/activeobjectmgr.cpp b/src/server/activeobjectmgr.cpp index 1fa191ac0a6e..543003e42ae7 100644 --- a/src/server/activeobjectmgr.cpp +++ b/src/server/activeobjectmgr.cpp @@ -25,16 +25,30 @@ with this program; if not, write to the Free Software Foundation, Inc., namespace server { -void ActiveObjectMgr::clear(const std::function &cb) +ActiveObjectMgr::~ActiveObjectMgr() { - // make a defensive copy in case the - // passed callback changes the set of active objects - auto cloned_map(m_active_objects); - - for (auto &it : cloned_map) { - if (cb(it.second, it.first)) { - // Remove reference from m_active_objects - m_active_objects.erase(it.first); + if (!m_active_objects.empty()) { + warningstream << "server::ActiveObjectMgr::~ActiveObjectMgr(): not cleared." + << std::endl; + clear(); + } +} + +void ActiveObjectMgr::clearIf(const std::function &cb) +{ + // Make a defensive copy of the ids in case the passed callback changes the + // set of active objects. + // The callback is called for newly added objects iff they happen to reuse + // an old id. + std::vector ids = getAllIds(); + + for (u16 id : ids) { + auto it = m_active_objects.find(id); + if (it == m_active_objects.end()) + continue; // obj was already removed + if (cb(it->second.get(), id)) { + // erase by id, `it` can be invalid now + removeObject(id); } } } @@ -43,13 +57,20 @@ void ActiveObjectMgr::step( float dtime, const std::function &f) { g_profiler->avg("ActiveObjectMgr: SAO count [#]", m_active_objects.size()); - for (auto &ao_it : m_active_objects) { - f(ao_it.second); + + // See above. + std::vector 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(ServerActiveObject *obj) +bool ActiveObjectMgr::registerObject(std::unique_ptr obj) { assert(obj); // Pre-condition if (obj->getId() == 0) { @@ -57,8 +78,6 @@ bool ActiveObjectMgr::registerObject(ServerActiveObject *obj) if (new_id == 0) { errorstream << "Server::ActiveObjectMgr::addActiveObjectRaw(): " << "no free id available" << std::endl; - if (obj->environmentDeletes()) - delete obj; return false; } obj->setId(new_id); @@ -70,8 +89,6 @@ bool ActiveObjectMgr::registerObject(ServerActiveObject *obj) if (!isFreeId(obj->getId())) { errorstream << "Server::ActiveObjectMgr::addActiveObjectRaw(): " << "id is not free (" << obj->getId() << ")" << std::endl; - if (obj->environmentDeletes()) - delete obj; return false; } @@ -80,15 +97,14 @@ bool ActiveObjectMgr::registerObject(ServerActiveObject *obj) warningstream << "Server::ActiveObjectMgr::addActiveObjectRaw(): " << "object position (" << p.X << "," << p.Y << "," << p.Z << ") outside maximum range" << std::endl; - if (obj->environmentDeletes()) - delete obj; return false; } - m_active_objects[obj->getId()] = obj; + auto obj_p = obj.get(); + m_active_objects[obj->getId()] = std::move(obj); verbosestream << "Server::ActiveObjectMgr::addActiveObjectRaw(): " - << "Added id=" << obj->getId() << "; there are now " + << "Added id=" << obj_p->getId() << "; there are now " << m_active_objects.size() << " active objects." << std::endl; return true; } @@ -97,15 +113,17 @@ void ActiveObjectMgr::removeObject(u16 id) { verbosestream << "Server::ActiveObjectMgr::removeObject(): " << "id=" << id << std::endl; - ServerActiveObject *obj = getActiveObject(id); - if (!obj) { + auto it = m_active_objects.find(id); + if (it == m_active_objects.end()) { infostream << "Server::ActiveObjectMgr::removeObject(): " << "id=" << id << " not found" << std::endl; return; } - m_active_objects.erase(id); - delete obj; + // Delete the obj before erasing, as the destructor may indirectly access + // m_active_objects. + it->second.reset(); + m_active_objects.erase(id); // `it` can be invalid now } // clang-format on @@ -115,7 +133,7 @@ void ActiveObjectMgr::getObjectsInsideRadius(const v3f &pos, float radius, { float r2 = radius * radius; for (auto &activeObject : m_active_objects) { - ServerActiveObject *obj = activeObject.second; + ServerActiveObject *obj = activeObject.second.get(); const v3f &objectpos = obj->getBasePosition(); if (objectpos.getDistanceFromSQ(pos) > r2) continue; @@ -130,7 +148,7 @@ void ActiveObjectMgr::getObjectsInArea(const aabb3f &box, std::function include_obj_cb) { for (auto &activeObject : m_active_objects) { - ServerActiveObject *obj = activeObject.second; + ServerActiveObject *obj = activeObject.second.get(); const v3f &objectpos = obj->getBasePosition(); if (!box.isPointInside(objectpos)) continue; @@ -155,7 +173,7 @@ void ActiveObjectMgr::getAddedActiveObjectsAroundPos(const v3f &player_pos, f32 u16 id = ao_it.first; // Get object - ServerActiveObject *object = ao_it.second; + ServerActiveObject *object = ao_it.second.get(); if (!object) continue; diff --git a/src/server/activeobjectmgr.h b/src/server/activeobjectmgr.h index d43f5643c279..5d333c2328b1 100644 --- a/src/server/activeobjectmgr.h +++ b/src/server/activeobjectmgr.h @@ -26,13 +26,16 @@ with this program; if not, write to the Free Software Foundation, Inc., namespace server { -class ActiveObjectMgr : public ::ActiveObjectMgr +class ActiveObjectMgr final : public ::ActiveObjectMgr { public: - void clear(const std::function &cb); + ~ActiveObjectMgr() override; + + // If cb returns true, the obj will be deleted + void clearIf(const std::function &cb); void step(float dtime, const std::function &f) override; - bool registerObject(ServerActiveObject *obj) override; + bool registerObject(std::unique_ptr obj) override; void removeObject(u16 id) override; void getObjectsInsideRadius(const v3f &pos, float radius, diff --git a/src/server/serveractiveobject.h b/src/server/serveractiveobject.h index 568295e29d61..e9cf3ee372a0 100644 --- a/src/server/serveractiveobject.h +++ b/src/server/serveractiveobject.h @@ -67,10 +67,6 @@ class ServerActiveObject : public ActiveObject virtual void addedToEnvironment(u32 dtime_s){}; // Called before removing from environment virtual void removingFromEnvironment(){}; - // Returns true if object's deletion is the job of the - // environment - virtual bool environmentDeletes() const - { return true; } // Safely mark the object for removal or deactivation void markForRemoval(); diff --git a/src/serverenvironment.cpp b/src/serverenvironment.cpp index 66c407c240db..c54fee6c45a4 100644 --- a/src/serverenvironment.cpp +++ b/src/serverenvironment.cpp @@ -633,9 +633,9 @@ void ServerEnvironment::savePlayer(RemotePlayer *player) PlayerSAO *ServerEnvironment::loadPlayer(RemotePlayer *player, bool *new_player, session_t peer_id, bool is_singleplayer) { - PlayerSAO *playersao = new PlayerSAO(this, player, peer_id, is_singleplayer); + auto playersao = std::make_unique(this, player, peer_id, is_singleplayer); // Create player if it doesn't exist - if (!m_player_database->loadPlayer(player, playersao)) { + if (!m_player_database->loadPlayer(player, playersao.get())) { *new_player = true; // Set player position infostream << "Server: Finding spawn place for player \"" @@ -662,12 +662,13 @@ PlayerSAO *ServerEnvironment::loadPlayer(RemotePlayer *player, bool *new_player, player->clearHud(); /* Add object to environment */ - addActiveObject(playersao); + PlayerSAO *ret = playersao.get(); + addActiveObject(std::move(playersao)); // Update active blocks quickly for a bit so objects in those blocks appear on the client m_fast_active_block_divider = 10; - return playersao; + return ret; } void ServerEnvironment::saveMeta() @@ -1230,13 +1231,10 @@ void ServerEnvironment::clearObjects(ClearObjectsMode mode) m_script->removeObjectReference(obj); // Delete active object - if (obj->environmentDeletes()) - delete obj; - return true; }; - m_ao_manager.clear(cb_removal); + m_ao_manager.clearIf(cb_removal); // Get list of loaded blocks std::vector loaded_blocks; @@ -1675,11 +1673,11 @@ void ServerEnvironment::deleteParticleSpawner(u32 id, bool remove_from_object) } } -u16 ServerEnvironment::addActiveObject(ServerActiveObject *object) +u16 ServerEnvironment::addActiveObject(std::unique_ptr object) { assert(object); // Pre-condition m_added_objects++; - u16 id = addActiveObjectRaw(object, true, 0); + u16 id = addActiveObjectRaw(std::move(object), true, 0); return id; } @@ -1831,10 +1829,11 @@ void ServerEnvironment::getSelectedActiveObjects( ************ Private methods ************* */ -u16 ServerEnvironment::addActiveObjectRaw(ServerActiveObject *object, +u16 ServerEnvironment::addActiveObjectRaw(std::unique_ptr object_u, bool set_changed, u32 dtime_s) { - if (!m_ao_manager.registerObject(object)) { + auto object = object_u.get(); + if (!m_ao_manager.registerObject(std::move(object_u))) { return 0; } @@ -1925,13 +1924,10 @@ void ServerEnvironment::removeRemovedObjects() m_script->removeObjectReference(obj); // Delete - if (obj->environmentDeletes()) - delete obj; - return true; }; - m_ao_manager.clear(clear_cb); + m_ao_manager.clearIf(clear_cb); } static void print_hexdump(std::ostream &o, const std::string &data) @@ -1968,12 +1964,12 @@ static void print_hexdump(std::ostream &o, const std::string &data) } } -ServerActiveObject* ServerEnvironment::createSAO(ActiveObjectType type, v3f pos, - const std::string &data) +std::unique_ptr ServerEnvironment::createSAO(ActiveObjectType type, + v3f pos, const std::string &data) { switch (type) { case ACTIVEOBJECT_TYPE_LUAENTITY: - return new LuaEntitySAO(this, pos, data); + return std::make_unique(this, pos, data); default: warningstream << "ServerActiveObject: No factory for type=" << type << std::endl; } @@ -1995,7 +1991,7 @@ void ServerEnvironment::activateObjects(MapBlock *block, u32 dtime_s) std::vector new_stored; for (const StaticObject &s_obj : block->m_static_objects.getAllStored()) { // Create an active object from the data - ServerActiveObject *obj = + std::unique_ptr obj = createSAO((ActiveObjectType)s_obj.type, s_obj.pos, s_obj.data); // If couldn't create object, store static data back. if (!obj) { @@ -2012,7 +2008,7 @@ void ServerEnvironment::activateObjects(MapBlock *block, u32 dtime_s) << "activated static object pos=" << (s_obj.pos / BS) << " type=" << (int)s_obj.type << std::endl; // This will also add the object to the active static list - addActiveObjectRaw(obj, false, dtime_s); + addActiveObjectRaw(std::move(obj), false, dtime_s); if (block->isOrphan()) return; } @@ -2168,13 +2164,10 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) m_script->removeObjectReference(obj); // Delete active object - if (obj->environmentDeletes()) - delete obj; - return true; }; - m_ao_manager.clear(cb_deactivate); + m_ao_manager.clearIf(cb_deactivate); } void ServerEnvironment::deleteStaticFromBlock( diff --git a/src/serverenvironment.h b/src/serverenvironment.h index 312b282021eb..fd556a211416 100644 --- a/src/serverenvironment.h +++ b/src/serverenvironment.h @@ -277,7 +277,7 @@ class ServerEnvironment final : public Environment Returns the id of the object. Returns 0 if not added and thus deleted. */ - u16 addActiveObject(ServerActiveObject *object); + u16 addActiveObject(std::unique_ptr object); /* Add an active object as a static object to the corresponding @@ -422,7 +422,8 @@ class ServerEnvironment final : public Environment Returns the id of the object. Returns 0 if not added and thus deleted. */ - u16 addActiveObjectRaw(ServerActiveObject *object, bool set_changed, u32 dtime_s); + u16 addActiveObjectRaw(std::unique_ptr object, + bool set_changed, u32 dtime_s); /* Remove all objects that satisfy (isGone() && m_known_by_count==0) @@ -518,5 +519,6 @@ class ServerEnvironment final : public Environment MetricGaugePtr m_active_block_gauge; MetricGaugePtr m_active_object_gauge; - ServerActiveObject* createSAO(ActiveObjectType type, v3f pos, const std::string &data); + std::unique_ptr createSAO(ActiveObjectType type, v3f pos, + const std::string &data); }; diff --git a/src/unittest/test_clientactiveobjectmgr.cpp b/src/unittest/test_clientactiveobjectmgr.cpp index c3ec40637a45..d3cd83dc97de 100644 --- a/src/unittest/test_clientactiveobjectmgr.cpp +++ b/src/unittest/test_clientactiveobjectmgr.cpp @@ -90,8 +90,9 @@ void TestClientActiveObjectMgr::testFreeID() // Register basic objects, ensure we never found for (u8 i = 0; i < UINT8_MAX; i++) { // Register an object - auto tcao = new TestClientActiveObject(); - caomgr.registerObject(tcao); + auto tcao_u = std::make_unique(); + auto tcao = tcao_u.get(); + caomgr.registerObject(std::move(tcao_u)); aoids.push_back(tcao->getId()); // Ensure next id is not in registered list @@ -105,8 +106,9 @@ void TestClientActiveObjectMgr::testFreeID() void TestClientActiveObjectMgr::testRegisterObject() { client::ActiveObjectMgr caomgr; - auto tcao = new TestClientActiveObject(); - UASSERT(caomgr.registerObject(tcao)); + auto tcao_u = std::make_unique(); + auto tcao = tcao_u.get(); + UASSERT(caomgr.registerObject(std::move(tcao_u))); u16 id = tcao->getId(); @@ -114,8 +116,9 @@ void TestClientActiveObjectMgr::testRegisterObject() UASSERT(tcaoToCompare->getId() == id); UASSERT(tcaoToCompare == tcao); - tcao = new TestClientActiveObject(); - UASSERT(caomgr.registerObject(tcao)); + tcao_u = std::make_unique(); + tcao = tcao_u.get(); + UASSERT(caomgr.registerObject(std::move(tcao_u))); UASSERT(caomgr.getActiveObject(tcao->getId()) == tcao); UASSERT(caomgr.getActiveObject(tcao->getId()) != tcaoToCompare); @@ -125,8 +128,9 @@ void TestClientActiveObjectMgr::testRegisterObject() void TestClientActiveObjectMgr::testRemoveObject() { client::ActiveObjectMgr caomgr; - auto tcao = new TestClientActiveObject(); - UASSERT(caomgr.registerObject(tcao)); + auto tcao_u = std::make_unique(); + auto tcao = tcao_u.get(); + UASSERT(caomgr.registerObject(std::move(tcao_u))); u16 id = tcao->getId(); UASSERT(caomgr.getActiveObject(id) != nullptr) @@ -140,8 +144,10 @@ void TestClientActiveObjectMgr::testRemoveObject() void TestClientActiveObjectMgr::testGetActiveSelectableObjects() { client::ActiveObjectMgr caomgr; - auto obj = new TestSelectableClientActiveObject({v3f{-1, -1, -1}, v3f{1, 1, 1}}); - UASSERT(caomgr.registerObject(obj)); + auto obj_u = std::make_unique( + aabb3f{v3f{-1, -1, -1}, v3f{1, 1, 1}}); + auto obj = obj_u.get(); + UASSERT(caomgr.registerObject(std::move(obj_u))); auto assert_obj_selected = [&] (v3f a, v3f b) { auto actual = caomgr.getActiveSelectableObjects({a, b}); diff --git a/src/unittest/test_serveractiveobjectmgr.cpp b/src/unittest/test_serveractiveobjectmgr.cpp index ac403d1df6a9..3e57eb99a7b0 100644 --- a/src/unittest/test_serveractiveobjectmgr.cpp +++ b/src/unittest/test_serveractiveobjectmgr.cpp @@ -53,15 +53,6 @@ void TestServerActiveObjectMgr::runTests(IGameDef *gamedef) TEST(testGetAddedActiveObjectsAroundPos); } -void clearSAOMgr(server::ActiveObjectMgr *saomgr) -{ - auto clear_cb = [](ServerActiveObject *obj, u16 id) { - delete obj; - return true; - }; - saomgr->clear(clear_cb); -} - //////////////////////////////////////////////////////////////////////////////// void TestServerActiveObjectMgr::testFreeID() @@ -78,8 +69,9 @@ void TestServerActiveObjectMgr::testFreeID() // Register basic objects, ensure we never found for (u8 i = 0; i < UINT8_MAX; i++) { // Register an object - auto sao = new MockServerActiveObject(); - saomgr.registerObject(sao); + auto sao_u = std::make_unique(); + auto sao = sao_u.get(); + saomgr.registerObject(std::move(sao_u)); aoids.push_back(sao->getId()); // Ensure next id is not in registered list @@ -87,14 +79,15 @@ void TestServerActiveObjectMgr::testFreeID() aoids.end()); } - clearSAOMgr(&saomgr); + saomgr.clear(); } void TestServerActiveObjectMgr::testRegisterObject() { server::ActiveObjectMgr saomgr; - auto sao = new MockServerActiveObject(); - UASSERT(saomgr.registerObject(sao)); + auto sao_u = std::make_unique(); + auto sao = sao_u.get(); + UASSERT(saomgr.registerObject(std::move(sao_u))); u16 id = sao->getId(); @@ -102,19 +95,21 @@ void TestServerActiveObjectMgr::testRegisterObject() UASSERT(saoToCompare->getId() == id); UASSERT(saoToCompare == sao); - sao = new MockServerActiveObject(); - UASSERT(saomgr.registerObject(sao)); + sao_u = std::make_unique(); + sao = sao_u.get(); + UASSERT(saomgr.registerObject(std::move(sao_u))); UASSERT(saomgr.getActiveObject(sao->getId()) == sao); UASSERT(saomgr.getActiveObject(sao->getId()) != saoToCompare); - clearSAOMgr(&saomgr); + saomgr.clear(); } void TestServerActiveObjectMgr::testRemoveObject() { server::ActiveObjectMgr saomgr; - auto sao = new MockServerActiveObject(); - UASSERT(saomgr.registerObject(sao)); + auto sao_u = std::make_unique(); + auto sao = sao_u.get(); + UASSERT(saomgr.registerObject(std::move(sao_u))); u16 id = sao->getId(); UASSERT(saomgr.getActiveObject(id) != nullptr) @@ -122,7 +117,7 @@ void TestServerActiveObjectMgr::testRemoveObject() saomgr.removeObject(sao->getId()); UASSERT(saomgr.getActiveObject(id) == nullptr); - clearSAOMgr(&saomgr); + saomgr.clear(); } void TestServerActiveObjectMgr::testGetObjectsInsideRadius() @@ -137,7 +132,7 @@ void TestServerActiveObjectMgr::testGetObjectsInsideRadius() }; for (const auto &p : sao_pos) { - saomgr.registerObject(new MockServerActiveObject(nullptr, p)); + saomgr.registerObject(std::make_unique(nullptr, p)); } std::vector result; @@ -160,7 +155,7 @@ void TestServerActiveObjectMgr::testGetObjectsInsideRadius() saomgr.getObjectsInsideRadius(v3f(), 750000, result, include_obj_cb); UASSERTCMP(int, ==, result.size(), 4); - clearSAOMgr(&saomgr); + saomgr.clear(); } void TestServerActiveObjectMgr::testGetAddedActiveObjectsAroundPos() @@ -175,7 +170,7 @@ void TestServerActiveObjectMgr::testGetAddedActiveObjectsAroundPos() }; for (const auto &p : sao_pos) { - saomgr.registerObject(new MockServerActiveObject(nullptr, p)); + saomgr.registerObject(std::make_unique(nullptr, p)); } std::queue result; @@ -188,5 +183,5 @@ void TestServerActiveObjectMgr::testGetAddedActiveObjectsAroundPos() saomgr.getAddedActiveObjectsAroundPos(v3f(), 740, 50, cur_objects, result); UASSERTCMP(int, ==, result.size(), 2); - clearSAOMgr(&saomgr); + saomgr.clear(); }