nos4r2zod

maybe such info should be stored on the server's side?

That's exactly what mangos does currently.

What bug does the patch fix? What features does the patch add?

This patch fixes a potential exploit with multicast bars (currently only totem bars use this).

Spell:EffectSummonAllTotems checks through the player's action buttons in order to find which totems to cast. This makes it a rare case where a mangos spell implementation is dependent on information on the client's UI.

This can currently be exploited by third party programs which can place spells on the totem bar that are not normally allowed by client. Doing so allows those spells (even passive abilities/talents) to be cast on self with Call of the Elements/Ancestors/Spirits instantly.

The patch prevents passive spells from being put on action bars in general, and checks that only totem spells are placed the totem bar using AttributesEx7 data.

For which repository revision was the patch created?

10755

Is there a thread in the bug report section or at lighthouse? If yes, please add a link to the thread.

Nope.

Who has been writing this patch? Please include either forum user names or email addresses.

Myself

Patch: http://paste2.org/p/1099765

Edit: Typo in defines fixed

Edit2: Update after 10693

GCD must be limited to between 1 second and 1.5 seconds, no matter what haste ratings/aura effects are applied. This can be tested from client-side only, because client calculated GCD independently from server based only on haste rating and auras.

Based on my understanding, GCD should be limited to between 1000ms and 1500ms:

With >>100% haste rating, I saw GCD only decrease to 1.0s, so for this part, patch as given is incorrect. Cap at bottom of 1.0s does not happen just for haste rating either: see comments at http://www.wowhead.com/spell=18288.

Haste debuffs (like http://www.wowhead.com/spell=72297) increase GCD, but not beyond 1.5s. When you receive this buff with 0 other haste mods, GCD remains 1.5s. When you decrease GCD to 1.0s via only haste rating at haste rating cap, then apply this debuff, GCD does increase again to 1.5s.

`event_param1` int(11) signed NOT NULL default '0',

so normal int ~ +-2000million

But even_t_hp store percents, not raw hp values

The problem exists not in the database table, but rather in how the way Mangos currently handles calculations involving HP/power, uint32 overflow is not protected against.

Here is the code that is causing Srage's bug:

case EVENT_T_HP:
       {
           if (!m_creature->isInCombat() || !m_creature->GetMaxHealth())
               return false;
           uint32 perc = (m_creature->GetHealth()*100) / m_creature->GetMaxHealth();
           if (perc > event.percent_range.percentMax || perc < event.percent_range.percentMin)
               return false;
           ....

For 25-man normal Halion's HP (43.5 million), GetHealth()*100 returns a value greater than 2^32, causing perc to be calculated as something much lower than expected.

(Another problem exists in that perc truncates this calculation, so that the range check passes when the percent is 75.99, rather than at 75.00 and below. For this reason I feel a ceil() is necessary here.)

From what I see, preferred style in code is to do Get[Health|Power]() * int / GetMax[Health|Power]() to avoid floating point arithmetic / casts from int to float and back, but I think this may be bad to do now, since correctness trumps efficiency.

For instance in health recalculation in Aura::HandleModTotalPercentStat, after receiving stamina % buff

    //recalculate current HP/MP after applying aura modifications (only for spells with 0x10 flag)
   if ((m_modifier.m_miscvalue == STAT_STAMINA) && (maxHPValue > 0) && (GetSpellProto()->Attributes & 0x10))
   if ((m_modifier.m_miscvalue == STAT_STAMINA) && (GetSpellProto()->Attributes & 0x10))
   {
       // newHP = (curHP / maxHP) * newMaxHP = (newMaxHP * curHP) / maxHP -> which is better because no int -> double -> int conversion is needed
       uint32 newHPValue = (target->GetMaxHealth() * curHPValue) / maxHPValue;
   ...

Comment may be true, but when target player has current hp >= 2^16 (65536 hp, which is reasonably possible), GetMaxHealth() * curHPValue returns ~0!

Other problem exists in lack of standard way for checking HP/power percent in spell code:

// Prey on the Weak
           if (spell->SpellIconID == 2983)
           {
               Unit *victim = target->getVictim();
               if (victim && (target->GetHealth() * 100 / target->GetMaxHealth() > victim->GetHealth() * 100 / victim->GetMaxHealth()))
   ....

// Decimation
else if (auraSpellInfo->Id == 63156 || auraSpellInfo->Id == 63158)
{
   // Looking for dummy effect
   Aura *aur = GetAura(auraSpellInfo->Id, EFFECT_INDEX_1);
   if (!aur)
       return SPELL_AURA_PROC_FAILED;

   // If target's health is not below equal certain value (35%) not proc
   if (int32(pVictim->GetHealth() * 100 / pVictim->GetMaxHealth()) > aur->GetModifier()->m_amount)
       return SPELL_AURA_PROC_FAILED;

Use of implicit uint32(GetHealth() * 100) allows rogues with Prey on the Weak to always gain 20% crit versus big raid bosses at high health or warlocks with Decimation to always gain that proc.

This is my attempted fix (rev. 10448).

(Personally, I think all instances of these calculations should be the same, despite the increased use of floating point and the cost of efficiency. But it is reasonable to say, for example, that only AI checks at %HP/power, health recalculation on gaining %stamina, and "pseudo" health aurastate checks for spells that could be used on bosses like Prey on the Weak or Decimation strictly need this change, since player vitals would never get close to the values that would cause overflow.)

Edit: Fixed typo as pointed out by Patman128 in post below, thanks!

I haven't had much time to work on my patches/keep them up-to-date, but iirc, I encountered some problems mainly with pet spells; they often share the same spell family mask, so using those as the main generic checks here wouldn't be ideal. (Check the ones with SpellFamilyFlags 0x10000000, SpellFamilyName = 9 for example)

I was thinking that SpellVisuals might be a better fall back method than SpellIcons, but I never finished checking. At the least, they might work for these pet spells, and they are the same for spells like Gift of the Wild and Mark of the Wild, Arcane Brilliance and Arcane Intellect, etc. where SpellIcons vary.

Anyway, I haven't looked in the latest client files, so there might be a better way to do these checks now.

the 5-stack effect on SoV got added on 3.2, so having it here is incorrect.

Actually, it was added in 2.2:

Seal of Vengeance duration increased to 15 seconds. In addition, when Seal of Vengeance strikes a target that already has 5 applications you will cause instant Holy damage.

But it just didn't do nearly as much damage; only around single digit damage.

You can see that 3.2 only bumped up the damage done by the triggered effect and nothing else.

Updated past 8428, which implemented the attack power bonus of Savage Defense; parts of this patch are still usable.

This is what remains of this patch after commits 8399-8401:

PATCH

Why offhand interpreted as _any_ weapon in offhand? Base at spell data this must be _real_ offhand weapon only. And i not see any references that this not like way.

This is the way it worked prior to patch to 3.0.8. And in 3.0.8 patch notes,

Lava Lash - Fixed a bug which allowed you to use the ability even if you had a shield in offhand. Now requires a 1hand axe, fist or dagger to be able to be used.

no mention that the weapon had to be specifically an off-hand weapon only item.

Updated to 8379, and added checks to prevent Food/Drinks/Well Fed buffs from stacking with each other.

Some explanation of these checks:

• * The Replenishment effects of food and drinks are handled in IsNoStackSpellDueToSpell. They are selected based on the AuraInterruptFlag AURA_INTERRUPT_FLAG_NOT_SEATED, which only food and drinks use. The spells are then compared to each other using IsSpellHaveAura, since there does not seem to be a better way to separate foods from drinks. I only found one food spell (48720) without this interrupt flag, but that can only be used underwater, where other foods shouldn't be usable anyway.
  * The Well Fed buffs of foods are handled using a spellspecific, since they were not all SPELLFAMILY_GENERIC. Some were also SPELLFAMILY_PRIEST (18125, 18141, 23697), and some were SPELLFAMILY_POTION (18191, 18192, 18193, 18194, 18222, 22730, 25661).
  * Of the SPELLFAMILY_GENERIC spells, only 46687 was not covered by SPELL_ATTR_EX2_FOOD_BUFF, so I added a SpellIcon check for that.
  

EDIT:

forget the spell_elixir flag for the Well Fed buffs that are SPELLFAMILY_POTION:

-- Well Fed (SPELLFAMILY_POTION)
DELETE FROM spell_elixir WHERE entry IN (18191, 18192, 18193, 18194, 18222, 22730, 25661);
INSERT INTO spell_elixir (entry, mask) VALUES
(18191, 16),
(18192, 16),
(18193, 16),
(18194, 16),
(18222, 16),
(22730, 16),
(25661, 16);

Meaning: must be moved to rejected as outdated?

Actually, I meant to say that it's not up to date and I haven't tested it since 8193, but I think the bugs covered still persist.

Edit: Updated for 8379. Maybe there's a better way to do

+    // Procs can miss, weapon enchants can miss, triggered spells and effects cannot miss (miss already calculated in triggering spell)
+    bool canMiss = (m_triggeredByAuraSpell || !m_IsTriggeredSpell);

though, it's too strict in that it requires scripted spells triggered from auras to NOT have a m_triggeredByAuraSpell for this spell to work (like Judgements as triggered from Seals, or Chimera Shot as triggered from stings).

Also, removed the cooldown hack in this patch, wasn't really related, and perhaps a better way to have done would have been in the AddSpellAndCategoryCooldowns, like this.

I guess I forgot to write a comment for Improved Concentration Aura in IsNoStackDueToSpell when I added it, so I also forgot the fact that it also needed an exception in INSDTS when the patch was changed to use SpellIcons.

As I said in the head of this topic, Aspect of the Dragonhawk won't work correctly without there being a way to not have a cooldown on the triggered spell. Relogging will give you the dodge bonus because when the triggered spell is cast on login, the player won't have a cooldown on the Aspects spell category...

The best way I could think of is to not add a cooldown on category spells that the player doesn't really have (except when the cast is based on an item) (This also works with freezing arrow)

http://paste2.org/p/375371

EDIT: Though the addition in 8358 prevents Concentration Aura from being deleted when the triggered spell is cast now, I think it's probably better to make a INSDTS exception consistent with the other auras than to use SetInUse, as long as Improved Concentration Aura was the only thing that was breaking.

It's because totem buffs use SPELL_ATTR_PASSIVE, so they don't call IsNoStackSpellDueToSpell at all. My apologies for not checking this more carefully. I would add a more generic check in IsRankSpellDueToSpell in that patch, but that'd require a bit more restructuring, because IsHighRankOfSpell assumes that both spells have spell_chain data after IsRankSpellDueToSpell returns true. So the only alternative for now is just to add the spell_chain data, I think.

Since you are here, do you have any ideas on how I would keep Curse of Tongues from applying on bosses?

I'm not certain about this. Most debuffs are checked in the ImmuneMechanicMask, but I see CoT doesn't have a mechanic.

On a whim, I think it might have something to do with SPELL_ATTR_EX4_UNK29. It looks like that is shared with many debuffs, with or without mechanics, that should be "partly" effective against bosses. In addition to CoT, Frostbrand Attack has it (slow not always effective, but Aura applied anyway for Frozen Power talent) and Disarm has it (disarm not always effective, but damage increase (for when the warrior has Improved Disarm Talent) should be applied).

Edit: nvm, don't think it'll work; rank 9 Frostbrand Attack doesn't have it (unless it's related to the SP bug that Frostbrand had in 3.1.x?)

If you want to know what creatures are considered raid bosses, it's in the Rank column of creature_template (I think it's set to 4).

Also, a friend of mine suggests to use '-1' as coefficient for Conflagrate (17962), Vampiric Touch Dispel proc (64085), Seal of Martyr self and enemy procs (53718, 53719), Seal of Blood (31893, 32221) and Swiftmend (18562). This is to avoid them to get ANY bonus damage.

A coefficient of zero is the correct way to show no bonus from coefficients, at least for now. Having a negative coefficient will result in the spell's damage decreasing as a result of spell power, or having the spell treated as a miss, or in some cases, having the spell's damage wrap around and causing damage orders of magnitude too high.

i look like this have some problems

in bear and cat form works fine, but i moonkin form does not give the aditional spell power that it must

didn't tryied in tree of life form

It doesn't increase spell power, it just increases the damage you do. You won't see it on your character tab.

http://www.wowhead.com/?spell=48412#comments:id=662334

OT: Maybe we should use the level of the triggering aura but not level of the spell itself. Would just require to give the pointer of the triggering aura to SpellBonusDamage() as argument...

OT: It won't work with judgements, no aura involved; but I supposed you could change the script effect to treat the seal as the proccing aura.

With this hack this patch has no chances to get into the official repo

Leav it as it was. This was a correct fix for this dummy proc. The issue with spell damage bonus is a completely other thing.

I agree.

maybe adding the correct coeff to base dmg and ignoring the one from db?

That would work, but it would be hacky and might be unnecessary when you consider that Earth Shield, Prayer of Mending, Paladin Judgements, etc. would also need the same fix.

Edit: I mean this patch is fine as is once you add the coefficient, and the downranking problem isn't an issue completely related to this patch. But back when the procs actually did the damage, there wasn't this problem with spellLevels/downranking (currently, they seem to just supply client display data with their unused dummy effect).

Edit2: No, I meant I personally liked it as is, without the hardcoded coefficient

Mangos coding style uses 4 spaces instead of tabs, because tabs vary in length across Git/IDEs/paste2/etc.

Your patch works alright, but SQL changes are necessary as well:

DELETE FROM `spell_bonus_data` WHERE `entry` IN (8026, 10444);

INSERT INTO `spell_bonus_data` VALUES
(10444, 0.1, 0, 0, "Shaman - Flametongue Weapon Proc");

Now this brings to light some more generic fixes that need to be made for Flametongue Weapon before it deals the correct amount of damage. The spell that is procced has a spellLevel of 1, so the coefficient is subject to downranking, and for a level 80, it'd only get ~0.007 * SP instead of 0.1 * SP.

Why need this part? triggered effects as i look not have 47 category...

So that false is returned when a presence is compared with anything at all. It's not explicitly comparing a presence with it's triggered spell, it's comparing a presence with any spell.

Edit: I think I misunderstood, but last I checked, this function was being called and the spells not stacking without this check.

Edit2: Ok sorry, you are right, this part is not needed

Also, in _RemoveAura(), I think the

if(IsSpellLastAuraEffect(GetSpellProto(),GetEffIndex()))

is not necessary, because it is within the "if (lastaura)" code block.

Edit: Also, I notice that in some cases, auras are not being removed properly.

Edit2: Specifically, Frost Presence's 10% hp bonus isn't being removed; not sure if there are others.

Fingers of frost affects spells that have AURA_STATE_FROZEN in their TargetAuraState in Spell.dbc.

Only Deep Freeze had this. Shatter and Ice Lance had no aurastate, so I'm not quite sure I follow what you're saying.

bug - after logout talent (bug is present in other talents) does not work

It was just a hack fix to make a point...

1. It would be terribly hacky to do so becuse of the direct calls to HasAuraState on the target, which still can be solved but this cannot be the general solution. Also note that this aura is used to override combat requirement too, and simply overriding aurastate checks would never solve that.

Well as Lynx3d pointed out, first we have to add the server-side check to prevent the casting of non-combat spells in combat I am also not sure if fingers of frost should have an aurastate.

2. Shadowdance uses 275 afaik that's ignore form aura.

That seems to be the case.

but won't a "let cast it in combat if they have talent" patch work?

That's what Lightguard did. The skills are grayed out and disabled on the client side. You can't even begin to cast them unless the client knows that Juggernaut/Warbringer's auras are in effect.

Edit: See this hack-fix to see what I mean ->Hack<- (not thoroughly tested).

No, you don't really need to assign Juggernaut and Warbringer different slots since you can't learn them both at the same time, but I did so for testing purposes.

Also, _RemoveAura needs to be edited so that auras above MAX_AURAS can be removed. This is only assuming that Warbringer/Juggernaut have special aura slots. If not, ignore this.

Actually, it's not a sniff, it was trial + error, so it probably will not be 100% correct.

I'm only working with the client-side problem of implementing this aura in Juggernaut and Warbringer. With Juggernaut and Warbringer, the spells affected remain greyed out in combat / with the wrong stances.

The other spells work fine on the client side since they are visible auras (Taste for Blood and Sudden Death).

Unfortunately, I think the way to implement Fingers of Frost will still be through a hack in Shatter/Ice Lance's custom script code.

Edit: @Pelle: patience, please... Sudden Death works with Lightguard's patch, I already stated Juggernaut is the same issue as Warbringer, Shadow Dance doesn't even use 262.

http://www.wowhead.com/?spell=57499 - Warbringer not worked but needs aura 275 (NYI)

ofc the side effects have to be implemented somewhere else. (Probably a new function.??)

I have wracked my head with this one for a very long time and I finally realize why Warbringer doesn't allow the client to use Charge in combat. It's because, as a passive, invisible aura, it is never sent to the client. So the client never does anything to enable spells affected by Warbringer, Juggernaut, etc. outside their bypassed restraints.

I think one logical way to solve this problem would be to, in the handler of 262, custom send this packet to the client. You can confirm this works by .debug send opcode with the following parameters in opcode.txt:

1174             uint 16  xxxxx             uint8 255             uint32 57499    uint8 19        uint8 80   uint8 1 uint8 0
SMSG_AURA_UPDATE whatever GetPackGUID() is  max allowed auraslot  Warbringer ID   see AFLAG enum  AuraLevel  Charges

What this opcode does is send the client the invisible Warbringer aura (you can also try this with Juggernaut), and allows the client to use charge in combat / use intercept and intervene in combat.

Edit: Also, in the patch you wrote, you could have just used (*i)->IsAffectedOn(m_spellInfo)

Great, now we also know why the blooming part doesn't crit! Here's a code snippet that should provide a hint.

Can you tell me how you've obtained the information about 33778's damage class? In other words where damage classes for spells are stored?

Wow, good observation!

You can look in the \\contrib\\dbcEditer\\bin\\ folder for something to view Spell.dbc with. Just look for the DmgClass column.