Do not suppose anymore than only cart can build a shipyard (super_dock template), and be more mod-friendly by also allowing a possible shipyard template.
Also don't use different entitycollections when one would be enough.
Details
- Reviewers
Sandarac - Commits
- rP19872: Be less restrictive for building shipyards for mods support
check that the ai behaves as expected
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2498 Build 4213: Vulcan Build (Windows) Jenkins Build 4212: Vulcan Build Jenkins Build 4211: arc lint + arc unit
Event Timeline
Tested it on a Cycladic Archipelago skirmish map. AI does indeed build a Shipyard. Nice.
But despite having the resources to, it did not train any warships. Also, it only built 1 dock and 1 shipyard. On "Naval" maps I think the AI should prioritize building these more. Fewer farms, more docks/fishing ships. Fewer soldiers, only planning an invasion, and more warships.
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/1684/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/navalManager.js | 309| » » » plan.units.forEach(function·(ent)·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. binaries/data/mods/public/simulation/ai/petra/navalManager.js | 320| » » » plan.units.forEach(function·(ent)·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/280/ for more details.
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/1685/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/navalManager.js | 309| » » » plan.units.forEach(function·(ent)·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. binaries/data/mods/public/simulation/ai/petra/navalManager.js | 320| » » » plan.units.forEach(function·(ent)·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/281/ for more details.
binaries/data/mods/public/simulation/ai/petra/navalManager.js | ||
---|---|---|
172 | You switch this.docks to include docks that are foundations, but there is still the loop here that calls setAccessIndices, |
binaries/data/mods/public/simulation/ai/petra/navalManager.js | ||
---|---|---|
172 | i agree, but it is called twice only for those foundations available at startup (so usually never happens). The main reason of this change was that now getUnconnectedSeas now also takes into account dock foundations (which was not the case before) and which i think is better. But your comment let me realize that it supposes that accesses are already set. |
Can you check again with the new version? i did some tests, and the ai trained ships at the shipyard. Maybe something which was fixed with the latest changes in the patch.
And are you sure it only build one dock? it should have (and always had) one per island, although the new patch only adds one shipyard.
For the other comments, please keep your wishlist on the forum which already have plenty of such discussions: that's not the purpose of this patch to solve these problems and that just hide the useful information.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/ai/petra/navalManager.js | 309| » » » plan.units.forEach(function·(ent)·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. binaries/data/mods/public/simulation/ai/petra/navalManager.js | 320| » » » plan.units.forEach(function·(ent)·{ | | [NORMAL] JSHintBear: | | Don't make functions within a loop. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/286/ for more details.
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/1695/ for more details.
Installed latest diff. Ran a simulation with 3 AIs--Athenians, Indians, Persians. All 3 built a warship @20:00.
I believe the diff is good, although to play devil's advocate, I'm really not sure about adding support for arbitrary mods which may or may not be modified later at a whim, making the code irrelevant (there is, of course, no comparable quality control nor review process for Delenda Est, and this is typical for most game mods). And in any case, such added code which panders to mods will be irrelevant/useless in the vast majority of cases anyways, as most people do not use mods (so this could be seen as effectively adding dead code), but only add unneeded complexity which will need to be maintained. I know there is some support in f.e. the structure tree, but that is perhaps different.
(Also relevant for the other diff that deals with phasing.)
I agree we should not add useless code, but the only non-vanilla code here is to look for a shipyard template (i.e. 2 lines of code) which looks to me acceptable if that may ease the use of a mod, and is a natural template to have, quite likely to be used in different mods (in fact, i'd be inclined to rename our cart_super_dock template to cart_shipyard)
Alright, the comment was addressed in the new diff, and I ran tests on naval maps with the new diff.