From 6244e2bc6531ff45df03af55b4a937726c38d684 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 16:49:09 +0000 Subject: [PATCH 1/2] fix: correct bot routing, node lifetime, reload cleanup and spawn placement Four defects found reviewing this against AzerothCore efe123fab and mod-playerbots 8d9f6aa6 (both 2026-08-14). 1. Bot routing did nothing. Engine::ChangeStrategy switches on name[0] and only handles '+', '-', '~' and '?'; a bare "new rpg" fell through every case. Bots still drifted toward the hotspot, but only because rpgInfo.ChangeToGoGrind() works and AiFactory already grants the strategy when AiPlayerbot.EnableNewRpgStrategy is on. Now sends "+new rpg", and only to BOT_STATE_NON_COMBAT -- the combat engine never carries it. 2. Node lifetime was off by 1000x. The respawnTime argument to Map::SummonGameObject lands in GameObject::SetRespawnTime(int32), which is seconds. Passing DurationMs made a 20-minute event's nodes live ~13.9 days, so they only ever vanished via the explicit despawn at event end. 3. LoadConfig cleared _spawnedNodes and _active without despawning, so a `.reload config` mid-event orphaned every node permanently (compounded by defect 2). It now tears the running event down first. 4. Spawns reused the anchor's exact Z for every scattered X/Y with no ground probe and no LOS check, leaving nodes floating or buried on the hilly zones in the default pool. Placement now probes ground height, rejects drift beyond 20y, requires LOS from the anchor, and retries up to 8 times. Verified with a clang -fsyntax-only pass using the live build's own compile flags plus the mod-playerbots include tree. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019vTNrJGsprcjdjjjDJD5ZM --- CHANGELOG.md | 27 ++++++++++++ src/GoldRush.cpp | 104 ++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 112 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c22e22c..dc8e312 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,33 @@ 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. + +### Changed +- 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 diff --git a/src/GoldRush.cpp b/src/GoldRush.cpp index 481bb75..3349957 100644 --- a/src/GoldRush.cpp +++ b/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; @@ -144,6 +152,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("GoldRush.Enable", true); _config.VerboseLogging = sConfigMgr->GetOption("GoldRush.VerboseLogging", false); _config.MinIntervalMs = sConfigMgr->GetOption("GoldRush.MinIntervalMinutes", 120) * MINUTE * IN_MILLISECONDS; @@ -844,7 +864,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(1, _config.DurationMs / IN_MILLISECONDS); + constexpr float twoPi = 6.28318530717958647692f; + float const anchorZ = anchor->GetPositionZ(); + for (uint32 i = 0; i < nodeCount; ++i) { std::vector const* pool = nullptr; @@ -861,44 +888,76 @@ private: continue; uint32 entry = Acore::Containers::SelectRandomContainerElement(*pool); - float angle = RandomFloat(0.0f, twoPi); - float distance = RandomFloat(0.0f, _config.SpawnRadius); - 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)) + // 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) { - go->SetRespawnTime(_config.DurationMs); - go->SetSpawnedByDefault(false); + float angle = RandomFloat(0.0f, twoPi); + + // 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 = 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(); } 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)) { - if (GameObject* go = map->GetGameObject(node.Guid)) - { - go->DespawnOrUnsummon(); - ++_telemetry.NodesRemoved; - } + go->DespawnOrUnsummon(); + ++_telemetry.NodesRemoved; } } @@ -957,8 +1016,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 +1078,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(); From 2857b0580ef89b75aba0e716a61d2a3734dc0f02 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 16:58:04 +0000 Subject: [PATCH 2/2] feat: hold the scheduler on an empty server instead of polling into an ambush 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 Claude-Session: https://claude.ai/code/session_019vTNrJGsprcjdjjjDJD5ZM --- CHANGELOG.md | 10 +++++ conf/gold_rush.conf.dist | 28 ++++++++++++ src/GoldRush.cpp | 96 +++++++++++++++++++++++++++++++++++----- 3 files changed, 123 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc8e312..b8c8298 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,17 @@ All notable changes to `mod-gold-rush` will be documented in this file. 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 diff --git a/conf/gold_rush.conf.dist b/conf/gold_rush.conf.dist index 35cacbc..71ebe93 100644 --- a/conf/gold_rush.conf.dist +++ b/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 diff --git a/src/GoldRush.cpp b/src/GoldRush.cpp index 3349957..ccc6048 100644 --- a/src/GoldRush.cpp +++ b/src/GoldRush.cpp @@ -80,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; @@ -173,6 +176,9 @@ public: _config.MaxNodeCount = sConfigMgr->GetOption("GoldRush.MaxNodes", 25); _config.BotsPerFaction = sConfigMgr->GetOption("GoldRush.BotsPerFaction", 6); _config.BotPulseMs = sConfigMgr->GetOption("GoldRush.BotPulseSeconds", 30) * IN_MILLISECONDS; + _config.RetryMs = std::max(1, sConfigMgr->GetOption("GoldRush.RetryMinutes", 5)) * MINUTE * IN_MILLISECONDS; + _config.GraceMs = sConfigMgr->GetOption("GoldRush.GraceMinutes", 15) * MINUTE * IN_MILLISECONDS; + _config.MinPlayersOnline = std::max(1, sConfigMgr->GetOption("GoldRush.MinPlayersOnline", 1)); _config.SpawnRadius = sConfigMgr->GetOption("GoldRush.SpawnRadiusYards", 25.0f); _config.NodeEntries = sConfigMgr->GetOption("GoldRush.NodeEntries", "191133;190176;190171;190172;189973"); _config.ZonePool = sConfigMgr->GetOption("GoldRush.ZonePool", "Un'Goro Crater|Fire Plume Ridge; Winterspring|Frostfire Hot Springs; Eastern Plaguelands|Terrorweb Tunnel; Sholazar Basin|The River's Heart"); @@ -191,6 +197,7 @@ public: _spawnedNodes.clear(); _routedBotIds.clear(); _telemetry = {}; + _worldWasEmpty = false; _timeUntilNextEventMs = 0; _eventTimeRemainingMs = 0; _nextBotPulseMs = _config.BotPulseMs; @@ -267,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) @@ -724,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::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); @@ -746,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; } @@ -760,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; } } @@ -770,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; } @@ -783,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; } @@ -1105,6 +1178,7 @@ private: uint32 _timeUntilNextEventMs = 0; uint32 _eventTimeRemainingMs = 0; uint32 _nextBotPulseMs = 0; + bool _worldWasEmpty = false; bool _sitesInitialized = false; bool _active = false; };