[WIP] Core/Movement: Targeted MoveGen Improvements #17850

chaodhib · 2016-08-23T18:31:34Z

Work in progress PR

Planned changes:

Major changes:

Changing how the way the destination point of each NPC path is determined. Up until now, each time a creature had to move, first a destination point was computed. And only then a path leading to that point was generated. Now it's the other way around. The destination point is actually deduced from the path itself. We first generate a path to the target's location and then remove from the path a certain length: the distance at which the npc should stand.

In the only commit yet, this is already implemented but only for MoveChase. Also, ReducePathLenghtByDist needs to be improved in order to make sure that after that method call, the last point of the path is always in LoS of the target & at the right distance.

Minor changes:

From sniff analysis, I found that the standard distance at which NPCs stand after chasing another unit is 'Chaser->CombatReach + Target->CombatReach' instead of 'Chaser->CombatReach + Target->CombatReach + 0.5'.
From sniff analysis, I believe pets follow their masters at a 3 constant yard distance.
Planning to remove the third parameter of MoveChase (the angle).

Target branch(es): 3.3.5

Issues addressed: Seems to at least
fix #16549
fix #9206
fix #16647

Tests performed: WIP. Build and tested IG for what I have so far.

Treeston · 2016-08-23T20:13:51Z

This logic is really interesting. @r00ty-tc I know you'll appreciate.

Aokromes · 2016-08-24T15:39:01Z

/home/travis/build/TrinityCore/TrinityCore/src/server/game/Movement/PathGenerator.cpp:933:17: fatal error:
comparison of unsigned expression >= 0 is always true
[-Wtautological-compare]
for ( ; idx >= 0; idx--)
~~~ ^ ~

Treeston · 2016-08-24T15:39:45Z

Naughty infinite loop.

ghost · 2016-08-24T17:28:04Z

src/server/game/Movement/PathGenerator.cpp

+    uint32 idx = _pathPoints.size() - 2;
+    float totalLengthSoFar = 0.0f;
+    G3D::Vector3 diffVec;
+    uint32 newPathSize;


C:\projects\trinitycore\src\server\game\Movement\PathGenerator.cpp(932): error C2220: warning treated as error - no 'object' file generated [C:\projects\trinitycore\build\src\server\game\game.vcxproj] C:\projects\trinitycore\src\server\game\Movement\PathGenerator.cpp(932): warning C4101: 'newPathSize': unreferenced local variable [C:\projects\trinitycore\build\src\server\game\game.vcxproj]

chaodhib · 2016-08-27T22:51:17Z

First version ready for testing. Only creatures chasing another unit work for now (to be more precise, only MoveChase without offset has been implemented. MoveChase with offset and MoveFollow as a whole are broken for now).

There is probably a lot more work to do but for now it looks stable and it should fix A LOT of movement related issues. At least the first 3 issues exposed here. Maybe more.

To summarize, this PR should fix:

chaodhib · 2016-08-28T20:18:57Z

@Treeston @r00ty-tc @jackpoz could you let me know what do you think about this PR when you find time plz?

jackpoz · 2016-08-29T14:42:09Z

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

@@ -106,6 +107,7 @@ void TargetedMovementGeneratorMedium<T, D>::_setTargetLocation(T* owner, bool up
        && owner->HasUnitState(UNIT_STATE_FOLLOW));

    bool result = i_path->CalculatePath(x, y, z, forceDest);
+    i_path->ReducePathLenghtByDist(owner->GetObjectSize() + i_target->GetObjectSize() + CONTACT_DISTANCE, i_target->ToUnit()); // this distance is correct only for MoveChase with no offset


there is little point reducing the path length before checking the result in the lines below (also it's length, not lenght)

ReducePathLenghtByDist also valide the last point of the path and if no right point is found in range and in los of the target, the path is set to flagged as unusable. This is why. (regarding the spelling, i didn't create the method. i just rewrote it, but yeah i will fix that)

there's no point to set the path as unusable (PATHFIND_NOPATH) if the path is already unusable, so if i_path->GetPathType() & PATHFIND_NOPATH is already true before this call.

jackpoz · 2016-08-29T14:47:35Z

could you describe more in detail how pathfinding calls are handled in this PR, in particular the "Now it's the other way around" part ? Does this change very little how and when pathfinding is used or does it change some things we should check (i.e. more/less queries, more/less detailed queries, etc).

jackpoz · 2016-08-29T14:52:22Z

src/server/game/Movement/PathGenerator.cpp

@@ -904,7 +904,7 @@ float PathGenerator::Dist3DSqr(G3D::Vector3 const& p1, G3D::Vector3 const& p2) c
    return (p1 - p2).squaredLength();
 }

-void PathGenerator::ReducePathLenghtByDist(float dist)
+void PathGenerator::ReducePathLenghtByDist(const float dist, Unit* target)


It would be nice if target wouldn't be mandatory (right now the code crashes with NULL dereference exception of the caller passes NULL since there's no validation at all), PathGenerator is not supposed to always have a Unit as target.

this method is called for now only at 2 points and for each of those cases, if i'm not mistaken, unit will never be null.

if it will never be null then add an ASSERT( )

please add an ASSERT(target); here

Treeston · 2016-08-29T14:53:23Z

Current logic:

Take a straight line from self to target
Shorten this line by offset
Take the resulting point, and generate a path to it

New logic:

Generate a path from self to target
Shorten this path by offset

jackpoz · 2016-08-29T14:53:57Z

Should fix quite a lot of issues related to NPC movements

could you provide test cases with previous and new behavior ?

Treeston · 2016-08-29T14:55:24Z

I haven't tested, but this should fix any issue where NPCs would path to just below you, then be unable to attack you due to range.

Ex.: Central storm peaks, stand on the rocks and pull a Magnataur.

jackpoz · 2016-08-29T15:00:56Z

src/server/game/Movement/PathGenerator.cpp

+    if (!IsValidFinalPoint(_pathPoints[_pathPoints.size() - 1], target, dist))
+    {
+        // even the last position of the generated path, which should be close if not on top of the target, was not good enough. we mark the path as unusable.
+        _type = PATHFIND_NOPATH;


this code doesn't handle PATHFIND_INCOMPLETE/PATHFIND_SHORT cases (and it's still to be defined after which distance it should be set to PATHFIND_INCOMPLETE)

jackpoz · 2016-08-29T15:23:28Z

Take a straight line from self to target

isn't the current 1st step "find a point close to target" https://github.com/TrinityCore/TrinityCore/pull/17850/files#diff-268a0e4eced8ee7bb5139f6c2b47034cL61 ?
what's the difference between "find a point close to target then walk to the point" to "walk to target then find a point close to target" ? Just the angle between where the unit is walking from, the target and when the unit will stand when it stops ?

Basically your code is trying to fix

TrinityCore/src/server/game/Entities/Object/Object.cpp

Line 2641 in e405ac5

// still not in LoS, give up and return first position found

case , shortening the path instead of finding another position at same distance but at different angle ?

Treeston · 2016-08-29T15:31:27Z

Example: Target is standing on a hill, with a path leading up the side.

Current behavior:

New behavior:

jackpoz · 2016-08-29T16:00:01Z

2nd SS looks better, I think a 3rd SS with both "try different angles and try to shorten the path" will get the best of both.
Keep in mind in the worst case the end point of the path is not in LoS with the target and reducing the path might not always give a point in LoS (in corridor/wall with enemy being the end of the wall but close to the angle)

chaodhib · 2018-01-04T00:36:43Z

In simple terms, in order to attack its target, a NPC needs to move closer to its target until the distance between itself and its target is below a known value (the combat reach, see note 1 below). So, what we do now is:

Find a walkable path from the point A (current position of the mover) to point B (position of the target), using Detour. It's worth repeating that here, we are only thinking in terms of 2 points and a path between those two points. No notion of unit with a certain combat reach or anything.

If we were to stop here and simply ask the NPC to follow this path, then at the end of the path, the NPC will be exactly on its target position. This is obviously overkill because the NPC doesn't need to get as close. Each NPC has a certain a certain range. Getting close enough to the target is what we need.

In this step, we simply "cut" from the path an excess distance: the combat reach. We can cut this extra distance because the NPC does not need to travel further in order to attack its target from there.

After that, we are done. The location pointed by the orange arrow will be our final position after moving. And the orange arrow will be our path. The final position satisfy our problem: a) we moved the least amount of distance. b) we are now close enough to our target in order to attack it.

Note:

I made a simplification regarding what i called the combat reach. Its value actually depends on the characteristics of both the mover and the target. The formula is in the code somewhere (and uses a field actually called "combat reach"). But this detail is irrelevant to the topic.
The example above explains the basic and most simple situation. The more general solutions takes also care of a few issues with this method (line of sight, ..).

Treeston · 2018-01-04T00:47:10Z

OK - that's exactly what I wanted. 👍 from me.

jackpoz · 2018-01-04T09:10:02Z

as suggested at #17850 (comment) , I think a mix of both New and Angle solutions should be implemented, in particular using your New method first, check LoS with target and then apply Angle to find a point in LoS (the way angles are chosen could be changed to jump from a side to the other instead of checking them like around the clock). I'm not sure if a 2nd path should be calculated to reach the new point, maybe just from the last point of New.

I'd suggest to merge this PR with the above suggestion first, then we can check how to modify Detour (what would you like to modify in Detour btw ?)

chaodhib · 2018-01-05T12:27:03Z

Could you describe as clearly as possible a situation where using a combination of the New method and the Angle method would be more suitable than just the New method?

the way angles are chosen could be changed to jump from a side to the other instead of checking them like around the clock

Indeed. It would be an improvement compared to the current way of doing. Nonetheless, it's still asymmetrical. And therefore, some situations will be handled awkwardly, depending on if the NPC approaches by the left or by the right (like in the blade's edge arena, when you go up on the platform. i will provide more details later).

what would you like to modify in Detour btw ?

Detour allows to find a walkable path from point A to point B. Ideally, the concept of LoS and combat reach should be integrated into Detour as well We don't need to move to a certain point. We need to move close enough and in LoS of this point. It would solve the situation mentioned above.

jackpoz · 2018-01-05T12:57:39Z

Could you describe as clearly as possible a situation where using a combination of the New method and the Angle method would be more suitable than just the New method?

Imagine an enemy hiding behind a crate: the New method finds a path (that would walk throught the crate), reduces the length by the combat reach and sets the end point in front of the crate with the enemy not in LoS.
At this point, checking for other points in a circle around the enemy allows to find a point in LoS.

Rochet2 · 2018-01-05T13:17:26Z

Would it be more efficient and good enough to walk along the originally calculated path rather than calculate the path to different spots around the target?

For example calculate path to target (use the new method proposed). Cut it by the combat reach. Check LOS at that point. Reduce the cut by X. Check LOS at that point. Reduce the cut by X. Repeat reducing cut until our cut is 0.
This way we calculate just one route and check LOS cut_portion/X times at maximum.

Sorry if this was somehow not possible. Not informed on the pathfinding. :/

Treeston · 2018-01-05T13:28:07Z

@jackpoz detour would route you around the crate, no?

chaodhib · 2018-01-05T14:50:55Z

@Rochet2 Yes. I didn't mentioned it in the example above because i wanted to keep it simple. But yes, my algorithm works exactly the way you described.

@jackpoz I agree with @Treeston. Detour will find a path that will go around the obstacle. Unless the crate is a DynObj. In which case, the problem is the same if you increase the size of the crate. The Angle approach would not help. A true solution to this situation would be to support Dynamic Objects in the pathfinding. Which is an entirely different topic.

jackpoz · 2018-01-05T18:47:13Z

If you think Detour will find a path around a GameObject then you should go back studying how TC uses Recast and Detour.
How does the Angle approach not solve the crate issue, no matter what the size is ? Any of the points I highlighted in the screenshot above will be valid no matter how big the crate is (since obviously the crate can't grow on the upper side where there's a NPC standing there, NPC can't be inside a crate).

chaodhib · 2018-01-05T18:58:38Z

If you think Detour will find a path around a GameObject then you should go back studying how TC uses Recast and Detour.

I said exactly the opposite. In your previous post, you didn't specify if the obstacle was a GO or not (sorry, in my previous post, by dynobj i meant GO). So there are 2 cases:

If the obstacle is not a GO, Detour will find a path around. --> there is no issue.
If the obstacle is a GO, it's a whole different problem. As you know, GO are not taken into account by Detour. And the solution is not to use the Angle approach. But to fix this. I think the Angle approach is too unreliable in order to be a good solution, even if it is used in combination with the New approach.

The Angle approach doesn't completely solve the GO issue on its own. At the same time, along with it comes a lot of issues because of its unreliability (point under the map, asymmetrical, ...). Therefore, I don't think it's a good approach.

ccrs · 2018-01-06T02:04:23Z

And dont forget about slopes.

Path calculated -> path reduced -> movegen checks real distance -> recalculates path (loop)
Plus, probably the creature wont even be on melee range

jackpoz · 2018-01-06T09:13:45Z

you didn't specify if the obstacle was a GO

Imagine an enemy hiding behind a crate

really ? why do you think I chose a crate ? it was obviously a GO.

along with it comes a lot of issues because of its unreliability (point under the map, asymmetrical, ...)

which point under map ? which asymmetrical ? where are all these "lot of issues" ?

Why is it so hard for you to just implement a part of algo that fixes some corner cases that your algo totally ignores ? are we discussing about philosophy here or are we trying to improve TC ? Just add it at the end so we can merge this PR and move on.

jackpoz · 2018-01-06T11:54:38Z

@Rochet2 Yes. I didn't mentioned it in the example above because i wanted to keep it simple. But yes, my algorithm works exactly the way you described.

@chaodhib does that means the cutting will notice that the target is not in LoS in my crate example and as result make the creature move to the exact location as the target ? that would mean the creature will be able to attack it and will cause pretty much no issue. If this is the case then this is how your New algo handles the crate case and it looks good to me. Please confirm if that's the case, then we can merge it.

chaodhib · 2018-01-06T17:16:53Z

Yes, in worst case scenario, the creature will move to the exact location of the target. It will cut less and less until it finds a position where the creature is both close enough and in LoS of the target. And that point can be the position of the target itself.

Treeston · 2018-01-06T17:24:49Z

Out of curiosity, does this do anything to address #20173 too?

ccrs · 2018-01-06T17:47:35Z

it should

chaodhib · 2018-01-06T18:15:10Z

No it doesn't. Last time I checked, that issue was relatively simple to solve (I could be mistaken though). I will take a look again when I can.

EDIT: Indeed. I mentioned the issue HERE in the second point. The problem is this value. It's too high.

Will submit PR when I can.

jackpoz

Couple of minor things and we can merge this one

chaodhib · 2018-01-07T15:40:48Z

Besides your 2 points @jackpoz, there are remaining things to implement before this PR can be merged: support of MoveChase with offset. And support of MoveFollow.

I'll get into it as soon as i can.

Aokromes · 2018-03-11T00:28:15Z

Conflicting files
src/server/game/Movement/MovementGenerators/TargetedMovementGenerator.cpp

jackpoz · 2019-03-12T19:19:26Z

@chaodhib , do you have any plan to continue working on this ?

chaodhib · 2019-03-12T20:39:13Z

@jackpoz At the moment, I'm focused on other PRs. I still believe the idea behind this one is sound though. Maybe I'll get back to it in the future. Until then, if someone wants to pick up where I left off and work on this, I'm more than happy to give the reins. Help is welcome.

ghost · 2019-03-12T20:53:40Z

Add the label [help wanted] , maybe?

chaodhib · 2019-03-12T20:58:52Z

Sure

jackpoz · 2019-07-02T08:21:52Z

Closing this PR as it seems abandoned. Feel free to ask here to reopen it if anyone would like to work on it.

ghost added Feedback-PatchFix Comp-Core Branch-3.3.5a labels Aug 23, 2016

ghost changed the title ~~Targeted MoveGen Improvements~~ [WIP] Core/Movement: Targeted MoveGen Improvements Aug 23, 2016

Aokromes added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Aug 24, 2016

ghost reviewed Aug 24, 2016
View reviewed changes

chaodhib force-pushed the targeted_movegen_improvements branch 2 times, most recently from 41af431 to d571172 Compare August 27, 2016 23:15

This was referenced Aug 28, 2016

Core/Movement: Improve unit chase and follow in 3D #17787

Closed

Core/Movement: Refactoring of TargetedMovementGenerator #17786

Closed

jackpoz reviewed Aug 29, 2016
View reviewed changes

chaodhib mentioned this pull request Jan 7, 2018

Core/Movement: Correct the allowed distance to target before a repositioning is necessary. #20173 #21193

Merged

3 tasks

jackpoz requested changes Jan 7, 2018

View reviewed changes

chaodhib mentioned this pull request Feb 28, 2018

Naxxramas: Maexxna pathfinding issue #21480

Closed

chaodhib added the help wanted label Mar 12, 2019

jackpoz added the Closed-Abandoned label Jul 2, 2019

jackpoz closed this Jul 2, 2019

[WIP] Core/Movement: Targeted MoveGen Improvements #17850

[WIP] Core/Movement: Targeted MoveGen Improvements #17850

Conversation

chaodhib commented Aug 23, 2016 • edited

Work in progress PR

Planned changes:

Treeston commented Aug 23, 2016

Aokromes commented Aug 24, 2016

Treeston commented Aug 24, 2016

Choose a reason for hiding this comment

chaodhib commented Aug 27, 2016 • edited

chaodhib commented Aug 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackpoz commented Aug 29, 2016

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 Aug 29, 2016

jackpoz commented Aug 29, 2016

Treeston commented Aug 29, 2016

jackpoz Aug 29, 2016 • edited

Choose a reason for hiding this comment

jackpoz commented Aug 29, 2016 • edited

Treeston commented Aug 29, 2016

jackpoz commented Aug 29, 2016 • edited

chaodhib commented Jan 4, 2018 • edited

Treeston commented Jan 4, 2018

jackpoz commented Jan 4, 2018

chaodhib commented Jan 5, 2018 • edited

jackpoz commented Jan 5, 2018

Rochet2 commented Jan 5, 2018 • edited

Treeston commented Jan 5, 2018

chaodhib commented Jan 5, 2018

jackpoz commented Jan 5, 2018

chaodhib commented Jan 5, 2018 • edited

ccrs commented Jan 6, 2018 • edited

jackpoz commented Jan 6, 2018

jackpoz commented Jan 6, 2018

chaodhib commented Jan 6, 2018 • edited

Treeston commented Jan 6, 2018

ccrs commented Jan 6, 2018

chaodhib commented Jan 6, 2018 • edited

jackpoz left a comment

Choose a reason for hiding this comment

chaodhib commented Jan 7, 2018 • edited

Aokromes commented Mar 11, 2018

jackpoz commented Mar 12, 2019

chaodhib commented Mar 12, 2019

ghost commented Mar 12, 2019

chaodhib commented Mar 12, 2019

jackpoz commented Jul 2, 2019

chaodhib commented Aug 23, 2016 •

edited

chaodhib commented Aug 27, 2016 •

edited

jackpoz Aug 29, 2016 •

edited

jackpoz commented Aug 29, 2016 •

edited

jackpoz commented Aug 29, 2016 •

edited

chaodhib commented Jan 4, 2018 •

edited

chaodhib commented Jan 5, 2018 •

edited

Rochet2 commented Jan 5, 2018 •

edited

chaodhib commented Jan 5, 2018 •

edited

ccrs commented Jan 6, 2018 •

edited

chaodhib commented Jan 6, 2018 •

edited

chaodhib commented Jan 6, 2018 •

edited

chaodhib commented Jan 7, 2018 •

edited