refactor(Scripts/VioletHold): Modernize Violet Hold dungeon scripts#25187
refactor(Scripts/VioletHold): Modernize Violet Hold dungeon scripts#25187Nyeriah merged 4 commits intoazerothcore:masterfrom
Conversation
Nyeriah
changed the title
There was a problem hiding this comment.
Pull request overview
Modernizes the Violet Hold instance/boss scripts to newer AzerothCore conventions, and adds an optional “alive-only” player counting mode in core map utilities.
Changes:
- Refactors Violet Hold instance/boss/trash scripts to use
BossAI,EventMap,ObjectData/LoadObjectData(), and a newRegisterVioletHoldCreatureAImacro. - Reworks encounter state handling (slots + redirection) and cleanup logic, including fixing
IsEncounterInProgress(). - Extends
Map::GetPlayersCountExceptGMs()with analiveOnlyflag used by Violet Hold cleanup.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/scripts/Northrend/VioletHold/violet_hold.h | Introduces new VH enums/IDs and a registration macro to support modernized instance/object tracking. |
| src/server/scripts/Northrend/VioletHold/violet_hold.cpp | Refactors gossip and multiple trash AIs to EventMap + registration macro; updates instance interactions. |
| src/server/scripts/Northrend/VioletHold/instance_violet_hold.cpp | Reworks instance state, object tracking, boss-state routing, cleanup logic, and uses persistent data for boss selection. |
| src/server/scripts/Northrend/VioletHold/boss_zuramat.cpp | Migrates Zuramat to BossAI and modernizes void sentry handling/instance interactions. |
| src/server/scripts/Northrend/VioletHold/boss_xevozz.cpp | Migrates Xevozz to BossAI and updates summon tracking via instance GUID bookkeeping. |
| src/server/scripts/Northrend/VioletHold/boss_moragg.cpp | Migrates Moragg to BossAI + event execution style. |
| src/server/scripts/Northrend/VioletHold/boss_lavanthor.cpp | Migrates Lavanthor to BossAI + event execution style. |
| src/server/scripts/Northrend/VioletHold/boss_ichoron.cpp | Migrates Ichoron to BossAI, reworks globule behavior, and replaces timers with EventMap. |
| src/server/scripts/Northrend/VioletHold/boss_erekem.cpp | Migrates Erekem and guards to modern AI patterns and updates inter-AI coordination. |
| src/server/scripts/Northrend/VioletHold/boss_cyanigosa.cpp | Migrates Cyanigosa to BossAI and keeps special vacuum/root behavior. |
| src/server/game/Maps/Map.h | Adds the aliveOnly parameter to the public API (defaulting to current behavior). |
| src/server/game/Maps/Map.cpp | Implements aliveOnly filtering, excluding Spirit of Redemption “alive” states. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool SetBossState(uint32 id, EncounterState state) override | ||
| { | ||
| if (!InstanceScript::SetBossState(id, state)) | ||
| return false; | ||
|
|
||
| switch (id) | ||
| { | ||
| case BOSS_MORAGG: | ||
| case BOSS_EREKEM: | ||
| case BOSS_ICHORON: | ||
| case BOSS_LAVANTHOR: | ||
| case BOSS_XEVOZZ: | ||
| case BOSS_ZURAMAT: | ||
| if (state == DONE) | ||
| { | ||
| if (WaveCount == 6) | ||
| SetBossState(DATA_1ST_BOSS, DONE); | ||
| else if (WaveCount == 12) | ||
| SetBossState(DATA_2ND_BOSS, DONE); | ||
| events.RescheduleEvent(EVENT_SUMMON_PORTAL, 35s); | ||
| } | ||
| else if (state == NOT_STARTED) | ||
| { | ||
| _cleaned = false; | ||
| InstanceCleanup(); | ||
| } | ||
| break; |
There was a problem hiding this comment.
The instance cleanup on boss reset only triggers when state == NOT_STARTED. BossAI commonly reports an evade as FAIL, not NOT_STARTED, which can leave the instance stuck (no cleanup/portal progression). Treat FAIL the same as NOT_STARTED here (and for DATA_CYANIGOSA below) so evades reliably reset the run.
| void FinishPointReached() | ||
| { | ||
| IsOpening = true; | ||
| Count = 0; |
There was a problem hiding this comment.
This now casts SABOTEUR_SHIELD_DISRUPTION immediately and then schedules 3 more casts via EVENT_SABOTEUR_SHIELD_DISRUPTION (since Count is only incremented in the event handler). Previously the sequence was 3 casts total. Consider either removing the immediate cast and letting the event drive all casts, or initialize/increment Count so the total number of casts matches the prior behavior.
| Count = 0; | |
| Count = 1; |
| void UpdateAI(uint32 diff) override | ||
| { | ||
| if (!UpdateVictim()) | ||
| return; | ||
|
|
||
| for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i) | ||
| if (Player* plr = i->GetSource()) | ||
| me->CastSpell(plr, spellId, triggered); | ||
| } | ||
| events.Update(diff); | ||
|
|
||
| void JustEngagedWith(Unit* /*who*/) override | ||
| if (!IsFrenzy && !IsExploded && HealthBelowPct(25)) | ||
| { | ||
| bIsExploded = false; | ||
| bIsFrenzy = false; | ||
| uiDrainedTimer = 15000; | ||
| uiWaterBoltVolleyTimer = urand(7000, 12000); | ||
| DoZoneInCombat(); | ||
| Talk(SAY_AGGRO); | ||
| me->CastSpell(me, SPELL_PROTECTIVE_BUBBLE, true); | ||
| if (pInstance) | ||
| pInstance->SetData(DATA_ACHIEV, 1); | ||
| Talk(SAY_ENRAGE); | ||
| me->CastSpell(me, SPELL_FRENZY, true); | ||
| IsFrenzy = true; | ||
| } | ||
|
|
||
| void UpdateAI(uint32 uiDiff) override | ||
| if (!IsFrenzy) | ||
| { | ||
| if (!UpdateVictim()) | ||
| return; | ||
|
|
||
| if (!bIsFrenzy && !bIsExploded && HealthBelowPct(25)) | ||
| if (!IsExploded) | ||
| { | ||
| Talk(SAY_ENRAGE); | ||
| me->CastSpell(me, SPELL_FRENZY, true); | ||
| bIsFrenzy = true; | ||
| } | ||
|
|
||
| if (!bIsFrenzy) | ||
| { | ||
| if (!bIsExploded) | ||
| if (!me->HasAura(SPELL_PROTECTIVE_BUBBLE)) | ||
| { | ||
| if (!me->HasAura(SPELL_PROTECTIVE_BUBBLE)) | ||
| me->InterruptNonMeleeSpells(false); | ||
| Talk(SAY_SHATTER); | ||
| DoZoneInCombat(); | ||
| IchoronDoCastToAllHostilePlayers(SPELL_WATER_BLAST, true); | ||
| me->CastSpell(me, SPELL_DRAINED, true); | ||
| IsExploded = true; | ||
| events.RescheduleEvent(EVENT_DRAINED_CHECK, 15s); | ||
| me->SetUnitFlag(UNIT_FLAG_NOT_SELECTABLE); | ||
| me->SetDisplayId(11686); | ||
| for (uint8 i = 0; i < MAX_SPAWN_LOC; ++i) | ||
| { | ||
| me->InterruptNonMeleeSpells(false); | ||
| Talk(SAY_SHATTER); | ||
| DoZoneInCombat(); | ||
| IchoronDoCastToAllHostilePlayers(SPELL_WATER_BLAST, true); | ||
| me->CastSpell(me, SPELL_DRAINED, true); | ||
| bIsExploded = true; | ||
| uiDrainedTimer = 15000; | ||
| me->SetUnitFlag(UNIT_FLAG_NOT_SELECTABLE); | ||
| me->SetDisplayId(11686); | ||
| for (uint8 i = 0; i < MAX_SPAWN_LOC; ++i) | ||
| { | ||
| float angle = rand_norm() * 2 * M_PI; | ||
| Position p1(SpawnLoc[i]), p2(SpawnLoc[i]); | ||
| p1.m_positionX += 2.5f * cos(angle); | ||
| p1.m_positionY += 2.5f * std::sin(angle); | ||
| p2.m_positionX -= 2.5f * cos(angle); | ||
| p2.m_positionY -= 2.5f * std::sin(angle); | ||
| DoSummon(NPC_ICHOR_GLOBULE, p1, 60000, TEMPSUMMON_TIMED_OR_DEAD_DESPAWN); | ||
| DoSummon(NPC_ICHOR_GLOBULE, p2, 60000, TEMPSUMMON_TIMED_OR_DEAD_DESPAWN); | ||
| } | ||
| float angle = rand_norm() * 2 * M_PI; | ||
| Position p1(SpawnLoc[i]), p2(SpawnLoc[i]); | ||
| p1.m_positionX += 2.5f * cos(angle); | ||
| p1.m_positionY += 2.5f * std::sin(angle); | ||
| p2.m_positionX -= 2.5f * cos(angle); | ||
| p2.m_positionY -= 2.5f * std::sin(angle); | ||
| DoSummon(NPC_ICHOR_GLOBULE, p1, 60000, TEMPSUMMON_TIMED_OR_DEAD_DESPAWN); | ||
| DoSummon(NPC_ICHOR_GLOBULE, p2, 60000, TEMPSUMMON_TIMED_OR_DEAD_DESPAWN); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (events.ExecuteEvent() == EVENT_DRAINED_CHECK) | ||
| DoExplodeCompleted(); |
There was a problem hiding this comment.
While exploded, events.ExecuteEvent() is used as a single if check. If any other event (e.g., EVENT_WATER_BOLT_VOLLEY still queued from before the explosion) becomes due first, it will be consumed and ignored, permanently desynchronizing Ichoron’s spell schedule. Recommended fix: cancel/ignore EVENT_WATER_BOLT_VOLLEY when entering the exploded state (e.g., CancelEvent) and process events using an eventId variable (or a while loop over due events) so unrelated due events aren’t accidentally consumed.
| uint32 ArcaneStreamTimerStartingValue; | ||
|
|
||
| void Reset() override | ||
| { | ||
| return GetVioletHoldAI<npc_azure_sorcerorAI>(creature); | ||
| events.Reset(); | ||
| events.RescheduleEvent(EVENT_SORCEROR_ARCANE_STREAM, 4s); | ||
| ArcaneStreamTimerStartingValue = 4000; | ||
| events.RescheduleEvent(EVENT_SORCEROR_MANA_DETONATION, 5s); | ||
| } |
There was a problem hiding this comment.
The previous sorceror logic gated SPELL_MANA_DETONATION based on the current Arcane Stream cooldown window; the refactor now repeats Mana Detonation every 2–6s unconditionally (see EVENT_SORCEROR_MANA_DETONATION), which is a significant behavior change (likely much more frequent casts). Either reintroduce the original gating (using ArcaneStreamTimerStartingValue as intended) or adjust scheduling so detonation frequency matches prior behavior. As-is, ArcaneStreamTimerStartingValue is also written but never read, which suggests the gating was accidentally dropped.
github-actions
Bot
added
CORE
Nyeriah
force-pushed
the
vh
branch
2 times, most recently
from
bf1b966 to
b00f212
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _instance = c->GetInstanceScript(); | ||
| _boss = _instance->GetData(DATA_WAVE_COUNT) == 6 | ||
| ? _instance->GetPersistentData(PERSISTENT_DATA_FIRST_BOSS) | ||
| : _instance->GetPersistentData(PERSISTENT_DATA_SECOND_BOSS); | ||
| _addedWaypoints = false; | ||
| _isOpening = false; | ||
| } |
There was a problem hiding this comment.
_boss is loaded from persistent data that now stores boss IDs (e.g., BOSS_MORAGG..BOSS_ZURAMAT), but the waypoint logic still switches on legacy 1..6 values. As a result, no case will match for boss IDs 3..8, preventing the saboteur from reaching the correct point and releasing the boss. Fix by switching on BOSS_MORAGG/BOSS_EREKEM/... (or map _boss to a 1..6 index like (_boss - BOSS_MORAGG + 1) before the switch) consistently in both WaypointReached and the waypoint-building logic.
| switch (_boss) | ||
| { | ||
| pInstance = c->GetInstanceScript(); | ||
| uiBoss = 0; | ||
| if (pInstance) | ||
| uiBoss = pInstance->GetData(DATA_WAVE_COUNT) == 6 ? pInstance->GetData(DATA_FIRST_BOSS_NUMBER) : pInstance->GetData(DATA_SECOND_BOSS_NUMBER); | ||
| bAddedWPs = false; | ||
| bOpening = false; | ||
| case 1: | ||
| if (waypointId == 2) | ||
| FinishPointReached(); | ||
| break; | ||
| case 2: | ||
| if (waypointId == 2) | ||
| FinishPointReached(); | ||
| break; | ||
| case 3: | ||
| if (waypointId == 1) | ||
| FinishPointReached(); | ||
| break; | ||
| case 4: | ||
| if (waypointId == 0) | ||
| FinishPointReached(); | ||
| break; | ||
| case 5: | ||
| if (waypointId == 0) | ||
| FinishPointReached(); | ||
| break; | ||
| case 6: | ||
| if (waypointId == 4) | ||
| FinishPointReached(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
_boss is loaded from persistent data that now stores boss IDs (e.g., BOSS_MORAGG..BOSS_ZURAMAT), but the waypoint logic still switches on legacy 1..6 values. As a result, no case will match for boss IDs 3..8, preventing the saboteur from reaching the correct point and releasing the boss. Fix by switching on BOSS_MORAGG/BOSS_EREKEM/... (or map _boss to a 1..6 index like (_boss - BOSS_MORAGG + 1) before the switch) consistently in both WaypointReached and the waypoint-building logic.
| if (Creature* c = DoSummon(RAND(NPC_PORTAL_GUARDIAN, NPC_PORTAL_KEEPER_1, NPC_PORTAL_KEEPER_2), me, 2.0f, 0, TEMPSUMMON_DEAD_DESPAWN)) | ||
| DoCast(c, SPELL_PORTAL_CHANNEL); |
There was a problem hiding this comment.
npc_vh_teleportation_portalAI now derives from NullCreatureAI but calls DoSummon/DoCast helpers. In AzerothCore/Trinity-style scripting these helpers are typically provided by ScriptedAI/BossAI (not NullCreatureAI), which can lead to compilation errors or unintended behavior depending on the base-class API. Prefer deriving this AI from ScriptedAI (or switch to explicit me->SummonCreature(...) / me->CastSpell(...) calls) to ensure the used helpers are valid.
|
Regarding the |
Nyeriah
force-pushed
the
vh
branch
3 times, most recently
from
3946fcf to
fbb24ea
Compare
Full modernization of the Violet Hold instance and boss scripts: - Convert all bosses from ScriptedAI to BossAI with encounter slot redirection (individual boss states map to DATA_1ST_BOSS/DATA_2ND_BOSS based on wave count, following TrinityCore's pattern by joschiwald) - Instance script uses SetBossState/GetBossState with override to handle wave progression and cleanup on evade - Add ObjectData arrays with LoadObjectData() for automatic creature and gameobject tracking, removing manual ObjectGuid members - Add RegisterVioletHoldCreatureAI macro; remove class wrappers from all boss and trash AI structs - Convert all trash mob manual timers to EventMap-based events - Remove redundant if(Instance) nullptr checks (guaranteed by registration macro) - Use persistent data storage for boss selection instead of custom save/load - Fix IsEncounterInProgress to return actual encounter state - Fix save/load: SecondBoss was written but never loaded - Add aliveOnly parameter to Map::GetPlayersCountExceptGMs(), excluding Spirit of Redemption; used in VH DoNeedCleanup - Use new HandleGameObject(uint32, bool) overload throughout - Fix variable naming per AzerothCore code standards Co-Authored-By: joschiwald <joschiwald@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Tested: Waves spawn correctly Above all works grand Crystal defense system seems a bit delayed when clicking them. I tried to use them on Icharon for the Dehydration achievement but they took so long to activate he still got a bunch of the water eles. Cyanigosa seems to take longer than before to become active after turning into dragon form. Maybe imagining it, but feels like a few extra seconds there. No big deal anyway. To do: |
Hmm, this PR shouldn't have changed any of this |
# Conflicts: # src/server/scripts/Northrend/VioletHold/boss_xevozz.cpp # src/server/scripts/Northrend/VioletHold/violet_hold.cpp
|
The cynagosa one may be me imagining it as I'm used to doing it as 5man where there's others around and more going on and boss fight prep. When alone it could just be that delay is highlighted tbh so can ignore that. The crystals.. I have never used them in game myself but I assumed they should go off instantly? I was right next to one and clicked when Icharon did his thing and 2 eles got to him before it went off I'll go in there again today and see if I can get icharon again |
|
Achievement: Dehydration works Crystals take 4-5 seconds from clicking them to killing the current mobs, as seen violet.crystals.mp4But the delay could be normal, a quick search says it takes 'a second' and the one video I could find with it, in the right time frame of OG wrath, another player goes up to one in the background and it takes a few seconds before the mobs die, that's assuming the player pressed the crystal as soon as they could. I did a run in game to see how they are and it takes about the same amount of time so I figure the crystals are a none issue. Only thing I haven't tested for this one is evading a boss as I can't get one to evade, but other than that everything seems grand! |
|
I got a notification this has been updated, do you need me to test it again, or a different aspect? |
|
No, its just the build that needed fixing, thanks |
Changes Proposed:
This PR proposes changes to:
Full modernization of the Violet Hold instance and boss scripts, bringing them in line with modern AzerothCore conventions.
Script changes (12 files, net -483 lines):
ScriptedAItoBossAIwith encounter slot redirection — individual bossSetBossStatecalls (e.g.BOSS_MORAGG) are caught by the instance script and redirected to encounter slots (DATA_1ST_BOSS/DATA_2ND_BOSS) based on wave countSetBossState/GetBossStateinstead of rawEncounter[]arrayObjectDataarrays withLoadObjectData()for automatic creature/GO tracking, removing manualObjectGuidmembersRegisterVioletHoldCreatureAImacro; remove class wrappers from all boss and trash AI structsEventMap-based eventsif (Instance)nullptr checks (guaranteed by registration macro)StorePersistentData/GetPersistentDatafor boss selection instead of custom save/loadHandleGameObject(uint32, bool)overload throughoutIsEncounterInProgress()to return actual encounter state (was hardcodedfalse)SecondBosswas written but never loaded backCore change:
aliveOnlyparameter toMap::GetPlayersCountExceptGMs()(defaultfalse, existing callers unaffected), excluding Spirit of Redemption players when setAI-assisted Pull Requests
Important
While the use of AI tools when preparing pull requests is not prohibited, contributors must clearly disclose when such tools have been used and specify the model involved.
Contributors are also expected to fully understand the changes they are submitting and must be able to explain and justify those changes when requested by maintainers.
Issues Addressed:
SOURCE:
The changes have been validated through:
SetBossStateencounter slot redirection pattern is based on TrinityCore's implementation by joschiwald, credited viaCo-Authored-By.Tests Performed:
This PR has been:
How to Test the Changes:
Known Issues and TODO List:
How to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.