Page MenuHomeWildfire Games
Differential D1653

Fix building population cost being ignored while placing it
AbandonedPublic
Actions

Authored by lyv on Oct 19 2018, 6:33 PM.

Details

Reviewers
None
Summary

Buildings can be placed even if the specified population cost exceeds the available population slots. This was probably not noticed before as building population cost remains as 0 in all vanilla buildings.

Test Plan

Set pop cost of a buildings to something and see if its possible to exceed the population available. Alternatively build some "Gohma eggs" from the Hyrule Conquest mod.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6401
Build 10605: Vulcan BuildJenkins
Build 10604: arc lint + arc unit

Event Timeline

lyv created this revision.Oct 19 2018, 6:33 PM
lyv updated this revision to Diff 6938.Oct 19 2018, 6:38 PM

Testing code snuck in

lyv retitled this revision from Fix building population cost ignored while placing it to Fix building population cost being ignored while placing it.Oct 19 2018, 6:41 PM
lyv added a comment.Oct 19 2018, 6:53 PM

I guess it still needs a notification.

Vulcan added a subscriber: Vulcan.Oct 19 2018, 8:14 PM

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
| 900| »   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
| 541| »   »   »   »   ····&&·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
| 729| »   »   »   »   var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGUIInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 900| »   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
| 900| »   var·ids·=·[·id·for·(id·in·members)·];
|    | [NORMAL] JSHintBear:
|    | Expected 'for' and instead saw 'id'.

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

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

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

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

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

binaries/data/mods/public/simulation/helpers/Commands.js
|1495| »   »   »   &&·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
|1521| »   »   »   »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

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

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

Harbormaster completed remote builds in B6399: Diff 6937.Oct 19 2018, 8:14 PM
Vulcan added a comment.Oct 19 2018, 8:18 PM

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
| 900| »   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
| 541| »   »   »   »   ····&&·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
| 729| »   »   »   »   var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGUIInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 900| »   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
| 900| »   var·ids·=·[·id·for·(id·in·members)·];
|    | [NORMAL] JSHintBear:
|    | Expected 'for' and instead saw 'id'.

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

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

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

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

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

binaries/data/mods/public/simulation/helpers/Commands.js
|1495| »   »   »   &&·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
|1521| »   »   »   »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

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

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

Harbormaster completed remote builds in B6401: Diff 6938.Oct 19 2018, 8:18 PM
elexis added a subscriber: elexis.Oct 19 2018, 8:57 PM

This file should be minimized, perhaps an || would already do, one could compare if its benefitial to move the condition to one of the components, also the inequation looks wrong, as it's equivalent to cmpPlayer.GetPopulationLimit() > cmpPlayer.GetPopulationCount().

lyv added a comment.Oct 19 2018, 9:04 PM

Good catch, it should be a "populationcount - popcost".

lyv updated this revision to Diff 6940.Oct 19 2018, 10:31 PM

Minor cleanup

Vulcan added a comment.Oct 19 2018, 10:34 PM

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
| 900| »   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
| 541| »   »   »   »   ····&&·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
| 729| »   »   »   »   var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGUIInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 900| »   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
| 900| »   var·ids·=·[·id·for·(id·in·members)·];
|    | [NORMAL] JSHintBear:
|    | Expected 'for' and instead saw 'id'.

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

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

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

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

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

binaries/data/mods/public/simulation/helpers/Commands.js
|1484| »   »   »   &&·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
|1510| »   »   »   »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

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

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

Harbormaster completed remote builds in B6403: Diff 6940.Oct 19 2018, 10:34 PM
shookees added a subscriber: shookees.Oct 19 2018, 10:55 PM

Just to reiterate the proposed change:
if the population limit has been reached, further house building would be disabled, right?

lyv added a comment.Oct 19 2018, 11:06 PM

Actually, no. This only affects when buildings which have a population cost is being built. Currently an unused feature in the game.

elexis added a comment.Oct 20 2018, 12:36 AM

Why would the check not consider the population that the building costs? (As mentioned perhaps one could move it to the components, i.e. to TrySubtractResources, but didn't check all occurrences)

lyv added a comment.EditedOct 20 2018, 6:26 AM

Unlike a unit, the entity (foundation) is already created before placing it. So this.popUsed from player.js would include the building’s population count. This is why it makes moving it to the component a bit more complicated. It doesnt work the same way as units.

lyv added a comment.Oct 20 2018, 8:09 AM

What about not creating the entity prematurely? Would make the code more consistent and reusable.

lyv planned changes to this revision.Dec 22 2018, 6:42 AM

One might wanna see templates/special/filters/foundation.xml first.
Also, the patch is not like how it should be. One should just not create instead of destroying it in such circumstances.

lyv abandoned this revision.Dec 31 2018, 8:16 PM

Might re-claim it in the future. But in the mean time.

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
simulation/
helpers/
12 lines

Diff 6938

View Options

binaries/data/mods/public/simulation/helpers/Commands.js

Loading...
Wildfire Games · Phabricator