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.
Details
Details
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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
Comment Actions
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? |
Comment Actions
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. |
Comment Actions
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.
Comment Actions
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. |
Comment Actions
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)) |
Comment Actions
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 |
| 1671 ↗ | (On Diff #1307) | sure |
Comment Actions
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.
Comment Actions
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.