Pet Patch 1.0: Amazing fun times with pets

[Phatcat]

Hunter's pets

• Checked the attack speed for all tamable beasts according to this and corrected them. I of course did not touch those that already had the correct value according to the list.
• Removed data from pet_levelstats that will be irrellevent with pet patch 1.0 once that gets merged.

---

This change is

[cala]

Thank you for this pull request. I had a few comments after looking quickly at it, but I see you fixed most of them recently (the removal of pets stats for Silithid Swarm as an example). The only one that still comes to mind is setting to 0 the melee damage values for a bunch of NPCs: I'm not sure this is right and that core will use 0 values. Chances are good that they will be changed to 1. If you are aiming to create non-melee NPCs, it would be wiser to use ACID for that, I think.

I will review very soon that PR more in details. Thanks again. :+1:

[cala]

Thank you for the recent updates. :+1:

I agree with the attack speed updates. I have to look, but I may have more to add. :smile:

However, I disagree with the removal of any pet_level_stats. The stats exists for those NPCs because there is at least one spell that summons those NPCs as pet and not as guardian. A good example would be the various Silithid NPCs with the little swarmers. They use spell 10722 at spawn which summons a Silithid Swarm (4196) with SPELL_EFFECT_SUMMON_PET. When engaged in combat, they periodically cast spell 6589 which also summons a Silithid Swarm but with SPELL_EFFECT_SUMMON_GUARDIAN.

As for the damage stats set to zero, I think this is probably a hack and the proper way to do it would be to use EventAI to prevent the NPCs to move/engage in melee combat. This reminds me of your question on IRC: if a creature_template has an empty field for AIName, this means that there is no entry for it in creature_ai_scripts table and thus they need to be written/added/backported. A list of the NPCs currently missing an AI is found in this issue: #740 We can discuss this later on IRC if you want. :smile:

[Phatcat]

The removal of pet_levelstats is very much done because the function spell_effect_summon_pet doesn't use pet_levelstats, it uses initstatsforlevel - initstatsforlevel uses pet_levelstats, and I am rewriting that one in this pr - which is why I said those changes shouldn't be applied till (if) that pr gets merged. In the same pr I also rewrote the summon_pet function to make summons default to guardians; It's done here

Attack speed updates are still WIP as doing the 2.0s seem like a nightmare given my current method of ctrl+f , so I skipped those initially. Have returned and done 1/10 locally - will push when completely done with them, then you can double check =)

I agree that setting damage to 0 for the npcs would not solves the issues with them, however the reason for that change is that they should not even be able to melee, so why would we keep some random values someone once gave them? - is there any reason they should not be defaulted to 0? as that is the default for entries not having values in those fields, if I am not mistaken?

I have not taken a look at any AI yet because the PetAI needs a bit of work (defensive is not good), and I am not sure petAI nor eventAI is suited for guardian pets? Given that nearly all guardian pets are supposed to do their own thing (eventAI), while also following and defending their owner (petAI).

[Phatcat]

@cala I would love to get in touch with you, hit me up on IRC any time.

[Phatcat]

@cala I have corrected this to not include any flags or 0 values for guardians, but only include the things relevant to pet patch 1.0 for the core (and just a few guardian corrections). That way these fit nicely together as a collected update.

[cala]

Thanks. I assume that it is best to use also that PR to test your patch core-side?

[Phatcat]

Well, if you want to be sure that I pulled the pet_levelstats out without breaking anything, then you should, but the core patch is not in any way dependant on the db patch, only the removal of pet_levelstats in the db patch is dependant on the core patch, that's all.

[Phatcat]

@cala here's the table =)

edit: removed everything relating to guardians (still got it locally) - will do a pr on some proper guardian updates once I get the ball rolling on Pet Patch 2.0 - that will however at least be after Engineering 2.0: The Tesla Tragedy (Gnomes Ahoy)

[Phatcat]

@cala - someone just came in and schooled me:

What the hell is this... Pet modifiers are spells, called Tamed Pet Passive (DND). For example 17206 gives 7% bonus damage to bats, 17208 gives -9% damage, 5% armor and 8% HP to bears. This kind of handling is wrong.

https://github.com/cmangos/issues/issues/931

Table and stat modifiers now removed.

[cala]

Yes, I saw the notification on core. I did not know or I would have lead you that way, of course. In case he does not provide you the spell IDs, I can try to look for them.

[Phatcat]

I hope I won't cause too many issues for people now that I dropped those 2 commits... I have added the comment to pet issues, so I can work it into pet patch 2.0

[cala]

Thanks, I merged the attack speed part now the core supports this. :+1: I see you removed the small update file that was fixing stats for one NPC (I don't remember which one) and the experience multiplier for another one. Is this intended?

Regarding the pet_levelstats table, I'm still reluctant to drop those entries:

• they are not loaded in core at startup so won't decrease performance if kept
• the whole table is quite small (55 kB) so it's not taking too much space
• the values seems to be good so they may be valuable later to test/define/check an improved stats system (it is hard enough to get reliable data for the DB, I prefer avoid dropping them needlessly)

[Phatcat]

@cala I'm sorry, but what do you mean that the values are good? the values are arrived at using a formula. Just give me that formula and I'll use that for guardians... Besides that, why would you want to hold on to data that should not, and probably will not, ever be used for anything ever again? I'm not saying throw them out completely, but remove them from the current db.

You say dropping them needless, I say keeping them needlessly.

[Phatcat]

Oh, forgot the other part of your message.

I see you removed the small update file that was fixing stats for one NPC (I don't remember which one) and the experience multiplier for another one. Is this intended?

Yes. Because they are intended to make a dreaded comeback along with all the other guardian changes when I get all those AI kinks worked out..

[Phatcat]

I'm gonna remove the beast attack speeds from this pr since you've already added those. Then we are down to the question, should this pr get closed or not then? - I doubt we'll ever come to terms any time soon about the removal of that data, so this pr could just end up going stale if left open.

[cala]

@cala I'm sorry, but what do you mean that the values are good? the values are arrived at using a formula. Just give me that formula and I'll use that for guardians... Besides that, why would you want to hold on to data that should not, and probably will not, ever be used for anything ever again?

I said that they seem good. We can discuss the reason of the emphasis on that verb on IRC. We do not have a formula or of course we would not need them, but in the eventuality we get a formula one day, I think these data could be a good set for testing. You are right we do not need to keep them in the DB for that specific purpose. I don't have the time to take a decision about that right now. I'll come back to you on this later. :)

[Phatcat]

We do not have a formula

Is why I made one; let's see how well it fits =)

Okay, leaving this pr open just for good meassure, then. Gonna do the exact opposite instead.

[cala]

Okay, leaving this pr open just for good meassure, then. Gonna do the exact opposite instead.

I lol'd. :laughing: