[PATCH] CCmpUnitMotion bug when unit inside obstruction

[mimo]

While trying to understand a case where a unit would not leave a foundation, i've found a bug in MoveToTargetRange of CCmpUnitMotion: the DistanceToSquare of lines 1574 and 1579 should have an additional parameter (countInsideAsZero) true, otherwise all the following logic does not make any sense. Doing such a change fixed the problem in my case. In the case of LeaveFoundation, such a change is definitively needed. But it should be checked that it is also needed in all use case of this MoveToTargetRange function when inside the obstruction.

[mimo]

I forgot to add the patch

[fatherbushido]

It has indeed no sense to not add that boolean. By the way there are not many case in wich we come from the inside of the target.

Perhaps we could/should/must also modify IsInTargetRange in the same way (it's use in UnitAI too).

I sum up here the different calls of that function (all are in UnitAI.js)

• L398 if (cmpUnitMotion.MoveToTargetRange(this.order.data.target, distance, -1)) for flee order.
• L4380 return cmpUnitMotion.MoveToTargetRange(target, 0, 0); in MoveToTarget function.
• L4394 return cmpUnitMotion.MoveToTargetRange(target, range.min, range.max); in MoveToTargetRange function.
• L4450 if (cmpUnitMotion.MoveToTargetRange(target, range.min, guessedMaxRange)) in MoveToTargetAttackRange function. (should be nuked in #4028).
• L4454 return cmpUnitMotion.MoveToTargetRange(target, range.min, Math.min(range.max, parabolicMaxRange)); in the same function.
• L4463 return cmpUnitMotion.MoveToTargetRange(target, min, max); in MoveToTargetRangeExplicit function.
• L4477 return cmpUnitMotion.MoveToTargetRange(target, range.min, range.max); in MoveToGarrisonRange function.

The UnitAI functions MoveToTargetRange and MoveToTargetRangeExplicit are used in many orders and states: Heal, LeaveFoundation, Guard, Gather, ReturnResource, Repair FORMATION WalkToTargetRange LeaveFoundation ESCORTING MoveCompleted GUARDING Timer ATTACKING Timer GATHERING Timer HEALING Timer REPAIRING Enter Timer

The UnitAI function MoveToTarget is used for markets, MoveToGarrisonRange for garrison, MoveToTargetAttackRange for attack.

(Btw, at L398 of UnitAI, perhaps we should/could/can/must call directly the UnitAI function instead of the UnitMotion one ?)

[mimo]

Replying to fatherbushido:

Perhaps we could/should/must also modify IsInTargetRange in the same way (it's use in UnitAI too).

You mean the one in CCmpUnitMotion? I would agree, but was not completely sure.

(Btw, at L398 of UnitAI, perhaps we should/could/can/must call directly the UnitAI function instead of the UnitMotion one ?)

Yep, UnitAI would need also such changes, for internal consistency. But these could wait for after A21 is released as the current code is not buggy.

[wraitii]

Would like a reproduction case, but this seems to make sense.

[mimo]

I saved this game when i noticed the problem. There are two cavs (ent 7525 and 7526) blocking a wonder foundation and not moving. Fixed when applying the patch.

[wraitii]

Is this possibly related to entities being stuck in "capturing" mode?

[mimo]

Replying to wraitii:

Is this possibly related to entities being stuck in "capturing" mode?

I don't believe so, as the bug is only present when units are inside the structure obstruction, and i don't see how capturing could bring units inside.

[fatherbushido]

You mean the one in CCmpUnitMotion? I would agree, but was not completely sure.

Yes that one

Yep, UnitAI would need also such changes, for internal consistency. But these could wait for after A21

sure, I mentionned it just to not forget

I fully agree with your patch (looking at the comments, it must be like that) but I have no clue if one of the uses on the list above could do a weird use of that function (It doesn't look like).

(I guess it's only noticeable with big target, low min range argument and obviously the entity in the target shape)

[mimo]

Looking back at the code, you were right that we also need to change the DistanceToSquare computation in IsInTargetRange so that both functions stay compatible. There is also such a call in IsInPointRange (line 1476) which i don't know what we should do as i don't know if and when it is used.

In addition, there is a small problem as all points inside obstruction will now have a distance zero, and most minRange are zero, so if we ask a builder to start a construction while the builder is inside the obstruction, he will first try to build where he is, and then leave the foundation because he has received the leaveFoundation order, and then really start building. Not really a problem, but not nice. So we should either explicitely check if we are inside (as in the patch attached) or change the minRange of builders (i think that's the only case where we can have a unit inside obstruction) to a small value (i.e. 0.5)

[fatherbushido]

Indeed the first solution (add the check) is better than changing minRange of builders. While testing the patch in game in weird situation, I 'succeed' to have a bug (see attached savegame). I hope that will help you. ((Btw, when compiling I have warnings (recommandation to add parenthesis, but they are not needed).))

[mimo]

Replying to fatherbushido:

While testing the patch in game in weird situation, I 'succeed' to have a bug (see attached savegame). I hope that will help you.

It has nothing to do with the patch (it happens also with svn). My understanding is that the cav has an order to go to the other side of the foundation, but receices a leaveFoundation, so goes back out of the obstruction, but then retries immediately to cross the foundation to obey to its previous order. And as the cav is fast and the leaveFoundation only push units at 4m, it has time to reenter the obstruction before the builder has started the construction (and thus gives real obstruction to the foundation). To fix it, we can either increase this 4m (at least for cav), or wait a bit before following a previous order after a leaveFoundation, or any other ideas, but this is independent from this patch.

Concerning the last sentence "I hope that will help you", I don't want to take responsability for this patch. I'm only trying to help the release with reporting bugs and giving some wip patches for propositions of a solution, but don't have time to develop and test a final patch.

((Btw, when compiling I have warnings (recommandation to add parenthesis, but they are not needed).))

Yep, I've seen it to. But i think that's our coding convention to not have such parentheses even if gcc seems to have a different convention. I would also prefer to have the parentheses as i think they improve readability.

[fatherbushido]

thx for the explanations!

[gameboy]

Has this problem been fully restored?

[fatherbushido]

I encounter the bug in a20 today and I had to destroy the units in middle to continue the construction.

[elexis]

wraitii, fatherbushido and Itms recommend the code to be committed, but we don't find the courage to commit it at this stage. Please let us know if you disagree, and thanks for the patch (it is a very old and annoying issue, especially with nomad cc foundations)

[mimo]

No problem. I agree that it has to be thoroughly tested (even if code wise, the problem is clear and the fix simple). Let's push it.

[wraitii]

You can go ahead with committing this patch. It does indeed fix the logic problem. UnitMotion wants to know if we're too close to the target, but that could fail if we were so inside that we were out of our midrange, which is obviously broken.

You might also want to look at lines 1474/1484: we're in an if block where midrange and maxrange are zero then we do strict comparisons on them: there's something wonky.

[mimo]

In 18921:

fix UnitMotion when inside the obstruction, refs #4278

[mimo]

Thanks wraitii for reviewing the patch. I've commited it but I let the ticket opened as this bug revealed some new problems (maybe some should open a new ticket for them):

• strange lines 1474/1484 mentionned in comment 17
• the infinite trial to reenter inside the obstruction with cav, comments 9 and 10

[elexis]

In 18924:

Add parenthesis suggested by gcc 5.4 following r18921, refs #4278.

[fatherbushido]

comment 9 ?

[elexis]

fatherbushido is correct, they should have been taken into account.

 ((Btw, when compiling I have warnings (recommandation to add parenthesis, but they are not needed).))

If you state the exact compiler warning, it will probably taken more seriously and they will also know at which place it occurs (if they don't have that warning with their compiler). The parenthesis are not needed, but we also don't want random people complaining about compiler warnings that obscure the compiler output.

[fatherbushido]

I meant should we precise something that in coding convention ?

[elexis]

Moving to the Work In Progress milestone, since there is a patch asking for feedback, but since it is not strictly bound to a specific release.

[wraitii]

The problem of cavalry going back in the foundation since it moves so fast still happens. I guess to make sure we should scale the "move away" based on speed of the entity.

Assigning to A24 and to me since that sounds doable enough after the UAI 'feature freeze'.

[wraitii]

​Phab:D3204 for the cavalry issue.

[wraitii]

The cavalry issue has been split off to #5901. Closing this as fixed in A22.