Page MenuHomeWildfire Games

Be less restrictive for building shipyards for mods support
ClosedPublic

Authored by mimo on Jul 3 2017, 7:13 PM.

Details

Summary

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.

Test Plan

check that the ai behaves as expected

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.Jul 3 2017, 7:13 PM
mimo updated this revision to Diff 2799.Jul 3 2017, 7:17 PM

remove a forgotten debug printout

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.

Vulcan added a subscriber: Vulcan.Jul 3 2017, 8:40 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/1684/ for more details.

Vulcan added a comment.Jul 3 2017, 8:41 PM
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.

Vulcan added a comment.Jul 3 2017, 9:30 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/1685/ for more details.

Vulcan added a comment.Jul 3 2017, 9:31 PM
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.

Sandarac added inline comments.Jul 3 2017, 10:31 PM
binaries/data/mods/public/simulation/ai/petra/navalManager.js
172 ↗(On Diff #2799)

You switch this.docks to include docks that are foundations, but there is still the loop here that calls setAccessIndices,
even though that is done for foundations in the events.ConstructionFinished loop. So setAccessIndices is called twice in this case.

mimo added inline comments.Jul 3 2017, 10:52 PM
binaries/data/mods/public/simulation/ai/petra/navalManager.js
172 ↗(On Diff #2799)

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.
So i guess the best is to replace the loop on ConstructionFinished by a loop on Create in checkEvents (also checking on the Shipyard class). I'll update the patch accordingly.

mimo updated this revision to Diff 2809.Jul 4 2017, 7:40 PM

patch update

mimo added a comment.Jul 4 2017, 7:51 PM

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.

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.

Vulcan added a comment.Jul 4 2017, 8:17 PM
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.

In D705#28169, @mimo wrote:

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.

Installed latest diff. Ran a simulation with 3 AIs--Athenians, Indians, Persians. All 3 built a warship @20:00.

Sandarac edited edge metadata.Jul 5 2017, 8:26 PM

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

mimo added a comment.Jul 5 2017, 8:48 PM

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)

Sandarac accepted this revision.Jul 5 2017, 9:36 PM

Alright, the comment was addressed in the new diff, and I ran tests on naval maps with the new diff.

This revision is now accepted and ready to land.Jul 5 2017, 9:36 PM
This revision was automatically updated to reflect the committed changes.