Page MenuHomeWildfire Games

Allow the AI to research techs in captured structures
ClosedPublic

Authored by mimo on Jul 5 2017, 7:29 PM.

Details

Summary

The ability to research techs in captured structures was withdrawn when capture was first implemented as was done in simulation, but since then, structures are now allowed to research techs in capture structures, so this should be reenabled for the ai too.
In addition, the patch anticipate some possible future {civ} replacement in the technology productionQueue

Test Plan

Rather check the code as testing in game this feature in not straightforward: you'd have to switch to the ai perspective, wololo an enemy structure that the ai haven't yet built (possibly disabling the ai structures) and wait for the ai to research techs.

Event Timeline

mimo created this revision.Jul 5 2017, 7:29 PM
Vulcan added a subscriber: Vulcan.Jul 5 2017, 8:18 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/1697/ for more details.

Vulcan added a comment.Jul 5 2017, 8:20 PM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/288/ for more details.

Sandarac added inline comments.Jul 5 2017, 9:14 PM
binaries/data/mods/public/simulation/ai/common-api/entity.js
342

I don't know if there needs to be the check for civ, as it will always be defined when this function is called.

mimo added inline comments.Jul 5 2017, 10:00 PM
binaries/data/mods/public/simulation/ai/common-api/entity.js
342

Yep, i agree it is currently not needed, but this make the function more generic if in the future we need to know what all all the possible techs researchables whatever the civ and that makes it completely symmetrical with the trainableEntities ones.
But i've no strong feeling about it and i can remove it if wanted.

Sandarac edited edge metadata.Jul 7 2017, 8:56 PM

I first had to do a bit more reading on how this function is used, but in fact I am still a bit confused about what this patch achieves (maybe I miss something obvious, or didn't read enough):

  • In all 5 uses of researchableTechs(), the civ passed to it will always be the player civ.
  • The trainableEntities() function, which is like a sister function, is sometimes passed a civ (which again is the player civ), or sometimes not, and so it is possible that it may return invalid templates.
binaries/data/mods/public/simulation/ai/common-api/entity.js
342

Okay, it would be good to keep it :)

mimo added a comment.Jul 8 2017, 11:20 AM

I first had to do a bit more reading on how this function is used, but in fact I am still a bit confused about what this patch achieves (maybe I miss something obvious, or didn't read enough):

removal of lines 339-340: this.civ() returns the civ from the Identity component. In the first implementation of capture, techs were not researchables from structure from another civ, but this is no more true, so this lines should be removed if we want to allow techs from captured structures.

addition of lines 342-343: there were some discussion to allow a {civ} replacement in techs productionQueue, mainly for the phase upgrades. This change is just a first move towards implementing it in the ai, although not complete as additional changes will be needed depending on the way it is done in the simulation. But those additional changes will (in principle) only be needed in the gamestate.js file.

Sandarac accepted this revision.Jul 8 2017, 10:28 PM

The patch removes the early return and now matches with the trainableEntities() function.

removal of lines 339-340: this.civ() returns the civ from the Identity component.

Right, it does, I had forgotten; I thought it returns the civ of the player that currently owns the entity :(

This revision is now accepted and ready to land.Jul 8 2017, 10:28 PM
This revision was automatically updated to reflect the committed changes.