Can you run the bash script here https://github.com/azerothcore/azerothcore-wotlk/tree/master/apps/git_tools ? it's to help with writing commits

And thank you for the PR (you said you would open it and you did, a man of his word!)

@lineagedr another review ? please :P

@BarbzYHOOL

The code is the same. Maybe worse than before because now it's unorganized. It's still using dozens of spell ids instead of their mechanic.

@BarbzYHOOL
The code is the same. Maybe worse than before because now it's unorganized. It's still using dozens of spell ids instead of their mechanic.

@lineagedr
I agreed with your previous comments, but this one is absurd.
It seems you don't know how switch statement and spells works.

As I said, _spell->Mechanic is the 4th column of Spell.dbc (https://wowdev.wiki/DB/Spell) and in a lot of cases it's not filled up with a mechanic even if the spell applies one.
So, the best way to check if the spell has a cc mechanic is to use HasAura like I'm doing in the default case.

// src/server/game/Spells/SpellInfo.cpp     L. 867
bool SpellInfo::HasAura(AuraType aura) const
{
    for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
        if (Effects[i].IsAura(aura))
            return true;
    return false;
}

Switch's have the same access time on all elements, so it's better to use it rather than a loop + if / else chaining like HasAura does.
We can also remove the default case if we don't want the CCDelay applied on creature's spells as it's mainly configured for the class spells (for PvP).

Moreover, saying "this code is worse" without giving details or solution is completely useless. So please, next time you want to correct someone, make yourself more useful and show him how to do :)

Checking by ids is really the way to go here in most cases.

Is this affecting spells casted by creatures? Can this break boss encounters?

Just a small question, is this affecting spells casted by creatures? Can this break boss encounters?

@GitVerge
It does affect spell casted by creatures only if they applies a cc aura like stuns, fear etc. And nope, it'll not break boss encounter, it's just going to add an extra time between when the effect is cast and when it reaches its target.
If you don't want to apply the delay on creature's spells, you can comment/remove the default case

@GitVerge
It does affect spell casted by creatures only if they applies a cc aura like stuns, fear etc. And nope, it'll not break boss encounter, it's just going to add an extra time between when the effect is cast and when it reaches its target.
If you don't want to apply the delay on creature's spells, you can comment/remove the default case

Yup I'll do that, thanks.

@Shard-MW I've already showed you how to do it. And I've said that for the spells that have no mechanic you can use their Ids:

Look at your code:
And you're telling me that this:

case 339: // Druid - Entangling roots
case 1062:
case 5195:
case 5196:
case 9852:
case 9853:
case 26989:
case 53308:

Is better than this lol:

if (_spell->Mechanic == MECHANIC_ROOT)

I've rewritten your code: https://pastebin.com/9p5MkDZt

You can see the difference.

@lineagedr
Since a switch statement doesn't check for each value, it's better than having a lot of if.

And nope, I'm saying that, this :

case 339: // Druid - Entangling roots
case 1062:
case 5195:
case 5196:
case 9852:
case 9853:
case 26989:
case 53308:

is better than this :

case SPELLFAMILY_DRUID:
        if (_spell->Mechanic == MECHANIC_BANISH)        // Cyclone
            return delayForBanishes;
        if (_spell->Mechanic == MECHANIC_ROOT)          // Entangling Root
            return delayForRoots;
        if (_spell->Mechanic == MECHANIC_STUN)          // Bash, Maim, Feral charge - Bear
            return delayForStuns;
        if (_spell->Mechanic == MECHANIC_SLEEP)         // Hibernate
            return delayForDisorients;
        if (_spell->SpellIconID == 495)                 // Pounce
            return delayForStuns;
        break;

because you'll have to check the familyName + check 5 if for Pounce for example.
Read pls https://www.geeksforgeeks.org/switch-vs-else/

And if you truly want to use CCDelay based on mechanics only,

for (uint8 i = 0; i < CCDArraySize; ++i)
     if (_spell->HasAura(auraWithCCD[i]))
          return auraCCDelay[i];

is enough to produce the same result as your entire (re)code.

@lineagedr
Since a switch statement doesn't check for each value, it's better than having a lot of if.

And nope, I'm saying that, this :

case 339: // Druid - Entangling roots
case 1062:
case 5195:
case 5196:
case 9852:
case 9853:
case 26989:
case 53308:

is better than this :

case SPELLFAMILY_DRUID:
        if (_spell->Mechanic == MECHANIC_BANISH)        // Cyclone
            return delayForBanishes;
        if (_spell->Mechanic == MECHANIC_ROOT)          // Entangling Root
            return delayForRoots;
        if (_spell->Mechanic == MECHANIC_STUN)          // Bash, Maim, Feral charge - Bear
            return delayForStuns;
        if (_spell->Mechanic == MECHANIC_SLEEP)         // Hibernate
            return delayForDisorients;
        if (_spell->SpellIconID == 495)                 // Pounce
            return delayForStuns;
        break;

because you'll have to check the familyName + check 5 if for Pounce for example.
Read pls https://www.geeksforgeeks.org/switch-vs-else/

And if you truly want to use CCDelay based on mechanics only,

for (uint8 i = 0; i < CCDArraySize; ++i)
     if (_spell->HasAura(auraWithCCD[i]))
          return auraCCDelay[i];

is enough to produce the same result as your entire (re)code.

No, this:

for (uint8 i = 0; i < CCDArraySize; ++i)
     if (_spell->HasAura(auraWithCCD[i]))
          return auraCCDelay[i];

Is for ALL spells with certain mods: Including creature spells.
Where the recode is for player spells and then for other spells

@lineagedr
Other spells are including creature spells, and you're doing it here...

// Other
for (uint8 i = 0; i < (sizeof(auraWithCCD) / sizeof(*auraWithCCD)); ++i)
     if (_spell->HasAura(auraWithCCD[i]))
          return delayForInstantSpells;   

And what's the point of using switch + conditions if you're doing the same treatment on player's and other's spells? T_T

@Shard-MW How is it doing the same treatment? Above the loop the player spells have custom delay. If every player spell was returning the delayForInstantSpells then it would be the same treatment.

Anyway here's a version with switches: https://pastebin.com/TYGLj9eu:

I do understand the way that @Shard-MW has done this, as some spell don't contain mechanics, but i have implented my own version of this into Oregoncore

talamortis/OregonCore@0b074fc

This way i have done it, reads the mechanics of the spell by using a loop for each spell to check the max effects and then returns the value.

I will go through this code when i have more time for a better review (most likely Friday) and can leave some feedback

make a case for each spell - good choice(no). for now best option at @talamortis oregon
btw you can also use spell ATTRS for this ;)
@lineagedr https://github.com/Expecto/Patchs/blob/master/Wotlk(TrinityCore)/CCDelay.diff this is old fucking hack and that hack break some spells, @Shard-MW do the shit more dirty but more right

@Viste If I may ask, which spells does it break?

make a case for each spell - good choice(no). for now best option at @talamortis oregon
btw you can also use spell ATTRS for this ;)
@lineagedr https://github.com/Expecto/Patchs/blob/master/Wotlk(TrinityCore)/CCDelay.diff this is old fucking hack and that hack break some spells, @Shard-MW do the shit more dirty but more right

also are you saying my way is incorrect?

make a case for each spell - good choice(no). for now best option at @talamortis oregon
btw you can also use spell ATTRS for this ;)
@lineagedr https://github.com/Expecto/Patchs/blob/master/Wotlk(TrinityCore)/CCDelay.diff this is old fucking hack and that hack break some spells, @Shard-MW do the shit more dirty but more right

also are you saying my way is incorrect?

no you way is best for now

@Viste
Actually, @talamortis 's code is my default case :

I'm using the first switch to :

• Don't break non-wanted spells like Priest's [Mind Control] or Hunter traps...
• List all player's spells and allow people to easily config their CCDelay
• Maybe add a config later to apply CCDelay or not, on creature's spells (default case)
• Because a switch is faster than multiple loops + if/else statements (even if it's micro optimization) since it have the same access time to all his elements

@Yehonal yes correct, it is a queue of actions. The weird thing is that because it impacts spells but not gameobject movement, that will generate some behaviours the classic players are familiar with but might confuse people. E.g. a spell evaluation happening before cast will happen at time X, but actually spell is cast at X + 400 where the player conditions changed - but still the spell will land.
Checks that are done AFTER target hit will instead be correctly based on when the action happens.

In WOTLK anyway, this "diff" time is not 400 MS. after a lot of research on videos by a group of tester in my old project, we came up with 150 to 200 ms as the correct number.

If you create a module we could directly add it inside the AC project as "officially supported module" and it can be enabled by default (but disabled on demand too) You could adapt this PR eventually. Module would be better instead of directly inject the code in core. We should do same thing also for features like ahbot and such things. So we could start from it.

after i've said it, why you closed it?

i don't understand why it's so difficult to move code in modules for you...we've such amazing possibility to keep the core clean and fixed (with module workarounds) at the same time...meh

i feel like it's a git accident but not sure yet

@Yehonal Keep calm, it's just an accident. I'll reopen it and all my others PR. I've just deleted some of my branch by mistake...

Any news about the module? :)

I used to have about a month and it feels pretty good now what state it is

what mikk said looks to me like it was an official feature as I expected, and not really "custom"

however if it's considered custom, @Shard-MW could make it into a module, but please answer something

I'll do it, I just don't have time yet
But, about :

This must be moved under a module that can be used as a workaround until the queue will be implemented

I don't think it's a good idea to implement the 400ms batch cuz players will feel the game more slower (and that's why it has been deleted in MoP? retail feels more dynamic now)

Any update?

Here are some interesting sources:

https://github.com/magey/classic-warrior/wiki/Spell-batching

Evolvee/Spell-Delay-Research#1

Blizzard forum:

Any updates on this?

I really interested in this fix. As result of this thread, this behavior is indeed correct aligned with the official (even if an emulation).

This behavior really makes difference on PvP experience.

I can volunteer to re-open the PR and work on improvements in order to merge this on mainline. Just I would need help/directives from maintainers

Thank you