Trinitycore: Core/Pools: Objects in the pool stop spawning

[sirikfoll]

Description: Creatures (probably GameObjects too) that belong to a pool, will stop to respawn after some time. I can confirm this happens with Webbed Crusaders(30268 30273) and Serfex the Reaver(28083), for example.
I only monitored it for a few hours, but I think they won't respawn again until a server restart.

Current behaviour: After some number os respawns, the creature no longer spawn.

Expected behaviour: Creatures should keep respawning within the right time.

Steps to reproduce the problem:
To force it happen:

1. Execute: DELETE FROM characters.respawn;
   UPDATE world.creature SET spawntimesecs = 3 where id IN (30273,30268);
2. Start server
3. .gm on
4. .go xyz 6394 229 395 571
5. .cast 44659
6. Repeat step 3 a few times, usally after the second/third time the Webbed Crusaders won't spawn anymore.

Branch(es): 3.3.5

TC rev. hash/commit: e45d54d80e3f

[jackpoz]

@r00ty-tc could you take a look at this ?

[Killyana]

https://github.com/TrinityCore/TrinityCore/issues/19257 the title is wrong and the main issue is also not quite clear.

[ghost]

Do you mean this issue or the one you linked?

[Killyana]

Talking about #19257 I will close it And keep this one, I will just copy the comment to reproduce the issue:

Try to kill this npc then respawn them many times, you will end with none of them spawned:
guid, pool_entry, chance, description
21317, 32492, 0, Silithyd Hive Drone
86076, 32492, 0, Silithyd Invader
86077, 32493, 0, Silithyd Invader
21318, 32493, 0, Silithyd Hive Drone
21314, 32494, 0, Silithyd Hive Drone
86075, 32494, 0, Silithyd Invader
21324, 32495, 0, Silithyd Hive Drone
86079, 32495, 0, Silithyd Invader
86078, 32496, 0, Silithyd Invader
21323, 32496, 0, Silithyd Hive Drone
21325, 32497, 0, Silithyd Hive Drone
86080, 32497, 0, Silithyd Invader
21327, 32498, 0, Silithyd Hive Drone
86081, 32498, 0, Silithyd Invader

[lachtanek]

Probably the same issue as I pointed out in https://github.com/TrinityCore/TrinityCore/pull/21170#issuecomment-355676066, you can be sure by checking if those creatures use new spawning mechanic or the compatibility one
If I find a quick fix for this thats not too hacky I may PR it, but the current pool implementation is weird anyway, needs full rewrite

[Keader]

@Treeston @r00ty-tc has time to check it?

[lost-illusion]

@jackpoz @Killyana @Keader
When we update pool, we select new object for respawn, and this object can be equal to initiator of this update and in this case we call "ReSpawn1Object" - all these function don't have code inside, but in map update we not do anything if object in pool = object stop spawning.

[Killyana]

Can you provide more information, and where this happen in the code?

[Keader]

Is inside of void PoolGroup<T>::SpawnObject(ActivePoolData& spawns, uint32 limit, uint32 triggerFrom) in PoolMgr.cpp

[lost-illusion]

@Killyana
https://i.imgur.com/xX96y5H.jpg

[r00ty-tc]

Actually we discussed this in IRC. I did make a "dirty" correction for this 4 years ago (yeah.. I'm feeling old, doesn't seem so long) in the very first pooling incarnation (defunct, in favour for a to be completed solution). https://github.com/r00ty-tc/TrinityCore/commit/3e7587eb6faaddcf80c3dac4730a2dbf2f30466f#diff-90acd023cd4bbb796987fe7e52212398R356 but, I don't think it's a good solution (On pools, with not many more possible spawns, than the limit, it will be running "searching" for new spots).

The proper way is to "consume" the spawns from an available list, and return them on despawn. But, actually I think it's quite a bit of work. We could build a shortlist before choosing a random value.

Incidentally the problem only happens on explicitly chanced pools. Equal chanced pools will work fine (it already excludes already spawned).

[lost-illusion]

@r00ty-tc we can implement ReSpawnObject functionality: Call DoRespawn for object with some corrections in ProcessRespawn
or
We can spawn only other objects != triggeredFrom

[sirikfoll]

This change alone already seems to solve the problem for dynamic spawns, but it will definitely break m_respawnCompatibilityMode

diff --git a/src/server/game/Pools/PoolMgr.cpp b/src/server/game/Pools/PoolMgr.cpp
index d4c78df0d3..26e03d1832 100644
--- a/src/server/game/Pools/PoolMgr.cpp
+++ b/src/server/game/Pools/PoolMgr.cpp
@@ -358,7 +358,7 @@ void PoolGroup<T>::SpawnObject(ActivePoolData& spawns, uint32 limit, uint32 trig
         {
             if (obj.guid == triggerFrom)
             {
-                ReSpawn1Object(&obj);
+                Spawn1Object(&obj);
                 triggerFrom = 0;
             }
             else

[Treeston]

@sirikfoll it will in this particular case because of respawn time being zero. It won't fix the general case.

Here's the problem that that would cause:

• Pooled objects A and B are destroyed non-simultaneously
• A's respawn timer finishes, pool decides to spawn B
• Before B's original respawn timer ends, the newly-spawned B is destroyed
• This does not generate a new respawn timer, since we already have a pending respawn of B
• The pool now only has one pending respawn timer ticking, when it should have two

[rblanca]

Hi , someone has any clue about where do i start to looking? (it's pretty annoying bug)

[Killyana]

The new pooling rewrite is almost ready for a PR, it will fix this issue.

[Rushor]

where is that pr btw?

so as above said:

// try to spawn rolled objects
        for (PoolObject& obj : rolledObjects)
        {
            if (obj.guid == triggerFrom)
            {
                ReSpawn1Object(&obj);
                triggerFrom = 0;
            }
            else
            {
                spawns.ActivateObject<T>(obj.guid, poolId);
                Spawn1Object(&obj);
            }
        }

if (obj.guid == triggerFrom)

is looking for an empty object then

[Rushor]

There is is https://github.com/TrinityCore/TrinityCore/pull/23603

[Rushor]

can someone provide a good temp solution to fix that? You can contact me in discord aswell Rushor#5576

[Rushor]

okay until the pr is merged:

Reverting it to this: https://github.com/TrinityCore/TrinityCore/commit/de80cd2e0de470522a734e8e7a65e81b82bfc5fb#gitext://gotocommit/de80cd2e0de470522a734e8e7a65e81b82bfc5fb%23diff-90acd023cd4bbb796987fe7e52212398R356 and removing the urand check makes them pool for longer time again.

But as said, that is all just nothing. we need to wait for the pr.

[jackpoz]

There are some pool PRs from Treeston, you could test them and see if they add any new issue and if they fix any

[Killyana]

I already tested Treeston PRs and they fix all pooling issues.

[jackpoz]

Let's merge them then ?

[Killyana]

If Treeton agree, why not

[Faq]

prolly this one first to go https://github.com/TrinityCore/TrinityCore/pull/23676

[Rushor]

would be awesome!

[Rushor]

revert and tempfix:

diff --git a/src/server/game/Pools/PoolMgr.cpp b/src/server/game/Pools/PoolMgr.cpp
index 22e9e5b404..260c4e4050 100644
--- a/src/server/game/Pools/PoolMgr.cpp
+++ b/src/server/game/Pools/PoolMgr.cpp
@@ -328,27 +328,36 @@ void PoolGroup<T>::SpawnObject(ActivePoolData& spawns, uint32 limit, uint32 trig
         // roll objects to be spawned
         if (!ExplicitlyChanced.empty())
         {
-            float roll = (float)rand_chance();
-
-            for (PoolObject& obj : ExplicitlyChanced)
+            while (count && ExplicitlyChanced.size() > rolledObjects.size())
             {
-                roll -= obj.chance;
-                // Triggering object is marked as spawned at this time and can be also rolled (respawn case)
-                // so this need explicit check for this case
-                if (roll < 0 && (obj.guid == triggerFrom || !spawns.IsActiveObject<T>(obj.guid)))
+                --count;
+                float roll = (float)rand_chance();
+
+                for (PoolObject& obj : ExplicitlyChanced)
                 {
-                    rolledObjects.push_back(obj);
-                    break;
+                    roll -= obj.chance;
+                    // Triggering object is marked as spawned at this time and can be also rolled (respawn case)
+                    // so this need explicit check for this case
+                    if (roll < 0 && (obj.guid == triggerFrom || !spawns.IsActiveObject<T>(obj.guid)))
+                    {
+                        rolledObjects.push_back(obj);
+                        break;
+                    }
                 }
             }
         }
-
-        if (!EqualChanced.empty() && rolledObjects.empty())
+        else if (!EqualChanced.empty())
         {
-            std::copy_if(EqualChanced.begin(), EqualChanced.end(), std::back_inserter(rolledObjects), [triggerFrom, &spawns](PoolObject const& object)
+            rolledObjects = EqualChanced;
+
+            for (auto itr = rolledObjects.begin(); itr != rolledObjects.end();)
             {
-                return object.guid == triggerFrom || !spawns.IsActiveObject<T>(object.guid);
-            });
+                // remove most of the active objects so there is higher chance inactive ones are spawned
+                if (spawns.IsActiveObject<T>(itr->guid))
+                    itr = rolledObjects.erase(itr);
+                else
+                    ++itr;
+            }

             Trinity::Containers::RandomResize(rolledObjects, count);
         }
@@ -356,6 +365,9 @@ void PoolGroup<T>::SpawnObject(ActivePoolData& spawns, uint32 limit, uint32 trig
         // try to spawn rolled objects
         for (PoolObject& obj : rolledObjects)
         {
+            if (spawns.IsActiveObject<T>(obj.guid))
+                continue;
+
             if (obj.guid == triggerFrom)
             {
                 ReSpawn1Object(&obj);

[jackpoz]

Could you PR C++ changes so they can be commented and reviewed ?

[Killyana]

The diff posted by @Rushor has a better result IG even if in a some cases after a server restart pooled units could respawn instantly ignoring the respawn time
The issue is caused by the remaining to do here #23603
To do: database update queries still assume one timer per spawnid, needs fix

[Rushor]

well i gonna pr it tomorrow, but there is not that much i can/will change because f the actual PR

[jackpoz]

Fixed by https://github.com/TrinityCore/TrinityCore/pull/24949