diff --git a/CHANGELOG.md b/CHANGELOG.md index f86e6bd..de30557 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,22 +17,6 @@ All notable changes to `mod-gold-rush` will be documented in this file. global `GoldRush.NodeEntries` pool exactly as before. ### Fixed -- Zone pool parsing no longer truncates every zone to its first resource entry. - `GoldRush.ZonePool` separates zones with `"; "` (semicolon + space) but the - node/bonus entry IDs within a zone with a bare `";"`. `BuildSites()` split on - the bare `";"`, so each zone's entry list was cut down to its first ID — which - is always a herb. That is why events spawned a single wrong-zone herb and no - ore at all (reported in PR #8). The parser now splits on the two-character - `"; "` zone delimiter, so every zone keeps its full node and bonus pools. -- Herb/ore categorization no longer guesses from display-name keywords. The old - `SplitByCategory()` matched "lotus"/"clover"/"thorn"/"bloom" for herbs and - "vein" for ore, which only worked for the original Northrend default pool and - silently fell back to the unsplit pool for almost every v2 zone (Outland/Classic - ore uses "Deposit", not "Vein"; most herb names match no keyword). Replaced with - a static ID→category lookup table built from the DB-verified entry IDs in - `docs/zone-specific-resources-spec.md`, with the old keyword check retained only - as a fallback for entries not in the table. See - `docs/herb-ore-category-fix-spec.md`. - Event start/end announcements no longer crash the server. `StartEvent()` and `EndEvent()` both used `ChatHandler(nullptr).SendWorldText(...)` to broadcast server-wide -- `ChatHandler::SendWorldText` assumes a real session and calls diff --git a/docs/herb-ore-category-fix-spec.md b/docs/herb-ore-category-fix-spec.md deleted file mode 100644 index 8f44440..0000000 --- a/docs/herb-ore-category-fix-spec.md +++ /dev/null @@ -1,114 +0,0 @@ -# Fix: herb/ore categorization breaks for almost every zone in the v2 pool - -## The bug, confirmed by reading the code - -`SplitByCategory()` (`GoldRush.cpp:648`) decides whether a resource entry -is a herb or ore purely by matching keywords in its display name: - -```cpp -bool isHerb = ContainsWord(name, "lotus") || ContainsWord(name, "clover") || ContainsWord(name, "thorn") || ContainsWord(name, "bloom"); -bool isOre = ContainsWord(name, "vein"); -``` - -This was written against the *original 5-entry Northrend default pool* -(Goldclover→"clover", Icethorn→"thorn", Frost Lotus→"lotus", Titanium -**Vein**→"vein") and never updated when the v2 zone-resources feature (PR -#5) introduced ~70 new resource names across 57 zones. - -**Confirmed failure case**: tested live in Hellfire Peninsula. Its -configured pool is Felweed, Dreaming Glory, Fel Iron **Deposit**, -Adamantite **Deposit**. None of these match any herb keyword, and neither -ore entry contains "vein" — most Outland/Classic ore uses "Deposit," not -"Vein." So both `herbEntries` and `oreEntries` come back empty, and -`SpawnHotspot()` (`GoldRush.cpp:998-1017`) falls back to -`fallbackEntries` (the whole site pool, unsplit) for every node. The -intended herb/ore alternation silently never happens for the large -majority of the 57 configured zones — only the handful whose names happen -to contain one of the four hardcoded herb words or "vein" categorize -correctly at all. - -**Observed symptom**: the test run in Hellfire produced overwhelmingly -Felweed with no observed ore, despite the fallback pool containing ore -entries. Whether that's pure sampling variance or a second, separate -issue (see "Worth checking after this fix" below) isn't confirmed from -reading the code alone — don't assume the placement-bias theory without -testing after this fix ships. - -## Fix: stop guessing category from the name, use a known lookup table - -Every entry ID used across the 57-zone pool was already individually -verified against `acore_world.gameobject_template` when the pool was -built (`docs/zone-specific-resources-spec.md`). Rather than continue -patching an ever-growing, ever-fragile keyword list — the next zone added -will just as easily introduce another unmatched name — replace the -name-based guess with a static ID→category lookup built from data that's -already verified correct. - -**Add a lookup table** (e.g. `static const std::unordered_map IsHerbById` or similar, `true` = herb, `false` = ore), populated -from every entry in the tables below. Change `SplitByCategory` to check -this table first; **keep the existing name-keyword check as a fallback -only** for any entry not found in the table (defensive — covers the -original global-pool default entries and anything added by hand later -without an explicit table update). - -### Herb entries (mark `true`) -``` -1618, 3724, 1617, 3725, 1619, 3726, // Peacebloom, Silverleaf, Earthroot -1620, 3727, 1621, 3729, 2045, 1622, 3730, // Mageroyal, Briarthorn, Stranglekelp, Bruiseweed -1623, 1624, 2041, 2042, 2046, 2043, 2866, // Wild Steelbloom, Kingsblood, Liferoot, Fadeleaf, Goldthorn, Khadgar's Whisker, Firebloom -142140, 180165, 142141, 176642, // Purple Lotus, Arthas' Tears -142142, 176636, 180164, 142143, 183046, // Sungrass, Blindweed -142144, 142145, 176637, 176587, 176641, // Ghost Mushroom, Gromsblood, Plaguebloom -176583, 176638, 180167, 176584, 176639, // Golden Sansam, Dreamfoil -180168, 176586, 176640, 180166, 176588, // Mountain Silversage, Icecap -191303, 176589, // Firethorn, Black Lotus -181270, 183044, 181271, 183045, 181275, // Felweed, Dreaming Glory, Ragveil -183043, 181277, 181276, 181279, 181280, 181281, // Terocone, Flame Cap, Netherbloom, Nightmare Vine, Mana Thistle -189973, 190171, 190172, 190176, 190170, // Goldclover, Lichbloom, Icethorn, Frost Lotus, Talandra's Rose -191019, 190169 // Adder's Tongue, Tiger Lily -``` - -### Ore entries (mark `false`) -``` -1731, 2055, 3763, 103713, 181248, // Copper Vein -1732, 2054, 3764, 103711, 181249, // Tin Vein -1733, 105569, // Silver Vein -1735, // Iron Deposit -1734, 150080, 181109, // Gold Vein -2040, 150079, 176645, // Mithril Deposit -2047, 150081, 181108, // Truesilver Deposit -324, 150082, 176643, 175404, // Small/Rich Thorium Vein -165658, // Dark Iron Deposit -181555, 181556, 181569, 181570, 181557, // Fel Iron, Adamantite (+ Rich), Khorium -189978, 189979, 189980, 189981, 191133 // Cobalt/Saronite Deposit (+ Rich), Titanium Vein -``` - -(This is every entry that appears anywhere in `gold_rush.conf.dist`'s -`GoldRush.ZonePool` plus the module's own default global -`GoldRush.NodeEntries` — cross-check against both files while -implementing, in case an entry was missed here.) - -## Worth checking after this fix — don't build blind for this part - -Once real ID-based categorization is in, re-test Hellfire Peninsula (or -another Outland zone) with `.goldrush teststart` a few times. If ore -still rarely/never appears despite `oreEntries` now correctly populated, -look at the placement-validation loop (`GoldRush.cpp:1019` onward) — -specifically the `MaxVerticalDrift` cliff/rock-face rejection. Ore -deposits are commonly placed on steep terrain in the actual game world; -if placement attempts for ore entries are failing validation far more -often than herb entries, that's a second, separate bug worth its own fix -— but confirm it's actually happening before changing that logic, rather -than assuming. - -## Testing - -- Hellfire Peninsula (the confirmed failure case) should now show a mix - of Felweed/Dreaming Glory *and* Fel Iron/Adamantite, not all-herb. -- Spot-check one or two zones whose original names *did* match the old - keyword list (e.g. Un'Goro Crater — Firethorn contains "thorn", Black - Lotus contains "lotus") to confirm the table-based path didn't regress - anything that used to work by keyword-coincidence. -- Confirm entries not in either table (if any exist) still fall through - to the old keyword check rather than being silently dropped. diff --git a/docs/herb-pool-scoping-regression.md b/docs/herb-pool-scoping-regression.md deleted file mode 100644 index 977d22c..0000000 --- a/docs/herb-pool-scoping-regression.md +++ /dev/null @@ -1,56 +0,0 @@ -# Bug: events spawning resources from the wrong zone entirely - -## Observed, live-tested after PR #7 merged and rebuilt - -`.goldrush teststart` in **Durotar** spawned **Mageroyal** almost -exclusively. Mageroyal is not in Durotar's configured pool at all — -Durotar's `GoldRush.ZonePool` entry is: - -``` -Durotar|1618;3724;1617;3725;1619;3726;1731;2055;3763 -``` - -(Peacebloom, Silverleaf, Earthroot, Copper Vein — verified directly -against the live config file, not corrupted.) Mageroyal (1620/3727) -belongs to a different zone's list entirely (The Barrens). No ore -(Copper Vein) appeared either, despite being in Durotar's list and -containing "vein" — a keyword that should have matched even under the -*old*, pre-PR-#7 categorization logic, which is a strong hint this isn't -purely a categorization problem. - -Same no-ore symptom was also observed in Zangarmarsh (Outland) before -this Durotar test — so it isn't zone-specific. - -## Hypothesis — check this first, don't assume - -The live config data is confirmed correct, and the herb that *did* spawn -is a real herb from the correct global category, just from the **wrong -zone's list**. That points at a scoping bug: somewhere in how -`herbEntries`/`oreEntries` get built, the code may now be filtering -against the *entire universe* of IDs the lookup table (from PR #7) knows -about as herbs, rather than filtering `_currentSite`'s own configured pool -by category. That would explain a wrong-zone herb appearing in Durotar, -and — if there's an analogous issue on the ore side, or the ore bucket -ends up empty/never selected due to how the fix restructured the -category-split logic — could also explain the missing ore. - -**Please verify directly against the actual merged code** (I haven't -re-opened the PR 7 diff for this report) exactly how the herb/ore split -is being built post-fix: -- Is it `SplitByCategory(_currentSite.NodeEntries or siteNormalPool, ...)` - — filtering the *site's own* entries by category — or did the fix - change this to build from some broader/global set? -- If the lookup table itself is being used correctly for - categorization, but something else changed which *pool* it's applied - to, that's the actual regression to find and fix. - -## Testing once fixed - -- Durotar: only Peacebloom/Silverleaf/Earthroot/Copper Vein should - appear, nothing from any other zone. -- Confirm ore (Copper Vein) actually shows up in Durotar, and Fel - Iron/Adamantite show up in Zangarmarsh — both were absent in testing - so far. -- Spot-check one more zone on a different continent (e.g. Sholazar - Basin) to confirm it isn't pulling in entries from unrelated zones - either. diff --git a/src/GoldRush.cpp b/src/GoldRush.cpp index e8837a0..9e7e873 100644 --- a/src/GoldRush.cpp +++ b/src/GoldRush.cpp @@ -24,7 +24,6 @@ #include #include #include -#include #include #include @@ -42,47 +41,6 @@ static constexpr float MaxVerticalDrift = 20.0f; // reject ground this far // Mirrors INVALID_HEIGHT from GridTerrainData.h without taking a dependency on that header. static constexpr float InvalidHeightSentinel = -99999.0f; -// Static ID -> category lookup for herb/ore classification. Every entry ID used -// across the 57-zone GoldRush.ZonePool (plus the module's own default global -// GoldRush.NodeEntries) was individually verified against -// acore_world.gameobject_template when the pool was built -// (docs/zone-specific-resources-spec.md). Guessing the category from the display -// name (the old keyword-list approach) broke for almost every zone: Outland/Classic -// ore uses "Deposit" rather than "Vein", and most herb names don't contain any of -// the four hardcoded herb keywords. true = herb, false = ore. -static const std::unordered_map IsHerbById = { - // Classic herbs - { 1618, true }, { 3724, true }, { 1617, true }, { 3725, true }, { 1619, true }, { 3726, true }, // Peacebloom, Silverleaf, Earthroot - { 1620, true }, { 3727, true }, { 1621, true }, { 3729, true }, { 2045, true }, { 1622, true }, { 3730, true }, // Mageroyal, Briarthorn, Stranglekelp, Bruiseweed - { 1623, true }, { 1624, true }, { 2041, true }, { 2042, true }, { 2046, true }, { 2043, true }, { 2866, true }, // Wild Steelbloom, Kingsblood, Liferoot, Fadeleaf, Goldthorn, Khadgar's Whisker, Firebloom - { 142140, true }, { 180165, true }, { 142141, true }, { 176642, true }, // Purple Lotus, Arthas' Tears - { 142142, true }, { 176636, true }, { 180164, true }, { 142143, true }, { 183046, true }, // Sungrass, Blindweed - { 142144, true }, { 142145, true }, { 176637, true }, { 176587, true }, { 176641, true }, // Ghost Mushroom, Gromsblood, Plaguebloom - { 176583, true }, { 176638, true }, { 180167, true }, { 176584, true }, { 176639, true }, // Golden Sansam, Dreamfoil - { 180168, true }, { 176586, true }, { 176640, true }, { 180166, true }, { 176588, true }, // Mountain Silversage, Icecap - { 191303, true }, { 176589, true }, // Firethorn, Black Lotus - // Outland herbs - { 181270, true }, { 183044, true }, { 181271, true }, { 183045, true }, { 181275, true }, // Felweed, Dreaming Glory, Ragveil - { 183043, true }, { 181277, true }, { 181276, true }, { 181279, true }, { 181280, true }, { 181281, true }, // Terocone, Flame Cap, Netherbloom, Nightmare Vine, Mana Thistle - // Northrend herbs - { 189973, true }, { 190171, true }, { 190172, true }, { 190176, true }, { 190170, true }, // Goldclover, Lichbloom, Icethorn, Frost Lotus, Talandra's Rose - { 191019, true }, { 190169, true }, // Adder's Tongue, Tiger Lily - // Classic ore - { 1731, false }, { 2055, false }, { 3763, false }, { 103713, false }, { 181248, false }, // Copper Vein - { 1732, false }, { 2054, false }, { 3764, false }, { 103711, false }, { 181249, false }, // Tin Vein - { 1733, false }, { 105569, false }, // Silver Vein - { 1735, false }, // Iron Deposit - { 1734, false }, { 150080, false }, { 181109, false }, // Gold Vein - { 2040, false }, { 150079, false }, { 176645, false }, // Mithril Deposit - { 2047, false }, { 150081, false }, { 181108, false }, // Truesilver Deposit - { 324, false }, { 150082, false }, { 176643, false }, { 175404, false }, // Small/Rich Thorium Vein - { 165658, false }, // Dark Iron Deposit - // Outland ore - { 181555, false }, { 181556, false }, { 181569, false }, { 181570, false }, { 181557, false }, // Fel Iron, Adamantite (+ Rich), Khorium - // Northrend ore - { 189978, false }, { 189979, false }, { 189980, false }, { 189981, false }, { 191133, false }, // Cobalt/Saronite Deposit (+ Rich), Titanium Vein -}; - struct GoldRushSite { std::string ZoneLabel; @@ -196,33 +154,6 @@ static AreaTableEntry const* ResolveAreaEntry(std::string const& areaName) return nullptr; } -// The GoldRush.ZonePool value separates zones with "; " (semicolon + space) and -// the node/bonus entry IDs within a zone with a bare ";". Acore::Tokenize only -// splits on a single character, so splitting on ';' cut every zone's entry list -// down to its first ID -- the bug behind the wrong-zone/incomplete resource -// spawning reported after PR #7 (every zone's first entry is a herb, so ore -// never appeared and only one herb type spawned). Split on the two-character -// zone delimiter instead. -static std::vector SplitZonePool(std::string_view zones) -{ - std::vector result; - size_t start = 0; - while (start < zones.size()) - { - size_t end = zones.find("; ", start); - if (end == std::string_view::npos) - { - result.push_back(zones.substr(start)); - break; - } - - result.push_back(zones.substr(start, end - start)); - start = end + 2; // skip the "; " delimiter - } - - return result; -} - class GoldRushManager { public: @@ -595,7 +526,7 @@ private: // area labels, change this parser to expect 4 segments and adjust // BuildLocationText/FormatLocationForAnnouncement accordingly. std::vector sites; - for (std::string_view zoneToken : SplitZonePool(zones)) + for (std::string_view zoneToken : Acore::Tokenize(zones, ';', true)) { std::string token = Trim(std::string(zoneToken)); if (token.empty()) @@ -726,23 +657,9 @@ private: if (!goinfo) continue; - // Prefer the verified ID -> category lookup table. The old name-keyword - // guess is kept only as a defensive fallback for entries not in the table - // (e.g. the original global-pool defaults or anything added by hand later - // without an explicit table update). - bool isHerb; - auto const it = IsHerbById.find(entry); - if (it != IsHerbById.end()) - { - isHerb = it->second; - } - else - { - std::string name = Normalize(goinfo->name); - isHerb = ContainsWord(name, "lotus") || ContainsWord(name, "clover") || ContainsWord(name, "thorn") || ContainsWord(name, "bloom"); - } - - bool isOre = !isHerb; + std::string name = Normalize(goinfo->name); + bool isHerb = ContainsWord(name, "lotus") || ContainsWord(name, "clover") || ContainsWord(name, "thorn") || ContainsWord(name, "bloom"); + bool isOre = ContainsWord(name, "vein"); if (herbs && isHerb) result.push_back(entry);