HomeWildfire Games

Unit Motion: MoveTo family of function no longer returns false if the move is…
Concern RaisedrP22313

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

Event Timeline

Angen added a subscriber: Angen.May 30 2019, 4:58 PM
Angen added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/UnitAI.js
1852

should not be here checkTargetAttackRange instead move ?

bb raised a concern with this commit.May 30 2019, 5:28 PM
bb added a subscriber: bb.

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)

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
Angen 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

wraitii marked 3 inline comments as done.Jun 2 2019, 6:52 PM

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.

bb added inline comments.Fri, Jun 28, 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.Sat, Jun 29, 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.