From 04839f233f37faa9af406ea66fc6c199127781eb Mon Sep 17 00:00:00 2001 From: sfan5 Date: Fri, 15 Sep 2017 12:19:01 +0200 Subject: [PATCH] ServerEnv: Clean up object lifecycle handling (#6414) * ServerEnv: Clean up object lifecycle handling --- src/content_sao.cpp | 20 +- src/network/serverpackethandler.cpp | 8 +- src/script/cpp_api/s_base.cpp | 4 + src/script/lua_api/l_env.cpp | 2 +- src/script/lua_api/l_object.cpp | 9 +- src/serverenvironment.cpp | 310 ++++++++++++---------------- src/serverenvironment.h | 11 +- src/serverobject.h | 24 ++- 8 files changed, 175 insertions(+), 213 deletions(-) diff --git a/src/content_sao.cpp b/src/content_sao.cpp index 2a2384b3d..f0004da01 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; } @@ -397,12 +397,11 @@ void LuaEntitySAO::step(float dtime, bool send_recommended) { ServerMap *map = dynamic_cast(&m_env->getMap()); assert(map); - if (!m_pending_deactivation && + if (!m_pending_removal && map->saoPositionOverLimit(m_base_position)) { - infostream << "Remove SAO " << m_id << "(" << m_init_name + infostream << "Removing SAO " << m_id << "(" << m_init_name << "), outside of limits" << std::endl; - m_pending_deactivation = true; - m_removed = true; + m_pending_removal = true; return; } } @@ -550,9 +549,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; } @@ -596,12 +595,10 @@ int LuaEntitySAO::punch(v3f dir, } if (getHP() == 0) { - m_removed = true; + m_pending_removal = true; m_env->getScriptIface()->luaentity_on_death(m_id, puncher); } - - return result.wear; } @@ -1353,11 +1350,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 07de20d60..c4c0c9d0d 100644 --- a/src/network/serverpackethandler.cpp +++ b/src/network/serverpackethandler.cpp @@ -1124,8 +1124,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 " @@ -1283,8 +1283,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 32a99826e..c0d9a4f10 100644 --- a/src/script/cpp_api/s_base.cpp +++ b/src/script/cpp_api/s_base.cpp @@ -343,6 +343,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 1d0716484..f9498a9a7 100644 --- a/src/script/lua_api/l_env.cpp +++ b/src/script/lua_api/l_env.cpp @@ -642,7 +642,7 @@ 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); - if (!obj->m_removed) { + if (!obj->isGone()) { // Insert object reference into table script->objectrefGetOrCreate(L, obj); lua_rawseti(L, -2, ++i); diff --git a/src/script/lua_api/l_object.cpp b/src/script/lua_api/l_object.cpp index 9b312b3ee..4e4901db5 100644 --- a/src/script/lua_api/l_object.cpp +++ b/src/script/lua_api/l_object.cpp @@ -140,15 +140,14 @@ int ObjectRef::l_remove(lua_State *L) return 0; const std::unordered_set &child_ids = co->getAttachmentChildIds(); - std::unordered_set::const_iterator it; - for (it = child_ids.begin(); it != child_ids.end(); ++it) { + 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 1abe5054c..a5f307554 100644 --- a/src/serverenvironment.cpp +++ b/src/serverenvironment.cpp @@ -971,24 +971,17 @@ void ServerEnvironment::clearObjects(ClearObjectsMode mode) << "Removing all active objects" << std::endl; std::vector objects_to_remove; for (auto &it : m_active_objects) { + u16 id = it.first; ServerActiveObject* obj = it.second; if (obj->getType() == ACTIVEOBJECT_TYPE_PLAYER) continue; - u16 id = it.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; } @@ -1302,16 +1295,14 @@ void ServerEnvironment::step(float dtime) for (auto &ao_it : m_active_objects) { ServerActiveObject* obj = ao_it.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(); } } @@ -1322,9 +1313,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(); } @@ -1444,7 +1432,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 @@ -1457,8 +1445,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(). @@ -1497,9 +1484,9 @@ void ServerEnvironment::getRemovedActiveObjects(PlayerSAO *playersao, s16 radius /* Go through current_objects; object is removed if: - object is not found in m_active_objects (this is actually an - error condition; objects should be set m_removed=true and removed - only after all clients have been informed about removal), or - - object has m_removed=true, or + error condition; objects should be removed only after all clients + have been informed about removal), or + - object is to be removed or deactivated, or - object is too far away */ for (u16 id : current_objects) { @@ -1512,7 +1499,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; } @@ -1684,7 +1671,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() { @@ -1692,62 +1679,54 @@ void ServerEnvironment::removeRemovedObjects() for (auto &ao_it : m_active_objects) { u16 id = ao_it.first; ServerActiveObject* obj = ao_it.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); @@ -1972,7 +1920,6 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) /* Update the static data */ - if(obj->isStaticAllowed()) { // Create new static object @@ -1983,6 +1930,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; @@ -2001,108 +1949,47 @@ 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(); @@ -2122,6 +2009,69 @@ void ServerEnvironment::deactivateFarObjects(bool _force_delete) } } +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, const std::string &savedir, const Settings &conf) { diff --git a/src/serverenvironment.h b/src/serverenvironment.h index 3fdd6d77e..b2d844048 100644 --- a/src/serverenvironment.h +++ b/src/serverenvironment.h @@ -35,6 +35,7 @@ class PlayerDatabase; class PlayerSAO; class ServerEnvironment; class ActiveBlockModifier; +struct StaticObject; class ServerActiveObject; class Server; class ServerScripting; @@ -368,7 +369,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(); @@ -388,6 +389,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.h b/src/serverobject.h index 33fc3e4a8..304281093 100644 --- a/src/serverobject.h +++ b/src/serverobject.h @@ -212,23 +212,27 @@ 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 = false; + 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 = 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 */ -- 2.25.1