Skip to content

Commit

Permalink
Aura stacking rewrite: Stage 1 (Positive/Negative effects)
Browse files Browse the repository at this point in the history 
This is the first stage of the ongoing aura rewrite task.

The changelog:

Revamp of positive/negatve effects handling:
* Introducing neutral target modes category: positive/negative sign depends on caster's and target's relations.
* Existing target modes are now split into 3 categories: friendly, hostile, neutral.
* Positive/negative effect detection revamped and cleaned up to use the new target mode logic. A few bugs may appear due to neutral targets behaviour, report them asap for hotfix.
* Small improvement for aura cancel handler.

Changes to proc/trigger system:
* Additional target triggers now use target mode sign check to not proc hostile procs on friendly targets (SP's Shadow Weaving).
* Spell proc now has a basic check for target mode relation between the target and the caster to not cast hostile procs on friendly targets, unless reflected (To be improved in the future).

Misc changes:
* Friendly Fire spells (such as Gruul's Shatter) should no longer put friendly players in PvP combat with each other.

Changes to spell hit detection system:
* Failed hostile spell hits now put caster in combat with creatures.
* Failed hostile spell hits now put caster in PvP combat with players.
* Fixes Polymorph and several other spells not initiating combat: SPELL_ATTR_STOP_ATTACK_TARGET does not prevent aggro.
  • Loading branch information
@Warlockbugs
Warlockbugs committed Sep 23, 2016
1 parent a5c1a89 commit 21f2f00
Show file tree
Hide file tree
Showing 9 changed files with 447 additions and 390 deletions.
2 changes: 1 addition & 1 deletion src/game/Player.cpp
View file
Expand Up @@ -7884,7 +7884,7 @@ void Player::CastItemCombatSpell(Unit* Target, WeaponAttackType attType)

if (roll_chance_f(chance))
{
if (IsPositiveSpell(spellInfo->Id))
if (IsPositiveSpell(spellInfo->Id, this, Target))
CastSpell(this, spellInfo->Id, true, item);
else
{
Expand Down
65 changes: 37 additions & 28 deletions src/game/Spell.cpp
View file
Expand Up @@ -1041,28 +1041,34 @@ void Spell::DoAllEffectOnTarget(TargetInfo* target)

if (missInfo == SPELL_MISS_NONE) // In case spell hit target, do all effect on that target
DoSpellHitOnUnit(unit, mask);
else if (missInfo == SPELL_MISS_REFLECT) // In case spell reflect from target, do all effect on caster (if hit)
else if (missInfo != SPELL_MISS_EVADE)
{
if (target->reflectResult == SPELL_MISS_NONE) // If reflected spell hit caster -> do all effect on him
if (missInfo == SPELL_MISS_REFLECT) // In case spell reflect from target, do all effect on caster (if hit)
{
DoSpellHitOnUnit(m_caster, mask);
unitTarget = m_caster;
if (target->reflectResult == SPELL_MISS_NONE) // If reflected spell hit caster -> do all effect on him
{
DoSpellHitOnUnit(m_caster, mask);
unitTarget = m_caster;
}
}
}
else if (missInfo == SPELL_MISS_MISS || missInfo == SPELL_MISS_RESIST)
{

// Failed hostile spell hits count as attack made against target (if detected)
if (real_caster && real_caster != unit)
{
// can cause back attack (if detected)
if (!m_spellInfo->HasAttribute(SPELL_ATTR_EX3_NO_INITIAL_AGGRO) && !IsPositiveSpell(m_spellInfo->Id) &&
m_caster->isVisibleForOrDetect(unit, unit, false))
if (!m_spellInfo->HasAttribute(SPELL_ATTR_EX3_NO_INITIAL_AGGRO) &&
!m_spellInfo->HasAttribute(SPELL_ATTR_EX_NO_THREAT) &&
!IsPositiveSpell(m_spellInfo->Id, real_caster, unit) &&
m_caster->isVisibleForOrDetect(unit, unit, false))
{
if (!unit->isInCombat() && unit->GetTypeId() != TYPEID_PLAYER && ((Creature*)unit)->AI())
((Creature*)unit)->AI()->AttackedBy(real_caster);

unit->AddThreat(real_caster);
unit->SetInCombatWith(real_caster);
real_caster->SetInCombatWith(unit);

if (Player* attackedPlayer = unit->GetCharmerOrOwnerPlayerOrPlayerItself())
real_caster->SetContestedPvP(attackedPlayer);
}
}
}
Expand Down Expand Up @@ -1249,9 +1255,11 @@ void Spell::DoSpellHitOnUnit(Unit* unit, uint32 effectMask)
if (!m_spellInfo->HasAttribute(SPELL_ATTR_EX_NOT_BREAK_STEALTH))
unit->RemoveSpellsCausingAura(SPELL_AURA_MOD_STEALTH);

// can cause back attack (if detected), stealth removed at Spell::cast if spell break it
if (!m_spellInfo->HasAttribute(SPELL_ATTR_EX3_NO_INITIAL_AGGRO) && !IsPositiveSpell(m_spellInfo->Id) &&
m_caster->isVisibleForOrDetect(unit, unit, false))
// Hostile spell hits count as attack made against target (if detected), stealth removed at Spell::cast if spell break it
if (!m_spellInfo->HasAttribute(SPELL_ATTR_EX3_NO_INITIAL_AGGRO) &&
!m_spellInfo->HasAttribute(SPELL_ATTR_EX_NO_THREAT) &&
!IsPositiveSpell(m_spellInfo->Id, realCaster, unit) &&
m_caster->isVisibleForOrDetect(unit, unit, false))
{
// use speedup check to avoid re-remove after above lines
if (m_spellInfo->HasAttribute(SPELL_ATTR_EX_NOT_BREAK_STEALTH))
Expand All @@ -1265,23 +1273,20 @@ void Spell::DoSpellHitOnUnit(Unit* unit, uint32 effectMask)

unit->AddThreat(realCaster);

if (!m_spellInfo->HasAttribute(SPELL_ATTR_STOP_ATTACK_TARGET))

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Oct 5, 2016

Member

@Warlockbugs this is needed, i recently introduced this to avoid creature start the combat before receiving possess aura. Possess still work but you the creature start chase a millisecond before aura apply. And we can see it in some case.

But now i face similar problem while iam working on charm aura. If you cast 13180 it fail because creature already in combat. Its probably due to this. I wonder now if all the code are in the good place, it maybe should not be here at all or placed after applying the aura.

This comment has been minimized.

Sign in to view

Copy link
@Warlockbugs

Warlockbugs Oct 6, 2016

Author Member

This also makes players being able to CC mobs without pulling them, this is wrong.

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Oct 6, 2016 •
edited

Member

You are right my mistake. But for ex. polymorph do same behavior. The mob start chasing before it was transformed. So there is something to do here. So i can say simply remove this check is also wrong.

This comment has been minimized.

Sign in to view

Copy link
@killerwife

killerwife Oct 6, 2016

Contributor

We are probably not setting new movement gen somewhere for some reason.

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Oct 6, 2016

Member

I prefer to think that we set the mob in combat in wrong place. Setting in combat should occur in spelleffect/aura itself or after all effects are applyed. (refer to my second exemple with charm)

This comment has been minimized.

Sign in to view

Copy link
@killerwife

killerwife Oct 6, 2016 •
edited

Contributor

recently i encountered a bug where we called Attack before AI->AttackStart when mob wasnt in combat for Creature, which caused it not to break waypoint movement on combat enter. Maybe similar issue?

This comment has been minimized.

Sign in to view

Copy link
@Warlockbugs

Warlockbugs Oct 6, 2016 •
edited

Author Member

@cyberium If you look closely at the structure of DoSpellHitOnUnit, you can notice that we put unit in combat before executing all spell effects on it. This is wrong. I was looking at the whole spell hit routine for some time for now, and i keep finding more and more issues with it. We need to remake spell hit to fix these issues and introduce major gameplay advancements such as proper binary spells support. If anyone else can join the process, maybe we can come up with solution faster. I am currently trying to fix some elusive combat log issues.

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Oct 6, 2016

Member

Hum my advice is dont try to fix Spell:: in one step. We all know from long time that there is problems with our implementation. This is one of the class that can have the most impact on gameplay in different way. We often fix some stuff and broke others whenever we try to change some lines of code here. So focus on each issue for now is an acceptable solution till we are able to rewrite the all those stuff to simplify, make it cleaner, more optimized and more correct.
So for now i'll just try to move that set in combat block in last line and check what happen.
{
if (!unit->isInCombat() && unit->GetTypeId() != TYPEID_PLAYER && ((Creature*)unit)->AI())
unit->AttackedBy(realCaster);
if (!unit->isInCombat() && unit->GetTypeId() != TYPEID_PLAYER && ((Creature*)unit)->AI())
unit->AttackedBy(realCaster);

unit->SetInCombatWith(realCaster);
realCaster->SetInCombatWith(unit);
unit->SetInCombatWith(realCaster);
realCaster->SetInCombatWith(unit);

if (Player* attackedPlayer = unit->GetCharmerOrOwnerPlayerOrPlayerItself())
realCaster->SetContestedPvP(attackedPlayer);
}
if (Player* attackedPlayer = unit->GetCharmerOrOwnerPlayerOrPlayerItself())
realCaster->SetContestedPvP(attackedPlayer);
}
}
else
{
// for delayed spells ignore negative spells (after duel end) for friendly targets
if (speed > 0.0f && !IsPositiveSpell(m_spellInfo->Id))
if (speed > 0.0f && !IsPositiveSpell(m_spellInfo->Id, realCaster, unit))
{
realCaster->SendSpellMiss(unit, m_spellInfo->Id, SPELL_MISS_EVADE);
ResetEffectDamageAndHeal();
Expand Down Expand Up @@ -3788,11 +3793,15 @@ void Spell::finish(bool ok)
{
SpellEntry const* auraSpellInfo = (*i)->GetSpellProto();
SpellEffectIndex auraSpellIdx = (*i)->GetEffIndex();
const uint32 procid = auraSpellInfo->EffectTriggerSpell[auraSpellIdx];
// Quick target mode check for procs and triggers (do not cast at friendly targets stuff against hostiles only)
if (IsPositiveSpellTargetMode(m_spellInfo, m_caster, unit) != IsPositiveSpellTargetMode(procid, m_caster, unit))
continue;
// Calculate chance at that moment (can be depend for example from combo points)
int32 auraBasePoints = (*i)->GetBasePoints();
int32 chance = m_caster->CalculateSpellDamage(unit, auraSpellInfo, auraSpellIdx, &auraBasePoints);
if (roll_chance_i(chance))
m_caster->CastSpell(unit, auraSpellInfo->EffectTriggerSpell[auraSpellIdx], true, nullptr, (*i));
m_caster->CastSpell(unit, procid, true, nullptr, (*i));
}
}
}
Expand Down Expand Up @@ -5143,7 +5152,7 @@ SpellCastResult Spell::CheckCast(bool strict)
if (!explicit_target_mode && m_caster->GetTypeId() == TYPEID_UNIT && m_caster->GetCharmerOrOwnerGuid())
{
// check correctness positive/negative cast target (pet cast real check and cheating check)
if (IsPositiveSpell(m_spellInfo->Id))
if (IsPositiveSpell(m_spellInfo->Id, m_caster, target))
{
if (!target_hostile_checked)
{
Expand All @@ -5168,7 +5177,7 @@ SpellCastResult Spell::CheckCast(bool strict)
}
}

if (IsPositiveSpell(m_spellInfo->Id))
if (IsPositiveSpell(m_spellInfo->Id, m_caster, target))
if (target->IsImmuneToSpell(m_spellInfo, target == m_caster) && !target->hasUnitState(UNIT_STAT_ISOLATED))
return SPELL_FAILED_TARGET_AURASTATE;

Expand Down Expand Up @@ -6244,7 +6253,7 @@ SpellCastResult Spell::CheckPetCast(Unit* target)
if (!_target->isTargetableForAttack())
return SPELL_FAILED_BAD_TARGETS; // guessed error

if (IsPositiveSpell(m_spellInfo->Id))
if (IsPositiveSpell(m_spellInfo->Id, m_caster, _target))
{
if (m_caster->IsHostileTo(_target))
return SPELL_FAILED_BAD_TARGETS;
Expand Down Expand Up @@ -7312,7 +7321,7 @@ bool Spell::CheckTarget(Unit* target, SpellEffectIndex eff)
if (((Player*)target)->GetVisibility() == VISIBILITY_OFF)
return false;

if (((Player*)target)->isGameMaster() && !IsPositiveSpell(m_spellInfo->Id))
if (((Player*)target)->isGameMaster() && !IsPositiveSpell(m_spellInfo->Id, m_caster, target))
return false;
}

Expand Down
10 changes: 5 additions & 5 deletions src/game/SpellAuras.cpp
View file
Expand Up @@ -387,7 +387,7 @@ Aura::Aura(SpellEntry const* spellproto, SpellEffectIndex eff, int32* currentBas

m_currentBasePoints = currentBasePoints ? *currentBasePoints : spellproto->CalculateSimpleValue(eff);

m_positive = IsPositiveEffect(spellproto, m_effIndex);
m_positive = IsPositiveAuraEffect(spellproto, m_effIndex, caster, target);
m_applyTime = time(nullptr);

int32 damage;
Expand Down Expand Up @@ -3992,7 +3992,7 @@ void Aura::HandleAuraTransform(bool apply, bool Real)
}

// update active transform spell only not set or not overwriting negative by positive case
if (!target->getTransForm() || !IsPositiveSpell(GetId()) || IsPositiveSpell(target->getTransForm()))
if (!target->getTransForm() || !IsPositiveSpell(GetId(), GetCaster(), target) || IsPositiveSpell(target->getTransForm(), GetCaster(), target))
target->setTransForm(GetId());

// polymorph case
Expand Down Expand Up @@ -4027,7 +4027,7 @@ void Aura::HandleAuraTransform(bool apply, bool Real)
for (Unit::AuraList::const_iterator i = otherTransforms.begin(); i != otherTransforms.end(); ++i)
{
// negative auras are preferred
if (!IsPositiveSpell((*i)->GetSpellProto()->Id))
if (!IsPositiveSpell((*i)->GetSpellProto()->Id, (*i)->GetCaster(), target))
{
handledAura = *i;
break;
Expand Down Expand Up @@ -5198,7 +5198,7 @@ void Aura::HandleAuraModSchoolImmunity(bool apply, bool Real)
// TODO: optimalize this cycle - use RemoveAurasWithInterruptFlags call or something else
if (Real && apply
&& GetSpellProto()->HasAttribute(SPELL_ATTR_EX_DISPEL_AURAS_ON_IMMUNITY)
&& IsPositiveSpell(GetId())) // Only positive immunity removes auras
&& IsPositiveSpell(GetId(), GetCaster(), target)) // Only positive immunity removes auras
{
uint32 school_mask = m_modifier.m_miscvalue;
Unit::SpellAuraHolderMap& Auras = target->GetSpellAuraHolderMap();
Expand Down Expand Up @@ -9454,7 +9454,7 @@ bool SpellAuraHolder::IsWeaponBuffCoexistableWith(SpellAuraHolder const* ref) co
(castItem->GetSlot() != EQUIPMENT_SLOT_MAINHAND && castItem->GetSlot() != EQUIPMENT_SLOT_OFFHAND))
return false;

// form different weapons
// from different weapons
return ref->GetCastItemGuid() && ref->GetCastItemGuid() != GetCastItemGuid();
}

Expand Down
4 changes: 2 additions & 2 deletions src/game/SpellEffects.cpp
View file
Expand Up @@ -11224,7 +11224,7 @@ void Spell::EffectCharge(SpellEffectIndex /*eff_idx*/)
m_caster->MonsterMoveWithSpeed(x, y, z, 24.f, true, true);

// not all charge effects used in negative spells
if (unitTarget != m_caster && !IsPositiveSpell(m_spellInfo->Id))
if (unitTarget != m_caster && !IsPositiveSpell(m_spellInfo->Id, m_caster, unitTarget))
m_caster->Attack(unitTarget, true);
}

Expand All @@ -11247,7 +11247,7 @@ void Spell::EffectCharge2(SpellEffectIndex /*eff_idx*/)
m_caster->MonsterMoveWithSpeed(x, y, z, 24.f, true, true);

// not all charge effects used in negative spells
if (unitTarget && unitTarget != m_caster && !IsPositiveSpell(m_spellInfo->Id))
if (unitTarget && unitTarget != m_caster && !IsPositiveSpell(m_spellInfo->Id, m_caster, unitTarget))
m_caster->Attack(unitTarget, true);
}

Expand Down
9 changes: 6 additions & 3 deletions src/game/SpellHandler.cpp
View file
Expand Up @@ -467,7 +467,12 @@ void WorldSession::HandleCancelAuraOpcode(WorldPacket& recvPacket)
if (IsPassiveSpell(spellInfo))
return;

if (!IsPositiveSpell(spellId))
SpellAuraHolder* holder = _player->GetSpellAuraHolder(spellId);

if (!holder)
return;

if (!IsPositiveSpell(spellId, holder->GetCaster(), _player))
{
// ignore for remote control state
if (!_player->IsSelfMover())
Expand Down Expand Up @@ -501,8 +506,6 @@ void WorldSession::HandleCancelAuraOpcode(WorldPacket& recvPacket)
return;
}

SpellAuraHolder* holder = _player->GetSpellAuraHolder(spellId);

// not own area auras can't be cancelled (note: maybe need to check for aura on holder and not general on spell)
if (holder && holder->GetCasterGuid() != _player->GetObjectGuid() && HasAreaAuraEffect(holder->GetSpellProto()))
return;
Expand Down

1 comment on commit 21f2f00

@xfurry
Copy link
Member

@xfurry xfurry commented on 21f2f00 Sep 27, 2016

Choose a reason for hiding this comment

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

👍

Looks very good and clean.
Can't wait to play around with this when I have the chance.

Please sign in to comment.