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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 ↗(On Diff #1307)

Maybe this if and the next could be combined?

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
116 ↗(On Diff #1307)

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

596 ↗(On Diff #1307)

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 ↗(On Diff #1307)

ok

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
116 ↗(On Diff #1307)

:) 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 ↗(On Diff #1307)

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 ↗(On Diff #1312)

Missing semicolon.

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
116 ↗(On Diff #1307)

Yes, the meaning is clearer with this change :)

596 ↗(On Diff #1307)

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
1591 ↗(On Diff #1312)

0.1?

2267 ↗(On Diff #1312)

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

1550 ↗(On Diff #1307)

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

1671 ↗(On Diff #1307)

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

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
1590 ↗(On Diff #1312)

ok thanks

1591 ↗(On Diff #1312)

ok

2267 ↗(On Diff #1312)

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).

1550 ↗(On Diff #1307)

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 ...

1671 ↗(On Diff #1307)

sure

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.