Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3586 closed enhancement (fixed)

[PATCH] Improve the choice of workers to be trained in petra

Reported by: mimo Owned by: otero
Priority: Should Have Milestone: Alpha 20
Component: AI Keywords: ai, patch
Cc: otero_xd@… Patch:

Description

Presently, the choice of workers by the ai (petra) is done in the trainMoreworkers function of the headquarters.js file. It is based on some asumptions: female with a small resources cost and citizen soldiers with higher costs but combat ability. As some civs/mods may not follow this pattern or even not have these templates, this function should be made more general. There is already a function findBestTrainableUnit which choose the best military units (scanning all available templates) according to some constraints, it could be extended.

refs: http://wildfiregames.com/forum/index.php?showtopic=20177#entry310809

Attachments (6)

petra_headquarters_template.patch (14.0 KB ) - added by otero 8 years ago.
headquarters_js_v2.patch (8.9 KB ) - added by otero 8 years ago.
config_js.patch (649 bytes ) - added by otero 8 years ago.
cleanup_config_js.patch (1016 bytes ) - added by otero 8 years ago.
romans_republic.png (2.0 MB ) - added by otero 8 years ago.
headquarters-v3.patch (10.4 KB ) - added by mimo 8 years ago.

Change History (16)

comment:1 by otero, 8 years ago

Cc: otero_xd@… added
Keywords: review patch added
Owner: set to otero
Summary: improve the choice of workers to be trained in petra[PATCH] Improve the choice of workers to be trained in petra

Fixed and changed the basic class where the AI is searching for the default templates. This could the beginning of a bigger patch that is required in petra AI, for some reason the methods findBestTrainableUnit and trainMoreWorkers are called constantly even when the AI doesn't have more populace or resources available. Probably is possible to improve performance if this little details are fixed.

As a suggestion I propose to add a ticket to make a little cleaning of the code respect to tabs and empty spaces. I've found several files (inside this patch an another I'm working) where the use of tabs and 4 spaces is combined, at the same time empty lines with tabs or spaces are in the files. In this patch my text editor made those changes and now the file changed in the patch has a combination, but the patch would become unnecessarily bloated if I change all the file, if there is a problem with this please let me know.

Last edited 8 years ago by otero (previous) (diff)

comment:2 by mimo, 8 years ago

Thanks for the patch. I'll have a more detailed look at it later. But here are a few fast comments:

  • for the conventions (tabs or space, no need of braces for one line block, ... see http://trac.wildfiregames.com/wiki/Coding_Conventions)
  • in the future, it is better to split patches (separating the simple cleanup from the code logic change) to ease the review. Cleanup only patches can be commited more quickly.
  • line 220 Traders have also the Support class, so your change requires also "if" -> "else if"
  • lines 474, 476, 478, 480, 489, 491, 493 and 495 have unneeded braces.

in reply to:  2 comment:3 by elexis, 8 years ago

comment:4 by elexis, 8 years ago

In 17752:

Remove trailing whitespace, refs #3586.

comment:5 by mimo, 8 years ago

  • line 220: better to have only: if (ent.hasClass("Worker"))
  • line 407: classesDef should be ["Support", "Worker"]

then all variables names referencing female should be changed (for example numFemales -> numSupport) and there is a conceptual problem, which is that the templateDef of the support unit could possibly change (because the one trained before is no more available, or a new cheaper one becomes available, or ...). So we can no more do as before (just counting the numbers based on template), but count based on the class: in the functions which count the number of workers, adding a count of the support workers should work.

Testing this patch can be done on https://wildfiregames.com/forum/index.php?/topic/20177-tried-to-add-the-inexisting-template-unitsimp_support_female_citizen-to-petra/ i.e. using the delenda_est mod with romans and checking that petra can indeed train different kind of support units.

comment:6 by otero, 8 years ago

Fixed and tested the code again according to commentaries.

In the patch I found an odd behavior and I don't know if a mistake in my code or in the mod delenda_est. When tested with public mod all civilizations seem to have a correct behavior, and when tested with Romans Republican in delenda_est the behavior is correct (Annex screenshot); but, when tested with civilization Romans Principals the woman workers don't work and the Js engine doesn't throw any error message. After reading the code and using other civilizations, I couldn't find the error.

The file config.js was divided in to patches. A patch for only the cleanup and a patch with changes of code. The patch for headquarter.js includes two lines of cleanup that I thought won't create a problem in reading the patch.

About some aspects in the code style like unnecessary parenthesis pointed above, in my opinion they improve the readability of the code in that section as they make explicit the expected behavior even if operator precedence already solves the execution order.

If you find any other problem, let me know.

by otero, 8 years ago

Attachment: headquarters_js_v2.patch added

by otero, 8 years ago

Attachment: config_js.patch added

by otero, 8 years ago

Attachment: cleanup_config_js.patch added

by otero, 8 years ago

Attachment: romans_republic.png added

comment:7 by otero, 8 years ago

Milestone: BacklogAlpha 20

by mimo, 8 years ago

Attachment: headquarters-v3.patch added

comment:8 by mimo, 8 years ago

Your latest patch had still the problem that the counting of workers would be wrong if the templateDef changed (the previous workers with the old templateDef would not be accounted). I've uploaded a modified version of your patch with the changes i tried to explain in my previous comment. I've also changed the name of one or two variables (so that workers refer to all worker, while support refer only to the support workers i.e. non fighting ones).

I also had a look at the delenda_est mod, and the problems come partly from the mod (some support units supposed to be workers, but not having the Worker class), and partly from petra which suppose that all Workers are gatherers AND builders while in this mod some units are only Builders. Could be a nice other ticket :-)

comment:9 by mimo, 8 years ago

Resolution: fixed
Status: newclosed

In 17827:

Petra: improve choice of workers, fixes #3586, patch from otero

comment:10 by mimo, 8 years ago

Keywords: simple review removed

Thanks for the patch

Note: See TracTickets for help on using tickets.