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
Details
Details
- Reviewers
Sandarac - Commits
- rP19894: Allow the AI to research techs in captured structures
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.
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 (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1697/ for more details.
Comment Actions
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.
| binaries/data/mods/public/simulation/ai/common-api/entity.js | ||
|---|---|---|
| 342 ↗ | (On Diff #2813) | I don't know if there needs to be the check for civ, as it will always be defined when this function is called. |
| binaries/data/mods/public/simulation/ai/common-api/entity.js | ||
|---|---|---|
| 342 ↗ | (On Diff #2813) | 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. |
Comment Actions
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 ↗ | (On Diff #2813) | Okay, it would be good to keep it :) |
Comment Actions
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.
Comment Actions
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 :(