Paths

Table of Contentst

Differential D1089

Consider that the point could be inside the goal area in some PathGoal functions
ClosedPublic
Actions

Authored by temple on Nov 30 2017, 9:04 PM.

Details

Reviewers

wraitii

Commits

rP20878: Consider that the point could be inside the goal area in some PathGoal functions

Summary

Sometimes units inexplicable run away from their targets before attacking.

What happens in this case is in unit motion they TryGoingStraightToTargetEntity where they then find NearestPointOnGoal. However, that only selects points on the boundary of the goal area and doesn't take into account that the unit might already be inside of it, so instead of just stopping they turn around and run away from the target to get to that "nearest" point.

If some part of the code wants to avoid some area then they should be using the inverted goal type.

History at rP7484 as well as rP8751 for TryGoingStraightToTargetEntity and rP16751 for inverted goals.

Test Plan

Check that there shouldn't be problems with the new behaviors of the two functions (used in CCmpUnitMotion.cpp and CCmpPathfinder_Vertex.cpp).

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

temple created this revision.Nov 30 2017, 9:04 PM

Owners added a subscriber: Restricted Owners Package.Nov 30 2017, 9:04 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Harbormaster completed remote builds in B3901: Diff 4471.Nov 30 2017, 9:17 PM

Code looks sane, but do you have a commands.txt which reproduces the problem for test.

source/simulation2/helpers/PathGoal.cpp
350 ↗	(On Diff #4471)	d is not used for POINT which is the majority of cases i think. I'd rather move that line later, even if that mean writing it twice.
source/simulation2/helpers/PathGoal.h
75 ↗	(On Diff #4471)	Strange naming! IsPointInsideGoal or GoalContainsPoint would be better

In D1089#43327, @mimo wrote:

Code looks sane, but do you have a commands.txt which reproduces the problem for test.

2017-11-30_0002 -- svn pathgoal.zip5 KBDownload

Entities 201 at 0:20, 200 at 0:26, 476 at 1:13 and 1:54.

source/simulation2/helpers/PathGoal.h
75 ↗	(On Diff #4471)	Agree, but I was following the other functions (which should instead be things like NavcellOverlapsGoal or NavcellIntersectsGoal).

In D1089#43379, @temple wrote:

In D1089#43327, @mimo wrote:

Code looks sane, but do you have a commands.txt which reproduces the problem for test.

2017-11-30_0002 -- svn pathgoal.zip5 KBDownload

Entities 201 at 0:20, 200 at 0:26, 476 at 1:13 and 1:54.

Thanks, i'll do test it soon.

source/simulation2/helpers/PathGoal.h
75 ↗	(On Diff #4471)	I think that new added functions should have a meaningful name from which we can easily guess what the function does (and not say the contrary of what it does).

wraitii added a subscriber: wraitii.Dec 2 2017, 10:57 AM

wraitii added inline comments.

source/simulation2/helpers/PathGoal.h
75 ↗	(On Diff #4471)	Agreed.

temple added inline comments.Dec 2 2017, 3:46 PM

source/simulation2/helpers/PathGoal.h
75 ↗	(On Diff #4471)	And what about old functions? :)

wraitii added inline comments.Dec 3 2017, 9:32 AM

source/simulation2/helpers/PathGoal.h
75 ↗	(On Diff #4471)	Make a separate patch or no don't change them imo.

Add some tests for this and I think it's committable.

source/simulation2/helpers/PathGoal.cpp
322 ↗	(On Diff #4471)	I'm not sure that calling this actually makes it faster. If the result doesn't change here, I'd rather just remove it, but it should be tested.
350 ↗	(On Diff #4471)	I doubt it matters much, this isn't atomic so basically any competent compiler will move it to where it's relevant. Not sure if it's better to write it above or not though.

This revision now requires changes to proceed.Dec 3 2017, 10:47 AM

temple added inline comments.Dec 3 2017, 4:14 PM

source/simulation2/helpers/PathGoal.cpp
322 ↗	(On Diff #4471)	? It makes it correct. Or do you mean to remove the `PointContainsGoal` function and instead put the logic inside each of `DistanceToPoint` and `NearestPointOnGoal`? Yeah, maybe that's the better thing to do.

Removed PointContainsGoal, added tests.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Harbormaster completed remote builds in B3958: Diff 4552.Dec 5 2017, 4:13 AM

Tests are good, code looks good.

source/simulation2/helpers/PathGoal.cpp
320 ↗	(On Diff #4552)	This runs the risk that we'll want to fix DistanceToSquare to return 0 when inside, but otherwise I'm OK with it (and the new tests would catch that)

This revision is now accepted and ready to land.Dec 5 2017, 10:21 AM

I don't know much about optimizing at this level, e.g. CFixedVector2D d = pos - g; versus CFixedVector2D d(pos.X - x, pos.Y - z);, so feel free to change things.

source/simulation2/helpers/PathGoal.cpp
320 ↗	(On Diff #4552)	DistanceToSquare has an option to countInsideAsZero, but I didn't use that because it doesn't have a countOutsideAsZero option and NearestPointOnSquare doesn't have either of them, so of the four square cases it'd only affect one of them (L312-313 becomes `return Geometry::DistanceToSquare(d, u, v, halfSize, true);`). And I thought it was better to have the code read consistently, especially since `PointIsInSquare` seems cheap. But you can change it if you'd like.