simulationcraft / simc

Simulationcraft engine/GUI
GNU General Public License v3.0
1.38k stars 693 forks source link

Add Option to reset cooldowns for pet spawners #6186

Closed Hinalover closed 2 years ago

Hinalover commented 2 years ago

Is your feature request related to a problem? Please describe.

Monk Fallen Order pet spawners spawn several of the same pets back-to-back and the cooldown of spells do not get reset upon despawn. So I have to manually force the cooldowns of spells to certain amounts so that they better align with how many casts are done in game.

However due to the fact that enemy summons also use pet spawners, this would need to be a boolean option.

Describe the solution you'd like

Because this would be a boolean option, defaulting the option to false would keep the current implementation intact. However if the option is true, it would reset all spell cooldowns for the pet.

scamille commented 2 years ago

Why would resetting the cooldowns for all pets on dismiss be a problem for enemy pets? I think it might be a good thing to have clean cooldown state when a pet gets dismissed in general.

So I have to manually force the cooldowns of spells to certain amounts so that they better align with how many casts are done in game.

Does that mean you already have some code in place for this? Can you add a link?

scamille commented 2 years ago

Or I guess to put it more generally: Currently we do not reset cooldowns on pet::dismiss or player::demise, as far as I can tell. That doesn't really impact players since they usually survive throughout the whole fight, but it might affect all pets, including summonable main pets as well as the triggered ones which usually use a pet spawner implementation nowadays.

Of course pet spawner implementations usually reuse the pet objects, which is one of the advantages of using it for triggered pets.

But to begin with, an answer to the following question would be interesting: If you use a long (eg. 30s) cooldown on a main pet, dismiss and summon it again, is the cooldown reset or not?

If the cooldown is reset, we could just reset cooldowns in general on pet demise, otherwise a configurable flag by default for pets that are reused would make more sense.

Hinalover commented 2 years ago

Does that mean you already have some code in place for this? Can you add a link?

Ok....I'll use Fallen Monk's Fist of Fury as an example:

https://www.wowhead.com/spell=330898/fists-of-fury

Here it shows that the spell has a 24 second cooldown. When a Monk summons their Fallen Order; 4 Fallen Order lasting 8 seconds each of the Summoned Spec get staggered summoned and (on live) 4 staggered random between the other two specs for 6 seconds (PTR standardize it as 2 of each). So if a Windwalker Monk summons Fallen Order, 4 Windwalker Fallen Monks get summoned, and 4 are randomly selected from Brewmaster and Mistweaver Fallen Order.

Each Windwalker Fallen Order Monk casts Fists of Fury (linked above) then 2 casts of Tiger Palm, and that is it (there is some slight changes on PTR but for this example, it's not necessary to know). As it stands, SimC would only cast 1 Fists of Fury from the very first Fallen Order Monk and not from any of the other 3 Windwalker Fallen Order Monks. since the cooldown would not allow the other 3 monks to cast Fists of Fury. So I manually lowered the cooldown to the maximum duration of them being up (8 seconds) as a work around.

https://github.com/simulationcraft/simc/blob/shadowlands/engine/class_modules/monk/sc_monk_pets.cpp#L1648

However this does cause an issue with the Venthyr Legendary that summons a spec specific Fallen order for the ENTIRE duration of the spell (24 seconds). Meaning I would have to adjust the cooldown even further so that a 5th Fists of Fury is cast in the 24 second cooldown.

Hinalover commented 2 years ago

But to begin with, an answer to the following question would be interesting: If you use a long (eg. 30s) cooldown on a main pet, dismiss and summon it again, is the cooldown reset or not?

As it currently stands, the cooldown is NOT reset.

scamille commented 2 years ago

But to begin with, an answer to the following question would be interesting: If you use a long (eg. 30s) cooldown on a main pet, dismiss and summon it again, is the cooldown reset or not?

As it currently stands, the cooldown is NOT reset.

I meant ingame. If that is the case, this would differ from what you describe for fallen order.

I understand how you want a clean cooldown reset on pet demise for fallen order - I am mainly trying to determine if this should not be a general requirement for all pet demises, which would then also simplify the implementation.

Hinalover commented 2 years ago

I meant ingame. If that is the case, this would differ from what you describe for fallen order.

I was referring to in SimC. But in terms of in game, then they do reset on demise.

Each pet in game has their own spell cooldown for the same spell. Windwalker Fallen Order A gets summoned So Fallen Order A casts Fists of Fury at zero second. Putting that specific spell on cooldown for 24 seconds Windwalker Fallen Order A despawns after 8 seconds Windwalker Fallen Order B gets summons shortly after Windwalker Fallen Order B casts Fists of Fury B's Fists of Fury spell goes on cooldown for 24 seconds Windwalker Fallen Order B despawns after 8 seconds etc.

Hinalover commented 2 years ago

I have been told that enemy npc targets for tank sims that require being attacked do need a standard attack pattern and that having cooldowns reset may screw things up with those attack patterns.

scamille commented 2 years ago

I am not really up to date with all the enemy stuff, but I don't think tank related quircks would really impact this. The tank enemies are normal players anyway, not pets. What could be relevant is add raid events, but I think those are not allowed to overlap anyway to keep things simple. Who has told you that it might be problematic for enemies?

Hinalover commented 2 years ago

Had to go through my Discord transcript but Kojiyama said (on August 9th, 2021 in the SimC discord):

I'm pretty sure this also affects damage taken for tanks with raid events Since the abilities cooldowns are likely carried across instances Raid events are pet spawners Since we don't clear any cooldowns on demise, any reused actor will probably be slightly borked other than static pets like Hunter/Elemental

scamille commented 2 years ago

Well that sounds like an argument for properly resetting cool down in all cases.

A pet spawner which has a longer cooldown than the respawn rate of the pet, and not resetting the cooldown is fishy and probably not what anyone intended.

Anyway, I'll see what I can do, create a PR and we will see the results.

Dorovon commented 2 years ago

While I don't know of many cases where it comes up, it is the case that permanent pets will not reset their cooldowns if you die and resummon them. For example, as a Frost Mage if I use the Water Elemental's Freeze and then resummon it, the cooldown will be the same as if I did not resummon it.

Back when there was a discussion about this, I think doing some sort of reset for pets when dynamic = true either on demise or on arise seemed like the best solution. If I remember correctly, this only really comes up with pet spawns and raid events (which could potentially be refactored to also use pet spawners, but currently have a custom implementation).