Refactor PetAI

[deiteris]

🍰 Pullrequest

Refactors PetAI logic to be more concise and possibly a bit more optimized by reducing number of checks:

• Remove PetAI::UpdateAllies() and timer and iterate over the owner's group directly (incl. owner and self check).
• Remove unnecessary duplicate check for disabled actions in PetAI::UpdateAI()
• Convert and store Creature pointer as a pet in PetAI to simplify checks.
• Create Pet::HasActionsDisabled() and refactor checks for disabled pet mode
• Refactor spell pick and cast into separate PetAI::Cast() and PetAI::PickSpellWithTarget() functions.
• Refactor autocast spell picking logic and order.
• Use a single spell-target pair instead of a vector storage and random choice.
• Reworks logic in a way some automated spell casts are meaningful (i.e., harmful AoE spells are cast in melee range, AoE buffs are cast only in combat).
• Adjusts logic in a way any other actions won't interfere with scripted actions.

Additionally, introduces conditional spell casts based on the info I managed to gather:

• Spell lock is automatically casted only when the target is casting a non-melee spell.
• Dive and Dash are only casted when enemy is not within the melee range. (post)
• Shell Shield is only casted when the pet's HP falls below 50%. (post)

Proof

• None

Issues

• None

How2Test

• None

Regression test checklist

I've tried to cover all scenarios that are affected by this refactor, but I can check more if something is missing.

• Pet autocasts combat spells and buffs in combat.
• Pet autocasts only non-combat spells outside combat.
• Pet buffs party members.
• Pet automatically attacks an enemy in line of sight in aggressive mode.
• Pet does not automatically attack a CCed target in aggressive mode.
• Pet does not interrupt for attack when retreating in aggressive mode.
• Pet returns to follow state after combat.
• Pet returns to its stay position after combat.
• Pet stops attacking CCed target and returns back to owner.
• Attack order overrides restriction to attack CCed target.

[killerwife]

Did you make sure a pet breaks attack off when a target was cced after attack order was issues? Because that currently works.

[deiteris]

Aha! So that additional check for victim was actually to support this behavior. I noticed that pet stops attacking polymorphed creature, but then ignores attack order.

Though I see that pet still attacks feared target, even though this a CC that we might not want to break. Is this expected? 🤔

[insunaa]

It's common for warlocks to fear-juggle enemies. If their pets would stop attacking with every fear that'd be a significant DPS loss, so I'm pretty sure the only CCs that should stop pet attacks are those that break with any damage, such as sap, sheep or gouge
There are probably still a few other issues that need solving with pets. For example they don't always cast sprint on attack order, even if it's off cd, but then occasionally in melee combat they cast sprint.

[killerwife]

The pet needs to ignore damage breakable cc that occurs after attack order. You can always tell pet to attack after the fact and it needs to comply.

[killerwife]

Lookup the commits adding it to see the rationale using git blame.

[deiteris]

The pet needs to ignore damage breakable cc that occurs after attack order. You can always tell pet to attack after the fact and it needs to comply.

Yes, I understand and I will fix this. I'm trying to identify the culprit of this issue since I see it occasionally happens even with the current code (and the AI logic does not seem to handle this in any way, it seems to me as a timing issue or improper state handling). The attack order may still be ignored sometimes after the target was polymorphed and only target switching helps to bypass the restriction to attack CCed target.

It's common for warlocks to fear-juggle enemies. If their pets would stop attacking with every fear that'd be a significant DPS loss, so I'm pretty sure the only CCs that should stop pet attacks are those that break with any damage, such as sap, sheep or gouge

I don't object this kind of logic, but I'd like to understand whether this is a blizzlike behavior. Another example, pets also don't stop attacking enemies affected by frost nova, but this is a CC that breaks immediately.

For example they don't always cast sprint on attack order, even if it's off cd, but then occasionally in melee combat they cast sprint.

I think this might be an issue with updated logic too (it most likely will always autocast it), but I'm not sure how to check. Can I somehow make pet learn spells with a command?

There is a similar issue (in both the current and updated logic) with Thunderstomp that's an AoE spell that has no min/max distance, and pets need to get into the victim to actually cast it.

[deiteris]

I think I've sorted the issues out. Pets stop attacking CCed targets correctly in this PR now, and there were also a few fixes added to different behaviors:

• Pets kept switching targets when commanded to retreat, causing them to stop attacking and not retreating until they're in the owner's following distance.
• Pets could target CCed targets in aggressive mode after you command attack and cancel attack with follow.

[killerwife]

Ok and now one last thing to bother you with

PetAI is notoriously known to be ass to script.

Something like this:

https://github.com/cmangos/mangos-tbc/blob/master/src/game/AI/ScriptDevAI/scripts/outland/hellfire_peninsula.cpp#L2231

When you start a script, nothing that a player can do, short of abandon or unsummon should be able to interrupt such a sequence. I understand its a tbc script but I am sure you could write a similar thing in vanilla where a pet moves a bit, does some emote and wont have its script broken.

[deiteris]

When you start a script, nothing that a player can do, short of abandon or unsummon should be able to interrupt such a sequence.

Done. I've tested on TBC and scripted pet shouldn't get stuck anymore. The issue you mentioned was mostly due to the fact that pet was interrupted on combat or on retreating and this messed up the combat and movement states.

EDIT: Though I noticed that it still may stuck when commanded to stay or retreat during/after scripted scene (pet gets stuck in following state and does not actually follow after stay, but continues to follow after attack). And I think it may also stuck when commanded to attack during script. I'll look into it.

[deiteris]

I think simply removing this line would suffice once the changes are ported to other cores.
https://github.com/cmangos/mangos-tbc/blob/master/src/game/AI/ScriptDevAI/scripts/outland/hellfire_peninsula.cpp#L2294

IMO, there's no point in sending pet back to owner after scripted sequence, since once you remove combat script status, the movement will be delegated back to AI (unless it's really necessary to send pet back even if it's supposed to stay, then setting asMain = false in MoveFollow probably should fix it. Otherwise, it affects the movement state and cannot be recovered later).

Works almost flawlessly:
https://github-production-user-asset-6210df.s3.amazonaws.com/6103913/280117133-2727f10c-e4ed-4fc4-9ab1-3cbd1bc09b2a.mp4

[Phatcat]

Regarding the direct commit to the history 59534bc (WIP of #520):

Please be aware that under the GNU General Public License v2.0, stripping the original authorship of @deiteris and re-committing their logic under your own name is a direct violation of Sections 1 and 2.

Simply linking to an old PR in a commit comment does not constitute legal attribution. Git commit metadata is the legal record of authorship; overwriting it is the definition of plagiarism. As shown in the comparison of PetAI.h below, your commit is a line-by-line carbon copy of the work proposed by @deiteris in 2023.

PR:

Merged commit:

As a former organization member and the 9th top commit contributor to this project, I find it alarming that the current leadership is 'laundering' community contributions into personal commits. I strongly advise you to revert and merge properly to maintain what shred of legal, no any, integrity this project has left.

[killerwife]

Nothing is merged. If it were, PR would be closed.

Your point is duly noted and will be resolved when PR code is merged into "master" branch. Any other branch is "my" personal copy that I use between computers. (and have publicly stated on discord multiple times that I use it that way)

[Phatcat]

A branch on the official organization repository is not a "personal copy", and even if it were on a personal fork, it would still be theft. The GPL does not have a "personal use" exception for stripping authorship and pushing someone else's work as your own.

By pushing this code to the cmangos namespace with git authorship removed, you are distributing a derivative work under the project's banner in direct violation of the GPL.

Claiming "it hasn't been merged to master yet" is a pathetic semantic distraction. The GPL does not have a "branch exception." Pushing code to any public GitHub repository constitutes distribution. Whether it’s on a branch or a fork, you are making stolen code available to the public under your own name.

If you wanted to respect the law, you would have kept the original commits, or at the very least authorship over the code itself. Pushing it here with authorship altered to yourself is a textbook license violation. Claiming otherwise is simply a confession that you do not understand, or choose to ignore, the license this project relies on.

But I am glad you are openly admitting you have hijacked the project for personal vanity.

cmangos/mangos-wotlk#228 (comment)

Edit*: Receipts;

EDIT: ID 53 is reserved by me, I have an upcoming feature

me not merging this PR

To any curious bystander, this was the turning point, a 'mask off' moment if you will, for when this became Killerwife's personal vanity project.

[cyberium]

We acknowledge the GPL constraint. That said, finding you here raising this as a critical issue on a WIP branch—before any distribution occurs—feels like history repeating itself. Some things never change.
And since you're glad about the admission, I'm glad you're glad.
However, framing this as 'hijacking for personal vanity' crosses a line. This is a collaborative effort, and attributing selfish motives to routine development work is both inaccurate and unproductive.
We are committed to proper credit in the final commit. Assuming bad faith at this stage hinders progress, not helps it.

[Phatcat]

You mean me looking out for the contributors against predatory practises, you are right, that is not changing. But you clearly have.

It has already been committed. Master branch is not the end-all-be-all. Attribution does not start at merge, it starts at git push. But it saddens me seeing you standing up for this. You are actively defending breaking the license and attribution theft, however 'temporary'. But since you are still on board and I guess still the custodian, you've effectively let this project be run into the ground.

Also, 'We acknowledge you are right we are breaking the license, but that being said we still don't like you'.
Like I care. This ain't about personal feelings, Cyber. It never was.

[Phatcat]

Also I am not assuming anything, I am pointing out this organizations history of attribution theft. This is not the first time Killerwife has done this. There was the infamous, always funny, 'I have already done this work on vengeance, will port soon'. *edit: receipts:

#139 (comment)

What ever happened to Chaosvex actual reverse engineering and implementation of the 2fa for cmangos? It just quietly became killerwife's commit history and both Chaosvex' original PR and my restructuring PR with in game commands got stalled till it became a joke keeping them open.

#193 magically became 4b10465

which makes your comment on his original PR:

such a nice time capsule doesn't it?

This PR is not the exception to the rule, it's the norm around here. If you are getting annoyed at me pointing this out you should look inward.

[Phatcat]

Another case of killerwife stealing attribution, that specific time from me but I am thick skinned.
#334 (comment)

Just pointing out the pattern Cyber, since you seem to have been asleep at the wheel for quite some time.

This was in 2018. Should we go further back, or forward? you decide.
And yes, I literally, in that PR, change the table name from FactionAlliance to Faction and drop FactionHorde.

Not even a mention of me in the commit, this was obviously at the time where I wasn't popular with the inner circles of cmangos, yet my work is fine enough to steal. And what did I do? quietly close the branch on my end with no complaints.

*Edit: Just to REALLY drive home the point, this pattern of stealing attribution started before he was even in the organization or had any push rights:

This was on MY PR: cmangos/mangos-tbc#59
Porting MY changes from Classic #124

Is anyone else getting this? otherwise let me make it painfully clear:
If you value integrity and your time; stay away. Find somewhere you are actually valued, probably far away from the wow emulation scene.

Scorched earth feels right about now.

[Phatcat]

But then again this project does have a tendency of not respecting legally binding documents and flip-flopping between licenses, was this what you meant by history repeating itself, Cyber?

cmangos/issues#1825
cmangos/tbc-db@f1d6d16#commitcomment-23040422

You done playing now?

[ileboii]

I very much appreciate this post @Phatcat . And this is not even sarcasm. This has been very educational post for me, and I agree 100% with your argument here.
Thank god someone stands up for the original authors.

[Phatcat]

Well let me give everyone lurking from the sidelines the incentive for all of this:

Gatekeep and steal code so that you look like the only 'real' contributor so that people are incentivized to transfer money directly into your paypal account to keep core development going.
Are code changes even still peer reviewed or are you just shoveling in commits as you please? A far cry from what this place was a decade ago, eh?

'Collaborative effort', sure, if what you mean is the collaborative effort of not respecting the license and authorship of contributors.

You can ban me and you can scrub my comments, you can vilify me, but the truth is etched into the very commit history for the better part of a decade. It's not just this repo either, every repo on this organization is tainted.

[cyberium]

All commits are public. You can check every change, every author, every fix. If we make a mistake with attribution, we correct it. Always.
Saying this is 'theft' when everything is visible and fixed in good faith is not accurate. It feels intentional.
This is my last reply. Next personal attack = ban.
Focus on the code, or stop posting here.

[killerwife]

So, I guess its time to reply. At first I thought I would stay out of this, because clearly I am in the wrong here, and everyone in both TC and cmangos org has been made known that I indeed made a mistake in regards to this. I particularly do not have the need to let anyone know, besides the ORG AND the original author.

Mind you, the bigger issue here is you. You have been the issue all along, especially since the whole debacle where Cyberium had to adjudicate. I figured I would remove myself from the discussion involving you, just giving a good faith response if possible.

For those who do not know, phatcat a couple years back, was a part of the Cmangos org. To my knowledge he has been the only person to be banned/largely shunned in the org in my whole tenure in wow emu since 2015. When given push rights not only did he break a majority of the pet codebase, but when given criticism the whole matter escalated into shouting matches and removed push rights and org rights as a whole.

The issue with his person has never been that he isnt right on some topics, just like everyone else, but that he has been largely the only agent with actions in downright bad faith. As a result, the majority of us under the org do not even respond, nor react to him whenever possible.

Despite me being largely the main contributor for some time now, under the hood whenever there is a complex topic at hand I ask for advice (just like I did after this stirrup in this PR happened). I personally do not care for attacks upon me myself (since ive been largely under attack from phatcats statements from his removal in 2019) however once someone is getting into the business of blaming the whole org for him being grossly offended, which to my knowledge largely stems from his feeling we have done some injustice to him, well then I take offense.

This will be the final response effective immediately and more comments from you will be deleted by me personally on this topic going forward. Should the original author reach out to me, that is however a whole different topic and I will deal with it when I am in the area. My branch I kept on mangos-classic is gone, I will resume working on it when I have time.

You can wage your war somewhere that is not the cmangos github or discord. I honestly think if there were more people like you, wow-emu would be even more dead than it is right now.

I will distribute this reply to anyone of relevance, because I do not take effectively banning anyone lightly. And I will face the music from them, not you.

[deiteris]

Dear god, this PR has turned almost 3 years and these are the notifications I see after all this time... Let me state my own position regarding this as an original author of this contribution.

Git commit metadata is NOT the legal record of authorship because it does not survive the carbon copy. The GPL compliance is preserved via appropriate copyright/license notices. Which this repository does by putting a license header in each file and referring to AUTHORS for all contributors (and by the way I did contribution before, but forgot to list myself there). Whether this is an appropriate compliance mechanism, I don't know, since a more robust one is to preserve copyright notices for each contribution right in the code (and that would show the carbon copy as a clear violation), and to be really honest, I don't want to discuss any legal details here specifically.

Do I care about improper attribution here? Not really since I had genuine interest in improving the code. Linked commit at very least had a back-reference to the original PR and didn't seem to have an intention to just launder my code to put it into master (and even if it was the case, this PR would've been closed and new commits pushed to master, which would get at least my attention to why this happened and aforementioned concerns would've been valid in that case).
Should maintainers AND contributors be concerned by the attribution quality? Probably should to avoid building a wrong premise for such accusations like in this case.
Do I care about all the historical drama listed here? I do not since I'm not a full-time contributor here.
Am I annoyed that after all these years I come back and see this conversation in my PR instead of it being merged? Absolutely.

[killerwife]

Merged it on tbc only for the pure reason of not wanting new features on multiple core branches at once. Otherwise will be cherry picked on next core sync. Cheers