fix(Scripts/VaultOfArchavon): reliably stone bosses and kick players before Wintergrasp#26428
Closed
Chinoske wants to merge 3 commits into
Closed
fix(Scripts/VaultOfArchavon): reliably stone bosses and kick players before Wintergrasp#26428Chinoske wants to merge 3 commits into
Chinoske wants to merge 3 commits into
Conversation
…before Wintergrasp The pre-war checks (15/10/2/1 minute warnings, stoning, kick-out) used 60-second-wide time WINDOWS evaluated on a fixed 60-second poll. Any time the Wintergrasp timer changed faster than that poll interval (a GM-set jump, but also just unlucky phase alignment under normal countdown), the narrow window could be skipped entirely and none of the mechanics would fire for that whole pre-war cycle. Replaced with threshold-crossing checks gated by one-shot flags, polled every 5s: each stage fires exactly once as soon as the timer is observed at or below its threshold, regardless of how it got there. Also treats an already-active war as "timer at 0" so a jump straight past every threshold still fires the missed stages retroactively instead of bailing out because IsWarTime() is already true. Flags reset once the timer is back above 15 minutes (previous cycle fully concluded). Closes azerothcore#26411
github-actions
Bot
added
Script
📝 WalkthroughWalkthroughThe reset countdown logic in the Vault of Archavon instance script was rewritten to use one-shot boolean flags tracking warning and kick states, replacing minute-based timer checks with a 5-second gated countdown that emits warnings, applies a stoning aura, and kicks players/evades bosses as Wintergrasp approaches. ChangesWintergrasp countdown behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/scripts/Northrend/VaultOfArchavon/instance_vault_of_archavon.cpp`:
- Around line 147-150: The TeleportTo call in the instance_vault_of_archavon.cpp
player loop exceeds the 120-column limit. Reformat the logic around
Map::PlayerList, the const_iterator loop, and player->TeleportTo so the
arguments are wrapped onto multiple lines while preserving the same behavior and
keeping the line length within the project’s codestyle limit.
- Around line 96-103: The reset logic in `Update()` currently depends on
`bf->GetTimer()` being greater than 15 minutes, which breaks when
`NoBattleTimer` is configured below that threshold. Update the
`instance_vault_of_archavon` state handling to reset `warned15`, `stoned`,
`warned2`, and `kicked` on the war-active to war-inactive transition instead of
an absolute timer check, and use the existing `kicked` state plus a new private
`wasWarTime` member to detect that edge reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cc1716b-3d0b-4f37-a8e5-09d4847ba04d
📒 Files selected for processing (1)
src/server/scripts/Northrend/VaultOfArchavon/instance_vault_of_archavon.cpp
Comment on lines
+96
to
+103
| if (!warTime && timer > 15 * MINUTE * IN_MILLISECONDS) | ||
| { | ||
| checkTimer -= 60000; // one minute | ||
| if (!sWorld->getBoolConfig(CONFIG_WINTERGRASP_KICK_VOA_PLAYERS)) | ||
| return; | ||
| if (Battlefield* bf = sBattlefieldMgr->GetBattlefieldByBattleId(BATTLEFIELD_BATTLEID_WG)) | ||
| { | ||
| if (!bf->IsWarTime()) | ||
| { | ||
| if (bf->GetTimer() <= (16 * MINUTE * IN_MILLISECONDS) && bf->GetTimer() >= (15 * MINUTE * IN_MILLISECONDS)) | ||
| { | ||
| Map::PlayerList const& PlayerList = instance->GetPlayers(); | ||
| if (!PlayerList.IsEmpty()) | ||
| for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i) | ||
| if (Player* player = i->GetSource()) | ||
| player->TextEmote("This instance will reset in 15 minutes.", nullptr, true); | ||
| } | ||
| else if (bf->GetTimer() <= (10 * MINUTE * IN_MILLISECONDS) && bf->GetTimer() >= (9 * MINUTE * IN_MILLISECONDS)) | ||
| { | ||
| for (uint8 i = 0; i < MAX_ENCOUNTER; ++i) | ||
| if (Creature* cr = instance->GetCreature(bossGUIDs[i])) | ||
| if (!cr->IsInCombat()) | ||
| { | ||
| cr->RemoveAllAuras(); | ||
| if (Aura* aur = cr->AddAura(SPELL_STONED_AURA, cr)) | ||
| { | ||
| aur->SetMaxDuration(60 * MINUTE * IN_MILLISECONDS); | ||
| aur->SetDuration(60 * MINUTE * IN_MILLISECONDS); | ||
| } | ||
| } | ||
|
|
||
| stoned = true; | ||
| } | ||
| else if (bf->GetTimer() <= (2 * MINUTE * IN_MILLISECONDS) && bf->GetTimer() > (MINUTE * IN_MILLISECONDS)) | ||
| { | ||
| Map::PlayerList const& PlayerList = instance->GetPlayers(); | ||
| if (!PlayerList.IsEmpty()) | ||
| for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i) | ||
| if (Player* player = i->GetSource()) | ||
| player->TextEmote("This instance is about to reset. Prepare to be removed.", nullptr, true); | ||
| } | ||
| else if (bf->GetTimer() <= MINUTE * IN_MILLISECONDS) | ||
| warned15 = false; | ||
| stoned = false; | ||
| warned2 = false; | ||
| kicked = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset relies on GetTimer() > 15 min, which can get stuck if NoBattleTimer is configured below 15 minutes.
After a war ends, timer becomes bf->GetTimer() again, which starts counting down from Wintergrasp.NoBattleTimer (default 150 min, safely above 15). If an operator configures NoBattleTimer to less than 15 minutes, this reset condition is never true again after the first cycle, so warned15/stoned/warned2/kicked stay true forever and the whole mechanic (warnings, stoning, kicking) silently stops firing for every subsequent war. Resetting on the war-active→inactive transition instead of on an absolute timer threshold would be robust to any config value.
🐛 Proposed fix: reset on war-end transition instead of absolute timer value
- void Initialize() override
+ void Initialize() override
{
SetHeaders(DataHeader);
memset(&m_auiEncounter, 0, sizeof(m_auiEncounter));
ArchavonDeath = 0;
EmalonDeath = 0;
KoralonDeath = 0;
checkTimer = 0;
stoned = false;
warned15 = false;
warned2 = false;
kicked = false;
+ wasWarTime = false;
} bool const warTime = bf->IsWarTime();
uint32 const timer = warTime ? 0 : bf->GetTimer();
- // Far from the next war again: previous cycle is fully over, reset for a fresh countdown.
- if (!warTime && timer > 15 * MINUTE * IN_MILLISECONDS)
+ // War just ended: begin a fresh countdown cycle regardless of the configured NoBattleTimer value.
+ if (wasWarTime && !warTime)
{
warned15 = false;
stoned = false;
warned2 = false;
kicked = false;
- return;
}
+ wasWarTime = warTime;
+
+ if (!warTime && timer > 15 * MINUTE * IN_MILLISECONDS)
+ return;(with a matching bool wasWarTime; private member added alongside kicked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/scripts/Northrend/VaultOfArchavon/instance_vault_of_archavon.cpp`
around lines 96 - 103, The reset logic in `Update()` currently depends on
`bf->GetTimer()` being greater than 15 minutes, which breaks when
`NoBattleTimer` is configured below that threshold. Update the
`instance_vault_of_archavon` state handling to reset `warned15`, `stoned`,
`warned2`, and `kicked` on the war-active to war-inactive transition instead of
an absolute timer check, and use the existing `kicked` state plus a new private
`wasWarTime` member to detect that edge reliably.
Comment on lines
+147
to
+150
| Map::PlayerList const& PlayerList = instance->GetPlayers(); | ||
| for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i) | ||
| if (Player* player = i->GetSource()) | ||
| player->TeleportTo(player->m_homebindMapId, player->m_homebindX, player->m_homebindY, player->m_homebindZ, player->GetOrientation()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 150 exceeds the 120-column limit.
With indentation this line runs to roughly 157 columns, well past the project's max-120-column hard rule (CI-enforced via the codestyle lint script per repository conventions).
🎨 Proposed fix: wrap the call
Map::PlayerList const& PlayerList = instance->GetPlayers();
for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i)
if (Player* player = i->GetSource())
- player->TeleportTo(player->m_homebindMapId, player->m_homebindX, player->m_homebindY, player->m_homebindZ, player->GetOrientation());
+ player->TeleportTo(player->m_homebindMapId, player->m_homebindX, player->m_homebindY,
+ player->m_homebindZ, player->GetOrientation());As per coding guidelines, "Maximum line length is 120 columns".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Map::PlayerList const& PlayerList = instance->GetPlayers(); | |
| for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i) | |
| if (Player* player = i->GetSource()) | |
| player->TeleportTo(player->m_homebindMapId, player->m_homebindX, player->m_homebindY, player->m_homebindZ, player->GetOrientation()); | |
| Map::PlayerList const& PlayerList = instance->GetPlayers(); | |
| for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i) | |
| if (Player* player = i->GetSource()) | |
| player->TeleportTo(player->m_homebindMapId, player->m_homebindX, player->m_homebindY, | |
| player->m_homebindZ, player->GetOrientation()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/scripts/Northrend/VaultOfArchavon/instance_vault_of_archavon.cpp`
around lines 147 - 150, The TeleportTo call in the
instance_vault_of_archavon.cpp player loop exceeds the 120-column limit.
Reformat the logic around Map::PlayerList, the const_iterator loop, and
player->TeleportTo so the arguments are wrapped onto multiple lines while
preserving the same behavior and keeping the line length within the project’s
codestyle limit.
Source: Coding guidelines
Update()'s flag reset cleared the stoned bool but never removed the actual aura from bosses, relying on OnPlayerEnter to do that cleanup. Since the timer-based reset fires within 5s (well before a player typically walks back in), OnPlayerEnter would see stoned already false and skip the cleanup, leaving bosses stuck with the aura for up to its 60-minute duration. Now cleared in the same place the flag resets.
Contributor
Author
|
Pushed a follow-up: the timer-based flag reset cleared |
- Reset the countdown flags on the war-active-to-inactive transition (tracked via a new wasWarTime bool) instead of relying on the timer being above an absolute 15-minute threshold, which would never be true again if Wintergrasp.NoBattleTimer is configured below 15 minutes. - Wrap the TeleportTo call, which exceeded the 120-column limit.
Contributor
Author
|
Addressed both CodeRabbit findings:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SPELL_STONED_AURA = 63080,Wintergrasp.KickVoAPlayersconfig, defaults totrue), but they don't reliably fire.timer <= 10min && timer >= 9min) evaluated on a fixed 60-second poll. If the Wintergrasp timer moves through a window faster than the next poll (a GM-set jump via.bf timer, but this can in principle also happen from ordinary phase misalignment), the window is skipped entirely and none of the mechanics fire for that whole pre-war cycle — matching the issue's repro steps (.bf timer 15m→10m→5m→10swith only a few seconds between each).warned15,stoned,warned2,kicked), polled every 5s. Each stage fires exactly once as soon as the timer is observed at or below its threshold, regardless of how it got there — no window to miss.IsWarTime()is already true.Test plan
Battlefield::Timersemantics (Battlefield.cpp): counts down continuously in both pre-war and war states;.bf timer(cs_bf.cpp) callsSetTimer()directly on the sameTimerfield my code readscodestyle-cpp.pypasses clean.bf timercalls worked, but a direct 15m→10s jump did nothing) with the first version of the fix, then fixed the war-already-started edge case and confirmed both the stepped case and the direct-jump case now correctly fire the 15-min/2-min warnings, boss stoning, and player kick-to-homebindCloses #26411
🤖 Generated with Claude Code