Page MenuHomeWildfire Games

Avoid creating a null formation
ClosedPublic

Authored by temple on Feb 17 2018, 7:36 PM.

Details

Summary

When units belonging to different formations are selected, Commands tries to make a new formation with them. If they used the same formation template then that'll be used for the new formation. But if they used different templates then we don't know which one to use in the new formation so instead we shouldn't make a formation. (Before rP17166 we defaulted to the "line closed" formation.)

That's fine, except instead of not making a formation, we actually make a formation using the null template. The formation breaks apart after we move again, but it shouldn't form in the first place.

Test Plan

Here's a video without the patch then with the patch:

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.Feb 17 2018, 7:36 PM
Vulcan added a subscriber: Vulcan.Feb 18 2018, 12:09 AM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/helpers/Commands.js
| 897| »   var·ids·=·[·id·for·(id·in·members)·];
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unexpected token for

binaries/data/mods/public/simulation/helpers/Commands.js
|  53| var·g_Commands·=·{
|    | [NORMAL] JSHintBear:
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 524| »   »   »   »   ····&&·player·!=·+cmd.owner)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/helpers/Commands.js
| 712| »   »   »   »   var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGUIInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 897| »   var·ids·=·[·id·for·(id·in·members)·];
|    | [NORMAL] JSHintBear:
|    | 'array comprehension' is only available in Mozilla JavaScript extensions (use moz option).

binaries/data/mods/public/simulation/helpers/Commands.js
| 897| »   var·ids·=·[·id·for·(id·in·members)·];
|    | [NORMAL] JSHintBear:
|    | Expected 'for' and instead saw 'id'.

binaries/data/mods/public/simulation/helpers/Commands.js
| 947| »   »   for·(var·i·=·0;·i·<·length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 960| »   »   var·count·=·0;
|    | [NORMAL] JSHintBear:
|    | 'count' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1107| »   »   var·cmpGuiInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGuiInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1359| »   »   var·piece·=·pieces[j];
|    | [NORMAL] JSHintBear:
|    | 'piece' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1442| »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1480| »   »   »   &&·cmpFormation.GetMemberCount()·==·formation.entities.length)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/helpers/Commands.js
|1506| »   »   »   »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1539| »   »   »   var·cmpFormation·=·Engine.QueryInterface(formationEnt,·IID_Formation);
|    | [NORMAL] JSHintBear:
|    | 'cmpFormation' is already defined.

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

temple requested review of this revision.Feb 18 2018, 12:09 AM
elexis accepted this revision.Apr 6 2018, 11:59 AM
elexis added a subscriber: elexis.

I can't confirm if it has a bad side effect under some rare circumstance (unitAI and formations are notorious for that), but in general this is a good change and the code itself reads very correct and the video you uploaded confirms that it is good in general.
(In particular it's good that it tests for the formation to be set, so it doesn't hardcode a default)

(Some code style thing: Using a constant for "special/formations/null" rather than repeating it everywhere would give us reference errors at compile time if there is a typo)

binaries/data/mods/public/simulation/helpers/Commands.js
1465 ↗(On Diff #5818)

eh, foundation = formation?

1532 ↗(On Diff #5818)

(Correct that no cmpUnitAI test is needed as those entities were filtered already)

This revision is now accepted and ready to land.Apr 6 2018, 11:59 AM
temple marked an inline comment as done.Apr 7 2018, 7:25 PM
temple added inline comments.
binaries/data/mods/public/simulation/helpers/Commands.js
1465 ↗(On Diff #5818)

I confused footprint and foundation a lot, don't think I ever mixed them with formation though.

This revision was automatically updated to reflect the committed changes.
temple marked an inline comment as done.