[3.3.5] Core/Movement: Pets go for the back unless being the victim

[necropola]

This restores the pet's original behavior to not always attack from the front and get cleaved down.

• Target branch: 3.3.5
• Issues addressed: Closes [3.3.5] Pets should attack from behind the target $150 #19925
• Tests performed:
  • build success
  • tested/verified in game

I was neither able to pinpoint the commit that broke it nor do I think I found the best place/way to fix it, but it's very effective and should make warlocks and hunters happy again.

Demo showing the new (restored) pet behavior: https://youtu.be/9p4CYmOOWaE

[Shauren]

Seems legit?

[necropola]

I mean, it's clearly not the best place to adjust the _angle value. But seeing that the code already adjusts offset and size for the pet runs to player case, I thought what the heck, why not fix it here!?

[ccrs]

If you've tested it... 👍

[necropola]

I'm running around with a warlock and a hunter for some time now and it's clearly an improvement. I haven't seen any pet dance regression (pet tries to reach the target's back while target tries to attack from the front) and the pet does no longer actively go for the front when it's not the target/victim.

It's still not 100% what it used to be. In older versions of TC-3.3.5 the pet would just approach the target in a straight line and attack from there. Like it still does when using Charge (which uses a different MovementGenerator). It would have been nicer to actually find the commit that changed the behavior and address the issue from there, but what can you do?

[necropola]

Anything I can do to increase the likelihood that this gets merged into TC-3.3.5?

[ghost]

Not really, it will be approved and merged when enough developers have had the chance to review it.

[Shauren]

Alright, looks like I found a possible problem with it
By manipulating _angle you prevent this code from triggering when reaching target https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Movement/MovementGenerators/TargetedMovementGenerator.cpp#L92
Its a problem also because when _angle is not 0 then spline doesnt get the fancy "chase this" visual (client would automatically draw proper pet facing even if target moves)

[necropola]

I understand that _angle is used to calculate a point at offset distance from the target relative to where the target is currently facing, i. e. if _angle == 0 this point is in front of the target and if _angle == M_PI it's in its back.
Currently the TargetedMovementGenerator seems to be used with _angle == M_PI/2 for the case that the pet returns to it's owner, and with _angle == 0 to approach a hostile target which has the undesired effect of going for the front, even when the target is after someone else (the tank).
My patch simply catches the latter case and prevents the pet from going suicidal.

[ccrs]

Thing is the pet must face the creature when reaches the target (thats what the section shauren linked does). The pet will face its target visually (clientside) because it is targeting it (unit::SetTarget) and client makes the unit always face its target.

But serverside the orientation will remaing incorrect, maybe being autocorrected for a second if it casts, but returning to old (incorrect) after the cast. This can break many things like autoattacks or spells that require facing.

[necropola]

Could we change it so that the pet (server-side) faces a hostile target even when in its back, i. e. face the target's back? Would owner->SetFacing(GetTarget()) work with _angle != 0 instead of owner->SetInFront(GetTarget())?

In older versions of TC the pet would simply approach in a straight line like it still does when using it's Charge ability and then attack from the point where it reached the target (ignoring the target's orientation by obviously using a different MovementGenerator, maybe Point?).

At some point along the line (when?) it has probably been changed to use the TargetedMovementGenerator and thereby introduced pet suicide mode ...

I have actually tried to bisect the source starting from 2 years back to find that commit ... but given up due to TBD and vmap format changes getting in the way ...

[Jildor]

any news?

[ghost]

If I have understood the conversation correctly, we are still waiting for a solution to the issue Shauren pointed out in #20128 (comment) .

[Keader]

Any news on this @necropola ?

[chaodhib]

Please don't put that logic in the MoveGen. Like you already pointed it out, it's not a suitable place for pet logic (or any non movement logic really).

The reason there already is pet logic there is because of the laziness from past contributors. Let's not keep doing the same thing.

PS: I'm in the process of refactoring the MoveGen to clean it up from this kind of stuff.
PS2: I'm also in the process of improving the MoveGen. The overhead of complexity created by these unnecessary coupling between the different parts of the core are slowing down my progress significantly.
PS3: Nice work nonetheless. I know contributing to the project for the first time is hard, because of the size and the complexity. Keep it up!

[ghost]

I do hope @necropola is still alive and around to receive the feedback.
Would be sad to see useful & practical feedback go to waste.

changed my mind

[Shauren]

Replaced by 2a84562