fix: restore Sell() fallthrough semantics, guard price overrides, live reload#23
Conversation
… Sell() Reverts a return()/if-else-if regression in the per-item listing loop. A failed pick (itemID==0, missing prototype/item, quality over the configured max) now skips to the next item instead of aborting the whole selling cycle for the bot, matching upstream continue semantics. The rarity/category selection block is restored to independent if blocks so a tier with no eligible pick correctly falls through to the next tier instead of being short-circuited by the chained else-if.
avgPrice - minPrice underflows a uint64 when minPrice > avgPrice, which Buy() then folds into maxPrice and eventually a uint32 cast, producing undefined behavior on the world thread. Clamp at the source in LoadPriceOverrides() with a warning naming the offending item, and add a matching guard at the Buy() call site as defense in depth.
priceOverridesLoaded/countOverridesLoaded were function-local static bools inside AuctionHouseBot::Initialize(), so once set they never went false again and .reload config left the bot running with whatever overrides were loaded at server start. Move the load-once- and-share step into AHBot_WorldScript::LoadSharedOverrides(), called once per startup and once per reload cycle before bots are (re)constructed, so both tables actually refresh on reload.
| itemID = getElement(config->GreyItemsVec, urand(0, config->GreyItemsVec.size() - 1), _id, config->DuplicatesCount, botItemCounts, config); | ||
| } | ||
| else if (itemID == 0 && !config->GreyTradeGoodsVec.empty() && (currentGreyTG < maxGreyTG)) | ||
| if (itemID == 0 && !config->GreyTradeGoodsVec.empty() && (currentGreyTG < maxGreyTG)) |
There was a problem hiding this comment.
will this change cause problems since the item is handled by all buckets?
There was a problem hiding this comment.
No — at most one bucket can produce the item per iteration. Every bucket after the first is gated on itemID == 0, so as soon as any getElement() returns a real item, all later buckets short-circuit. Fall-through only continues while nothing has been picked yet (bucket empty, percentage cap reached, or the random pick sat at its count-override cap). An item also can't appear in two buckets: the bins are partitioned by quality and class (items vs trade goods), so each item id lives in exactly one vector.
itemTypeSelectedToSell does get set optimistically before getElement() resolves, but it's overwritten by whichever bucket actually delivers, and if the loop exits empty the iteration is discarded before the value is read. This matches upstream azerothcore/master exactly — the else if chain was the deviation this PR reverts.
1 participant
Fixes the must-fix findings from a review of the merkerhood branch, plus two follow-ups the review of this branch itself surfaced.
Sell() control flow (silent under-listing)
The Sell() refactor had turned upstream's
continueintoreturnin all four failure branches and chained the rarity-tier selection intoif/else if. First failed pick (e.g. an item at its count-override cap) aborted the entire cycle, so the AH chronically undershot MinItems/MaxItems. Restored upstream skip-and-continue and per-tier fallthrough, keeping all merkerhood features (priority list, count overrides, percentage caps).Count-override cap now applies to priority-list picks
Priority-list picks from the price-override set went straight to listing without ever consulting the count override — the cap only existed in the (unreachable) random-pick fallback. Items with curated liquidity caps were exactly the ones the bot could flood without limit. Priority picks now check the cap like
getElement()does; capped picks fall through to the rarity-tier selection.Price override validation (UB in world thread)
LoadPriceOverrides()accepted rows withminPrice > avgPrice;Buy()then computedavgPrice + (avgPrice - minPrice)in uint64, wrapping to ~UINT64_MAX and casting out-of-range to uint32 — undefined behavior reachable from a single malformed backfill row. Load now clampsminPrice = avgPricewith a warning naming the item;Buy()re-clamps as defense in depth.Overrides reload with
.reload configThe per-bot
static boolload guards meant price/count override edits needed a server restart. Loading moved toAHBot_WorldScript::LoadSharedOverrides(), called on startup and in the reload branch ofOnBeforeConfigLoad(afterDeleteBots(), beforePopulateBots()). Maps are cleared before loading so row deletions take effect too.Compile-checked against the full core tree (RelWithDebInfo, gcc/ninja).