Page MenuHomeWildfire Games
Differential D591

Set `special/player_gaia` template to inherit from `special/player`
AbandonedPublic
Actions

Authored by s0600204 on Jun 2 2017, 2:57 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Using the filtered attribute to only pass though/inherit what's needed.

Written in response to one of @wowgetoffyourcellphone's problems with his Delenda Est mod : https://wildfiregames.com/forum/index.php?/topic/21573-ugh-okay-help/&do=findComment&comment=332268, problem no. 1.

New solution: Make all parts of the Player component optional so player_gaia doesn't need them to be set.

Test Plan

Run a game session in 0ad. There should be no error or warning messages.

Also, it might help to run a game session with the Delenda Est mod enabled (and that mod's copy of templates/special/player_gaia.xml deleted). Again, there should be no warnings.

To check that only the Player component is being inherited, and the others are not:

  • Add warn(uneval(this.template)) to the init() function of the Player component and one of the other player-level components (such as TechnologyManager).
  • Reload 0ad, and start a new game
  • You should get a line printed for each player, plus gaia, from the Player component. (With the Delenda Est mod enabled, the resource glory should also be present in all lines from this component, not just the ones from players).
  • But for the other component, you should only get lines printed for the players, and not one for gaia.

Diff Detail

Event Timeline

s0600204 created this revision.Jun 2 2017, 2:57 PM
Owners added a subscriber: Restricted Owners Package.Jun 2 2017, 2:57 PM
elexis added a subscriber: elexis.Jun 2 2017, 3:01 PM
elexis added inline comments.
binaries/data/mods/public/simulation/templates/special/player_gaia.xml
1–2

Isn't this replacing the Player entry and instead should use filtered="" too or do I recall incorrectly?

s0600204 added inline comments.Jun 2 2017, 3:11 PM
binaries/data/mods/public/simulation/templates/special/player_gaia.xml
1–2

I'd originally put <Player merge=""> as that is what's used in the special_filter/* templates, but through testing before uploading it seems that it's unnecessary...

elexis accepted this revision.Jun 2 2017, 3:45 PM
elexis added a subscriber: • leper.

Result of the test in 0ad public for gaia and player 1:

WARNING: ({BarterMultiplier:{Buy:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}, Sell:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}}, SharedDropsitesTech:(void 0), SharedLosTech:(void 0), SpyCostMultiplier:"1.0"})
WARNING: ({BarterMultiplier:{Buy:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}, Sell:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}}, SharedDropsitesTech:"unlock_shared_dropsites", SharedLosTech:"unlock_shared_los", SpyCostMultiplier:"1.0"})

For delenda est, which annoyingly has a copy of the Player component for any reason:

WARNING: ({BarterMultiplier:{Buy:{food:"1.0", glory:"1000", metal:"1.0", stone:"1.0", wood:"1.0"}, Sell:{food:"1.0", glory:"0.001", metal:"1.0", stone:"1.0", wood:"1.0"}}, SharedDropsitesTech:(void 0), SharedLosTech:(void 0), SpyCostMultiplier:"1.0"})
WARNING: ({BarterMultiplier:{Buy:{food:"1.0", glory:"1000", metal:"1.0", stone:"1.0", wood:"1.0"}, Sell:{food:"1.0", glory:"0.001", metal:"1.0", stone:"1.0", wood:"1.0"}}, SharedDropsitesTech:"unlock_shared_dropsites", SharedLosTech:"unlock_shared_los", SpyCostMultiplier:"1.0"})

Actually it looks wrong that we have SharedDropsitesTech:(void 0) entry and not an empty string (ping @leper), but that's not an issue of this patch,

nor is it an issue of rP19302, because without the gaia player diff, we have these undefined entries for the two mandatory text attributes too:

WARNING: ({BarterMultiplier:{Buy:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}, Sell:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}}, SharedDropsitesTech:(void 0), SharedLosTech:(void 0), SpyCostMultiplier:"1.0"})
WARNING: ({BarterMultiplier:{Buy:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}, Sell:{food:"1.0", metal:"1.0", stone:"1.0", wood:"1.0"}}, SharedDropsitesTech:"unlock_shared_dropsites", SharedLosTech:"unlock_shared_los", SpyCostMultiplier:"1.0"})

Also not sure that gaia really needs the multipliers, but it's probably better to keep the differences between gaia and the other players minimal to avoid it bugging in case the player changes perspective to gaia or does something with a gaia entity like #4603

binaries/data/mods/public/simulation/templates/special/player_gaia.xml
1–2

Ah, reading rP19302 again, merge="" was used to only add the attribute if the parent has the attribute too, while it is actually merged in both cases.

So adding the merge="" here could be done to make this template not add Player if player.xml for any reason (mods?) removed it too.
Seems like an unlikely case, because what else do we have that component for and if someone deleted it, he should delete these two templates as well. So @coin.

This revision is now accepted and ready to land.Jun 2 2017, 3:45 PM
Vulcan added a subscriber: Vulcan.Jun 2 2017, 3:45 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1443/ for more details.

Vulcan added a comment.Jun 2 2017, 3:49 PM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/104/ for more details.

Harbormaster completed remote builds in B2110: Diff 2376.Jun 2 2017, 3:49 PM
mimo added a subscriber: mimo.Jun 2 2017, 6:35 PM

I'm not convinced this is the right way to do it. I guess we initially added the SharedLosTech to gaia player only to avoid having it optionnal, and then we added the other properties one by one just because there was a precedent, and now we add the full inheritence (including i guess the entityLimits and the other stuff?). The most logical imo would be to make all these proprieties optionnals in the player component.
Ideally player_gaia.xml should be empty if none of its content is used.

s0600204 added a comment.Jun 2 2017, 7:15 PM
In D591#24317, @mimo wrote:

[...] now we add the full inheritence (including i guess the entityLimits and the other stuff?).

Nope. Entity limits, limit changers, limit removers, etc. are not part of the Player component and, thanks to the filtered attribute, are not inherited.

mimo added a comment.Jun 2 2017, 7:27 PM
In D591#24334, @s0600204 wrote:
In D591#24317, @mimo wrote:

[...] now we add the full inheritence (including i guess the entityLimits and the other stuff?).

Nope. Entity limits, limit changers, limit removers, etc. are not part of the Player component and, thanks to the filtered attribute, are not inherited.

ok sorry, i'm not used to the filtered attribute.
But still wouln't it be better to either put all these proprieties optional in Player.js, or possibly have a new GaiaPlayer component? Gaia is completely different than a normal player, and trying to have it included is maybe not the right approach.

elexis added a comment.Jun 2 2017, 7:37 PM

Mods (like gaia here) might want to add many weird civs objects, so adding support to omit Player properties that are never going to be relevant sounds preferable over the proposed patch or a new component.
The proposed patch doesn't add a regression but fixes an issue by removing hardcoding, so it depends on whether s0600204 wants to implement it (or write a simple ticket), fix it before the release or not, or split it.

s0600204 updated this revision to Diff 2389.Jun 2 2017, 10:05 PM

Make all parts of the Player component optional.

s0600204 added a comment.Jun 2 2017, 10:07 PM
In D591#24335, @mimo wrote:

ok sorry, i'm not used to the filtered attribute.

That's ok - I only came across it myself a couple of weeks ago...

elexis added a comment.Jun 2 2017, 10:11 PM

-> request review

s0600204 requested review of this revision.Jun 2 2017, 10:24 PM
s0600204 edited edge metadata.
Vulcan added a comment.Jun 2 2017, 11:05 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["sell"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Barter.js
|  96|  96| 	amountsToSubtract[resourceToSell] = amount;
|  97|  97| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  98|  98| 	{
|  99|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  99|+		var amountToAdd = Math.round(prices.sell[resourceToSell] / prices["buy"][resourceToBuy] * amount);
| 100| 100| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 101| 101| 		var numberOfDeals = Math.round(amount / 100);
| 102| 102| 
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["buy"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Barter.js
|  96|  96| 	amountsToSubtract[resourceToSell] = amount;
|  97|  97| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  98|  98| 	{
|  99|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  99|+		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices.buy[resourceToBuy] * amount);
| 100| 100| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 101| 101| 		var numberOfDeals = Math.round(amount / 100);
| 102| 102| 

binaries/data/mods/public/simulation/components/Barter.js
|  99| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['sell'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
|  99| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['buy'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
| 147| »   »   if·(this.priceDifferences[resource]·!=·0)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Player.js
| 142| »   if·(num·!=·0·&&·num·>·(this.GetPopulationLimit()·-·this.GetPopulationCount()))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Player.js
| 274| »   »   if·(this.resourceCount[type]·!=·undefined·&&·amounts[type]·>·this.resourceCount[type])
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.

binaries/data/mods/public/simulation/components/Player.js
| 277| »   if·(Object.keys(amountsNeeded).length·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Player.js
| 331| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.

binaries/data/mods/public/simulation/components/Player.js
| 702| »   return·this.diplomacy[id]·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/111/ for more details.

Harbormaster failed remote builds in B2124: Diff 2389!Jun 2 2017, 11:05 PM
Vulcan added a comment.Jun 3 2017, 12:41 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1452/ for more details.

mimo added inline comments.Jun 3 2017, 1:45 PM
binaries/data/mods/public/simulation/components/Player.js
67

wouldn't it be better to init to 1 instead of undefined so that all components will still work as now (if we ever need to bribe a gaia unit)
+this.template.SpyCostMultiplier does not need the parenthesis

71

same thing here, instead of undefined, having a loop on resCodes and putting all to 1 would avoid the changes you did in Barter code
We could even always do this init to 1 before, line 68 and then modify those defined in the template

585

I wonder if it would not be worth to cache it (as is done for this.sharedDropsites)

762

if (this.template.SharedLosTech && msg.tech == this.template.SharedLosTech) ?

764

same comment

binaries/data/mods/public/simulation/templates/special/player_gaia.xml
2

do we still need such inheritance?

s0600204 updated this revision to Diff 2398.Jun 3 2017, 5:03 PM
s0600204 marked 6 inline comments as done.
s0600204 edited the summary of this revision. (Show Details)

Changes made in response to @mimo's comments.

Also contains changes required by the linter to make sure it "builds".

binaries/data/mods/public/simulation/templates/special/player_gaia.xml
2

So long as no staff member, contributor, or modder adds a non-optional property to the Player component, no.

Vulcan added a comment.Jun 3 2017, 5:49 PM

Build has FAILED

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests).WARNING: JavaScript warning: simulation/components/Player.js line 85
reference to undefined property this.template.BarterMultiplier.Buy[res]

In TestComponentScripts::test_scripts:
../../../source/test_setup.cpp:134: Error: Test failed: L"Stack trace:\n@simulation/components/tests/test_Player.js:66:1\nExpected equal, got ({buy:{food:(void 0), metal:1, stone:1, wood:1}, sell:{food:(void 0), metal:1, stone:1, wood:1}}) !== ({buy:{wood:1, stone:1, metal:1}, sell:{wood:1, stone:1, metal:1}})"
................................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 306 tests
Success rate: 99%

Link to build: http://jw:8080/job/phabricator/1457/
See console output for more information: http://jw:8080/job/phabricator/1457/console

Harbormaster failed remote builds in B2134: Diff 2398!Jun 3 2017, 5:50 PM
Vulcan added a comment.Jun 3 2017, 5:51 PM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/115/ for more details.

s0600204 updated this revision to Diff 2404.Jun 3 2017, 6:11 PM

Fix test broken by changes,

• leper removed a reviewer: elexis.Jun 3 2017, 7:41 PM

(So this actually shows up for review.)

Part of the reasoning for adding player_gaia was that it does not need all those things actual players need, so making it inherit from that seems wrong, but I guess you got to that conclusion by now.

binaries/data/mods/public/simulation/templates/special/player_gaia.xml
2

They shouldn't.

• leper added a reviewer: Restricted Owners Package.Jun 3 2017, 7:45 PM
Vulcan added a comment.Jun 3 2017, 10:02 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1460/ for more details.

Vulcan added a comment.Jun 3 2017, 10:04 PM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/118/ for more details.

Harbormaster completed remote builds in B2138: Diff 2404.Jun 3 2017, 10:04 PM
wowgetoffyourcellphone added a comment.Jun 13 2017, 11:36 AM

delenda est, which annoyingly has a copy of the Player component for any reason

Side note: I edit the Player component to change the default population cap. That's currently the only reason I have that file in the mod.

wraitii abandoned this revision.Jul 28 2020, 10:17 AM
wraitii added a subscriber: wraitii.

Ain't happening I guess.

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
simulation/
components/
6 lines
59 lines
templates/
special/
22 lines

Diff 2389

View Options

binaries/data/mods/public/simulation/components/Barter.js

Loading...
View Options

binaries/data/mods/public/simulation/components/Player.js

Loading...
View Options

binaries/data/mods/public/simulation/templates/special/player_gaia.xml

Loading...
Wildfire Games · Phabricator