[4.3.4] Added Baleroc Encounter #11558

Closed
wants to merge 12 commits into
from
@Krudor
Contributor
Krudor commented Feb 3, 2014

Implements everything from the Baleroc Encounter except for:

Vital Spark
http://www.wowhead.com/spell=99262

Vital Flame
http://www.wowhead.com/spell=99263

And the after-fight conversation initation for Majordomo Staghelm.
"Well well... I admire your tenacity. Baleroc stood guard over this keep
for a thousand mortal lifetimes. But NONE may enter the Firelord's
abode! Beg for mercy now, and I may yet allow you to live. Well,
‘heroes,’ what is your answer?"

This is my first real contribution to TrinityCore as a long-term world
of warcraft hardcore raider and I believe I can help TrinityCore on the
PvE side.

Apologies for any error I may have done uploading this, as it's my first
time using the GitHub client to submit a pull request as well.

Krudor added some commits Feb 2, 2014
@Krudor Krudor Added Baleroc Encounter
Implements everything from the Baleroc Encounter except for:

Vital Spark
http://www.wowhead.com/spell=99262

Vital Flame
http://www.wowhead.com/spell=99263

And the after-fight conversation initation for Majordomo Staghelm.
"Well well... I admire your tenacity. Baleroc stood guard over this keep
for a thousand mortal lifetimes. But NONE may enter the Firelord's
abode! Beg for mercy now, and I may yet allow you to live. Well,
‘heroes,’ what is your answer?"
GateIntro2: But none may enter the Firelord's abode
GateIntro3: Beg for mercy now, and I may yet allow you to live. Well
heroes, what is your answer?

This is my first real contribution to TrinityCore as a long-term world
of warcraft hardcore raider and I believe I can help TrinityCore on the
PvE side.

Apologies for any error I may have done uploading this, as it's my first
time using the GitHub client to submit a pull request as well.
1b799be
@Krudor Krudor Baleroc creature_text
Added missing creature_text for Baleroc
bab55ae
@Krudor Krudor Update Vehicle.cpp
Changed so that players are not provided with a parachute, if kicked out
of a hostile flying vehicle.
61425a4
@Krudor Krudor Tuned Baleroc Timers
Tuned Baleroc timers slightly, it's a tight enrage encounter so timers
has to be right.
5ad57a1
@Krudor Krudor instance_firelands updated
Updated instance_firelands door data and missing GetData64 part for
Baleroc.
23fd86b
@Krudor Krudor Update 2014_02_03_00_world_creature_text.sql 80b9b72
@Krudor Krudor Update boss_baleroc.cpp 3f88d27
@Krudor Krudor Update 2014_02_03_00_world_spell_script_names.sql e4e911e
@Krudor Krudor Update 2014_02_03_00_world_creature_template.sql e7a875c
@Krudor Krudor Update 2014_02_03_00_world_creature.sql 02b5b10
@Krudor Krudor Update instance_firelands.cpp 591f205
@Krudor Krudor Missing Baleroc SQL
Added a required missing Baleroc SQL update
b7684bd
@Kylroi
Kylroi commented Feb 3, 2014

Isn't REPLACE a single query version of the DELETE/INSERT pair of queries? Sure looks that way when I read the MySQL documentation.

REPLACE works exactly like INSERT, except that if an old row in the table has the same value as a new row for a PRIMARY KEY or a UNIQUE index, the old row is deleted before the new row is inserted.

@joschiwald joschiwald commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ {
+ instance->SendEncounterUnit(ENCOUNTER_FRAME_DISENGAGE, me);
+ instance->DoRemoveAurasDueToSpellOnPlayers(SPELL_BLAZE_OF_GLORY);
+ me->GetMotionMaster()->MoveTargetedHome();
+ summons.DespawnAll();
+ _DespawnAtEvade();
+ }
+
+ void DoMeleeAttackIfReady() OVERRIDE
+ {
+ if (me->HasUnitState(UNIT_STATE_CASTING))
+ return;
+
+ Unit* victim = me->GetVictim();
+ if (me->isAttackReady(BASE_ATTACK) && me->IsWithinMeleeRange(victim))
+ {
@joschiwald
joschiwald Feb 3, 2014 Member

this is hack, SPELL_DECIMATION_BLADE, SPELL_DECIMATION_BLADE_2, SPELL_INFERNO_BLADE have SPELL_AURA_PROC_TRIGGER_SPELL_2, if it doesn't work, maybe this aura type needs better implementation, or something is missing, proc flags or something else

@joschiwald
joschiwald Feb 3, 2014 Member

check InitTriggerAuraData() in unit.cpp

@joschiwald joschiwald commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+
+ bool Load()
+ {
+ if (GetCaster()->GetTypeId() != TYPEID_UNIT)
+ return false;
+ return true;
+ }
+
+ bool Validate(SpellInfo const* /*spellInfo*/)
+ {
+ if (!sSpellMgr->GetSpellInfo(SPELL_DECIMATING_STRIKE))
+ return false;
+ return true;
+ }
+
+ void ChangeDamage()
@joschiwald
joschiwald Feb 3, 2014 Member

use effect basepoints

@Warpten
Warpten Feb 8, 2014 Member

What josh means:
Use GetSpellInfo()->Effects[EFFECT_3].BasePoints (can't remember the new way to access these, that's what i get for not touching the core for over 6 months) and GetHitDamage()

Also if i recall correctly SetHitDamage expects an int32, not _u_int32

@joschiwald joschiwald commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ }
+
+ targets.sort(Trinity::ObjectDistanceOrderPred(GetCaster(), true));
+ targets.resize(1);
+
+ if ((*targets.begin())->GetDistance2d(GetCaster()) > 15.0f)
+ GetCaster()->ToCreature()->AI()->DoCast(SPELL_WAVE_OF_TORMENT);
+ else
+ {
+ if ((*targets.begin())->ToPlayer()->GetAura(SPELL_TORMENT_PERIODIC))
+ if ((*targets.begin())->ToPlayer()->GetAura(SPELL_TORMENT_PERIODIC)->GetCaster() != GetCaster())
+ GetCaster()->CastSpell((*targets.begin())->ToPlayer(), SPELL_TORMENT_PERIODIC, false);
+ else{
+ }
+ else
+ GetCaster()->CastSpell((*targets.begin())->ToPlayer(), SPELL_TORMENT_PERIODIC, false);
@joschiwald
joschiwald Feb 3, 2014 Member

don't cast spells in target select hooks

@Krudor
Krudor Feb 3, 2014 Contributor

There's no clean way to move the spellcast to a proper hook without making it even more ugly, at least what I know of.

@Shauren
Shauren Feb 3, 2014 Member

I'm goint to agree with @joschiwald here, strictly no casting spells in target select hooks

@Warpten
Warpten Feb 8, 2014 Member

@Krudor: use an OnEffectHitTarget hook for that (will be called for every worldobject in the target list once OnObjectAreaTargetSelect is done), access current target with GetHitUnit()

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ break;
+ }
+ else if (_sharedThePain[i].player == 0)
+ {
+ _sharedThePain[i].player = guid;
+ _sharedThePain[i].tormented++;
+ break;
+ }
+ }
+ break;
+ default:
+ break;
+ }
+ }
+
+ bool SharedThePain()
@Shauren
Shauren Feb 3, 2014 Member

Could have been done using GetData - that would remove the need for type casting

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+typedef boss_baleroc::boss_balerocAI BalerocAI;
+
+class npc_shard_of_torment : public CreatureScript
+{
+ public:
+ npc_shard_of_torment() : CreatureScript("npc_shard_of_torment") { }
+
+ struct npc_shard_of_tormentAI : public ScriptedAI
+ {
+ npc_shard_of_tormentAI(Creature* creature) : ScriptedAI(creature)
+ {
+ me->SetReactState(REACT_PASSIVE);
+ me->SetFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_DISABLE_MOVE | UNIT_FLAG_NOT_SELECTABLE | UNIT_FLAG_NON_ATTACKABLE);
+ me->SetFlag(UNIT_FIELD_FLAGS_2, UNIT_FLAG2_DISABLE_TURN);
+ me->SetDisplayId(me->GetCreatureTemplate()->Modelid2);
+ _instance = creature->GetInstanceScript();
@Shauren
Shauren Feb 3, 2014 Member

All of these modifications in code are hacks, use proper creature_template fields (unit flags and flags_extra)

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ switch (eventId)
+ {
+ case EVENT_SHARD_SPAWN_EFFECT:
+ me->RemoveAurasDueToSpell(SPELL_TORMENT_COSMETIC_1);
+ DoCast(SPELL_TORMENT);
+ break;
+ default:
+ break;
+ }
+ }
+ }
+
+ private:
+ InstanceScript* _instance;
+ EventMap _events;
+ Creature* _baleroc;
@Shauren
Shauren Feb 3, 2014 Member

Do not store pointers to game objects (creatures/go/players/items, whatever)

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ {
+ if (GetCaster()->GetTypeId() != TYPEID_UNIT)
+ return false;
+ return true;
+ }
+
+ bool Validate(SpellInfo const* /*spellInfo*/)
+ {
+ if (!sSpellMgr->GetSpellInfo(SPELL_DECIMATING_STRIKE))
+ return false;
+ return true;
+ }
+
+ void ChangeDamage()
+ {
+ if (GetCaster()->GetVictim())
@Shauren
Shauren Feb 3, 2014 Member

And always use spell targets in spell scripts, do not refer to what caster is targeting

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ void FilterTargets(std::list<WorldObject*>& targets)
+ {
+ uint8 numtargets;
+ if (GetCaster()->GetMap()->Is25ManRaid())
+ numtargets = 2;
+ else
+ numtargets = 1;
+
+ while(targets.size() < numtargets)
+ numtargets--;
+
+
+ if ((targets.size() > numtargets) && GetCaster()->GetVictim())
+ targets.remove(GetCaster()->ToCreature()->GetVictim()); //Safe to remove tank from list
+
+ Trinity::Containers::RandomResizeList(targets, numtargets);
@Shauren
Shauren Feb 3, 2014 Member

This needs special filtering. On 25 man mode, one crystal is designed to always land on a player in melee range and one on a ranged player

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+
+ void HandleScript(SpellEffIndex effIndex)
+ {
+ PreventHitDefaultEffect(effIndex);
+ GetCaster()->CastSpell(GetHitUnit(), SPELL_SHARDS_OF_TORMENT_2, true);
+ }
+
+ void FilterTargets(std::list<WorldObject*>& targets)
+ {
+ uint8 numtargets;
+ if (GetCaster()->GetMap()->Is25ManRaid())
+ numtargets = 2;
+ else
+ numtargets = 1;
+
+ while(targets.size() < numtargets)
@Shauren
Shauren Feb 3, 2014 Member

could be just replaced with numtargets = std::min<size_t>(numtargets, targets.size());

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+
+class spell_baleroc_tormented : public SpellScriptLoader
+{
+ public:
+ spell_baleroc_tormented() : SpellScriptLoader("spell_baleroc_tormented") { }
+
+ class spell_baleroc_tormented_SpellScript : public SpellScript
+ {
+ PrepareSpellScript(spell_baleroc_tormented_SpellScript);
+
+ void ChangeDamage()
+ {
+ //SetHitDamage(GetHitDamage()*GetHitUnit()->GetAuraCount(m_scriptSpellId));
+
+ //The above example seems wrong, wowhead say the damage is 3000 per tick on normal, and 4250 on heroic,
+ //while logs from retail say its 4000 normal, and 5000 heroic.
@Shauren
Shauren Feb 3, 2014 Member

Depending on how old are your logs, this code might be invalid as damage was nerfed at some point between 4.2 and 4.3

@Shauren Shauren commented on the diff Feb 3, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ public:
+ spell_baleroc_tormented_heroic() : SpellScriptLoader("spell_baleroc_tormented_heroic") { }
+
+ class spell_baleroc_tormented_heroic_SpellScript : public SpellScript
+ {
+ PrepareSpellScript(spell_baleroc_tormented_heroic_SpellScript);
+
+ bool Load()
+ {
+ return GetCaster()->GetTypeId() == TYPEID_PLAYER;
+ }
+
+ void HandleScript(SpellEffIndex effIndex)
+ {
+ PreventHitDefaultEffect(effIndex);
+ if (GetCaster()->GetMap()->IsHeroic())
@Shauren
Shauren Feb 3, 2014 Member

This check is not needed. Normal mode versions of spell 99257 (Tormented) do not have the 3rd periodic effect triggering 99489

@Krudor
Contributor
Krudor commented Feb 4, 2014

Havings sniffs for this would make it a whole lot easier...

@Krudor
Contributor
Krudor commented Feb 5, 2014

Update coming to Baleroc improvements, bridge cinematic, fandral intro and fandral achievement.

@Subv
Contributor
Subv commented Feb 5, 2014

I find it hard to believe that this is your first contribution to TC, it is a pretty big script which is mostly good, it has a few things that should be fixed but overall it looks pretty good, good job!

P. S: Drop by the IRC if you need some help from the devs

@Shauren Shauren commented on the diff Feb 5, 2014
src/server/game/Entities/Vehicle/Vehicle.cpp
@@ -529,7 +529,8 @@ Vehicle* Vehicle::RemovePassenger(Unit* unit)
// only for flyable vehicles
if (unit->IsFlying())
- _me->CastSpell(unit, VEHICLE_SPELL_PARACHUTE, true);
+ if (_me->IsFriendlyTo(unit))
+ _me->CastSpell(unit, VEHICLE_SPELL_PARACHUTE, true);
@Shauren
Shauren Feb 5, 2014 Member

I believe this doesn't belong in this patch :)

@Krudor
Krudor Feb 6, 2014 Contributor

Indeed, it seems whenever I push to my private repo it automatically updates and pushes to this pull request :o!

@Shauren Shauren commented on the diff Feb 5, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+
+ void JustDied(Unit* /*killer*/) OVERRIDE
+ {
+ _JustDied();
+ Talk(EMOTE_DEATH);
+ SetEquipmentSlots(true);
+ me->SetCanDualWield(true);;
+ instance->SendEncounterUnit(ENCOUNTER_FRAME_DISENGAGE, me);
+ if (Map* map = me->GetMap())
+ {
+ Map::PlayerList const &PlayerList = map->GetPlayers();
+ if (!PlayerList.isEmpty())
+ for (Map::PlayerList::const_iterator i = PlayerList.begin(); i != PlayerList.end(); ++i)
+ if (i->GetSource()->ToPlayer()->HasQuestForItem(69848))
+ {
+ DoCast(101093);
@Shauren
Shauren Feb 5, 2014 Member

use enum for this
also, do not use DoCast(uint32) method, it is not meant for scripts like this one (its for the crappy default ais)

@Krudor
Krudor Feb 6, 2014 Contributor

Ops, must have forgot to add enum for those, it was in an about 40~ hour long session.
Have not taken a look at the DoCast vs CastSpell function yet, but I'll stop using DoCast now then ;] Just thought DoCast(101093); was easier to use than me->CastSpell((Unit)*NULL, 101093, true/false);

@gigatotem
Contributor

The Majordomo conversation appeared for me after I had defeated Baleroc and the bosses before him

The order I killed them was Shannox -> Baleroc -> Alysrazor -> Rhyolith -> Beth'tilac and the speech did not show until defeating Beth.

Live realms ofc.

@Kittnz
Member
Kittnz commented Feb 5, 2014

@gigatotem You might want to blur the picture mate.

@Krudor
Contributor
Krudor commented Feb 6, 2014

Yes gigatotem, thank you for pointing that out. Did a live run 1-2 days ago and could confirm this myself, that it triggers after all bosses previous to Majordomo must be defeated before he does the speach and activates the magma orb.

This has been updated and fixed in my upcoming update to the pull request in a day or two.
I also have a request to add another player UInt32 value to store player emotes, as there's no function on trinitycore to find a player's current emotestate.

More info when I update pull request.

@Shauren
Member
Shauren commented Feb 6, 2014

I also have a request to add another player UInt32 value to store player emotes, as there's no function on trinitycore to find a player's current emotestate

You dont need this for baleroc

@Krudor
Contributor
Krudor commented Feb 6, 2014

Needed it to finish the targeting filter for "Only The Penitent Achievement".
http://www.wowhead.com/achievement=5799

I was abled to use player->IsStandState() for this purpose though.

@Shauren
Member
Shauren commented Feb 6, 2014

As I said, that achievement is not part of Baleroc but Majordomo Fandral Staghelm

@Krudor
Contributor
Krudor commented Feb 6, 2014

Yeah, it's whats coming along with the next update im pushing with baleroc updates.

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ void FilterTargets(std::list<WorldObject*>& targets)
+ {
+
+ //Remove current tank if we have one
+ if (GetCaster()->GetVictim())
+ targets.remove(GetCaster()->ToCreature()->GetVictim());
+
+ if (targets.size() < 2)
+ {
+ FinishCast(SPELL_FAILED_NO_VALID_TARGETS);
+ return;
+ }
+
+ Trinity::Containers::RandomResizeList(targets, 2);
+
+ std::list<WorldObject*>::const_iterator itr = targets.begin();
@Warpten
Warpten Feb 8, 2014 Member

You could use std::list::front and std::list::back here.

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+{
+ public:
+ spell_countdown_p1() : SpellScriptLoader("spell_countdown_p1") { }
+
+ class spell_countdown_p1_SpellScript : public SpellScript
+ {
+ PrepareSpellScript(spell_countdown_p1_SpellScript);
+
+ bool Load()
+ {
+ return GetCaster()->GetTypeId() == TYPEID_UNIT;
+ }
+
+ void CastSpellLink()
+ {
+ if (target1->ToPlayer() && target2->ToPlayer())
@Warpten
Warpten Feb 8, 2014 Member

Could be avoided on SelectTargets by using std::list::remove_if(...) where the predicate's name is along the name of Trinity::ObjectTypeIdCheck

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ GetTarget()->ToPlayer()->RemoveAurasDueToSpell(SPELL_COUNTDOWN_5);
+ }
+
+ void Register() OVERRIDE
+ {
+ AfterEffectRemove += AuraEffectRemoveFn(spell_countdown_p2_AuraScript::OnRemove, EFFECT_0, SPELL_AURA_PERIODIC_TRIGGER_SPELL, AURA_EFFECT_HANDLE_REAL);
+ }
+ };
+
+ AuraScript* GetAuraScript() const OVERRIDE
+ {
+ return new spell_countdown_p2_AuraScript();
+ }
+};
+
+class spell_countdown_p3 : public SpellScriptLoader
@Warpten
Warpten Feb 8, 2014 Member

I would also suggest giving more relevant names to these:
SPELL_COUNTDOWN
SPELL_COUNTDOWN_TRIGGER
SPELL_COUNTDOWN_DAMAGE
etc

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ return true;
+ }
+
+ bool Validate(SpellInfo const* /*spellInfo*/)
+ {
+ if (!sSpellMgr->GetSpellInfo(SPELL_DECIMATING_STRIKE))
+ return false;
+ return true;
+ }
+
+ void ChangeDamage()
+ {
+ if (GetCaster()->GetVictim())
+ {
+ uint32 health = GetCaster()->GetVictim()->GetMaxHealth();
+ if (health*0.9 < 250000)
@Warpten
Warpten Feb 8, 2014 Member

What about SetHitDamage(std::max(uint32(CalculatePct(health, GetHitDamage()), 250000u)); ?

@joschiwald
joschiwald Feb 8, 2014 Member

SetHitDamage(std::max(CalculatePct(health, GetHitDamage()), GetSpellInfo()->Effects[EFFECT_2].CalcValue()));
that uses effect basepoints

@Warpten
Warpten Feb 8, 2014 Member

where did my edit go ;_;

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ }
+
+ void Register() OVERRIDE
+ {
+ OnEffectHitTarget += SpellEffectFn(spell_shards_of_torment_SpellScript::HandleScript, EFFECT_0, SPELL_EFFECT_DUMMY);
+ OnObjectAreaTargetSelect += SpellObjectAreaTargetSelectFn(spell_shards_of_torment_SpellScript::FilterTargets, EFFECT_0, TARGET_UNIT_SRC_AREA_ENEMY);
+ }
+ };
+
+ SpellScript* GetSpellScript() const OVERRIDE
+ {
+ return new spell_shards_of_torment_SpellScript();
+ }
+};
+
+class PlayerCheck
@Warpten
Warpten Feb 8, 2014 Member

Err ? i also believe the Trinity namespace has this kind of predicates (also that gm check needs to go)

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ OnObjectAreaTargetSelect += SpellObjectAreaTargetSelectFn(spell_shards_of_torment_SpellScript::FilterTargets, EFFECT_0, TARGET_UNIT_SRC_AREA_ENEMY);
+ }
+ };
+
+ SpellScript* GetSpellScript() const OVERRIDE
+ {
+ return new spell_shards_of_torment_SpellScript();
+ }
+};
+
+class PlayerCheck
+{
+ public:
+ bool operator()(WorldObject* object) const
+ {
+ if (object->GetTypeId() != TYPEID_PLAYER)
@Warpten
Warpten Feb 8, 2014 Member

Logic fail, you check if not an player and if so, you still cast to Player

@joschiwald
joschiwald Feb 8, 2014 Member

why does the spell hit gamemasters?

@Krudor
Krudor Feb 11, 2014 Contributor

Made several changes to it, missed changing it.

It does not hit gamemasters, it checks if the target is a gamemaster, if so it REMOVES it from the list of eligible targets. I dont think this check is even needed, gamemasters are probably already excluded from aoe spell targeting by default.

@joschiwald
joschiwald Feb 11, 2014 Member

that was my point

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+
+ void FilterTargets(std::list<WorldObject*>& targets)
+ {
+ targets.remove_if(PlayerCheck());
+
+ if (targets.empty())
+ {
+ //No targets found, start pulsating immediately.
+ GetCaster()->ToCreature()->AI()->DoCast(SPELL_WAVE_OF_TORMENT);
+ return;
+ }
+
+ targets.sort(Trinity::ObjectDistanceOrderPred(GetCaster(), true));
+ targets.resize(1);
+
+ if ((*targets.begin())->GetDistance2d(GetCaster()) > 15.0f)
@Warpten
Warpten Feb 8, 2014 Member

*targets.begin() to targets.front() looks nicer

Player* target = targets.front()->ToPlayer()

@Warpten Warpten commented on the diff Feb 8, 2014
src/server/scripts/Kalimdor/Firelands/boss_baleroc.cpp
+ {
+ return new spell_baleroc_tormented_heroic_SpellScript();
+ }
+};
+
+class achievement_share_the_pain : public AchievementCriteriaScript
+{
+ public:
+ achievement_share_the_pain() : AchievementCriteriaScript("achievement_share_the_pain") { }
+
+ bool OnCheck(Player* /*source*/, Unit* target) OVERRIDE
+ {
+ if (!target)
+ return false;
+
+ if (BalerocAI* balerocAI = CAST_AI(BalerocAI, target->GetAI()))
@Warpten
Warpten Feb 8, 2014 Member

This is where GetData and SetData would save you from using CAST_AI:
return target->GetAI()->GetData(DATA_SHARE_THE_PAIN) == 0; (or 1, don't recall how you save it)

@Krudor
Krudor Feb 8, 2014 Contributor

I'm not the best programmer, I can go ahead and cancel this pull request so you can rewrite the script and post it yourself if you like.

@Walkum
Walkum Feb 9, 2014 Contributor

Don't be angry man, they only are making tips for improve the boss code, but your boss are fantastic and is working perfectly, not all the people can do this work, congratulations seriously :)

@Krudor
Krudor Feb 9, 2014 Contributor

Yeah well you have to understand that at some point, when all people can do it point out error after error after error in your code, you'll get slightly frustrated.

I do appriciate the comments and I'll make use the suggestions of improvement you're providing however, following my previous statement.

@Warpten
Warpten Feb 11, 2014 Member

I am far from being the best either, I wasn't denegating your work, just suggesting changes.

There are sadly places where the CAST_AI macro has to stay, but in 99.99% of the cases the SetData and GetData combo is preferred.

I'm not a douche, sorry if you feel offended. This PR is one of the best looking i've seen in a long time. And yes, I've been through these situations where people hint you seemingly roughly about minor changes, and you feel either dumb for not using those, not knowing them, or just get gradually mad because you've put so much work into this and it still isn't "perfect" to the other guy's eyes. I can't remember how much time I spent on Kaelima's Halion start before he came to help, and the time past that event. Still the PR didn't go in instantaneously.

Nonetheless, that's still really good work. I'm on IRC 24/7 on a BNC if you need me (make sure to ask me to add you to my accept list first, I'm in +g mode).

@Krudor
Krudor Feb 18, 2014 Contributor

Thanks for this comment Warpten, I really appriciate it.

As for the update, I'm still going to be busy this week as well so there'll be more delays, but I have not forgot about it!

Cheers.

@Krudor
Contributor
Krudor commented Feb 10, 2014

Update coming hopefully on friday, have an important exam i'll have to finish before this.

@Walkum
Contributor
Walkum commented Feb 10, 2014

Good luck in your exam!

@Kittnz
Member
Kittnz commented Mar 2, 2014

Any update @Krudor ?

@Misheel
Misheel commented Mar 11, 2014

@Krudor It would be Cool If you can finish it man. GJ

@digz6666
digz6666 commented May 1, 2014

Any updates?

@tkrokli
Member
tkrokli commented May 1, 2014

@digz6666 : Seeing that there are no assigned person and that there is no milestone for this, it should be evident that any updates will be random or depending on when the OP / other participants have got time to work on this. We just have to wait and see. Also, if the exam Krudor mentioned 3 months ago still is going on, you may have to wait until closer to summer before this gets updated. OK? :-)

@jackpoz jackpoz changed the title from Added Baleroc Encounter to [4.3.4] Added Baleroc Encounter May 20, 2014
@Aokromes
Member

Can you put all the sql files on a single sql and rebase?

@Krudor
Contributor
Krudor commented Aug 18, 2014

10 years later I got caught up with other important things and did not have enough time to finish this.
At this point I've gathered enough experience to wrap this up but I'm not certain if it's worth it at this point.

I'll put this somewhere on my to-do list, you I might go ahead and close this and I'll re-open a new pull request with all the small sql files bunched up.

@Aokromes
Member

You can simply make a new commit merging them and then rebase on that way you don't need to open a new pr.

@Aokromes Aokromes added this to the 4.3.4 pve milestone Aug 28, 2014
@Warpten
Member
Warpten commented Sep 21, 2014

@Krudor: as I asked you on IRC yesterday night, any news on this?

@Krudor
Contributor
Krudor commented Oct 3, 2014

Hey sorry for the late reply Warpten, I will finish this at some point after I'm done with my current project for TrinityCore, I'm not ready for a reveal yet.

I'm fully okay with this being removed for now, it's been on standby for such a long time.

@Kinzcool Kinzcool closed this Oct 18, 2014
@jackpoz jackpoz reopened this Oct 18, 2014
@DDuarte
Member
DDuarte commented Oct 18, 2014

Putting this on hold for now. We'll review it again once things with WoD and such are sorted out.
Thanks for the PR anyway!

@DDuarte DDuarte closed this Oct 18, 2014
@Aokromes
Member
Aokromes commented Jul 3, 2016

over 1 year after and still unmerged.

@Krudor
Contributor
Krudor commented Jul 4, 2016

I've got some free time to work on TrinityCore, and I will give this another pass after I'm done wrapping up what I'm currently working on.

Because of the unknown amount of issues that could arise when trying to reapply this PR to 6.X core, all I can offer is an empty promise that I'll spend some time looking over this once more, patching up whatever inefficiency/missing features I find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment