[3.3.5] [NEEDS TESTING] Core/Movement: expand Movement Slot implementation #21888

ccrs · 2018-04-26T19:15:04Z

basic structure changes, nothing behavioural
future PRs will have the real juice

Shauren · 2018-04-26T22:12:31Z

What exactly is your plan with this?

ccrs · 2018-04-26T22:21:46Z

having 3 slots each capable of containing different generators

then motionmaster will "consume" from CONTROLLED to IDLE, like right now; but finishing all the generators before going to the next inferior slot.

ccrs · 2018-04-26T22:25:13Z

I still think having 3 slots is something actually desirable, see more pros than cons

but then again, we can swift to something like u once suggested, priority_queue, and forget about slots

ccrs · 2018-04-26T22:29:06Z

I feel like having different slots makes it actually easier/more efficient in regards of accessing a group of generators, or even a single one.... a multimap can also do the trick I guess

ccrs · 2018-04-27T00:38:26Z

src/server/game/Movement/MotionMaster.h

        uint8 _cleanFlag;
+        std::unordered_multimap<uint32, MovementGenerator*>  _baseUnitStatesMap;


the idea is to have an exact control of the unitstates that the owner must have and must remove.

example:

moment 1
SLOT 2 -> PointMovement -> ROAMING State
SLOT 3 -> Empty

moment 2
SLOT 2 -> PointMovement -> ROAMING State
SLOT 3 -> new Generator -> new Top -> PointMovement -> ROAMING State

moment 3
SLOT 2 -> want to erase -> PointMovement -> Finalize -> wants to remove ROAMING State
SLOT 3 -> still active, still Top -> PointMovement -> still wants ROAMING State

ccrs · 2018-04-27T00:40:55Z

the main structure can be seen now... more or less, thoughts @Shauren @Treeston @ariel- ?

ghost · 2018-04-27T03:06:54Z

src/server/game/Movement/MotionMaster.h

+{
+    public:
+        MovementSlot(uint8 type);
+        ~MovementSlot();


Should MovementSlot be renamed to MovementSlots here too (+ class , uint8 type) ?
build error: https://travis-ci.org/TrinityCore/TrinityCore/builds/371823289#L1385-L1391

dont worry about build errors, yet

Shauren · 2018-04-27T07:18:06Z

Similar to something I had in mind long time ago

IDLE slot remains without changes (idle or db waypoints)
ACTIVE - all core movement goes here
CONTROLLED - for special cases (more on special cases later)

Your deque should be std::priority_queue instead, with priorities something like root > point > chase > ... (order up for discussion really, havent thought about it that much). Notice I listed root as movement generator type - you should implement it as .Stop() on init, empty on update.

Here is where CONTROLLED slot comes in - scripts could use it to "override" defult generator priority (for example forcing movement while stunned)

Ref states - if you mean UNIT_STATE enum then I think you should not have any fancy maps for it, just always override it with highest priority generator from highest slot

ccrs · 2018-04-27T08:44:12Z

Sounds good, will keep new things out of this PR though (Root generator), I just want to set the structure for future changes

About UnitStates, I still think a detailed control is desirable, think about a jump spline while feared, you still are feared, cannot perform any action that is not allowed while feared.

Or the case I pointed before #21888 (comment). Example, while a creature is moving towards a point, long distance travel, it can charge for example another creature, long distance charge too, and while charging maybe the AI tells MotionMaster to clear/delete the point movement, erasing ROAMING when it should still be applied because of the charge.

Shauren · 2018-04-27T08:59:35Z

Yeah but you don't need to store it, just erase "all movement related states" when current movegen changes and compute (just take it off highest new generator) new state (basically how certain auras do it and phasing flags on master)

ccrs · 2018-04-27T09:49:54Z

maybe its just too obvious for me to notice, but sorry for being stubborn, cant see what you mean

on phases there are a couple reference counters to know what to do https://github.com/TrinityCore/TrinityCore/blob/master/src/server/game/Phasing/PhaseShift.cpp#L148
and for auras usually a getter is used to know if there are any other auras using the same mechanic or w/e, like GetAuraEffectsByType or https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Entities/Unit/Unit.cpp#L11133

I'm just talking about "Base" UnitStates such as "UNIT_STATE_ROAMING" or "UNIT_STATE_CONFUSED", "Move" UnitStates ("UNIT_STATE_ROAMING_MOVE", "UNIT_STATE_CONFUSED_MOVE" and so on) are always cleared indeed.

Shauren · 2018-04-27T10:35:40Z

why are unit states for movement needed in the first place? arent they the current way of trying to solve movement type priority issue?

ccrs · 2018-04-27T10:59:06Z

they signal the different movements that are active on the creature I guess, in the end the check is usually done against UNIT_STATE_MOVING

Shauren · 2018-04-27T11:08:15Z

I bet most of these could be replaced with a real moving check (active spline/movement flag)

ccrs · 2018-04-27T11:15:39Z

most likely, the only use is in

bool IsStopped() const { return !(HasUnitState(UNIT_STATE_MOVING)); }

Treeston · 2018-04-27T11:42:11Z

movement unit states are a bitmask, keep that in mind.

a lot of movegens use them to check whether another unit has a specific kind of movegen active (chase uses this to prevent infinite repositioning when two units try to be behind each other, for example...)

ccrs · 2018-04-27T11:56:17Z

eitherway, the removal or not of those unitstates is out of the scope of this PR

Treeston · 2018-04-27T14:43:58Z

Is there any reason to actually still have three slots?

If this is transformed into a priority_queue as @Shauren suggests (and I concur), I don't see any reason to keep the slots around at all.

ED: If you keep them around, then just keep them around as part of the priority system on a single priority_queue...

ccrs · 2018-04-27T15:14:09Z

@Treeston #21888 (comment)

ccrs · 2018-04-27T15:15:30Z

oook MotionMaster can be reviewed far more clearly now https://github.com/ccrs/TrinityCore/blob/movementinform/src/server/game/Movement/MotionMaster.cpp

Treeston · 2018-04-27T15:18:28Z

Yes, but why do we need three different "slot" instances per se, instead of just one big priority_queue on motionmaster?

Everything @Shauren mentioned is still possible with a single queue.

Shauren · 2018-04-27T18:31:10Z

@Treeston making a lower priority movement type be on top for scripts?

Treeston · 2018-04-27T18:43:57Z

{ PRIORITY_CONTROLLED, PRIORITY_CHASE_MOVE }?

PS: Though I disagree with using _CONTROLLED for this - fourth priority (_SCRIPT_OVERRIDE) or whatever should exist.

Shauren · 2018-04-27T19:26:53Z

Thing is, I don't see a need to have 4 slots then

ccrs · 2018-04-29T16:57:59Z

I see Treeston's point, actually I think its a good idea. Let me do the changes and we can talk about it with actual code

… argument that is not explicit.

ccrs · 2018-05-30T14:53:16Z

well, now its review ready 💃

ccrs · 2018-05-31T14:00:09Z

All test done on my part (ingame and ASan). I consider it merge ready.

jackpoz · 2018-05-31T14:54:42Z

.travis.yml

-      - libboost-regex1.58-dev
-      - libboost-system1.58-dev
-      - libboost-thread1.58-dev
+      - libboost1.62-dev


are these changes still required or can they be reverted ?

up to you/all to decide, not required indeed

maybe save them somewhere in case we have to upgrade boost but remove these changes from the PR, ty

https://gist.github.com/ccrs/53d049c0dd8b392b3aa3f3c5de91a04b

Shauren · 2018-05-31T16:47:05Z

src/server/game/AI/SmartScripts/SmartScript.cpp

@@ -2230,16 +2232,16 @@ void SmartScript::ProcessAction(SmartScriptHolder& e, Unit* unit, uint32 var0, u
                    target->ToUnit()->RemoveAllGameObjects();
            break;
        }
-        case SMART_ACTION_STOP_MOTION:
+        case SMART_ACTION_REMOVE_MOVEMENT:


Is the change to this action backwards compatible? If not then you need to update db data to keep behavior the same

there is not, currently, any script using the action

backwards compatible?

I guess so... the old one made 0 sense and hardly ever worked

Shauren · 2018-05-31T16:54:18Z

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

 {
+    MovementGenerator::Mode = MOTION_MODE_DEFAULT;


pretty sure you are supposed to use this-> in this situation

that sounds right, it did never make too much sense to mee either, like it works, but thats how u declare a local variable? or call a static one? dunnow, will change 👍

Because this (or base) class is a template its "dependent name lookup", by qualifying it with this-> you change when compiler resolves it (template two phase stuff)

got it 👍

Shauren · 2018-05-31T17:06:02Z

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

+    AddFlag(MOVEMENTGENERATOR_FLAG_FINALIZED);
+
+    /*
+     *  TODO: ugh


aka will change with time and future work, something more descriptive would be better though xd

Shauren · 2018-05-31T17:14:59Z

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

+        owner->ClearUnitState(UNIT_STATE_ROAMING_MOVE);
+        owner->StopMoving();
+
+        // TODO: ???????????


???????????

same, but dont tell me its not worth the meme

Shauren · 2018-05-31T17:21:27Z

src/server/game/Movement/MotionMaster.cpp

+        case MOTION_SLOT_ACTIVE:
+            if (!_generators.empty())
+            {
+                MotionMasterContainer::const_iterator itr = std::find_if(_generators.begin(), _generators.end(), [filter](MovementGenerator const* a) -> bool


auto (its an iterator)
also you can rewrite it as simply auto itr = std::find_if(_generators.begin(), _generators.end(), std::ref(filter)); // ref is here to avoid copying the function into find_if

the function is referenced, there was no copy there, right?
eitherway, much cleaner your way 👍

There is a copy, default lambda capture mode is by value, it does not matter what the actual captured type is (you would need capture by ref, [&filter])

reference the reference, the more you know 💃

Shauren · 2018-05-31T17:26:09Z

src/server/game/Movement/MotionMaster.cpp

+        case MOTION_SLOT_ACTIVE:
+            if (!_generators.empty())
+            {
+                MotionMasterContainer::const_iterator itr = std::find_if(_generators.begin(), _generators.end(), [filter](MovementGenerator const* a) -> bool


same here, auto itr = std::find_if(_generators.begin(), _generators.end(), std::ref(filter));

Shauren · 2018-05-31T17:27:33Z

src/server/game/Movement/MotionMaster.cpp

-        DirectClean(slot);
+    if (HasFlag(MOTIONMASTER_FLAG_UPDATE))
+    {
+        std::function<void()> action = [this, movement, slot]() {


codestyle, { placement (applies to all std::function objects you added for _delayedActions)

Shauren · 2018-05-31T17:27:50Z

src/server/game/Movement/MotionMaster.cpp

+        case MOTION_SLOT_ACTIVE:
+            if (!_generators.empty())
+            {
+                MotionMasterContainer::iterator itr = _generators.find(movement);


auto with iterators

Shauren · 2018-05-31T17:28:19Z

src/server/game/Movement/MotionMaster.cpp

+        case MOTION_SLOT_ACTIVE:
+            if (!_generators.empty())
+            {
+                MotionMasterContainer::iterator itr = std::find_if(_generators.begin(), _generators.end(), [type](MovementGenerator const* a) -> bool


auto iterator

Shauren · 2018-05-31T17:30:16Z

src/server/game/Movement/MotionMaster.cpp

+    if (cache.empty())
+        return;
+
+    for (MovementGenerator* movement : cache)


you can do this without a temporary container, just use iterator directly instead of this loop syntax

I guess, since the container size wont ever be > 20 (and thats maybe too much to say), I thought this was """"""safer"""""" and the setback was minimal. Will change though 👍

Shauren · 2018-05-31T17:31:12Z

src/server/game/Movement/MotionMaster.cpp

+                }
+                else
+                {
+                    MotionMasterContainer::iterator itr = std::find_if(_generators.begin(), _generators.end(), [movement](MovementGenerator const* a) -> bool


Shauren · 2018-05-31T17:32:05Z

src/server/game/Movement/MotionMaster.cpp


-    while (!empty() && !top())
-        --_top;
+    _baseUnitStatesMap.insert(std::make_pair(movement->BaseUnitState, movement));


emplace should work here without make_pair step

did I try that? I cant remember it was like 3-4 weeks ago, will try again 👍
plus the current one is a copy insert sooo 💃

Should work (would be a different story if you tried to construct pair elements using their own constructors passing even more args, std::piecewise_construct would be needed)

Shauren · 2018-05-31T17:35:09Z

src/server/game/Movement/MotionMaster.cpp

-    curr->Finalize(_owner);
-    delete curr;
-}
+    std::pair<MotionMasterUnitStatesContainer::iterator, MotionMasterUnitStatesContainer::iterator> bounds = _baseUnitStatesMap.equal_range(movement->BaseUnitState);


Trinity::Containers::MultimapErasePair is designed exactly for what you are doing here, after erasing follow with

if (_baseUnitStatesMap.count(movement->BaseUnitState) _owner->ClearUnitState(movement->BaseUnitState);

ow, thats a nice one 👍

ccrs · 2018-06-03T14:43:34Z

doing some last tests (ingame with ASan) after the last commit before merging 👍

Gooyeth · 2018-08-26T03:46:17Z

@ccrs: This is causing problems with Harvest Soul of The Lich King, the characters die for fall damage.

.go xyz 529.302 -2124.49 840.857 631
.npc add temp 36598

Keader · 2018-08-26T03:53:01Z

Open a ticket @Gooyeth

TheWinchesters · 2018-12-24T03:51:43Z

Is there an open ticket about what Gooyeth says or do I open one?

jackpoz · 2018-12-24T04:43:29Z

Open one please and references this PR, easy way to track things

Internal structure and handling changes, nothing behavioural (or thats the intention at least). (cherry picked from commit 982643c)

ccrs changed the title ~~[3.3.5] Core/Movement: expand Movement Slot implementation~~ [WIP] [3.3.5] Core/Movement: expand Movement Slot implementation Apr 26, 2018

ccrs added Feedback-FixOutdatedMissingWIP Comp-Core Branch-3.3.5a Sub-Movement labels Apr 26, 2018

ccrs commented Apr 27, 2018

View reviewed changes

ghost reviewed Apr 27, 2018

View reviewed changes

ccrs added 5 commits May 30, 2018 01:33

upsie

c816579

fix build

2bc2b7d

upsie

acb927b

AddressSanitizer report: new-delete-type-mismatch

af4a5b5

Codacy report: Class 'HoRStartMovementEvent' has a constructor with 1…

2ace140

… argument that is not explicit.

ccrs added 4 commits May 30, 2018 17:27

fix no PCH build - gcc7

06e8056

storing lambdas, this one is fancy

30abf25

upsie

91e1c75

remove now unneded forward dcl + fix VS build

ffa93ce

jackpoz reviewed May 31, 2018

View reviewed changes

Shauren reviewed May 31, 2018

View reviewed changes

Shauren requested changes May 31, 2018

View reviewed changes

ccrs added 2 commits June 1, 2018 16:41

Feedback

b73ce52

maybe another time

ae6ca6b

Shauren approved these changes Jun 2, 2018

View reviewed changes

ccrs merged commit 982643c into TrinityCore:3.3.5 Jun 3, 2018

TheWinchesters mentioned this pull request Dec 24, 2018

[3.3.5] ICC / Lich King: Harvest Soul #22883

Closed

Aokromes removed Feedback-WaitingResponse labels Mar 27, 2019

Shauren pushed a commit that referenced this pull request Sep 28, 2021

Core/Movement: MotionMaster reimplementation (#21888)

426f9f2

Internal structure and handling changes, nothing behavioural (or thats the intention at least). (cherry picked from commit 982643c)

		uint8 _cleanFlag;
		std::unordered_multimap<uint32, MovementGenerator*> _baseUnitStatesMap;

[3.3.5] [NEEDS TESTING] Core/Movement: expand Movement Slot implementation #21888

[3.3.5] [NEEDS TESTING] Core/Movement: expand Movement Slot implementation #21888

Conversation

ccrs commented Apr 26, 2018

Shauren commented Apr 26, 2018

ccrs commented Apr 26, 2018 • edited

ccrs commented Apr 26, 2018

ccrs commented Apr 26, 2018

ccrs Apr 27, 2018 • edited

Choose a reason for hiding this comment

ccrs commented Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shauren commented Apr 27, 2018 • edited

ccrs commented Apr 27, 2018 • edited

Shauren commented Apr 27, 2018 • edited

ccrs commented Apr 27, 2018 • edited

Shauren commented Apr 27, 2018

ccrs commented Apr 27, 2018

Shauren commented Apr 27, 2018 • edited

ccrs commented Apr 27, 2018

Treeston commented Apr 27, 2018

ccrs commented Apr 27, 2018

Treeston commented Apr 27, 2018 • edited

ccrs commented Apr 27, 2018

ccrs commented Apr 27, 2018

Treeston commented Apr 27, 2018

Shauren commented Apr 27, 2018

Treeston commented Apr 27, 2018

Shauren commented Apr 27, 2018

ccrs commented Apr 29, 2018

ccrs commented May 30, 2018

ccrs commented May 31, 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

ccrs Jun 1, 2018 • edited

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 May 31, 2018 • edited

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

ccrs commented Jun 3, 2018 • edited

Gooyeth commented Aug 26, 2018

Keader commented Aug 26, 2018

TheWinchesters commented Dec 24, 2018 • edited

jackpoz commented Dec 24, 2018

ccrs commented Apr 26, 2018 •

edited

ccrs Apr 27, 2018 •

edited

ccrs commented Apr 27, 2018 •

edited

Shauren commented Apr 27, 2018 •

edited

ccrs commented Apr 27, 2018 •

edited

Shauren commented Apr 27, 2018 •

edited

ccrs commented Apr 27, 2018 •

edited

Shauren commented Apr 27, 2018 •

edited

Treeston commented Apr 27, 2018 •

edited

ccrs Jun 1, 2018 •

edited

Shauren May 31, 2018 •

edited

ccrs commented Jun 3, 2018 •

edited

TheWinchesters commented Dec 24, 2018 •

edited