From f47cb2c711608df373c2b6f2baa355db5216daee Mon Sep 17 00:00:00 2001 From: Itms Date: Sun, 30 Aug 2015 17:42:10 +0000 Subject: [PATCH] Fix the OOS reported by elexis in #3335, and clean some whitespace and style. The list of modified entities was thrown out on global visibility updates (those happen on rejoin) but only in-world entities were actually updated then, causing problems with garrisoning. Now the list of modified entities can happen to be large, so replace the hacky check for infinite loops by a real check. This was SVN commit r16962. --- .../simulation/components/GarrisonHolder.js | 26 +++++++++---------- .../components/CCmpRangeManager.cpp | 20 +++++++------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/binaries/data/mods/public/simulation/components/GarrisonHolder.js b/binaries/data/mods/public/simulation/components/GarrisonHolder.js index 2545253f16..d1d5f84d4b 100644 --- a/binaries/data/mods/public/simulation/components/GarrisonHolder.js +++ b/binaries/data/mods/public/simulation/components/GarrisonHolder.js @@ -284,14 +284,14 @@ GarrisonHolder.prototype.PerformGarrison = function(entity) var cmpAura = Engine.QueryInterface(entity, IID_Auras); if (cmpAura && cmpAura.HasGarrisonAura()) - cmpAura.ApplyGarrisonBonus(this.entity); + cmpAura.ApplyGarrisonBonus(this.entity); Engine.PostMessage(this.entity, MT_GarrisonedUnitsChanged, { "added" : [entity], "removed": [] }); - + var cmpUnitAI = Engine.QueryInterface(entity, IID_UnitAI); if (cmpUnitAI && cmpUnitAI.IsUnderAlert()) Engine.PostMessage(cmpUnitAI.GetAlertRaiser(), MT_UnitGarrisonedAfterAlert, {"holder": this.entity, "unit": entity}); - + return true; }; @@ -306,7 +306,7 @@ GarrisonHolder.prototype.Eject = function(entity, forced) // Error: invalid entity ID, usually it's already been ejected if (entityIndex == -1) return false; // Fail - + // Find spawning location var cmpFootprint = Engine.QueryInterface(this.entity, IID_Footprint); var cmpHealth = Engine.QueryInterface(this.entity, IID_Health); @@ -331,10 +331,10 @@ GarrisonHolder.prototype.Eject = function(entity, forced) return false; } } - + var cmpNewPosition = Engine.QueryInterface(entity, IID_Position); this.entities.splice(entityIndex, 1); - + for (var vgp of this.visibleGarrisonPoints) { if (vgp.entity != entity) @@ -357,15 +357,15 @@ GarrisonHolder.prototype.Eject = function(entity, forced) var cmpAura = Engine.QueryInterface(entity, IID_Auras); if (cmpAura && cmpAura.HasGarrisonAura()) - cmpAura.RemoveGarrisonBonus(this.entity); + cmpAura.RemoveGarrisonBonus(this.entity); + - cmpNewPosition.JumpTo(pos.x, pos.z); cmpNewPosition.SetHeightOffset(0); // TODO: what direction should they face in? - + Engine.PostMessage(this.entity, MT_GarrisonedUnitsChanged, { "added" : [], "removed": [entity] }); - + return true; }; @@ -432,7 +432,7 @@ GarrisonHolder.prototype.PerformEject = function(entities, forced) else { var cmpObstruction = Engine.QueryInterface(entity, IID_Obstruction); - failedRadius = cmpObstruction ? cmpObstruction.GetUnitRadius() : 0; + failedRadius = cmpObstruction ? cmpObstruction.GetUnitRadius() : 0; } } } @@ -506,7 +506,7 @@ GarrisonHolder.prototype.UnloadAllByOwner = function(owner, forced) if (cmpOwnership && cmpOwnership.GetOwner() == owner) entities.push(entity); } - + return this.PerformEject(entities, forced); }; @@ -530,7 +530,7 @@ GarrisonHolder.prototype.UnloadAllOwn = function(forced) if (cmpOwnership && cmpOwnership.GetOwner() == owner) entities.push(entity); } - + return this.PerformEject(entities, forced); }; diff --git a/source/simulation2/components/CCmpRangeManager.cpp b/source/simulation2/components/CCmpRangeManager.cpp index d933a1fb94..c3aa248ce5 100644 --- a/source/simulation2/components/CCmpRangeManager.cpp +++ b/source/simulation2/components/CCmpRangeManager.cpp @@ -1606,16 +1606,13 @@ public: { PROFILE("UpdateVisibilityData"); - if (m_GlobalVisibilityUpdate) - m_ModifiedEntities.clear(); - for (i32 n = 0; n < m_LosTilesPerSide * m_LosTilesPerSide; ++n) { for (player_id_t player = 1; player < MAX_LOS_PLAYER_ID + 1; ++player) { if (IsVisibilityDirty(m_DirtyVisibility[n], player) || m_GlobalPlayerVisibilityUpdate[player-1] == 1 || m_GlobalVisibilityUpdate) { - for (auto& ent : m_LosTiles[n]) + for (const entity_id_t& ent : m_LosTiles[n]) UpdateVisibility(ent, player); } } @@ -1625,16 +1622,17 @@ public: std::fill(m_GlobalPlayerVisibilityUpdate.begin(), m_GlobalPlayerVisibilityUpdate.end(), 0); m_GlobalVisibilityUpdate = false; - // Calling UpdateVisibility can modify m_ModifiedEntities, so be careful - // Infinite loops could be triggered by feedback between entities and their mirages - int check = 0; + // Calling UpdateVisibility can modify m_ModifiedEntities, so be careful: + // infinite loops could be triggered by feedback between entities and their mirages. + std::map attempts; while (!m_ModifiedEntities.empty()) { - ++check; - ENSURE(check < 1000 && "Possible infinite loop in UpdateVisibilityData"); - entity_id_t ent = m_ModifiedEntities.back(); m_ModifiedEntities.pop_back(); + + ++attempts[ent]; + ENSURE(attempts[ent] < 100 && "Infinite loop in UpdateVisibilityData"); + UpdateVisibility(ent); } } @@ -1656,7 +1654,7 @@ public: if (oldVis == newVis) return; - + itEnts->second.visibilities = (itEnts->second.visibilities & ~(0x3 << 2 * (player - 1))) | (newVis << 2 * (player - 1)); CMessageVisibilityChanged msg(player, ent, oldVis, newVis);