yrtria/mod-gold-rush
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix bot routing no-op, 1000x node lifetime, reload orphaning, floating spawns, and empty-server scheduling #1

Merged
yrtria merged 2 commits from fix/review-round-1 into main 2026-09-03 11:01:12 -06:00
Conversation 0 Commits 2 Files changed 3 +235 -30
yrtria commented 2026-09-03 10:49:34 -06:00
Owner
Copy link

Fixes the four defects found reviewing upstream Old-Man-Warcraft/mod-gold-rush (last upstream commit 2026-04-01) against this server's AzerothCore efe123fab and mod-playerbots 8d9f6aa6, both 2026-08-14.

Fixed

1. Bot routing was a silent no-op. Engine::ChangeStrategy dispatches on name[0] and only acts on +, -, ~, ?. The bare "new rpg" fell through every case. Bots still drifted to the hotspot, but only because rpgInfo.ChangeToGoGrind() works and AiFactory.cpp:615 already grants the strategy when AiPlayerbot.EnableNewRpgStrategy is on (it is, on this server). Now "+new rpg", non-combat engine only.

2. Node lifetime off by 1000x. Map::SummonGameObject's respawnTime reaches GameObject::SetRespawnTime(int32) -- seconds. Passing DurationMs gave every node a ~13.9 day life, so nothing self-expired.

3. Config reload orphaned nodes. LoadConfig wiped _spawnedNodes and _active without despawning. With defect 2 in play, a .reload config mid-event left 15-25 gathering nodes in the world for two weeks.

4. No terrain height on spawn. Every node reused the anchor's exact Z. On Un'Goro / Winterspring / Sholazar -- the hilly zones in the default pool -- a good fraction floated or sank. Now probes ground height, rejects >20y drift, requires LOS from the anchor, retries up to 8 times per node.

Also changed

  • sqrt-distributed scatter radius so nodes spread evenly instead of clustering near the anchor
  • DespawnHotspotNodes resolves the map once instead of per node, and warns when it can't
  • dropped the redundant SetRespawnTime / SetSpawnedByDefault after Map::SummonGameObject

Verification

clang++ -fsyntax-only against the live build's own compile flags plus the mod-playerbots include tree: clean. Not yet built or run -- that needs a worldserver relink.

Scheduler behaviour on an empty server (commit 2)

The module does fail gracefully with nobody online -- no crash, no spawns, no announcement, _active never set. But the bail-out path had a pacing bug worth fixing.

Every failed scheduled start overwrote the rolled Min/MaxInterval with a hard-coded 5 minutes and never restored it. On a server that sits empty this degenerates into a permanent 5-minute poll, so the first player to log in gets an event fired within one retry window, 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 indefinitely, since Debug defaults to 1.

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

New config keys:

Key Default Effect
GoldRush.RetryMinutes 5 recheck cadence while holding
GoldRush.GraceMinutes 15 delay after the world repopulates; 0 rolls a fresh interval
GoldRush.MinPlayersOnline 1 eligible anchors required; playerbots count

.goldrush start and .goldrush teststart pass scheduled=false and bypass both gates, so GM testing is unaffected.

Deliberately not fixed

  • ZonePool is largely advisory. SelectSiteForPlayer prefers the anchor's current zone, and IsEligibleAnchor counts bots as players, so with no humans online the event fires wherever a random bot is standing -- possibly a low-level zone that then gets 25 Titanium Veins. This is upstream's intended "loosened eligibility" behaviour; changing it is a design decision, not a bug fix.
  • Ore/herb classification is English substring matching on gameobject names (vein, lotus, clover, thorn, bloom). It catches all five defaults by luck, but only Titanium Vein classifies as ore, so the alternation yields ~50% Titanium Vein / 50% mixed herbs. Add Saronite Deposit or Adder's Tongue and they fall silently to the fallback pool.
  • bot->GetLevel() < 80 is hard-coded, correct for the default 450-skill node set but it won't follow if NodeEntries changes.
  • IsHotspotNodeEntry is dead code -- defined, never called.
  • SpawnRadiusYards = 25 with 15-25 nodes is roughly one node every 2-3 yards. Worth raising to 60-80 in config; left alone here so the default matches upstream.
Fixes the four defects found reviewing upstream `Old-Man-Warcraft/mod-gold-rush` (last upstream commit 2026-04-01) against this server's AzerothCore `efe123fab` and mod-playerbots `8d9f6aa6`, both 2026-08-14. ## Fixed **1. Bot routing was a silent no-op.** `Engine::ChangeStrategy` dispatches on `name[0]` and only acts on `+`, `-`, `~`, `?`. The bare `"new rpg"` fell through every case. Bots still drifted to the hotspot, but only because `rpgInfo.ChangeToGoGrind()` works and `AiFactory.cpp:615` already grants the strategy when `AiPlayerbot.EnableNewRpgStrategy` is on (it is, on this server). Now `"+new rpg"`, non-combat engine only. **2. Node lifetime off by 1000x.** `Map::SummonGameObject`'s `respawnTime` reaches `GameObject::SetRespawnTime(int32)` -- seconds. Passing `DurationMs` gave every node a ~13.9 day life, so nothing self-expired. **3. Config reload orphaned nodes.** `LoadConfig` wiped `_spawnedNodes` and `_active` without despawning. With defect 2 in play, a `.reload config` mid-event left 15-25 gathering nodes in the world for two weeks. **4. No terrain height on spawn.** Every node reused the anchor's exact Z. On Un'Goro / Winterspring / Sholazar -- the hilly zones in the default pool -- a good fraction floated or sank. Now probes ground height, rejects >20y drift, requires LOS from the anchor, retries up to 8 times per node. ## Also changed - sqrt-distributed scatter radius so nodes spread evenly instead of clustering near the anchor - `DespawnHotspotNodes` resolves the map once instead of per node, and warns when it can't - dropped the redundant `SetRespawnTime` / `SetSpawnedByDefault` after `Map::SummonGameObject` ## Verification `clang++ -fsyntax-only` against the live build's own compile flags plus the mod-playerbots include tree: clean. Not yet built or run -- that needs a worldserver relink. ## Scheduler behaviour on an empty server (commit 2) The module does *fail gracefully* with nobody online -- no crash, no spawns, no announcement, `_active` never set. But the bail-out path had a pacing bug worth fixing. Every failed scheduled start overwrote the rolled `Min/MaxInterval` with a hard-coded 5 minutes and never restored it. On a server that sits empty this degenerates into a permanent 5-minute poll, so the first player to log in gets an event fired within one retry window, 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 indefinitely, since `Debug` defaults to 1. `StartEvent` now counts eligible anchors when the schedule expires, holds *without consuming the interval* while below `MinPlayersOnline`, and inserts a grace period once the world repopulates. The count runs only on schedule expiry, never per world tick. New config keys: | Key | Default | Effect | | --- | --- | --- | | `GoldRush.RetryMinutes` | 5 | recheck cadence while holding | | `GoldRush.GraceMinutes` | 15 | delay after the world repopulates; 0 rolls a fresh interval | | `GoldRush.MinPlayersOnline` | 1 | eligible anchors required; playerbots count | `.goldrush start` and `.goldrush teststart` pass `scheduled=false` and bypass both gates, so GM testing is unaffected. ## Deliberately not fixed - **`ZonePool` is largely advisory.** `SelectSiteForPlayer` prefers the anchor's current zone, and `IsEligibleAnchor` counts bots as players, so with no humans online the event fires wherever a random bot is standing -- possibly a low-level zone that then gets 25 Titanium Veins. This is upstream's intended "loosened eligibility" behaviour; changing it is a design decision, not a bug fix. - **Ore/herb classification is English substring matching** on gameobject names (`vein`, `lotus`, `clover`, `thorn`, `bloom`). It catches all five defaults by luck, but only Titanium Vein classifies as ore, so the alternation yields ~50% Titanium Vein / 50% mixed herbs. Add Saronite Deposit or Adder's Tongue and they fall silently to the fallback pool. - **`bot->GetLevel() < 80` is hard-coded**, correct for the default 450-skill node set but it won't follow if `NodeEntries` changes. - **`IsHotspotNodeEntry` is dead code** -- defined, never called. - **`SpawnRadiusYards = 25` with 15-25 nodes** is roughly one node every 2-3 yards. Worth raising to 60-80 in config; left alone here so the default matches upstream.
yrtria added 1 commit 2026-09-03 10:49:35 -06:00 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019vTNrJGsprcjdjjjDJD5ZM
yrtria added 1 commit 2026-09-03 10:58:09 -06:00 
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
yrtria changed title from Fix bot routing no-op, 1000x node lifetime, reload orphaning, and floating spawns to Fix bot routing no-op, 1000x node lifetime, reload orphaning, floating spawns, and empty-server scheduling 2026-09-03 10:58:21 -06:00
yrtria merged commit 4f7c9f8c62 into main 2026-09-03 11:01:12 -06:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: yrtria/mod-gold-rush#1
No description provided.
Delete branch "fix/review-round-1"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?