Shouldn't Spell::Prepare() return a SpellCastResult?

So while looking into why pet spells were not auto-casting properly it appears they are failing for two different reasons, one of them being SPELL_FAILED_NOT_READY.

This was because the PetAI was setting the cooldown before calling spell->prepare() which in turn calls CheckCast(). In the case of the AI, it looks like this:

[ol][li]Spell is added to the queue[/li] [li]Cooldown is activated by the AI using AddCreatureSpellCooldown()[/li] [li]spell->prepare() is called[/li] [li]spell->prepare calls CheckCast() which passes because it’s the first cast[/li] [li]PetAI::UpdateAI() is called[/li] [li]Spell is added to the queue[/li] [li]Cooldown is reset by the AI using AddCreatureSpellCooldown()[/li] [li]spell->prepare() is called[/li] [li]spell->prepare calls CheckCast() which fails because the cooldown hasn’t run out[/li] [li]Steps 1 to 9 are repeated until combat ends[/li][/ol]
Notice I made the distinction between activated and reset. This is because on the second pass of say a 40 second cooldown it may still have 20 seconds left. The AI however resets it to 40 before spell->prepare() checks if the cooldown is active and fails the cast.

Since spell->prepare doesn’t return a success / fail result, it is difficult to know if the cooldown should be activated. Calling CheckCast() directly from the AI seems redundant because it is called again from spell->prepare(). Also putting the call to AddCreatureSpellCooldown() after spell->prepare() is useless because you don’t know if it fails.

Right now I’m testing an intermediate patch to fix the AI because even a bandaid is better than nothing.

Suggested solutions:

[ul][li]Let Spell::prepare() return the SpellCastResult so the caller can activate the cooldown[/li]
[li]Let Spell::cast() activate the cooldown so the caller doesn’t have to worry about it and allow Spell::prepare() to return the SpellCastResult anyway so the caller can make decisions[/li]Note: Spell::SendSpellCooldown() is called from Spell::cast() but it only activates cooldowns for _player and doesn’t handle the pet.

I personally think this is where pet cooldowns should be handled as well and calls to AddCreatureSpellCooldown() should be removed from the AI and other places.

Well I think I’ve fixed the AI problem by handling pet cooldowns in Spell::SendSpellCooldown() so changing the return type of Spell::prepare() isn’t as important but it still is an interesting question.

Having it return a value would make coding more flexible in the future.