Skip to content

fix(Core/BG): award correct Alterac Valley reputation - #1

Open
icemansparks wants to merge 1 commit into
merkerhoodfrom
fix/av-reputation
Open

fix(Core/BG): award correct Alterac Valley reputation#1
icemansparks wants to merge 1 commit into
merkerhoodfrom
fix/av-reputation

Conversation

@icemansparks

@icemansparks icemansparks commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Players report gaining no Alterac Valley reputation. Two independent defects in src/server/game/Battlegrounds/Zones/BattlegroundAV.cpp, both inherited from upstream AzerothCore: our copy of the file is byte-identical to azerothcore/azerothcore-wotlk (blob 0941c82a), so this is not local drift, and no upstream issue covers either one. Realm config is clean on both realms (Rate.Reputation.Gain.AV = 1, Battleground.Alterac.ReputationOnBossDeath = 350), so the tuning knobs are not the cause.

1. End-of-battle reputation overflowed a uint8

EndBattleground accumulates every end-of-match bonus into one counter per team, then hands it to RewardReputationToTeam:

uint8 rep[2] = {0, 0};          // line 514
rep[m_Nodes[i].OwnerId]  += _reputationSurvivingTower;      // 12 (18 on BG weekend), up to 8 nodes
rep[m_Nodes[i].OwnerId]  += _reputationPerOwnedGraveyard;   // 12 (18), up to 7 nodes
rep[m_Mine_Owner[mine]]  += _reputationPerOwnedMine;        // 24 (36), up to 2 mines
rep[iTeamId]             += _reputationSurvivingCaptain;    // 125 (175)
RewardReputationToTeam(iTeamId == TEAM_ALLIANCE ? 730 : 729, rep[iTeamId], iTeamId);

The node ranges are 8 towers (BG_AV_NODES_DUNBALDAR_SOUTH 7 … BG_AV_NODES_FROSTWOLF_WTOWER 14) and 7 graveyards (BG_AV_NODES_FIRSTAID_STATION 0 … BG_AV_NODES_FROSTWOLF_HUT 6), so the ceiling is 353 at default rate and 517 on a BG weekend, well past 255. It does not take a perfect match to wrap: 4 towers + 5 graveyards + 1 mine + a surviving captain is 257, which truncates to 1 reputation. The failure gets worse the better the team played, and any server running Rate.Reputation.Gain.AV above 1 wraps sooner. This is the largest single reputation award in the battleground, which is why the symptom reads as "AV gives no rep".

The counter is the only thing that is wrong here. The reward values themselves are correct Blizzlike numbers, RewardReputationToTeam takes uint32 reputation (Battleground.h:472), and Battleground::RewardReputationToTeam does the rate/aura math in float before calling ReputationMgr::ModifyReputation. So the accumulator is the one narrow link in an otherwise 32-bit path, and widening it to uint32 makes the local type match the parameter it feeds rather than introducing a new convention.

uint8 kills[2] on the line above is deliberately left alone: its ceiling is 4 + 8 × BG_AV_KILL_SURVIVING_TOWER (2) + BG_AV_KILL_SURVIVING_CAPTAIN (2) = 22. It cannot overflow, and changing it would be unrelated churn.

This line dates back to the initial 2016 import (e8e94a0a), i.e. it predates AzerothCore and was never revisited.

2. Commander quest turn-ins rewarded faction id 0/1

RewardReputationToTeam(teamId, uint32(1 * _avReputationRate), teamId);   // lines 197, 204, 211

The first parameter is uint32 factionId, not a team. TeamId is 0/1, so these three call sites ask for reputation with faction 0 or faction 1. Battleground::RewardReputationToTeam resolves the faction through GetRealRepFactionForPlayer, whose switch only knows BG_REP_AV_ALLIANCE/BG_REP_AV_HORDE (and the AB/WSG pairs), then does sFactionStore.LookupEntry(realFactionId) and only calls ModifyReputation inside if (FactionEntry const* factionEntry = ...). Faction 0/1 is not a valid entry, so the lookup fails and nothing is awarded at all — not the wrong faction, no faction. The mercenary-mode reversal that GetRealRepFactionForPlayer exists for is also silently skipped, because the switch never matches.

The three call sites now pass teamId == TEAM_ALLIANCE ? 730 : 729, which is exactly what every other reputation call in this same file already does — boss kills (105, 113), captain kills (126, 144), tower destruction (661) and the end-of-battle award (560). Same argument order, same ternary, same literals, so the file stays internally consistent and the change is four characters of logic rather than a new helper.

Note that only the amount of these three calls was touched recently, by da5fb6c9 / azerothcore#22685 ("BG reputation modifier for WSG, AB and AV", Oct 2025), which wrapped the constant 1 in the new AV rate multiplier. The teamId first argument was already there before that PR and is untouched by it, so the defect is long-standing, not a regression from the rate work. The awarded amount (1 × rate per turn-in) is left exactly as it is: whether 1 is the correct Blizzlike value for the soldier/lieutenant/commander turn-ins is a separate question from the faction being invalid, and mixing the two would make this PR unreviewable.

Alternatives considered

  • Use the named constants BG_REP_AV_ALLIANCE/BG_REP_AV_HORDE (Battleground.h:143-144) instead of 730/729. Semantically identical. Rejected for consistency: every reward call in the BG zone files uses the bare literals, and the named enum is referenced only inside GetRealRepFactionForPlayer. Introducing the constants at three of the file's nine call sites would leave the file half-converted. Converting all of them is a readability cleanup that deserves its own commit, not a bugfix that has to be cherry-picked and verified.
  • Clamp or saturate the accumulator instead of widening it. That preserves the truncation as intended behaviour. There is no cap in Blizzlike AV; the values are meant to add up.
  • Shrink the reward constants so the sum fits in a uint8. Would break the Blizzlike values (12/18, 24/36, 125/175) that the surrounding code and the Battleground.Alterac.ReputationOnBossDeath config are built around.
  • Award each source immediately instead of accumulating. More RewardReputationToTeam calls per match end, more packets, and it changes observable behaviour (several small reputation messages instead of one). No benefit over fixing the type.
  • Fix it in the DB. Not possible: all of these values are hardcoded in the core, none come from battleground_template or any world table.

Scope and verification

Four changed lines, one file, no signature or API changes, no DB migration. Both fixes are independent, so a failure in play-testing points at exactly one of them.

Merging here first is what makes a test build possible at all: ops config repo branch pins the core repo to merkerhood because it calibrates the config matrix and module set, so a side branch cannot be checked out for a testcore build. Upstream PR to azerothcore/azerothcore-wotlk follows once a test AV run confirms the reputation numbers.

End-of-battle bonus reputation was accumulated in a uint8, so a normal
win (towers + graveyards + mines + surviving captain) overflowed 255 and
awarded almost nothing. Widen the accumulator to uint32.

The three Commander quest turn-ins passed teamId as the faction id, so
they modified faction 0/1 instead of Stormpike Guard (730) /
Frostwolf Clan (729).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant