Paths

Table of Contentst

Diffusion 0 A.D. Public Repository rP22313

Unit Motion: MoveTo family of function no longer returns false if the move is…
Needs VerificationrP22313
Actions

Description

Unit Motion: MoveTo family of function no longer returns false if the move is un-necessary, instead unitAI checks explicitly.

This also moves the actual "moving" code to states instead of orders, making states more self-contained and removing the change of errors when cleaning up a state.

Differential Revision: https://code.wildfiregames.com/D1865

Details

Auditors

Committed

wraitii

May 28 2019, 1:38 PM

Differential Revision

D1865: Unit Motion / AI - check ranges explicitly, move in the the State handlers instead of the orders.

Parents

rP22312: Don't clear pathnames in vfs::GetPathnames so it can be called several times.

Branches

Unknown

Tags

Unknown

Build Status

Buildable 7647
Build 12470: Post-Commit Build	Jenkins

Event Timeline

wraitii committed rP22313: Unit Motion: MoveTo family of function no longer returns false if the move is….May 28 2019, 1:38 PM

wraitii mentioned this in D981: Make commutative behaviour in the IsInRange function family and migrate them to a better home.May 28 2019, 1:41 PM

Harbormaster completed building B7647: rP22313: Unit Motion: MoveTo family of function no longer returns false if the move is….May 28 2019, 2:10 PM

Silier added a subscriber: Silier.May 30 2019, 4:58 PM

Silier added inline comments.

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
1852	should not be here checkTargetAttackRange instead move ?

While investigating an error messages found two issues in this commit, see the inlines.

I also guess that this commit made that error message pop up, since we moved some fleeing code from the order to the state. However the two inlines don't fix the issue. What caused the error message is that, a unit attacked a female repairing a building and with that promoted, thus moved out of world (and a new entity was created, but that isn't passed on to the female) and so the female couldn't flee anymore (the attacker is out of world). However we still go in the Fleeing state (as off this commit), then when we return to the repairing state (via the finishOrder in the fleeing.enter). Then somehow (that part I don't entirely understand) we get a onMotionChange on the same turn and end up in Fleeing.MotionComplete, causing another finishOrder, which results the unit going towards the IDLE state without stopping the timer from the repairing state. This will then throw and error since we can't have two timers (see startTimer).

To reproduce:

Have some females (or other passive units) building/repairing/gathering something
Get a unit which will promote with 1 hit
attack the female (so the attacker will promote on his first hit)

See the attached replay (notice in the same replay that rather often units end up on maxRange+epsilon, letting them "idle", not sure if that is this commit, but certainly needs investigation)

commands.txt67 KBDownload

Obviously it is wrong that the female isn't fleeing (which has an easy fix btw), but that doesn't fix this commit: We should always be able to reject a fleeing call (for whatever reason, in this case because the attacker is out of world) and not error out.

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
1713–1714	that maxrange is rather different...
2748	Don't understand why we need this call, though I see it in the old code too
/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
1647	How does that comment comply with the returned value below?

This commit now has outstanding concerns.May 30 2019, 5:28 PM

bb mentioned this in rP20393: Move out of world promoted, packed and upgraded entity as they are not….May 30 2019, 5:36 PM

Silier added inline comments.May 31 2019, 7:11 PM

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
1515	We should not select idle animation when we do not know that we are idle, even after this will be selected another animation, this idle is still noticeable and it looks weird

Then somehow (that part I don't entirely understand) we get a onMotionChange on the same turn and end up in Fleeing.MotionComplete

Probable explanation: the move requests a path, that path is computed on the same turn, the path does not exist (since the unit is out of the world), so the "start" fails, and this triggers the motion complete hook.

The "start" messages are removed by D1885 to avoid these kind of issues, since they aren't really useful anyways.

The fact that we're still in the fleeing state is probably that I don't return true when fleeing, that's a mistake on my part.

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
1515	True, this is fixed downstream by removing these calls from unitAI so I won't bother with a specific fix.
1713–1714	Not sure what you mean, it's the same as the old code (L403 of this file).
1852	It's done in L1831
2748	We don't really but it makes some sense, the unit that enters REPAIRING may still be walking so we tell it to stop.
/ps/trunk/source/simulation2/components/CCmpUnitMotion.cpp
1647	This should indeed return true.

wraitii mentioned this in D1947: Resolve issues with rP22313 - unitAI order cleanup and unitMotion MoveTo change.Jun 3 2019, 8:46 PM

wraitii mentioned this in rP22339: Resolve issues with rP22313 - unitAI order cleanup and unitMotion MoveTo change.Jun 5 2019, 7:28 PM

wraitii mentioned this in D1956: Additional fixes following rP22313 - units no longer return to gather nearby resources when depleting a chicken/tree/....Jun 5 2019, 9:04 PM

wraitii mentioned this in rP22342: Additional fixes following rP22313 - units no longer return to gather nearby….Jun 5 2019, 10:45 PM

wraitii mentioned this in D1968: Fix chasing / Repair-approaching / .Jun 10 2019, 8:24 PM

wraitii mentioned this in rP22367: Fix leftovers from rP22313 - improve repairing, chasing and fleeing behaviour..Jun 11 2019, 10:07 PM

wraitii mentioned this in D1975: Fix resource-returning (missed in rP22367 / from rP22313).Jun 15 2019, 4:15 PM

wraitii mentioned this in rP22374: Fix resource-returning order sometimes not working correctly (introduced by….Jun 15 2019, 5:08 PM

wraitii mentioned this in D1943: UnitAI fix return value on "enter" according to FSM logic.Jun 26 2019, 3:17 PM

bb added inline comments.Jun 28 2019, 9:58 PM

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
904	While this call looks same it is causing a bug: consider a unit walking and one orders "stop" it will here receive a stopMoving call and another from the `order.stop` code, so it calls the same function twice. Besides the fact that we shouldn't call the same function twice on a command, unitMotionFlying doesn't like it either: for some reason (this might be considered a bug, but it revealed the double calling here), on stopMoving unitMotionFlying inverts `this.landing` such that when flying we will start landing and while landing, we keep flying. However if we call it twice as in here, we just fly on, which obviously is a bug. Given this we need to be more careful where we call stopMoving, this commit seems to add it everywhere where it looks sane, but that isn't the correct place. IMO we shouldn't call stopmoving when leave a "moving state", since we can just as well go into another "moving state" and thus have some weird behaviour in case units want to to stuff when stopping (sliding and such). Instead we should call stopMoving when we enter a "non-moving state" as then we really need to stand still.

wraitii marked 3 inline comments as done.Jun 29 2019, 10:49 AM

wraitii added inline comments.

/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
904	Mh, I would call this a UnitMotionFlying bug - have admittedly left that kind of on its own. Calling `StopMoving` twice is certainly redundant but should really be idempotent. Further - No, we shouldn't remove these calls to StopMoving. They have nothing to do with sliding, and they don't even prevent units from immediately moving if they enter another moving state.

As said on #5460:
I think the reason why we seem to have more infinite loops right now is that rP22313 moved some checks from orders to states, so the check introduced in rP22059 is not protecting us from as many problems.

wraitii mentioned this in D2035: Ensure units do get in attacking range since that range can change over time because of elevation differences.Jul 3 2019, 7:23 PM

wraitii mentioned this in D2096: Correctly face towards target when repairing, gathering, guarding..Jul 18 2019, 9:01 PM

wraitii mentioned this in rP22505: Correctly face towards target when repairing, gathering, guarding..Jul 18 2019, 9:56 PM

wraitii requested verification of this commit.Jul 19 2019, 2:45 PM

This commit now requires verification by auditors.Jul 19 2019, 2:45 PM

wraitii mentioned this in D2735: Trigger an exit-reentry when the target entity of an order is renamed.May 17 2020, 8:42 AM

wraitii mentioned this in rP24032: Reuse "APPROACHING"-state in formation combat..Sep 10 2020, 10:45 AM