Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripts/BlackrockDepths: fix Ironhand Guardian #21816

Merged

Conversation

martintopholm
Copy link
Contributor

In BlackrockDepths in the Iron Hall Ironhand Guardians in the sides
should only cast "Gout of Flame" during the fight with Magmus (src:
classicdb/database#715).

Changes proposed:

Target branch(es): 335

Issues addressed: No issue open on this.

Tests performed: Patch is based on upstream commit 1e25b21 (DB/Creature:
Scarlet Infantryman equipments). This builds builds and described functionality
validated using:

  • .gm on
  • .cheat god on
  • .tele blackrock
  • .go creature magmus
  • check that Ironhand Guradians don't cast
  • `.gm off
  • check that Ironhand Guradians now do cast
  • /target Magmus
  • `.damage 25000
  • check that Ironhand Guradians stop casting

Known issues and TODO list: No known issues with this patch. Although it
probably would be prettier to move the SAI to BlackrockDepths scripts instead.

@Killyana
Copy link
Contributor

Killyana commented Apr 13, 2018

I don't see any call to put Ironhand Guardians in combat when the boss aggro, and stop the combat when it dies.

Also according to some videos they keep casting once the boss dies without putting players in combat https://youtu.be/GR_CQ-eC6ns?t=32m26s

To do that, better to use, the event SMART_EVENT_UPDATE
And to stop the combat I'm not sure how it could be done (maybe they never enter the combat mode.)

@martintopholm
Copy link
Contributor Author

The call to have them start casting is the SetData from instance_blackrock_depths.cpp line 245 and line 455.

I agree from the video they continue the casting and apparently doesn't cause the player to be in combat. Also from the video shows that they don't start casting before Magmus is engaged. So dropping line 247-248 from instance_blackrock_depths.cpp would keep them going, but leave the in combat issue to be fixed.

@martintopholm
Copy link
Contributor Author

@Killyana I've tried to adapt the patches to keep the Ironhand Guardians casting and clearing CombatStatus "after" each cast. This should enable a player that (move out of the area of effect) to go out of combat when the spell stops.

@ghost
Copy link

ghost commented Apr 15, 2018

Recommended PR title format: Scripts/BlackrockDepths: fix Ironhand Guardian using phases
([Scripts/Category] or [Scripts/DungeonName] + capitalization of creature names)
Also based on /src/server/game/ = "Core", /src/server/scripts = "Scripts".

@martintopholm martintopholm changed the title Core/Scripts: fix ironhand guardian using phases Scripts/BlackrockDepths: fix Ironhand Guardian Apr 15, 2018
@martintopholm
Copy link
Contributor Author

When integrating the improvements mentioned above. Should I put a commit on top or squash the entire set into one commit?

@Aokromes
Copy link
Member

you don't need to squash, we squash on merge


npc_ironhand_guardianAI(Creature* creature) : ScriptedAI(creature)
{
_instance = me->GetInstanceScript();
Copy link
Contributor

@Keader Keader Apr 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing initialize _lastState here.

Copy link
Contributor

@Keader Keader Apr 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not realy needed, but you can use octor list to initialize it.
like:
npc_ironhand_guardianAI(Creature* creature) : ScriptedAI(creature), _instance(creature->GetInstanceScript()), _lastState(0) { }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghost
Copy link

ghost commented Apr 15, 2018

to any TC member with label management access:
replace [Comp-Core] with [Comp-C++Script] and add [Sub-Instances]


void UpdateAI(uint32 uiDiff) override
{
if (_lastState == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing UpdateVictim() call, this creature will never evade?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Ironhand Guardian should not evade. When the party has entered the should keep casting Gout of Flame. Whether it should have a UpdateVictim anyways I am unsure about.

@jackpoz
Copy link
Contributor

jackpoz commented Jun 5, 2019

@Keader is there anything left to change in this PR or can it be merged ?

@Keader
Copy link
Contributor

Keader commented Jun 5, 2019

can be merged i belive

@offl
Copy link
Contributor

offl commented Jul 3, 2019

https://youtu.be/rhhfpQGXnmY?t=81
Interestingly they cast this spell even if Magmus is not engaged. Seems like classicdb/database#715 (comment) speaks the truth as in the video above it's not his first attempt(look at the corpses near Magmus)

I believe they shouldn't have threat list at all + the spell shouldn't generate threat(shown in the video above). (#23435)

@Wyrserth Wyrserth force-pushed the fix-brd-ironhand-guardians-v2 branch from d2082fe to b37b353 Compare July 5, 2019 21:46
@Wyrserth Wyrserth merged commit 4529a94 into TrinityCore:3.3.5 Jul 5, 2019
Aokromes pushed a commit to Aokromes/TrinityCore that referenced this pull request Jul 8, 2019
Shauren pushed a commit that referenced this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants