Merge PR #1: fix bot routing, node lifetime, reload cleanup, spawn placement and empty-server scheduling

Every failed scheduled start overwrote the rolled Min/MaxInterval with a
hard-coded 5 minutes and never restored it, so on a server that sits empty
the scheduler degenerated into a permanent 5-minute poll. The moment the
first player logged in, an event fired within one retry window and anchored
on their exact position, with a server-wide announcement -- login ambush
rather than a scheduled world event. It also wrote a retry line to
gold-rush.log every 5 minutes forever (Debug defaults to 1).

StartEvent now counts eligible anchors when the schedule expires, holds
without consuming the interval while below MinPlayersOnline, and inserts
GraceMinutes once the world repopulates. The count runs only on schedule
expiry, never per world tick.

New config: GoldRush.RetryMinutes (5), GoldRush.GraceMinutes (15),
GoldRush.MinPlayersOnline (1). GraceMinutes = 0 rolls a fresh interval
instead. GM-forced starts pass scheduled=false and bypass both gates.

Verified with clang -fsyntax-only against the live build's compile flags.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019vTNrJGsprcjdjjjDJD5ZM

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.",
-                    anchor->GetName());
+                LOG_INFO(GoldRushLogFilter, "Gold Rush rejected anchor {} because the zone is no longer eligible; retrying in {} minute(s).",
+                    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))
-            {
-                go->SetRespawnTime(_config.DurationMs);
-                go->SetSpawnedByDefault(false);
+                float z = map->GetHeight(x, y, anchorZ + HeightProbeUp, true, HeightSearchDist);
+                if (z <= InvalidHeightSentinel)
+                    continue;
+
+                // 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 {}.",
-                        entry, x, y, z, map->GetId());
+                    LOG_INFO(GoldRushLogFilter, "Gold Rush node spawned: entry {} at ({:.2f}, {:.2f}, {:.2f}) on map {} after {} attempt(s).",
+                        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;
 };