Page MenuHomeWildfire Games

Unit Motion - account for entity moving out of world
ClosedPublic

Authored by wraitii on Jun 15 2019, 7:52 PM.

Details

Reviewers
minohaka
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22415: Unit Motion - Stop when targets have an invalid position.
Summary

Previously, unitMotion had no code that checked particularly if the target was still in the world.
When the target moved out of the world, unitMotion would follow the path to its last known position, then send a "MoveSucceeded" message because the "at destination" check would do that in the situation where the target was out of the world (because MoveToTargetRange fails).

rP22364 moved code around and, in particular, no longer called "MovementSucceeded" when PossiblyAtDestination returned true (that must have been a mistake from a poor rebase). - this was a bug -
rP22365 immediately fixed that state of affair by calling MovementSucceeded() when calling PossiblyAtDestination. This returned unitAI to its A23 state.

rP22366 subsequently changed "PossiblyAtDestination" logic, in such a way that when the target of unit motion is moved out of the world, instead of returning true, it returns false (which is internally logical, but with consequences):

This means that unitMotion will no longer send messages if the target is moved out of the world. Thus unit will follow its path to its last waypoint and stay there in its current state, unable to carry on.

This was missed in rP22366, and thus requires fixing.

This fixes that problem by explicitly checking . If ComputeTargetPosition become computationally expensive, it might be worth doing another check.

Furthermore, this still changes behaviour from A23 (the entity will stop right where it is) - requiring D1992 to re-introduce A23 behaviour.

Test Plan

Check that entities behave sanely when the target disappears.

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

wraitii created this revision.Jun 15 2019, 7:52 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1730/display/redirect

Yeah I think the code "handling" this before was this line which happened when we reached the end of our path.

minohaka accepted this revision.Jun 15 2019, 10:08 PM
minohaka added a subscriber: minohaka.

After testing the the issue is fixed

This revision is now accepted and ready to land.Jun 15 2019, 10:08 PM
wraitii added inline comments.Tue, Jun 18, 11:14 AM
source/simulation2/components/CCmpUnitMotion.cpp
823 ↗(On Diff #8488)

Probably not the best behaviour in light of D1992, what I think should be done instead is:

  • If the move request hasn't changed (i.e. we didn't stop) - drop all waypoints, since that's the only sane behaviour.
  • if the move request has changed, and we're now computing a new path, keep waypoints so it doesn't look like we're stopping for one turn.
wraitii updated this revision to Diff 8591.Mon, Jun 24, 12:25 PM

Do what I say above.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1802/display/redirect

wraitii updated this revision to Diff 8593.Mon, Jun 24, 5:28 PM

Actually working correctly this time.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1804/display/redirect

Actually working correctly this time.

What exactly needs to happen for these fixes to land in the public repo? It seems a bunch are testing and get turn off when the unit motion doesn't work as expected?

wraitii added a subscriber: elexis.Mon, Jun 24, 7:44 PM

Actually working correctly this time.

What exactly needs to happen for these fixes to land in the public repo? It seems a bunch are testing and get turn off when the unit motion doesn't work as expected?

I am waiting on @elexis to complete an audit of my patches.

I think at some point in the rewrite the code that "handled" the target moving out of world got squished

Which is?

This comment was removed by wraitii.
wraitii edited the summary of this revision. (Show Details)Wed, Jun 26, 7:33 PM
wraitii updated this revision to Diff 8657.Sun, Jun 30, 5:38 PM

Introduce a function, simply logic a bit.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1853/display/redirect

This revision was automatically updated to reflect the committed changes.