Trinitycore: [Dev] WIP Catmullrom waypoint movement

[Pitcrawler]

I started to play around a little with the core and made the following change to WaypointMovementGenerator.cpp

diff --git a/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp b/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
index 942996b..5b8dd8b 100755
--- a/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
+++ b/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
@@ -153,6 +153,16 @@ bool WaypointMovementGenerator<Creature>::StartMove(Creature* creature)
             trans->CalculatePassengerPosition(formationDest.x, formationDest.y, formationDest.z, &formationDest.orientation);
     }

+   if (creature->IsFlying())
+   {
+       for (uint32 i = 0; i < i_path->size() - i_currentNode; i++)
+       {
+           init.Path().push_back(G3D::Vector3(i_path->at(i_currentNode + i)->x, i_path->at(i_currentNode + i)->y, i_path->at(i_currentNode + i)->z));
+       }
+       init.SetSmooth();
+       init.Launch();
+       return true;
+   }
     //! Do not use formationDest here, MoveTo requires transport offsets due to DisableTransportPathTransformations() call
     //! but formationDest contains global coordinates
     init.MoveTo(node->x, node->y, node->z);

This made all the flying NPCs that have waypoint data use catmullrom movement. But there are some things that certainly need improvements which I can't take care of because I have really little knowledge when it gets to core programming:
At each WP the remaining path is rebuild which is unnecessary (memory consumption) and not blizzlike.
At the last WP there is no next WP so no path is build and point movement is used instead of catmullrom.
Flying creatures that have inhabitType!=4 (i.e. 5 or 7) don't use catmullrom movement. I guess the core doesn't handle them correctly so they don't get their movementflags.
Swimming creatures don't use catmullrom movement.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[jackpoz]

C++ changes fit more in a pull request so they can be commented and discussed.

For example

i_currentNode + i < i_path->size()

is more clear, or you could even just initialize "i" to i_currentNode and avoid any other "+ i_currentNode"

Storing the value of

i_path->at(i_currentNode + i)

is better than writing the same code 3 times (otherwise if you need to change 1 place, you have to remember to do it 3 times)

[Subv]

About the issue with recalculating the path every time, you can just store the previously calculated path in the WayPointMovementGenerator class, you'll have to modify the code that calls MovementInform though, because as far as I remember it works by using _spline->Finished() which won't ever be true in this case except when the creature reaches the final waypoint.

[Pitcrawler]

@joschiwald thanks for the tips. The reason why I don't create a PR is simple. I don't know how to and haven't got the time to learn it at the moment. I just want to share what I found out so far because someone else with more experience might bring this to an end meanwhile.

@Subv I investigated a little more and am now convinced that the path isn't recalculated at every WP. It is only calculated when the creature spawns and at end of path which is fine imo.

I modified my piece of code from above and got the following improvements

• For smooth movement at the last WP I added the first WP after the loop. This might need a check for repeating flag though...
• Flying creatures with inhabitType=5 or 7 now also use catmullrom movement.
• Swimming creatures now use catmullrom movement, too.

--- a/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
+++ b/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
@@ -153,6 +153,17 @@ bool WaypointMovementGenerator<Creature>::StartMove(Creature* creature)
             trans->CalculatePassengerPosition(formationDest.x, formationDest.y, formationDest.z, &formationDest.orientation);
     }

+   if (creature->IsFlying() || creature->HasUnitMovementFlag(MOVEMENTFLAG_SPLINE_ENABLED & MOVEMENTFLAG_CAN_FLY) || creature->HasUnitMovementFlag(MOVEMENTFLAG_SWIMMING))
+   {
+       for (uint32 i = i_currentNode; i < i_path->size(); i++)
+       {
+           init.Path().push_back(G3D::Vector3(i_path->at(i)->x, i_path->at(i)->y, i_path->at(i)->z));
+       }
+       init.Path().push_back(G3D::Vector3(i_path->at(0)->x, i_path->at(0)->y, i_path->at(0)->z));
+        init.SetSmooth();
+       init.Launch();
+       return true;
+   }
     //! Do not use formationDest here, MoveTo requires transport offsets due to DisableTransportPathTransformations() call
     //! but formationDest contains global coordinates
     init.MoveTo(node->x, node->y, node->z);

I got one new issue with flying creatures with inhabitType=5 or 7. They sort of fall and jump through the air. I think this is a core issue because they don't have movementflag MOVEMENTFLAG_FLYING | MOVEMENTFLAG_DISABLE_GRAVITY but MOVEMENTFLAG_SPLINE_ENABLED & MOVEMENTFLAG_CAN_FLY when they actually moving in the air. This needs further research.

[akrom23]

Use init.SetSmooth only for non-flying creatures - for flying you've got init.EnableFlying().

init.Path().push_back(G3D::Vector3(i_path->at(0)->x, i_path->at(0)->y, i_path->at(0)->z));
Use current creature position instead and add this point at the begin of the path.

To call Movement Inform, look at FlightPathGenerator::update and use this code in waypointgenerator code.

[MrSmite]

Nice work.

Would this type of path calculation be able to fix the taxi paths that fly inside Ironforge when they're supposed to go around it (eg: Stormwind to Menethil should not fly inside Ironforge).

I know taxipaths are loaded from the DBC but we should be able to recalculate them. I've tried in the past and only succeeded in either crashing the server or flying through the ground.

[Kittnz]

That's a grid problem @MrSmite if i recall correctly.
Ontopic: I guess this can be used for normal walking creatures too? Ill test tonight!

[MrSmite]

That's a grid problem @MrSmite if i recall correctly.

Nope, it's a waypoint problem. The taxipath that is loaded includes points that go inside IF to the flightmaster where you temporarily land and then take off again for the second half of the flight.

[Shauren]

No, it would not be able to fix taxi paths, there is more going on there - i once wrote a hack for that (it sort of worked but was also crashing, never looked more at that)

[Aokromes]

Yeah i remember that XD

[Pitcrawler]

@akrom23 thanks for the suggestions. But I have some questions about them.

• I think you are speaking about init.SetFly() because there is no init.EnableFlying() at least on my core.
• I don't see the point in adding the current position at the beginning of the spline instead of adding the first WP of the path in the end to get a smooth movement at the end of the spline. Maybe you can explain that?
• Why would I want to call movementInform?

By using SetFly I fixed the strange flying behavior of creatures with inhabitType=5 or 7. Btw creature's movementflags are blizzlike after that change. They were wrong before compared to sniff from 3.3.5.

Unfortunately I found some flying creatures that are not respecting smooth movement at the end of the path when they start to repeat it but others do it like a charm. I don't have any idea about that right now.

[akrom23]

@Pitcrawler:
I think you are speaking about init.SetFly() because there is no init.EnableFlying() at least on my core.
Yes, i meant SetFly

I don't see the point in adding the current position at the beginning of the spline instead of adding the first WP of the path in the end to get a smooth movement at the end of the spline. Maybe you can explain that?
Because in case of only 2 waypoint points creature's spline will have only 1 arg in path and you will have errors in console. Besides, without this, you will have some visual problems - spline always begins with current position. See, how init.Moveto is designed - first point is the current unit position -> the start from where spline is launched.

Why would I want to call movementInform?
See OnArrived code. In some nodes creature stops or do sth.

[MrSmite]

@Shauren Too bad. Maybe it will correct itself in 6.x :)

[Pitcrawler]

I have checked a number of sniffs and haven't found any spline that contains current position of the creature. So I will leave this as it is until somebody can give a proof of this. I think WP scripts are handled via OnArrived() which is called when a WP is reached. So I left this as it is as well.

But I changed the build process of the spline. Now the spline is always build for the whole path and the first WP is then taken from the current WP+1.

I also added the cyclic movement flag when the spline is meant to be repeated which is correct for all the waypoint NPCs. Maybe this check is not necessary.

I had to set speed because when such an NPC is aggroed and it returns back to its homeposition and continues the spline it will use walking speed otherwise.

--- a/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
+++ b/src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
@@ -153,6 +153,21 @@ bool WaypointMovementGenerator<Creature>::StartMove(Creature* creature)
             trans->CalculatePassengerPosition(formationDest.x, formationDest.y, formationDest.z, &formationDest.orientation);
     }

+    if (creature->IsFlying() || creature->HasUnitMovementFlag(MOVEMENTFLAG_SPLINE_ENABLED | MOVEMENTFLAG_CAN_FLY) || creature->HasUnitMovementFlag(MOVEMENTFLAG_SWIMMING))
+    {
+        for (uint32 i = 0; i < i_path->size(); i++)
+        {
+            init.Path().push_back(G3D::Vector3(i_path->at(i)->x, i_path->at(i)->y, i_path->at(i)->z));
+        }
+        init.SetFirstPointId(i_currentNode + 1);
+        if (repeating)
+            init.SetCyclic();
+        init.SetFly();
+        init.SetVelocity(creature->GetSpeed(MOVE_RUN));
+        init.Launch();
+        return true;
+    }
+
     //! Do not use formationDest here, MoveTo requires transport offsets due to DisableTransportPathTransformations() call
     //! but formationDest contains global coordinates
     init.MoveTo(node->x, node->y, node->z);

I couldn't get rid of two minor things:

• When the creature reaches the last WP of the spline it sort of rotates about half a second or so and then starts a new round of the spline as suppossed. I can only describe the behavior in a very poor way. I'd be best to see it for yourself.
  I tracked this down to the function MoveSpline::UpdateResult MoveSpline::_updateState(int32& ms_time_diff). I think the handling of cyclic splines needs to be improved there. As far as I understand it the first WP of the spline is loaded when the last one is reached and at the next update the new cycle is started. Possibly this takes too long so the creature doesn't know where to go between two updates. But this can be completly wrong as I already said that I have only very poor core knowledge.
• After returning to its homeposition the creature also rotates about half a second before continuing the spline.

It would be nice if @Shauren or @Subv had the time to say something about that or even fix it.

[Shauren]

creature->HasUnitMovementFlag(MOVEMENTFLAG_SPLINE_ENABLED & MOVEMENTFLAG_CAN_FLY) this is always false. (MOVEMENTFLAG_SPLINE_ENABLED & MOVEMENTFLAG_CAN_FLY = 0, HasUnitMovementFlag(0)... go figure)

[Pitcrawler]

Well, that is really embarrassing even for me...
But I added this check, which should have been with bitwise OR, because of those creatures with inhabitType=5 or 7. I changed it above.

The best solution to this would be when they get MOVEMENTFLAG_DISABLE_GRAVITY as soon as they are in the air. Currently they just get MOVEMENTFLAG_CAN_FLY. I think this not correct compared to sniffs.

[Shauren]

MOVEMENTFLAG_DISABLE_GRAVITY and MOVEMENTFLAG_CAN_FLY are exclusive with each other, disable is set for inhabit = 4, fly for 4 | 1

[Kittnz]

Is anyone still into this?

[jackpoz]

any update on this ?

[Kittnz]

No one is working on it, hope someone picks it up.

[click]

Ok, let's continue on this one ...

Single pathing :
Example with 20 "real" waypoints:
1-2-3...18-19-20-[21]
[21] = 20 - describes that we're on the end of a single-run path.

Cyclic pathing :
Example with 20 "real" waypoints:
[1-2-3]-4-5-6-7....18-19-20-[21-22-23]
[21-22-23] = [1-2-3] - describes that we are on a cyclic path.

(Courtesy of Malcroms research on the issue).

[MrSmite]

Question: After a path is generated would it make sense to save it to the DB (since the path should technically never change)?

[malcrom]

Isn't the client doing the smoothing? we have many catmullrom paths in waypoint_data the same as offi sends to client. We are just sending them a point at a time.

[click]

@MrSmite : no, the path is actually picked up FROM the database, core just need to know the criterias above.
Final point twice = Full endpoint
First three repeated as last three = cyclic
The client most probably also knows about this criteria, so it can interpolate correctly clientside.

@malcrom: The client does the smoothing based on standard Catmullrom definitions and us shipping the right info to it. Which we obviously don't per today.

[MrSmite]

@click Ah, ok. I thought the goal was to calculate a path for use with mmaps to replace the waypoints we already had.

[Aokromes]

If the fix here is valid, plz open a PR.

[Riztazz]

We took a crack at it and this is what we came up with. Thank you dude who doesnt want to be named for your help

There is few issues which sadly i have no idea how to fix.

Issue 1:
https://youtu.be/8Pbq1BdkmyU
You can see at the end of path how npc slides on the ground before it turns

issue 2:
https://youtu.be/UdHsr3r6Hm0

Again, no clue why this happens, probably because a lot of waypoints are stacked on top of each other when we extract them via waypointCreator( http://i.imgur.com/S7ffv0V.png visualization of that npc path), or issue with spline implementation(doubt it ;p)

Also code itself would probably need some refactoring to fit tc standards, sadly i do not have time to work on that anymore so here's the code and i hope someone, one day will pick it up and finish this or get some cool idea how to to write it in a better way :P

code: http://paste2.org/ZDKwEGFB

[Aokromes]

15$ bounty added here.

[MrSmite]

https://youtu.be/8Pbq1BdkmyU

This technically isn't a slide but rather a version of the Wagon Wheel Effect. The NPC changes her facing direction before the final waypoint. In doing so her walk animation appears to be going against the direction of travel (she's still traveling left but walking right), causing the "slide" visual.

Somewhere the code is altering her facing info before the path completes.

https://youtu.be/UdHsr3r6Hm0

Same issue here.

[Kittnz]

points with facing in should not be using catmullrom

[Riztazz]

MrSmite, sounds legit - ill take a look when i have few mins

[Kittnz]

I guess it depence in what case, but a lot of times you see npc's reach a point and then keep standing on that same spot and change facing. but it could have catmullrom too i guess

[MrSmite]

but a lot of times you see npc's reach a point and then keep standing on that same spot and change facing

Yes, that's what I think is expected behavior. For some reason though it seems that the waypoint prior to the final one causes the NPC to change her facing, at least in the two videos supplied:

wp1........wp2.....wp3......wp4.........wp5.....wp6
facing-> walk -> walk -> walk -> <-facing walk ->

[Shauren]

I see the mistake in that code
for (auto i = i_currentNode; i < end; ++i)
end is currently assigned to i_path->size() but you cant put in the ENTIRE path to spline, you have to stop adding nodes at the first point with delay after i_currentNode and only call init.SetFacing(node->orientation) if the LAST point you are adding to points sent to client has orientation, not the first one
Additionally (creature->CanFly() || creature->IsFlying()) ? init.SetFly() : init.SetSmooth(); is horrible, write if properly
Do not call SetSmooth() on ground splines, there is no need for that, client will cheat here and make it smooth anyway (and isnt blizzlike)

[funjoker]

https://www.bountysource.com/issues/5546590-dev-wip-catmullrom-waypoint-movement-15-bounty

[funjoker]

Bountysource do something xD

[Aokromes]

i mailed them, still waiting for answer.

[Keader]

Fix reverted in a3c6880579f3326088ecbe5b8c08c4b75ed91a59 so... reopen this

[jackpoz]

lul the bounty was awarded

[robinsch]

ez money

[funjoker]

Well xD RIP my money, it was fun. :P