CHANGELOG.md

@@ -2,6 +2,43 @@
 
 All notable changes to `mod-gold-rush` will be documented in this file.
 
+## [Unreleased — hallsworth fork]
+
+### Fixed
+- Bot routing is no longer a silent no-op. `Engine::ChangeStrategy` dispatches on the
+  first character of the strategy name and ignores anything without a `+`/`-`/`~`/`?`
+  prefix, so the bare `"new rpg"` calls did nothing. Now sends `"+new rpg"`, and only
+  to the non-combat engine, which is the only engine `AiFactory` ever adds it to.
+- Temporary node lifetime is no longer 1000x too long. The `respawnTime` argument to
+  `Map::SummonGameObject` reaches `GameObject::SetRespawnTime(int32)`, which is in
+  seconds; the module was passing milliseconds, turning a 20-minute event into a
+  ~14-day one. Nodes never expired on their own.
+- A config reload during a live event no longer orphans its gameobjects. `LoadConfig`
+  cleared the tracked node list without despawning, leaving nodes in the world with
+  nothing left to remove them.
+- Nodes are placed on real ground. Spawns previously reused the anchor's exact Z for
+  every scattered X/Y, so on sloped terrain they floated or sank. Placement now probes
+  ground height, rejects cliffs and lower floors, requires line of sight to the anchor,
+  and retries a few times before giving up on a node.
+
+### Added
+- `GoldRush.RetryMinutes` (default 5), `GoldRush.GraceMinutes` (default 15) and
+  `GoldRush.MinPlayersOnline` (default 1).
+
+### Changed
+- The scheduler no longer collapses to a fixed retry poll on an empty server. Previously
+  every failed attempt overwrote the rolled Min/MaxInterval with a hard-coded 5 minutes
+  and stayed there, so the first player to log in got an event announced on top of them
+  within one retry window. It now holds while the world is below `MinPlayersOnline`, and
+  waits `GraceMinutes` once the world repopulates before running. GM-forced starts
+  (`.goldrush start` / `teststart`) bypass both checks.
+- Node scatter uses a square-root radius so points spread evenly across the disc
+  instead of bunching near the anchor.
+- `DespawnHotspotNodes` resolves the map once instead of per node, and warns if the
+  map cannot be resolved.
+- Dropped the redundant `SetRespawnTime` / `SetSpawnedByDefault` calls after
+  `Map::SummonGameObject`, which already does both.
+
 ## [Unreleased]
 
 ### Added

conf/gold_rush.conf.dist

@@ -46,6 +46,34 @@ GoldRush.MaxIntervalMinutes = 240
 
 GoldRush.DurationMinutes = 20
 
+#
+#    GoldRush.RetryMinutes
+#        Description: How long to wait before trying again when a scheduled event cannot
+#                     start (nobody online, no eligible zone, nowhere to place nodes).
+#                     The rolled Min/MaxInterval is not consumed while holding.
+#        Default:     5
+
+GoldRush.RetryMinutes = 5
+
+#
+#    GoldRush.GraceMinutes
+#        Description: After the server has been sitting below MinPlayersOnline, wait this
+#                     long once it repopulates before running an event. Stops the first
+#                     person to log in from having a Gold Rush announced on top of them.
+#                     Set to 0 to roll a fresh Min/MaxInterval instead.
+#        Default:     15
+
+GoldRush.GraceMinutes = 15
+
+#
+#    GoldRush.MinPlayersOnline
+#        Description: Minimum number of players standing in an event-eligible zone before
+#                     a scheduled event may run. Playerbots count toward this. GM-forced
+#                     starts ignore it.
+#        Default:     1
+
+GoldRush.MinPlayersOnline = 1
+
 #
 #    GoldRush.MinNodes
 #        Description: Minimum number of temporary nodes to spawn in the hotspot

src/GoldRush.cpp

@@ -32,6 +32,14 @@ namespace
 {
 static char const* const GoldRushLogFilter = "module.goldrush";
 
+// Node placement tuning.
+static constexpr uint8 MaxPlacementAttempts = 8;      // candidate spots tried per node
+static constexpr float HeightProbeUp        = 5.0f;   // start the ground probe above the anchor
+static constexpr float HeightSearchDist     = 50.0f;  // how far down the probe may search
+static constexpr float MaxVerticalDrift     = 20.0f;  // reject ground this far off the anchor's
+// Mirrors INVALID_HEIGHT from GridTerrainData.h without taking a dependency on that header.
+static constexpr float InvalidHeightSentinel = -99999.0f;
+
 struct GoldRushSite
 {
     std::string ZoneLabel;
@@ -72,6 +80,9 @@ public:
     uint32 MaxNodeCount = 25;
     uint32 BotsPerFaction = 6;
     uint32 BotPulseMs = 30 * IN_MILLISECONDS;
+    uint32 RetryMs = 5 * MINUTE * IN_MILLISECONDS;
+    uint32 GraceMs = 15 * MINUTE * IN_MILLISECONDS;
+    uint32 MinPlayersOnline = 1;
     float SpawnRadius = 25.0f;
     std::string NodeEntries;
     std::string ZonePool;
@@ -144,6 +155,18 @@ class GoldRushManager
 public:
     void LoadConfig()
     {
+        // A config reload (or `.reload config`) used to wipe the tracked node list while an
+        // event was live, orphaning every temporary gameobject in the world with no owner
+        // left to despawn it. Tear the running event down first, then rebuild state.
+        if (_active)
+        {
+            LOG_INFO(GoldRushLogFilter, "Gold Rush config reload during an active event; despawning {} temporary node(s) and releasing routed bots first.",
+                _spawnedNodes.size());
+
+            DespawnHotspotNodes();
+            ClearBotRouting();
+        }
+
         _config.Enabled = sConfigMgr->GetOption<bool>("GoldRush.Enable", true);
         _config.VerboseLogging = sConfigMgr->GetOption<bool>("GoldRush.VerboseLogging", false);
         _config.MinIntervalMs = sConfigMgr->GetOption<uint32>("GoldRush.MinIntervalMinutes", 120) * MINUTE * IN_MILLISECONDS;
@@ -153,6 +176,9 @@ public:
         _config.MaxNodeCount = sConfigMgr->GetOption<uint32>("GoldRush.MaxNodes", 25);
         _config.BotsPerFaction = sConfigMgr->GetOption<uint32>("GoldRush.BotsPerFaction", 6);
         _config.BotPulseMs = sConfigMgr->GetOption<uint32>("GoldRush.BotPulseSeconds", 30) * IN_MILLISECONDS;
+        _config.RetryMs = std::max<uint32>(1, sConfigMgr->GetOption<uint32>("GoldRush.RetryMinutes", 5)) * MINUTE * IN_MILLISECONDS;
+        _config.GraceMs = sConfigMgr->GetOption<uint32>("GoldRush.GraceMinutes", 15) * MINUTE * IN_MILLISECONDS;
+        _config.MinPlayersOnline = std::max<uint32>(1, sConfigMgr->GetOption<uint32>("GoldRush.MinPlayersOnline", 1));
         _config.SpawnRadius = sConfigMgr->GetOption<float>("GoldRush.SpawnRadiusYards", 25.0f);
         _config.NodeEntries = sConfigMgr->GetOption<std::string>("GoldRush.NodeEntries", "191133;190176;190171;190172;189973");
         _config.ZonePool = sConfigMgr->GetOption<std::string>("GoldRush.ZonePool", "Un'Goro Crater|Fire Plume Ridge; Winterspring|Frostfire Hot Springs; Eastern Plaguelands|Terrorweb Tunnel; Sholazar Basin|The River's Heart");
@@ -171,6 +197,7 @@ public:
         _spawnedNodes.clear();
         _routedBotIds.clear();
         _telemetry = {};
+        _worldWasEmpty = false;
         _timeUntilNextEventMs = 0;
         _eventTimeRemainingMs = 0;
         _nextBotPulseMs = _config.BotPulseMs;
@@ -247,7 +274,7 @@ public:
         if (!site.ZoneId || !anchor || !anchor->IsInWorld() || !anchor->GetMap())
             return false;
 
-        return StartEvent(anchor, site);
+        return StartEvent(anchor, site, false);
     }
 
     bool ForceTestStart(Player* anchor)
@@ -704,11 +731,72 @@ private:
         return Acore::Containers::SelectRandomContainerElement(candidates);
     }
 
-    bool StartEvent(Player* anchorOverride = nullptr, GoldRushSite siteOverride = {})
+    // Counts players (bots included) standing somewhere an event could legally anchor.
+    // Only called when the schedule expires, never per world tick.
+    uint32 CountEligibleAnchors() const
+    {
+        uint32 count = 0;
+        std::shared_lock lock(*HashMapHolder<Player>::GetLock());
+
+        for (auto const& [guid, player] : ObjectAccessor::GetPlayers())
+        {
+            if (IsEligibleAnchor(player))
+                ++count;
+        }
+
+        return count;
+    }
+
+    bool StartEvent(Player* anchorOverride = nullptr, GoldRushSite siteOverride = {}, bool scheduled = true)
     {
         if (_nodeEntries.empty())
             return false;
 
+        // On an empty (or near-empty) server the scheduler used to burn its rolled
+        // interval down to a flat retry and hold there, so the first person to log in
+        // got an event dropped on their head within one retry window. Hold the schedule
+        // while nobody is around, then give the world a grace period once it repopulates.
+        // GM-forced starts skip all of this.
+        if (scheduled)
+        {
+            uint32 const eligible = CountEligibleAnchors();
+
+            if (eligible < _config.MinPlayersOnline)
+            {
+                if (!_worldWasEmpty)
+                {
+                    _worldWasEmpty = true;
+
+                    if (_config.Debug)
+                    {
+                        LOG_INFO(GoldRushLogFilter, "Gold Rush is holding: {} eligible anchor(s) online, {} required. Rechecking every {} minute(s).",
+                            eligible, _config.MinPlayersOnline, _config.RetryMs / (MINUTE * IN_MILLISECONDS));
+                    }
+                }
+                else
+                {
+                    LogVerbose("Gold Rush still holding; no eligible anchors online.");
+                }
+
+                _timeUntilNextEventMs = _config.RetryMs;
+                return false;
+            }
+
+            if (_worldWasEmpty)
+            {
+                _worldWasEmpty = false;
+                _timeUntilNextEventMs = _config.GraceMs ? _config.GraceMs : RollIntervalMs();
+
+                if (_config.Debug)
+                {
+                    LOG_INFO(GoldRushLogFilter, "Gold Rush sees {} eligible anchor(s) after an idle period; waiting {} minute(s) before the next event.",
+                        eligible, _timeUntilNextEventMs / (MINUTE * IN_MILLISECONDS));
+                }
+
+                return false;
+            }
+        }
+
         if (!siteOverride.ZoneId)
             siteOverride = anchorOverride ? BuildEligibleSiteFromPlayer(anchorOverride) : SelectSiteForPlayer(anchorOverride);
 
@@ -726,9 +814,11 @@ private:
 
         if (!siteOverride.ZoneId)
         {
-            _timeUntilNextEventMs = 5 * MINUTE * IN_MILLISECONDS;
+            _worldWasEmpty = true;
+            _timeUntilNextEventMs = _config.RetryMs;
             if (_config.Debug)
-                LOG_INFO(GoldRushLogFilter, "Gold Rush could not find any eligible world zone with a live player anchor; retrying in 5 minutes.");
+                LOG_INFO(GoldRushLogFilter, "Gold Rush could not find any eligible world zone with a live player anchor; retrying in {} minute(s).",
+                    _config.RetryMs / (MINUTE * IN_MILLISECONDS));
             return false;
         }
 
@@ -740,9 +830,11 @@ private:
             anchor = FindAnyEligibleWorldPlayer();
             if (!anchor || !anchor->GetMap())
             {
-                _timeUntilNextEventMs = 5 * MINUTE * IN_MILLISECONDS;
+                _worldWasEmpty = true;
+                _timeUntilNextEventMs = _config.RetryMs;
                 if (_config.Debug)
-                    LOG_INFO(GoldRushLogFilter, "Gold Rush hotspot in {} could not find an anchor player; retrying in 5 minutes.", BuildLocationText(_currentSite));
+                    LOG_INFO(GoldRushLogFilter, "Gold Rush hotspot in {} could not find an anchor player; retrying in {} minute(s).",
+                        BuildLocationText(_currentSite), _config.RetryMs / (MINUTE * IN_MILLISECONDS));
                 return false;
             }
         }
@@ -750,10 +842,10 @@ private:
         GoldRushSite anchorSite = BuildEligibleSiteFromPlayer(anchor);
         if (!anchorSite.ZoneId)
         {
-            _timeUntilNextEventMs = 5 * MINUTE * IN_MILLISECONDS;
+            _timeUntilNextEventMs = _config.RetryMs;
             if (_config.Debug)
-                LOG_INFO(GoldRushLogFilter, "Gold Rush rejected anchor {} because the zone is no longer eligible; retrying in 5 minutes.",
+                LOG_INFO(GoldRushLogFilter, "Gold Rush rejected anchor {} because the zone is no longer eligible; retrying in {} minute(s).",
-                    anchor->GetName());
+                    anchor->GetName(), _config.RetryMs / (MINUTE * IN_MILLISECONDS));
             return false;
         }
 
@@ -763,9 +855,10 @@ private:
 
         if (!SpawnHotspot(anchor))
         {
-            _timeUntilNextEventMs = 5 * MINUTE * IN_MILLISECONDS;
+            _timeUntilNextEventMs = _config.RetryMs;
             if (_config.Debug)
-                LOG_INFO(GoldRushLogFilter, "Gold Rush hotspot in {} could not spawn nodes; retrying in 5 minutes.", BuildLocationText(_currentSite));
+                LOG_INFO(GoldRushLogFilter, "Gold Rush hotspot in {} could not spawn nodes; retrying in {} minute(s).",
+                    BuildLocationText(_currentSite), _config.RetryMs / (MINUTE * IN_MILLISECONDS));
             return false;
         }
 
@@ -844,7 +937,14 @@ private:
                 nodeCount, oreEntries.size(), herbEntries.size(), fallbackEntries.size());
         }
 
+        // Map::SummonGameObject feeds this straight into GameObject::SetRespawnTime(int32),
+        // which is measured in SECONDS. Passing milliseconds made every node live ~14 days
+        // instead of the configured duration, so nothing ever expired on its own.
+        uint32 const nodeLifetimeSeconds = std::max<uint32>(1, _config.DurationMs / IN_MILLISECONDS);
+
         constexpr float twoPi = 6.28318530717958647692f;
+        float const anchorZ = anchor->GetPositionZ();
+
         for (uint32 i = 0; i < nodeCount; ++i)
         {
             std::vector<uint32> const* pool = nullptr;
@@ -861,25 +961,53 @@ private:
                 continue;
 
             uint32 entry = Acore::Containers::SelectRandomContainerElement(*pool);
+
+            // Try a handful of candidate spots; a node that cannot be placed on real ground
+            // within line of sight of the anchor is skipped rather than left floating.
+            for (uint8 attempt = 0; attempt < MaxPlacementAttempts; ++attempt)
+            {
                 float angle = RandomFloat(0.0f, twoPi);
-            float distance = RandomFloat(0.0f, _config.SpawnRadius);
+
+                // sqrt() spreads points evenly across the disc; a plain uniform radius piles
+                // most of them near the anchor.
+                float distance = _config.SpawnRadius * std::sqrt(RandomFloat(0.0f, 1.0f));
                 float x = anchor->GetPositionX() + (std::cos(angle) * distance);
                 float y = anchor->GetPositionY() + (std::sin(angle) * distance);
-            float z = anchor->GetPositionZ();
 
-            if (GameObject* go = map->SummonGameObject(entry, x, y, z, anchor->GetOrientation(), 0.0f, 0.0f, 0.0f, 0.0f, _config.DurationMs, true))
+                float z = map->GetHeight(x, y, anchorZ + HeightProbeUp, true, HeightSearchDist);
-            {
+                if (z <= InvalidHeightSentinel)
-                go->SetRespawnTime(_config.DurationMs);
+                    continue;
-                go->SetSpawnedByDefault(false);
+
+                // Reject cliff faces, rooftops and lower floors picked up by the probe.
+                if (std::fabs(z - anchorZ) > MaxVerticalDrift)
+                    continue;
+
+                if (!anchor->IsWithinLOS(x, y, z + 1.0f))
+                    continue;
+
+                GameObject* go = map->SummonGameObject(entry, x, y, z, anchor->GetOrientation(), 0.0f, 0.0f, 0.0f, 0.0f, nodeLifetimeSeconds, true);
+                if (!go)
+                    continue;
+
+                // Map::SummonGameObject already applied the respawn time and cleared
+                // spawned-by-default; re-doing it here is redundant.
                 _spawnedNodes.push_back({ go->GetGUID(), entry });
                 ++_telemetry.NodesSpawned;
 
                 if (_config.VerboseLogging)
                 {
-                    LOG_INFO(GoldRushLogFilter, "Gold Rush node spawned: entry {} at ({:.2f}, {:.2f}, {:.2f}) on map {}.",
+                    LOG_INFO(GoldRushLogFilter, "Gold Rush node spawned: entry {} at ({:.2f}, {:.2f}, {:.2f}) on map {} after {} attempt(s).",
-                        entry, x, y, z, map->GetId());
+                        entry, x, y, z, map->GetId(), attempt + 1);
+                }
+
+                break;
             }
         }
+
+        if (_config.VerboseLogging && _spawnedNodes.size() < nodeCount)
+        {
+            LOG_INFO(GoldRushLogFilter, "Gold Rush placed {} of {} requested node(s); the rest found no valid ground near the anchor.",
+                _spawnedNodes.size(), nodeCount);
         }
 
         return !_spawnedNodes.empty();
@@ -887,20 +1015,24 @@ private:
 
     void DespawnHotspotNodes()
     {
+        Map* map = sMapMgr->FindMap(_currentSite.MapId, 0);
+        if (!map && !_spawnedNodes.empty())
+        {
+            LOG_WARN(GoldRushLogFilter, "Gold Rush could not resolve map {} to despawn {} temporary node(s); they will expire on their own respawn timer.",
+                _currentSite.MapId, _spawnedNodes.size());
+        }
+
         for (SpawnedNode const& node : _spawnedNodes)
         {
-            if (!node.Guid)
+            if (!node.Guid || !map)
                 continue;
 
-            if (Map* map = sMapMgr->FindMap(_currentSite.MapId, 0))
-            {
             if (GameObject* go = map->GetGameObject(node.Guid))
             {
                 go->DespawnOrUnsummon();
                 ++_telemetry.NodesRemoved;
             }
         }
-        }
 
         _spawnedNodes.clear();
     }
@@ -957,8 +1089,12 @@ private:
             {
                 WorldPosition pos(_hotspot.GetMapId(), _hotspot.GetPositionX(), _hotspot.GetPositionY(), _hotspot.GetPositionZ());
                 botAI->rpgInfo.ChangeToGoGrind(pos);
-                botAI->ChangeStrategy("new rpg", BOT_STATE_NON_COMBAT);
+
-                botAI->ChangeStrategy("new rpg", BOT_STATE_COMBAT);
+                // Engine::ChangeStrategy dispatches on the first character and ignores any
+                // name without a +/-/~/? prefix, so the old bare "new rpg" calls were silent
+                // no-ops. "new rpg" is a non-combat strategy (AiFactory only ever adds it to
+                // the non-combat engine), so the old BOT_STATE_COMBAT call was wrong as well.
+                botAI->ChangeStrategy("+new rpg", BOT_STATE_NON_COMBAT);
                 _routedBotIds.insert(bot->GetGUID().GetCounter());
                 ++routedCount;
 
@@ -1015,6 +1151,9 @@ private:
             if (!bot)
                 continue;
 
+            // Only the rpg target is cleared. "new rpg" is part of the bots' default
+            // non-combat loadout when AiPlayerbot.EnableNewRpgStrategy is on, so removing
+            // it here would strip behaviour the module never granted.
             if (PlayerbotAI* botAI = GET_PLAYERBOT_AI(bot))
                 botAI->rpgInfo.ChangeToIdle();
 
@@ -1039,6 +1178,7 @@ private:
     uint32 _timeUntilNextEventMs = 0;
     uint32 _eventTimeRemainingMs = 0;
     uint32 _nextBotPulseMs = 0;
+    bool _worldWasEmpty = false;
     bool _sitesInitialized = false;
     bool _active = false;
 };