Page MenuHomeWildfire Games

petra should be less demanding when low resources available if it already starts with a market
ClosedPublic

Authored by mimo on Apr 17 2017, 6:56 PM.

Details

Summary

as the title say. In addition, with the patch, it can already use the available market at phase 1 while before, it waited for phase 2.

Test Plan

check on polar sea. Without the patch, petra does not build barracks nor towers before building a second civ center, and does not barter before town phase. Should be fixed with the patch.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1160
Build 1829: Vulcan BuildJenkins
Build 1828: arc lint + arc unit

Event Timeline

mimo created this revision.Apr 17 2017, 6:56 PM
Vulcan added a subscriber: Vulcan.Apr 17 2017, 7:41 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

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

I haven't tested the diff yet, but I just looked at it quickly.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
1623

Maybe this if and the next could be combined?

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
116

I'm just curious as to why you made this change.

596

You have added the check for barterMarket for this variable in headquarters, but there is still an exact same check in the first few lines of performBarter?

mimo added a comment.Apr 17 2017, 9:04 PM

Thanks for the comments.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
1623

ok

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
116

:) i think the meaning is clearer than just testing if a variable is defined. Doesn't it make easier to understand what is required?

596

The test in performBarter is a bit different: it transforms the entitycollection into an array and take the first entry as the barterer. I agree that the test that the length is non zero there is in principle not needed because of that test, that is currently just a sanity check, checking that the array built from a non-empty collection is indeed non-empty. But as canBarter is defined in headquarter.js and could be modified one day without paying attention to the fact that it is also used in tradeManager.js, i think it is safer to keep it.

mimo updated this revision to Diff 1312.Apr 17 2017, 9:09 PM

with Sandarac comments

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

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

From my point of view, these are good changes.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
1590–1591

Missing semicolon.

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
116

Yes, the meaning is clearer with this change :)

596

Ah, okay, I see.

elexis added a subscriber: elexis.Apr 18 2017, 7:34 PM

Didn't get to test it yet, but improving AI support for these odd maps is greatly appreciated

binaries/data/mods/public/simulation/ai/petra/headquarters.js
1550

(unneeded parenthesis, && binds stronger than ||, equally to * and +)

1591

0.1?

1671

(Perhaps < 80 is sufficient too (refs ai config to separate data from logic))

2270

The hardcoded phase check might be replaced with a template requirements sometime in case we or a modder wants to make that market available in phase 1

mimo added a comment.Apr 18 2017, 8:36 PM

Thanks Sandarac and elexis, i'll upload a new version soon.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
1550

yes, i know, but i find it more readable with parenthesis
And anyway, i think now gcc-6 complains when we don't put these "uneeded" parentheses and we added them in the c++ so ...

1590–1591

ok thanks

1591

ok

1671

sure

2270

yes, i agree that it would be nice to change such hardcoded checks, but that's outside the scope of that ticket (not so trivial to implement).

mimo updated this revision to Diff 1340.Apr 18 2017, 8:40 PM

update patch

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

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

elexis accepted this revision.Apr 25 2017, 3:46 PM

Can't claim that I'm fully aware of all interactions of that code.
I've tested the patch in four games and it works nicely, really helps the AI working on this specific map (and other scenario maps where it starts with a market in age 1).
It also doesn't break when maliciously cheat-deleting the AIs markets in age 1.
Nor do I see any serialization problems.
It would be really great to have this committed in this release, so that the maps are not optimized for multiplayer but can also be enjoyed by the other majority of players.
So perhaps wait for Sandarac or anyone else posting a more complete review or commit this as "tested by" instead.

This revision is now accepted and ready to land.Apr 25 2017, 3:46 PM
This revision was automatically updated to reflect the committed changes.