[3.3.5] Split TargetedMovementGenerator #21722

Treeston · 2018-03-28T00:43:17Z

We currently have a TargetedMovementGenerator, which was originally intended to handle both ChaseMovement and FollowMovement. On paper, this seems sensible, as they function similarly.

However, there are quite some differences between the two, which has left TargetedMovementGenerator an unreadable mess of special cases. I am splitting the two apart so as to make work on them easier.

Ref issue #19925.

ccrs · 2018-03-28T08:00:02Z

https://trinitycore.atlassian.net/wiki/spaces/tc/pages/2130103/C+Development+Standards

Methods are always UpperCamelCase

ccrs · 2018-03-28T08:04:09Z

I agree with this, the separation is needed. Sadly I dont have the time to go through the new logic and review it.

ccrs · 2018-03-28T08:07:50Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.h

+        Optional<float> const _angle;
+
+        void startMovement(Unit* owner);
+        void pause(Milliseconds duration = 100ms)


careful with naming https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Movement/MovementGenerator.h#L42

currently implemented in WaypointMovementGenerator only, called in several places for movement slot 0

ghost · 2018-03-28T20:57:44Z

Looks like you have got 3x unused parameter 'owner' there. Should be simple to fix, unless that only uncovers more needed changes.

Gooyeth · 2018-03-29T03:00:53Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.h

+
+        ChaseMovementGenerator(Unit* target, Optional<float> range = {}, Optional<float> angle = {}) : AbstractFollower(ASSERT_NOTNULL(target)), _range(range), _angle(angle) {}
+
+        void Initialize(Unit* owner) override {}


Change to /*owner*/

Gooyeth · 2018-03-29T03:01:13Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.h

+        ChaseMovementGenerator(Unit* target, Optional<float> range = {}, Optional<float> angle = {}) : AbstractFollower(ASSERT_NOTNULL(target)), _range(range), _angle(angle) {}
+
+        void Initialize(Unit* owner) override {}
+        void Reset(Unit* owner) override {}


Change to /*owner*/

Gooyeth · 2018-03-29T03:03:59Z

src/server/game/Movement/MovementGenerators/FollowMovementGenerator.h

-}
+        void Initialize(Unit* owner) override {}
+        void Finalize(Unit* owner) override {}
+        void Reset(Unit* owner) override {}


Change to

void Initialize(Unit* /*owner*/) override { } void Finalize(Unit* /*owner*/) override { } void Reset(Unit* /*owner*/) override { }

ghost · 2018-04-01T09:29:39Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

@@ -0,0 +1,182 @@
+/*
+* Copyright (C) 2008-2018 TrinityCore <https://www.trinitycore.org/>


Cosmetic/codestyle: missing 1 space indent for lines 2-16

Treeston · 2018-04-01T21:13:14Z

done, review please

ghost · 2018-04-01T22:08:06Z

Could you fix the travis-ci build error ~~(and maybe the current one in 3.3.5 as well)~~ so we can see what else may need a follow-up?
~~(edit: https://travis-ci.org/TrinityCore/TrinityCore/builds/360911820#L1436 - in case you want a link)~~
My bad, I presumed too much. I just realized that TargetedMovementGenerator.cpp et alia gets removed in this PR.

…ntGenerator and FollowMovementGenerator, full rewrite for both. - Chase to angle is now functional. Pets use this to chase behind the target. Closes TrinityCore#19925. - Chase to arbitrary range interval works. Not used anywhere, but you can technically make hunter-like mobs. - Pets now follow the hunter cleanly and without stutter stepping. Also fix some other things. Closes TrinityCore#8924.

Killyana · 2018-04-02T02:05:12Z

I got a crash: https://paste2.org/LwEWNvxk
.go xyz -127.13346, 2154.41699, 30.654285 631
and send your pet to attack the npc SpawnID: 201097

Looks like it fixes: #14120 #12817 #13686
Issues related but not fixed: #21237 #14242 #16647

ghost · 2018-04-02T09:51:50Z

I noticed that in current 3.3.5 source (as of e350cfc), hunter pet positions itself in front of the player instead of to the left. Does this PR revert that change?

edit: (you don't need a pet class player to see this happening, it is enough to watch Warp-Huntress Kula (entry 32711) in Dalaran or any other moving pet class NPC to see the pet move through the creature to stay in front of it.)

Treeston · 2018-04-02T10:32:14Z

In this PR, the pet will always follow the player somewhere in the rear 90° arc (though this could easily be adjusted).

ccrs · 2018-04-02T12:19:47Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+    // the owner might be unable to move (rooted or casting), pause movement
+    if (owner->HasUnitState(UNIT_STATE_NOT_MOVE) || owner->IsMovementPreventedByCasting())
+    {
+


hello there

ccrs · 2018-04-02T12:20:29Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+#include "MoveSplineInit.h"
+#include "G3DPosition.hpp"
+
+static bool isMutualChase(Unit* owner, Unit* target)


naming sitll, UpperCamelCase unless u want to change the standard 💃

not a member function

so? this applies to ALL functions, not just members (the only exceptions are begin/end for range loops)

ccrs · 2018-04-02T12:20:57Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+    return (static_cast<ChaseMovementGenerator const*>(gen)->GetTarget() == owner);
+}
+
+inline static float sq(float a) { return a*a; }


maybe move this to somewhere else?

ccrs · 2018-04-02T12:21:55Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+static bool isMutualChase(Unit* owner, Unit* target)
+{
+    MovementGenerator const* gen = target->GetMotionMaster()->top();
+    if (gen->GetMovementGeneratorType() != CHASE_MOTION_TYPE)


gen can be null, updating slots, expiring top or w/e

though this should only be called on top... guess its ok

Shauren · 2018-04-05T06:57:50Z

src/server/game/Movement/MotionMaster.h

+    ChaseRange(float min, float tMin, float tMax, float max) : minRange(min), minTolerance(tMin), maxRange(max), maxTolerance(tMax) {}
+
+    // this contains info that informs how we should path!
+    float minRange;     // we have to move if we are within this range...    (min. attack range)


Naming - public members

Shauren · 2018-04-05T06:57:56Z

src/server/game/Movement/MotionMaster.h

+{
+    ChaseAngle(float angle, float tol = M_PI_4) : relativeAngle(Position::NormalizeOrientation(angle)), tolerance(tol) {}
+
+    float relativeAngle; // we want to be at this angle relative to the target (0 = front, M_PI = back)


Naming - public members

Shauren · 2018-04-05T06:58:04Z

src/server/game/Movement/MotionMaster.h

+    float relativeAngle; // we want to be at this angle relative to the target (0 = front, M_PI = back)
+    float tolerance;     // but we'll tolerate anything within +- this much
+
+    float upperBound() const { return Position::NormalizeOrientation(relativeAngle + tolerance); }


Naming - functions

Shauren · 2018-04-05T06:58:42Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+#include "MoveSplineInit.h"
+#include "G3DPosition.hpp"
+
+static bool isMutualChase(Unit* owner, Unit* target)


so? this applies to ALL functions, not just members (the only exceptions are begin/end for range loops)

Shauren · 2018-04-05T06:59:00Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+    return !angle || angle->isAngleOkay(target->GetRelativeAngle(owner));
+}
+
+static const Optional<float> NONE;


remove this

Shauren · 2018-04-05T07:00:51Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.h

+
+        ChaseMovementGenerator(Unit* target, Optional<ChaseRange> range = {}, Optional<ChaseAngle> angle = {}) : AbstractFollower(ASSERT_NOTNULL(target)), _range(range), _angle(angle) {}
+
+        void Initialize(Unit* owner) override { owner->AddUnitState(UNIT_STATE_CHASE); owner->SetWalk(false); }


Why is walk/run mode tied to chasing? (http://www.wowhead.com/npc=20064/thaladred-the-darkener)

Shauren · 2018-04-05T07:01:51Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.h

+        Optional<ChaseRange> const _range;
+        Optional<ChaseAngle> const _angle;
+
+        std::unique_ptr<PathGenerator> _path;


forward declare PathGenerator instead of including, move constructor body to .cpp and add ~ChaseMovementGenerator::ChaseMovementGenerator() = default to .cpp

Shauren · 2018-04-05T07:04:34Z

src/server/game/Movement/MovementGenerators/FollowMovementGenerator.cpp

+#include "Pet.h"
+
+static inline float sq(float a) { return a*a; }
+static bool positionOkay(Unit* owner, Unit* target, float range, Optional<ChaseAngle> angle = {})


Shauren · 2018-04-05T07:05:57Z

src/server/game/Movement/MovementGenerators/FollowMovementGenerator.h

+        void UnitSpeedChanged() override { _lastTargetPosition.Relocate(0.0f, 0.0f, 0.0f); }
+
+    private:
+        static const uint32 CHECK_INTERVAL = 500;


Shauren · 2018-04-05T07:06:36Z

src/server/game/Spells/Spell.cpp

@@ -5498,7 +5499,7 @@ SpellCastResult Spell::CheckCast(bool strict, uint32* param1 /*= nullptr*/, uint
                    else if (m_preGeneratedPath->IsInvalidDestinationZ(target)) // Check position z, if in a straight line
                            return SPELL_FAILED_NOPATH;

-                    m_preGeneratedPath->ReducePathLenghtByDist(objSize); // move back
+                    m_preGeneratedPath->ShortenPathUntilDist(PositionToVector3(target), objSize); // move back


just pass x,y,z separately?

sure - but in how many places do we have to do that before it's unneeded code duplication? helpers are lightweight

you know what I'm hinting at here (include hater)

the only thing new is the .hpp - we already had Vector3 (from PathGenerator itself)

…handled by movespline)

Shauren · 2018-04-05T11:29:20Z

btw the forward declare business of PathGenerator also applies to FollowMovementGenerator

Treeston · 2018-04-05T12:13:17Z

right, forgot

Treeston · 2018-04-05T12:52:18Z

anything else, @Shauren?

ccrs · 2018-04-05T14:03:50Z

the setinfront part might be still needed, to correct serverside orientation on reach

btw funny how I told you twice about the naming 💃
and also, having a sq inline declared twice might be a good sign to move it to util or w/e

ccrs · 2018-04-05T14:07:29Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.h

+#define TRINITY_CHASEMOVEMENTGENERATOR_H
+
+#include "MovementGenerator.h"
+#include "Unit.h"


move Initialize to cpp to remove this

ccrs · 2018-04-05T14:08:12Z

src/server/game/Movement/MovementGenerators/FollowMovementGenerator.h

+#define TRINITY_FOLLOWMOVEMENTGENERATOR_H
+
+#include "MovementGenerator.h"
+#include "Unit.h"


if its necessary for abstractfollower, maybe move it to a separate h?

Treeston · 2018-04-05T14:15:30Z

the setinfront part might be still needed, to correct serverside orientation on reach

movespline has facing set

Shauren · 2018-04-05T14:30:51Z

movespline does not update facing for MoveSplineFlag::Final_Target (it only does for point and angle)

Treeston · 2018-04-05T14:32:11Z

Hrm, alright. Feels like it should?

Treeston · 2018-04-05T23:16:18Z

changes done.

i still feel like the orientation should be set by the spline logic (since the spline specifies a facing target), but don't see any quick way to do it - movespline code is painfully not unit aware, and just setting stuff to public and handling it outside feels like a hack.

so i re-added setinfront as a temporary workaround.

ccrs · 2018-04-05T23:57:11Z

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

+            if (PositionOkay(owner, target, _movingTowards ? Optional<float>() : minTarget, _movingTowards ? maxTarget : Optional<float>(), angle))
+            {
+                owner->StopMoving();
+                _path = nullptr;


consider maybe adding a return true here?

~~no - if the position is invalid, we want to immediately start pathfinding again (maybe the target moved?)~~

scratch that, was thinking of a different (similar) branch

ccrs · 2018-04-05T23:57:49Z

src/server/game/Movement/PathGenerator.cpp

-        else if (i == 0) // at second point
+        // we know that pathPoints[i] is too close already (from the previous iteration)
+        if ((_pathPoints[i-1] - target).squaredLength() >= distSq)
+            break; // bingo!


love the comment ❤️

Shauren · 2018-04-06T06:44:13Z

movespline code is painfully not unit aware

This means its well designed as standalone system

Treeston · 2018-04-06T10:04:43Z

careful with that back patting, it includes Creature.h...

ccrs · 2018-04-06T12:12:52Z

tbf I dont recall it being his entirely 😄

btw cant do much more, dont have the time to test. I feel its a really solid work, as always ❤️, so trusting your logic I'd say its merge ready.

Treeston added Comp-Core Branch-3.3.5a Sub-Movement labels Mar 28, 2018

Treeston self-assigned this Mar 28, 2018

Treeston requested a review from Shauren March 28, 2018 00:43

ccrs reviewed Mar 28, 2018

View reviewed changes

Gooyeth reviewed Mar 29, 2018

View reviewed changes

ghost reviewed Apr 1, 2018

View reviewed changes

Treeston force-pushed the 3.3.5-targetedmovegen branch 4 times, most recently from f7ab8f5 to 1b1d9e8 Compare April 1, 2018 16:35

Treeston added the Feedback-PatchFix label Apr 1, 2018

Treeston changed the title ~~[3.3.5] [WIP] Split TargetedMovementGenerator~~ [3.3.5] Split TargetedMovementGenerator Apr 1, 2018

Treeston force-pushed the 3.3.5-targetedmovegen branch from 1aa917d to 831f5cf Compare April 1, 2018 22:54

fix a crash

f1c8887

ccrs reviewed Apr 2, 2018

View reviewed changes

TrinityCore deleted a comment Apr 4, 2018

some cleanup

4839274

Shauren requested changes Apr 5, 2018

View reviewed changes

Treeston added 3 commits April 5, 2018 12:18

code style: UpperCase edition

8bf4f7a

remove that ::Attack block, AI will handle this stuff anyway (facing …

56e7714

…handled by movespline)

and a few constexpr

312f2dc

PathGenerator -> cpp

7f2b571

ccrs reviewed Apr 5, 2018

View reviewed changes

Treeston added 2 commits April 6, 2018 00:59

AbstractFollower is its own file now

ad9d1a0

sq -> util.h

0f1c50b

Treeston requested review from Shauren and ccrs April 5, 2018 23:15

ccrs reviewed Apr 5, 2018

View reviewed changes

some shortcut returns

101f13a

Shauren approved these changes Apr 6, 2018

View reviewed changes

Treeston merged commit 2a84562 into TrinityCore:3.3.5 Apr 6, 2018

Treeston deleted the 3.3.5-targetedmovegen branch April 6, 2018 16:09


		ChaseMovementGenerator(Unit* target, Optional<float> range = {}, Optional<float> angle = {}) : AbstractFollower(ASSERT_NOTNULL(target)), _range(range), _angle(angle) {}

		void Initialize(Unit* owner) override {}

		@@ -0,0 +1,182 @@
		/*
		* Copyright (C) 2008-2018 TrinityCore <https://www.trinitycore.org/>


		ChaseMovementGenerator(Unit* target, Optional<ChaseRange> range = {}, Optional<ChaseAngle> angle = {}) : AbstractFollower(ASSERT_NOTNULL(target)), _range(range), _angle(angle) {}

		void Initialize(Unit* owner) override { owner->AddUnitState(UNIT_STATE_CHASE); owner->SetWalk(false); }

[3.3.5] Split TargetedMovementGenerator #21722

[3.3.5] Split TargetedMovementGenerator #21722

Conversation

Treeston commented Mar 28, 2018

ccrs commented Mar 28, 2018 • edited

ccrs commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Treeston commented Apr 1, 2018

ghost commented Apr 1, 2018 • edited by ghost

Killyana commented Apr 2, 2018 • edited

ghost commented Apr 2, 2018 • edited by ghost

Treeston commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shauren commented Apr 5, 2018

Treeston commented Apr 5, 2018

Treeston commented Apr 5, 2018

ccrs commented Apr 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Treeston commented Apr 5, 2018

Shauren commented Apr 5, 2018

Treeston commented Apr 5, 2018

Treeston commented Apr 5, 2018

Choose a reason for hiding this comment

Treeston Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shauren commented Apr 6, 2018

Treeston commented Apr 6, 2018

ccrs commented Apr 6, 2018 • edited

ccrs commented Mar 28, 2018 •

edited

ghost commented Apr 1, 2018 •

edited by ghost

Killyana commented Apr 2, 2018 •

edited

ghost commented Apr 2, 2018 •

edited by ghost

ccrs commented Apr 5, 2018 •

edited

Treeston Apr 6, 2018 •

edited

ccrs commented Apr 6, 2018 •

edited