From c2a0333901a696f7dcb67356aeb0206b89be14e6 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 15 Sep 2017 12:19:01 +0200 Subject: ServerEnv: Clean up object lifecycle handling (#6414) * ServerEnv: Clean up object lifecycle handling --- src/content_sao.cpp | 13 +- src/network/serverpackethandler.cpp | 8 +- src/script/cpp_api/s_base.cpp | 4 + src/script/lua_api/l_env.cpp | 8 +- src/script/lua_api/l_object.cpp | 11 +- src/serverenvironment.cpp | 326 +++++++++++++++--------------------- src/serverenvironment.h | 11 +- src/serverobject.cpp | 2 +- src/serverobject.h | 26 +-- 9 files changed, 185 insertions(+), 224 deletions(-) diff --git a/src/content_sao.cpp b/src/content_sao.cpp index be1c52fe6..d60a30840 100644 --- a/src/content_sao.cpp +++ b/src/content_sao.cpp @@ -59,7 +59,7 @@ public: m_age += dtime; if(m_age > 10) { - m_removed = true; + m_pending_removal = true; return; } @@ -415,7 +415,7 @@ void LuaEntitySAO::step(float dtime, bool send_recommended) infostream << "Remove SAO " << m_id << "(" << m_init_name << "), outside of limits" << std::endl; m_pending_deactivation = true; - m_removed = true; + m_pending_removal = true; return; } } @@ -555,9 +555,9 @@ int LuaEntitySAO::punch(v3f dir, ServerActiveObject *puncher, float time_from_last_punch) { - if (!m_registered){ + if (!m_registered) { // Delete unknown LuaEntities when punched - m_removed = true; + m_pending_removal = true; return 0; } @@ -601,7 +601,7 @@ int LuaEntitySAO::punch(v3f dir, } if (getHP() == 0) - m_removed = true; + m_pending_removal = true; @@ -1361,11 +1361,10 @@ void PlayerSAO::setWieldIndex(int i) } } -// Erase the peer id and make the object for removal void PlayerSAO::disconnected() { m_peer_id = 0; - m_removed = true; + m_pending_removal = true; } void PlayerSAO::unlinkPlayerSessionAndSave() diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp index 62ffdde17..452abdea5 100644 --- a/src/network/serverpackethandler.cpp +++ b/src/network/serverpackethandler.cpp @@ -1441,8 +1441,8 @@ void Server::handleCommand_Interact(NetworkPacket* pkt) playersao->noCheatDigStart(p_under); } else if (pointed.type == POINTEDTHING_OBJECT) { - // Skip if object has been removed - if (pointed_object->m_removed) + // Skip if object can't be interacted with anymore + if (pointed_object->isGone()) return; actionstream<getName()<<" punches object " @@ -1600,8 +1600,8 @@ void Server::handleCommand_Interact(NetworkPacket* pkt) if (pointed.type == POINTEDTHING_OBJECT) { // Right click object - // Skip if object has been removed - if (pointed_object->m_removed) + // Skip if object can't be interacted with anymore + if (pointed_object->isGone()) return; actionstream << player->getName() << " right-clicks object " diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp index 4d7461c5b..2537d895f 100644 --- a/src/script/cpp_api/s_base.cpp +++ b/src/script/cpp_api/s_base.cpp @@ -322,6 +322,10 @@ void ScriptApiBase::objectrefGetOrCreate(lua_State *L, ObjectRef::create(L, cobj); } else { push_objectRef(L, cobj->getId()); + if (cobj->isGone()) + warningstream << "ScriptApiBase::objectrefGetOrCreate(): " + << "Pushing ObjectRef to removed/deactivated object" + << ", this is probably a bug." << std::endl; } } diff --git a/src/script/lua_api/l_env.cpp b/src/script/lua_api/l_env.cpp index e7284b035..393c4ed5f 100644 --- a/src/script/lua_api/l_env.cpp +++ b/src/script/lua_api/l_env.cpp @@ -537,9 +537,11 @@ int ModApiEnvMod::l_get_objects_inside_radius(lua_State *L) std::vector::const_iterator iter = ids.begin(); for(u32 i = 0; iter != ids.end(); ++iter) { ServerActiveObject *obj = env->getActiveObject(*iter); - // Insert object reference into table - script->objectrefGetOrCreate(L, obj); - lua_rawseti(L, -2, ++i); + if (!obj->isGone()) { + // Insert object reference into table + script->objectrefGetOrCreate(L, obj); + lua_rawseti(L, -2, ++i); + } } return 1; } diff --git a/src/script/lua_api/l_object.cpp b/src/script/lua_api/l_object.cpp index aaab0d98e..0195dc399 100644 --- a/src/script/lua_api/l_object.cpp +++ b/src/script/lua_api/l_object.cpp @@ -137,16 +137,15 @@ int ObjectRef::l_remove(lua_State *L) if (co->getType() == ACTIVEOBJECT_TYPE_PLAYER) return 0; - const UNORDERED_SET &child_ids = co->getAttachmentChildIds(); - UNORDERED_SET::const_iterator it; - for (it = child_ids.begin(); it != child_ids.end(); ++it) { + const std::unordered_set &child_ids = co->getAttachmentChildIds(); + for (int child_id : child_ids) { // Child can be NULL if it was deleted earlier - if (ServerActiveObject *child = env->getActiveObject(*it)) + if (ServerActiveObject *child = env->getActiveObject(child_id)) child->setAttachment(0, "", v3f(0, 0, 0), v3f(0, 0, 0)); } - verbosestream<<"ObjectRef::l_remove(): id="<getId()<m_removed = true; + verbosestream << "ObjectRef::l_remove(): id=" << co->getId() << std::endl; + co->m_pending_removal = true; return 0; } diff --git a/src/serverenvironment.cpp b/src/serverenvironment.cpp index 5e39ce6a5..e4fcf626d 100644 --- a/src/serverenvironment.cpp +++ b/src/serverenvironment.cpp @@ -1018,26 +1018,18 @@ void ServerEnvironment::clearObjects(ClearObjectsMode mode) infostream << "ServerEnvironment::clearObjects(): " << "Removing all active objects" << std::endl; std::vector objects_to_remove; - for (ActiveObjectMap::iterator i = m_active_objects.begin(); - i != m_active_objects.end(); ++i) { - ServerActiveObject* obj = i->second; + for (auto &it : m_active_objects) { + u16 id = it.first; + ServerActiveObject* obj = it.second; if (obj->getType() == ACTIVEOBJECT_TYPE_PLAYER) continue; - u16 id = i->first; + // Delete static object if block is loaded - if (obj->m_static_exists) { - MapBlock *block = m_map->getBlockNoCreateNoEx(obj->m_static_block); - if (block) { - block->m_static_objects.remove(id); - block->raiseModified(MOD_STATE_WRITE_NEEDED, - MOD_REASON_CLEAR_ALL_OBJECTS); - obj->m_static_exists = false; - } - } + deleteStaticFromBlock(obj, id, MOD_REASON_CLEAR_ALL_OBJECTS, true); + // If known by some client, don't delete immediately if (obj->m_known_by_count > 0) { - obj->m_pending_deactivation = true; - obj->m_removed = true; + obj->m_pending_removal = true; continue; } @@ -1054,9 +1046,8 @@ void ServerEnvironment::clearObjects(ClearObjectsMode mode) } // Remove references from m_active_objects - for (std::vector::iterator i = objects_to_remove.begin(); - i != objects_to_remove.end(); ++i) { - m_active_objects.erase(*i); + for (u16 i : objects_to_remove) { + m_active_objects.erase(i); } // Get list of loaded blocks @@ -1399,16 +1390,14 @@ void ServerEnvironment::step(float dtime) for(ActiveObjectMap::iterator i = m_active_objects.begin(); i != m_active_objects.end(); ++i) { ServerActiveObject* obj = i->second; - // Don't step if is to be removed or stored statically - if(obj->m_removed || obj->m_pending_deactivation) + if (obj->isGone()) continue; + // Step object obj->step(dtime, send_recommended); // Read messages from object - while(!obj->m_messages_out.empty()) - { - m_active_object_messages.push( - obj->m_messages_out.front()); + while (!obj->m_messages_out.empty()) { + m_active_object_messages.push(obj->m_messages_out.front()); obj->m_messages_out.pop(); } } @@ -1420,9 +1409,6 @@ void ServerEnvironment::step(float dtime) if(m_object_management_interval.step(dtime, 0.5)) { ScopeProfiler sp(g_profiler, "SEnv: remove removed objs avg /.5s", SPT_AVG); - /* - Remove objects that satisfy (m_removed && m_known_by_count==0) - */ removeRemovedObjects(); } @@ -1542,7 +1528,7 @@ void ServerEnvironment::getAddedActiveObjects(PlayerSAO *playersao, s16 radius, player_radius_f = 0; /* Go through the object list, - - discard m_removed objects, + - discard removed/deactivated objects, - discard objects that are too far away, - discard objects that are found in current_objects. - add remaining objects to added_objects @@ -1556,8 +1542,7 @@ void ServerEnvironment::getAddedActiveObjects(PlayerSAO *playersao, s16 radius, if (object == NULL) continue; - // Discard if removed or deactivating - if(object->m_removed || object->m_pending_deactivation) + if (object->isGone()) continue; f32 distance_f = object->getBasePosition(). @@ -1615,7 +1600,7 @@ void ServerEnvironment::getRemovedActiveObjects(PlayerSAO *playersao, s16 radius continue; } - if (object->m_removed || object->m_pending_deactivation) { + if (object->isGone()) { removed_objects.push(id); continue; } @@ -1757,7 +1742,7 @@ u16 ServerEnvironment::addActiveObjectRaw(ServerActiveObject *object, } /* - Remove objects that satisfy (m_removed && m_known_by_count==0) + Remove objects that satisfy (isGone() && m_known_by_count==0) */ void ServerEnvironment::removeRemovedObjects() { @@ -1766,62 +1751,54 @@ void ServerEnvironment::removeRemovedObjects() i != m_active_objects.end(); ++i) { u16 id = i->first; ServerActiveObject* obj = i->second; + // This shouldn't happen but check it - if(obj == NULL) - { - infostream<<"NULL object found in ServerEnvironment" - <<" while finding removed objects. id="<m_static_block) << std::endl; } } else { - infostream<<"Failed to emerge block from which an object to " - <<"be deactivated was loaded from. id="<getStaticData(&staticdata_new); StaticObject s_obj(obj->getType(), objectpos, staticdata_new); - block->m_static_objects.insert(id, s_obj); - obj->m_static_block = blockpos_o; - block->raiseModified(MOD_STATE_WRITE_NEEDED, - MOD_REASON_STATIC_DATA_ADDED); + // Save to block where object is located + saveStaticToBlock(blockpos_o, id, obj, s_obj, MOD_REASON_STATIC_DATA_ADDED); - // Delete from block where object was located - block = m_map->emergeBlock(old_static_block, false); - if(!block){ - errorstream<<"ServerEnvironment::deactivateFarObjects(): " - <<"Could not delete object id="<m_static_objects.remove(id); - block->raiseModified(MOD_STATE_WRITE_NEEDED, - MOD_REASON_STATIC_DATA_REMOVED); continue; } - // If block is active, don't remove + // If block is still active, don't remove if(!force_delete && m_active_blocks.contains(blockpos_o)) continue; - verbosestream<<"ServerEnvironment::deactivateFarObjects(): " - <<"deactivating object id="<m_known_by_count > 0 && !force_delete); @@ -2058,7 +2000,6 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) /* Update the static data */ - if(obj->isStaticAllowed()) { // Create new static object @@ -2069,6 +2010,7 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) bool stays_in_same_block = false; bool data_changed = true; + // Check if static data has changed considerably if (obj->m_static_exists) { if (obj->m_static_block == blockpos_o) stays_in_same_block = true; @@ -2087,88 +2029,29 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) (static_old.pos - objectpos).getLength() < save_movem) data_changed = false; } else { - errorstream<<"ServerEnvironment::deactivateFarObjects(): " - <<"id="<m_static_block)<m_static_block) << std::endl; } } } + /* + While changes are always saved, blocks are only marked as modified + if the object has moved or different staticdata. (see above) + */ bool shall_be_written = (!stays_in_same_block || data_changed); + u32 reason = shall_be_written ? MOD_REASON_STATIC_DATA_CHANGED : MOD_REASON_UNKNOWN; // Delete old static object - if(obj->m_static_exists) - { - MapBlock *block = m_map->emergeBlock(obj->m_static_block, false); - if(block) - { - block->m_static_objects.remove(id); - obj->m_static_exists = false; - // Only mark block as modified if data changed considerably - if(shall_be_written) - block->raiseModified(MOD_STATE_WRITE_NEEDED, - MOD_REASON_STATIC_DATA_CHANGED); - } - } + deleteStaticFromBlock(obj, id, reason, false); // Add to the block where the object is located in v3s16 blockpos = getNodeBlockPos(floatToInt(objectpos, BS)); - // Get or generate the block - MapBlock *block = NULL; - try{ - block = m_map->emergeBlock(blockpos); - } catch(InvalidPositionException &e){ - // Handled via NULL pointer - // NOTE: emergeBlock's failure is usually determined by it - // actually returning NULL - } - - if(block) - { - if (block->m_static_objects.m_stored.size() >= g_settings->getU16("max_objects_per_block")) { - warningstream << "ServerEnv: Trying to store id = " << obj->getId() - << " statically but block " << PP(blockpos) - << " already contains " - << block->m_static_objects.m_stored.size() - << " objects." - << " Forcing delete." << std::endl; - force_delete = true; - } else { - // If static counterpart already exists in target block, - // remove it first. - // This shouldn't happen because the object is removed from - // the previous block before this according to - // obj->m_static_block, but happens rarely for some unknown - // reason. Unsuccessful attempts have been made to find - // said reason. - if(id && block->m_static_objects.m_active.find(id) != block->m_static_objects.m_active.end()){ - warningstream<<"ServerEnv: Performing hack #83274" - <m_static_objects.remove(id); - } - // Store static data - u16 store_id = pending_delete ? id : 0; - block->m_static_objects.insert(store_id, s_obj); - - // Only mark block as modified if data changed considerably - if(shall_be_written) - block->raiseModified(MOD_STATE_WRITE_NEEDED, - MOD_REASON_STATIC_DATA_CHANGED); - - obj->m_static_exists = true; - obj->m_static_block = block->getPos(); - } - } - else{ - if(!force_delete){ - v3s16 p = floatToInt(objectpos, BS); - errorstream<<"ServerEnv: Could not find or generate " - <<"a block for storing id="<getId() - <<" statically (pos="<m_pending_deactivation = true; continue; } - - verbosestream<<"ServerEnvironment::deactivateFarObjects(): " - <<"object id="<removingFromEnvironment(); @@ -2203,10 +2085,72 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) } // Remove references from m_active_objects - for(std::vector::iterator i = objects_to_remove.begin(); - i != objects_to_remove.end(); ++i) { - m_active_objects.erase(*i); + for (u16 i : objects_to_remove) { + m_active_objects.erase(i); + } +} + +void ServerEnvironment::deleteStaticFromBlock( + ServerActiveObject *obj, u16 id, u32 mod_reason, bool no_emerge) +{ + if (!obj->m_static_exists) + return; + + MapBlock *block; + if (no_emerge) + block = m_map->getBlockNoCreateNoEx(obj->m_static_block); + else + block = m_map->emergeBlock(obj->m_static_block, false); + if (!block) { + if (!no_emerge) + errorstream << "ServerEnv: Failed to emerge block " << PP(obj->m_static_block) + << " when deleting static data of object from it. id=" << id << std::endl; + return; } + + block->m_static_objects.remove(id); + if (mod_reason != MOD_REASON_UNKNOWN) // Do not mark as modified if requested + block->raiseModified(MOD_STATE_WRITE_NEEDED, mod_reason); + + obj->m_static_exists = false; +} + +bool ServerEnvironment::saveStaticToBlock( + v3s16 blockpos, u16 store_id, + ServerActiveObject *obj, const StaticObject &s_obj, + u32 mod_reason) +{ + MapBlock *block = nullptr; + try { + block = m_map->emergeBlock(blockpos); + } catch (InvalidPositionException &e) { + // Handled via NULL pointer + // NOTE: emergeBlock's failure is usually determined by it + // actually returning NULL + } + + if (!block) { + errorstream << "ServerEnv: Failed to emerge block " << PP(obj->m_static_block) + << " when saving static data of object to it. id=" << store_id << std::endl; + return false; + } + if (block->m_static_objects.m_stored.size() >= g_settings->getU16("max_objects_per_block")) { + warningstream << "ServerEnv: Trying to store id = " << store_id + << " statically but block " << PP(blockpos) + << " already contains " + << block->m_static_objects.m_stored.size() + << " objects." << std::endl; + return false; + } + + block->m_static_objects.insert(store_id, s_obj); + if (mod_reason != MOD_REASON_UNKNOWN) // Do not mark as modified if requested + block->raiseModified(MOD_STATE_WRITE_NEEDED, mod_reason); + + obj->m_static_exists = true; + obj->m_static_block = blockpos; + + return true; } PlayerDatabase *ServerEnvironment::openPlayerDatabase(const std::string &name, diff --git a/src/serverenvironment.h b/src/serverenvironment.h index 7c370fd54..f6c2f17fc 100644 --- a/src/serverenvironment.h +++ b/src/serverenvironment.h @@ -33,6 +33,7 @@ class PlayerDatabase; class PlayerSAO; class ServerEnvironment; class ActiveBlockModifier; +struct StaticObject; class ServerActiveObject; class Server; class ServerScripting; @@ -362,7 +363,7 @@ private: u16 addActiveObjectRaw(ServerActiveObject *object, bool set_changed, u32 dtime_s); /* - Remove all objects that satisfy (m_removed && m_known_by_count==0) + Remove all objects that satisfy (isGone() && m_known_by_count==0) */ void removeRemovedObjects(); @@ -382,6 +383,14 @@ private: */ void deactivateFarObjects(bool force_delete); + /* + A few helpers used by the three above methods + */ + void deleteStaticFromBlock( + ServerActiveObject *obj, u16 id, u32 mod_reason, bool no_emerge); + bool saveStaticToBlock(v3s16 blockpos, u16 store_id, + ServerActiveObject *obj, const StaticObject &s_obj, u32 mod_reason); + /* Member variables */ diff --git a/src/serverobject.cpp b/src/serverobject.cpp index 191247829..b3cb85b83 100644 --- a/src/serverobject.cpp +++ b/src/serverobject.cpp @@ -25,7 +25,7 @@ with this program; if not, write to the Free Software Foundation, Inc., ServerActiveObject::ServerActiveObject(ServerEnvironment *env, v3f pos): ActiveObject(0), m_known_by_count(0), - m_removed(false), + m_pending_removal(false), m_pending_deactivation(false), m_static_exists(false), m_static_block(1337,1337,1337), diff --git a/src/serverobject.h b/src/serverobject.h index 38204980e..31af5d6a7 100644 --- a/src/serverobject.h +++ b/src/serverobject.h @@ -210,22 +210,26 @@ public: it anymore. - Removal is delayed to preserve the id for the time during which it could be confused to some other object by some client. - - This is set to true by the step() method when the object wants - to be deleted. - - This can be set to true by anything else too. + - This is usually set to true by the step() method when the object wants + to be deleted but can be set by anything else too. */ - bool m_removed; + bool m_pending_removal = false; /* - This is set to true when an object should be removed from the active - object list but couldn't be removed because the id has to be - reserved for some client. + Same purpose as m_pending_removal but for deactivation. + deactvation = save static data in block, remove active object - The environment checks this periodically. If this is true and also - m_known_by_count is true, object is deleted from the active object - list. + If this is set alongside with m_pending_removal, removal takes + priority. */ - bool m_pending_deactivation; + bool m_pending_deactivation = false; + + /* + A getter that unifies the above to answer the question: + "Can the environment still interact with this object?" + */ + inline bool isGone() const + { return m_pending_removal || m_pending_deactivation; } /* Whether the object's static data has been stored to a block -- cgit v1.2.3