Opened 16 months ago

Closed 9 months ago

#4142 closed defect (fixed)

[PATCH] Petra AI - Regicide Support

Reported by: elexis Owned by:
Priority: Nice to Have Milestone: Alpha 22
Component: AI Keywords: patch
Cc: Sandarac Patch:

Description (last modified by Sandarac)

If regicide gamemode is enabled, a player loses if the hero dies. So the AI should do its best to avoid that. There are two more options planned (prohibiting garrisoning the hero and prohibiting attacks/auras in age 1/2). The hero could be used to scout and to kill women in age 1, but needs to be retreated if too many men attack it. Some heroes like elephants are very slow at escaping, thus more vulnerable.

Attachments (12)

petra_regicide_support_v1.patch (1.3 KB) - added by Sandarac 15 months ago.
Petra will attempt to garrison the hero for healing if the hero's health drops lower than 60%, including cases when the hero is part of an army (hasn't been extensively tested).
petra_regicide_support_v1.1.patch (2.0 KB) - added by Sandarac 15 months ago.
Petra will try to return the hero to a base (and garrison the hero if it can) using class specific health thresholds.
commands.txt (56.0 KB) - added by elexis 15 months ago.
In this r18614 replay the hero of the yellow AI player died in the first attack wave.
petra_regicide_support_v1.2.patch (3.2 KB) - added by Sandarac 15 months ago.
Petra will not add the hero to an army if its destination is across water, or if the hero is hurt. The hero will retreat if there are more than six enemy units surrounding it.
4142_petra_regicide_support_v1.3.patch (22.1 KB) - added by Sandarac 11 months ago.
4142_gameTypeManager_v1.patch (19.1 KB) - added by Sandarac 11 months ago.
4142_gameTypeManager_v1.1.patch (19.6 KB) - added by Sandarac 11 months ago.
Fixes.
4142_petra_regicide_support_v1.4.patch (5.7 KB) - added by Sandarac 11 months ago.
Healers will be assigned to guard the hero.
4142_petra_regicide_support_v1.5.patch (8.7 KB) - added by Sandarac 11 months ago.
Update and apply mimo's remarks, except without Maps.
4142_petra_regicide_support_v1.6.patch (10.8 KB) - added by Sandarac 10 months ago.
Handle multiple heroes in regicide, and cache entities important for specific victory conditions.
4142_petra_regicide_support_v1.7.patch (12.9 KB) - added by Sandarac 10 months ago.
Fix issues.
4142_petra_regicide_support_v1.8.patch (15.0 KB) - added by Sandarac 10 months ago.
More refinements.

Download all attachments as: .zip

Change History (50)

Changed 15 months ago by Sandarac

Petra will attempt to garrison the hero for healing if the hero's health drops lower than 60%, including cases when the hero is part of an army (hasn't been extensively tested).

comment:1 Changed 15 months ago by Sandarac

Keywords: rfc patch added
Summary: Petra AI - Regicide Support[PATCH] Petra AI - Regicide Support

comment:2 Changed 15 months ago by elexis

Thanks for working on the AI :)

Some considerations:

  • Hero elephants can't be garrisoned (also that option might be enabled later). If it can't be garrisoned, just retreating to the civic center or a place with many defenses might be an idea.
  • Cavalry heroes can retreat easily, while hero elephants might need a really long time to get back to the CC. So perhaps the 0.6 number could be adapted (0.3 for cavalry, 0.5 for infantry, 0.7 for elephants?)
  • What does it do with the mauryan hero healer?
  • Surrounding the enemy hero is an easy way to defeat a player, but I guess that's hard to avoid with the AI.

Changed 15 months ago by Sandarac

Petra will try to return the hero to a base (and garrison the hero if it can) using class specific health thresholds.

Changed 15 months ago by elexis

Attachment: commands.txt added

In this r18614 replay the hero of the yellow AI player died in the first attack wave.

comment:3 Changed 15 months ago by elexis

It seems this is a UnitAI issue too. You should perhaps set the UnitAI stance of the hero to passive, so that it won't start attacking the attackers if it should retreat.

Also "50%" HP might be an improvable consideration. If there are only 5 men attacking him, he can fight them off, but if there are 200 attackers, it just shouldn't go there. Perhaps it could try to avoid enemies in range N (perhaps too hard to do).

Since the AI has a revealed map, it can't do anything useful with the hero it in the beginning (other than trying to rush some enemies if it has a cavalry or infantry hero).

There were character traits planned for the AI, like aggressiveness. Don't know if that has been implemented partially yet, likely not.

comment:4 Changed 15 months ago by elexis

Right, the code already sets heroes as passive on retreat.

I could notice successful retreats in some games, but in other games the heroes were still killed when they got surrounded. Retreating earlier doesn't really help to avoid the surround. Perhaps it would be better to not move the hero at all?

Your idea mentioned on 2016-08-23 in irc to use ExecuteQueryAroundPos might even be possible, considering that attackPlan.js also uses Engine.ComputePath.

Changed 15 months ago by Sandarac

Petra will not add the hero to an army if its destination is across water, or if the hero is hurt. The hero will retreat if there are more than six enemy units surrounding it.

comment:5 Changed 14 months ago by elexis

In 18731:

Simple petra AI regicide support. Patch by Sandarac, refs #4142.

The AI will retreat the hero in regicide games once it reaches less than 70% health.
It will garrison it in healing structures. Elephant heroes will retreat to the home base.
Once the hero has healed, it will be used for attacks again.

comment:6 Changed 14 months ago by elexis

Thanks for the patch Sandarac! I have committed your patch since it works very well (and before your patch, regicide AI games were guaranteed to be over after 10 minutes in the first attack). Hope to see more of your patches, even if it's hard to get reviews.

I have replaced the hurt() check with a healthLevel < 0.8 check, so that it would attack with heroes if they have sufficient HP. Also merged those hero type distinctions for now.

  • That check for near enemies is unfortunately extremely slow. It was lagging so much when I ran some tests with the patch, but not nearly as much without the patch. It was freezing every few turns for 2 or 3 seconds. After removing the check, it was working fine. Ideally we could use ExecuteQueryAroundPos of RangeManager, but it is out of reach currently.
  • TODO: It were great if the AI would focus it's planned attacks on the enemy hero.
Last edited 14 months ago by elexis (previous) (diff)

comment:7 Changed 14 months ago by elexis

Keywords: rfc removed

comment:8 Changed 14 months ago by mimo

I've not tried any regicide game, but just looking at the patch, here are some remarks

changes in attackPlan line 646: what is the logic of preventing a hurt hero to be affected to an army here, and not in other cases like 603, 626 and 659? and i think other heroes are not disabled in a regicide game? if yes, this restriction (specially the overseas one) should apply only to the right "hero" and not all of them. Maybe when #252 is used, a new class REGICIDE should be added to the concerned "hero" and all your current tests on "HERO" be changed to "REGICIDE". You should add a TODO here.

changes in defenseManager line 391: same remark about other heroes. Should add a TODO there.

line 485: what is the use of the stopMoving method?

lines 487-492 and 503-510: looks weird here. It would be better done in the checkEvent method of the attackPlan. Or even better, you could add a new gameTypeManager which would contains a checkEvents method responding to the different gameTypes.

I don't understand how this code can work: whatever the result of the garrisonUnitForHealing, you try to do a move if the hero was in an attack? And as the checkEvents methods are called every turn the ai is run, you will try to redo this garrisoning always and always up to the time the hero reaches its holder if it ever reaches it (as the move of line 509 can put him in a different direction than the healing structure he wanted to be garrisoned in)? that has no sense. Check what is done in garrisonManager: a unit garrisoned by the garrisonManager will have its plan changed to -2 or -3 up to the time it reaches its garrisonHolder, and then it will have the standard garrisoned flag. So all your block 583-512 should not be done if the hero has plan=-2 or -3, and just after the garrisonUnitForHealing, you should recompute its plan and do a continue if it is now -2 or -3. Futhermore garrisonUnitForHealing (as indicated by its name) only looks for a structure with some BuffHeal?. You should add another function to try to garrison the hero in other useful structures as fortresses when it is attacked and already severely wounded. And also eject some unimportant garrisoned units if a nearby buffheal structure to leave room for the hero.

line 505: here also, you set all heroes to passive when "the hero" is slighly hurt, and never change back their stance (except what was already done when starting an attack). So all your other heroes will be useless when you are attacked. Here also a TODO when the special class would be added.

comment:9 Changed 14 months ago by elexis

Milestone: BacklogAlpha 22

Thanks for your remarks mimo!

comment:10 Changed 11 months ago by mimo

Cc: Sandarac added

Sandarac, do you intent to work on the remaining issues on that patch in the near future? near future being before A22, so let's say in the next two months.

comment:11 Changed 11 months ago by Sandarac

Yes mimo.

comment:12 Changed 11 months ago by mimo

OK thanks for the quick answer

Changed 11 months ago by Sandarac

comment:13 Changed 11 months ago by Sandarac

Several of the points in comment:8 assumed that players could be in possession of more than one hero at a given time; since r12832 this has not been possible (players always have an EntityLimit? of 1 for heroes and I don't believe there are plans to change this).

  • The stopMoving method is needed because without it, if the hero is in a UnitAi? state like "COMBAT.CHASING", there is a chance that it will ignore the garrison order (At least this appears to be what happens).
  • Now when the hero's health falls below 70%, the garrison order will be sent only once (instead of over and over in the original patch), but if the hero's health falls below 40%, it will try once more to find a closer structure, ignoring healing this time (non-garrisonable units will still try to find the closest base). A unit will be ejected if there is no room. These checks have been moved to a gameTypeManager, along with wonder-specific events.
  • The AI will now target enemy heroes in Regicide, and it will not garrison the hero on a ship.
  • The AI will assign some healers as guards to the hero. It will try to keep around four healers guarding the hero. I tested this fairly thoroughly, but if the hero is garrisoned (or has a different access value) then units cannot be assigned to guard the hero. I guess the gameTypeManager could check events.UnGarrison? and then assign all units that have the "regicideHealer" role to guard the hero if they are not already.
  • At some point Champion units should be assigned as guards as well.
  • I know that UnitAI's canGuard function checks if the entity is already guarded as well as the entity template, but just a template check should be enough for the AI (there likely won't be extensive guarding).

comment:14 Changed 11 months ago by Sandarac

Keywords: rfc added

comment:15 Changed 11 months ago by mimo

Thanks sandarac for the patch. That's quite a complete patch, but also complex to review. I think in general it is better to provide small well-contained patches rather than such a big one. For example, during your dev process, i guess you started by creating the new gametypeManager and moved some already existing code inside it before adding new functionnalities. Such a patch would already have been really useful as easy to review, instead of the complete one with everything mixed. Also the part about guard seems to be self-contained and could make a second patch by itself once the infrastructure changes have been made. Such small patches would really ease (and fasten) the review process, and as i'm the only one reviewing ai patches currently, i would really appreciate them :-) (at least for future patches if you can't cut this one)

Concerning the comment about several heroes, it's true that in vanilla game, we have only one hero, but this limit can easily be modified and we will certainly see some mods without it in the future. So if we can easily take it into account aready now, it's better to do it, otherwise just add a TODO.

Edit: i looked a bit at the changes in attackManager.js (the AI targeting the player with the nearest hero) and do not support this change because

  • it is too much cheating as the ai is not supposed to know where other players'heroes are. I agree that what is currently done (i.e. targeting the player with the nearest cc) is also cheating, but here the cc info is only used as a representation of the enemy base which is in principle easily obtainable by a human player with just a bit of scouting, while the location of the hero is much more difficult to obtain
  • the hero can move, so you can exploit such a feature (moving your hero to the extremity of the map or on an isolated island) to avoid being attacked by the AIs.

I've not yet had time to look at the other features. Will comment on them when done.

Last edited 11 months ago by mimo (previous) (diff)

Changed 11 months ago by Sandarac

comment:16 Changed 11 months ago by Sandarac

Ok, yes I should have split the patch up. This new patch moves the wonder and regicide code to the gameTypeManager, and includes the improvements for when the hero is attacked. I removed the changes to attackManager.

I added a TODO for the "multiple heroes in Regicide" issue, and I made a function that checks the status of the hero and reduces some of the duplication in assignUnits of attackPlan. Also, in assignUnits, the variable added is not used and the return value is not needed, so I removed them. In defenseManager, I made a change to prevent the hero from being assigned as a defense unit.

comment:17 Changed 11 months ago by mimo

Hi sandarac, i've been able to go through your latest patch.

attackPlan changes: the changes are good, except that I would keep the "return added" in assignUnits. It is not used right now, but it may be useful to know that a unit has been added for some future use (and in fact this info was used some time ago). otherwise, line 616, i would keep

if (!ent.hasClass("Cavalry") || !this.isAvailableUnit(gameState, ent))

which is more analoguous to what is done in other blocks and allow to remove the third argument of isAvailable. and lines 852 and followings can be rewritten

 if (wondermode)
   targets = ...
 else if (regicide)
   targets = ...
 if (targets && targets.hasEntities())
   return targets;

defenseManager line 634 you could have function(gameState, unit, emergency=false) and then remove the third argument of line 533 lines 651 to 656 can be replaced by

if (m.GetLandAccess(gameState, ent) !== unitAccess)

lines 672 remove the else (not needed after return) the logic in lines 675-677 won't work as the unload is an Engine command which will only be done on the next turn, so the number of garrisoned units will only be updated the next turn the AI will run. In addition, it may be dangerous to try to update it like that as numberOfGarrisonedUnits contains the units really garrisoned and those being garrisoned by the garrison manager. So it could happen that nearest.garrisoned() has no units while nearest is full (garrisoned will only be updated on the next turn). You should count the pop cap of units you can unload (i.e. which are already in the garrisoned() array) and if that's smaller than what is needed to garrison your hero, go to the next structure. line 679: it think it should be garrisonManager.garrison(gameState, unit, nearest, (nearest.buffHeal() ? "protection" : "emergency")); or you should change line 225 of garrisonManager to return enemiesAround && ent.needsHeal();

gameTypeManager lines 21-38 this loop could have a if (wonder) or maybe rewrite the function with a switch(gameState.getGameType()) line 31 could become " if (builders)"

line 72 the first condition (plan == -2 line 92 could be cut after
plan == -3) is always satisfied here
to avoid too long line

line 110 putting the hero in passive stance is a bit dangereous if never put back to a standard stance. Maybe having some check which, every 50 turns or so, put back the hero in its normal stance if its health > 0.7 ?

One important check to do is the serialization of the regicide. Did you try to save a game and reload it without problems

Changed 11 months ago by Sandarac

Fixes.

comment:18 Changed 11 months ago by Sandarac

Hello mimo, thank you for the review, this new patch should fix all of the remarks.

I also added

if (m.GetLandAccess(gameState, ent) !== unitAccess)

to the garrisonSiegeUnit function in defenseManager.

For garrisonAttackedUnit, the function will not select a structure if it is full according to the garrisonManager and there are no units physically in the structure (i.e. in ent.garrisoned()) to eject. But if it is full and there is at least one unit in ent.garrisoned(), it will eject one to make room. I believe that the population cost is irrelevant when it comes to space taken up in a garrison holder (for example, many heroes have a population cost of two, but they always take up one spot in a garrison holder). So in any case only one unit will need to be ejected.

I do not believe there are serialization issues. I tried reloading several save games without issue, and in any case I followed what is done in the other Petra managers for serialization/deserialization.

Edit: After some more testing, the changes in the new patch do not really help the garrison issue (the number of garrisoned units isn't updated in time after a unit is ejected, so the hero doesn't end up being garrisoned, which you pointed out). And even if the AI waits for one turn, there is no guarantee that some other unit won't be assigned to the new available spot in the holder during that turn instead of the hero, so I'm not sure about adding the "eject" feature.

Last edited 11 months ago by Sandarac (previous) (diff)

comment:19 Changed 11 months ago by mimo

In 19085:

Petra: improve regicide support, patch by Sandarac, refs #4142
add a gametypeManager and improve garrisoning of hurt hero

comment:20 Changed 11 months ago by mimo

Thanks for the last patch. As it looks good now, i've commited it, with the few changes listed below. Let me know if you agree with them. changes compared to your patch in gametypemanager line 71-72, replaced emergency by this.heroGarrisonEmergency line 77 leaveGarrison was not supposed to be called outside the garrisonManager as it would then require to also update the holderList. I've added a new function cancelGarrison.

possible cleanings in gametypeManager for future patches

  • when regicide, cache in a gametypemanager Map the hero id and its garrsion emergency, with a generic name so that the same array can be reused in possible future herocide, regicide and whatever xxxxcide mode, and instead of testing if the entity has the Hero class in the various places, test that it is in this cached Map. That would incidentally solve the case of a mod allowing several heroes, as only the initial ones would be cached and thus taken into account for the herocide, and if we would like to include the new ones, it would be enough to update the Map in gametypeManager.checkevents when a new one is created.
  • caching the stance when changed in the Map would allow to revert the stance (line 127-129) only if needed, or maybe we should add a common-api function getStance.

and in garrisonManager

  • possibly improve the naming of the garrison cases (for example "protection" -> "healing", and "emergency" -> "protection"), and the Siege+!Melee could go in the emergency case when garrisoned from garrisonSiegeUnit, so removing the need to have its special case in the keepGarrisoned function.

Concerning serialization, i didn't mean that there was a problem :) (in fact the code looks good and i did a quick test myself), i just wanted to be sure you also tested it and that i didn't miss anything.

comment:21 Changed 11 months ago by elexis

Offtopic, but r19050 setAccessIndices instead of SetAccessIndices as pointed out by Sandarac.

comment:22 Changed 11 months ago by mimo

already fixed in r19080 :)

Changed 11 months ago by Sandarac

Healers will be assigned to guard the hero.

comment:23 Changed 11 months ago by Sandarac

The changes were all good; in particular I should have realized I could have used this.heroGarrisonEmergency = target.healthLevel() < 0.4 instead of another variable.

For the Map object in gameTypeManger, if I understand correctly, the map's keys would be hero IDs (not entities?), and the values would be objects holding garrisonEmergency and the heroes' current stances? i.e. on init: this.heroes.set(heroEnt.id(), { "heroGarrisonEmergency": false, "stance": "aggressive" }); ?

This new patch includes the "healer guards" portion. The last three comments in comment:13 still hold for this patch, and so if the hero happens to be garrisoned when a new healer is spawned (or if the AccessValue? is different), it cannot be added as a guard, so the counter increases but no new guards are assigned.

Also, there was an error in my previous patch. In the for (let evt of events.Garrison) loop in gameTypeManager, it did not check that the hero entity belonged to the AI player, so I fixed that.

Last edited 11 months ago by Sandarac (previous) (diff)

comment:24 in reply to:  23 Changed 11 months ago by mimo

Replying to Sandarac:

For the Map object in gameTypeManger, if I understand correctly, the map's keys would be hero IDs (not entities?), and the values would be objects holding garrisonEmergency and the heroes' current stances? i.e. on init: this.heroes.set(heroEnt.id(), { "heroGarrisonEmergency": false, "stance": "aggressive" }); ?

Yes that is what i had in mind, but that could be improved if you have better idea. We could also possibly add there the list of guards (which should just become a list of ids, see following comments).

Concerning your new patch, i had no time to look at it in details, but here are a few suggestions entity.js

  • move canGuard above, after canGarrison where all internal functions are, while the last part is for the Engine functions.

gametypeManager

  • may be better if this.heroGuards only contains the guards id and not the full entity. That would be much simpler for serialization (i'm not sure having entities will work on deserialization as all entities are recreated from the AIInterface info) and i'm also not sure how indexOf works with such a complex object (could lead to poor performance).
  • lines 108-113: what will happen to the healer if not in the same accessibility? requiring a transport may be more efficient than training a new healer, specially in naval map where you may not have a temple in the island where you have the hero. Same thing when a hero is garrisoned in a ship and its guards are removed.

A temporary solution would to check every 50 turns (in the block 184-189) if you have some healer on the same accessibility than the hero and not guarding it, and make them guard it, and if not accessible, require a transport.

  • assignHealers: this function is not really assigning healers, but training new healers

the AI currently tries to build a temple only at phase 3 (or if it's ready to update to phase 3 but needs town structures). You may want to start building temples already at phase 2 when in regicide (see buildtemple in headquarter.js).

  • line 168 I very rarely play with healer, so i'm not good at guessing the needed number, but something like 2 + Math.round(this.Config.personality.defensive * 2)

would look better.

Changed 11 months ago by Sandarac

Update and apply mimo's remarks, except without Maps.

comment:25 Changed 11 months ago by mimo

Looks good. I will commit it. There are nonetheless a few things which can be improved:

  • needed: before assigning a guard to a hero, check that it has not already required a transport (ent.getMetadata(PlayerID, "transport") !== undefined if waiting for a transport, !ent.position() if inside a transport). Maybe check that on top of the assignGuardToRegicideHero function.
  • nice to have: line 190 of gametypeManager have a hardcoded healer template. There are already a lot of such hardcoded template names, so not a real problem to leave it here, but it would be nicer to avoid it by having some function which loops on the productionQueues for entities with the Healer class. We already have such functions (see findBestTrainableUnits in headquarters or getBestShip in navalManager).

and thanks for fixing my typo in cancelGarrison.

Edit: while commiting, i've added the missing check on ent.getMetadata(PlayerID, "transport")

Last edited 11 months ago by mimo (previous) (diff)

comment:26 Changed 11 months ago by mimo

In 19089:

petra: add healer guards in regicide mode, patch by Sandarac, refs #4142

comment:27 Changed 11 months ago by elexis

I don't know how feasible this is (and it's cheap to give feedback after the commit), but perhaps the code that is only relevant to regicide mode could be moved to a new file (I imagine the file could become confusing if we add more gamemodes).

comment:28 Changed 11 months ago by mimo

r19085 added a new gametypeManager file to deal with the specific code of the different gametypes (currently wonder and regicide). I think that's sufficient for the time being. If this file grow too much, it will be easy to split it later.

Changed 10 months ago by Sandarac

Handle multiple heroes in regicide, and cache entities important for specific victory conditions.

comment:29 Changed 10 months ago by Sandarac

Description: modified (diff)

In this patch, the heroes spawned at the start of the game in regicide will be considered the "critical" ones necessary for the game, and the AI will assign healers to each of them. A generic Map is used that can be utilized for other victory conditions.

But there is a small issue: when init is called for the various managers at the start of a new game (turn 0), the trigger script has not yet spawned the hero unit(s). So the "init" of gameTypeManager (caching the gameType-critical ents like heroes in regicide) has to be called on the first turn. In this patch this issue is handled in update of gameTypeManager.js.

There is a possibility that the AI will train too many healers relative to its population cap if there are many units that it needs to guard; I guess this could be avoided by setting the max number of healers/guards in total that it can train.

comment:30 Changed 10 months ago by mimo

Hi sandarac, thanks for the patch. Here are some comments

line 32: should not presuppose the stances. You should add hero.setStance to put it in the wanted stance. line 72: the test on evt.target could be move before line 70 (i.e. before getEntityById) line 113-122: you treat the case when one of the guards dying, but not the case of the criticalEnt dying? you should remove its guards and remove the hero from the criticalEnts (in case we have a regicide variant with two heros). Futhermore, i think the code could be simpler if data.guards was a Set instead of an array (Set is more adapted when frequent removal and addition). line 138: same comment as for line 72. And in fact, we don't need anymore the checks on ent. line 165: shouldn't it be !this.guardEnts.has(evt.entity)? line 235: could be renamed assignGuardsToCriticalEnts loop 242-246: could have an early-continue (if (length > min) continue;) for less indentation line 268: for consistency, important -> critical

and agreed that we should add a max number of healers (something like pop/10 ?)

comment:31 Changed 10 months ago by Sandarac

Hello, I updated the patch, but concerning line 72 I used !evt.target || !this.criticalEnts.has(evt.target) (I don't know if the first check is needed).

Also, there was an error in the previous patch. In the assignguard function, if the hero was garrisoned when a new healer was trained, the healer would not be added to this.guardEnts (and so it would never be assigned even on Ungarrison of the hero).

I changed line 165 to !this.guardEnts.get(evt.entity) because all ents used as guards are in that Map whether they are guarding at the time or not.

I renamed some variables in assignGuardToCriticalEnt to make it clearer which ent is guarding and which is being guarded.

Changed 10 months ago by Sandarac

Fix issues.

comment:32 Changed 10 months ago by mimo

Hi Sandarac, I've been through the new patch, and here are my new comments (some of them i missed on the previous patch, sorry about that but hopefully we'll soon review through phabricator which ease a bit the review process).

lines 29 and 35: would be better with a temporary variable (let stance = hero.hasClass("Soldier") ? "aggressive" : "passive") as otherwise somebody could change it in one place and miss the second one. line 74: first test not needed as we can count on the fact that evt.target is defined for such message, and futhermore i think undefined is a valid key for a Map (at least Map.has(undefined) works). line 77: as the AI is run only every n turns, the target may have been killed in the meantime, and thus getEntityById would return undefined. So better keep the test on !target line 168: here also it could be safer to test that gameState.getEntityById(guardId) !== undefined before calling removeGuard line 179: for the same reason, it should be followed by a "if (!ent) continue;" line 192: ent.id() could be replaced by evt.entity (seems clearer to me?) line 198: again protect the getEntityById call in case the healer was killed line 215: pickHeroRetreatLocation could now be renamed (pickCriticalEntRetreatLocation or shorter pickCriticalEntShelter or whatever) line 235: same comment (trainCriticalEntHealer?) line 263: we should protect that code (if (!criticalEntId) return) line 265-266: why not moving them on top of that function? maybe also exiting the function if this.guardEnts.get(guardEnt.id()) is true? although I suppose that should never happen when this function is called line 312: do you really want to use getPopulationMax() and not getPopulation() which is the current population?

Changed 10 months ago by Sandarac

More refinements.

comment:33 Changed 10 months ago by Sandarac

Hello mimo, thank you for your help. I noticed that there was also another missing check on the value returned from getEntityById (the check on the ship if (!gameState.getEntityById(evt.holder).hasClass("Ship"))), so I fixed it.

The two functions have been renamed, and I changed some of the variable names in trainCriticalEntHealer to be more generic.

For the choice between getPopulationMax() and getPopulation(), I thought that it would make sense for the AI to train many healers regardless of the current population cap (as long as it has the resources). But it does now occur to me that it could use getPopulation() instead, maybe if its aggressive trait is above a certain level.

I am planning to add champion guards to the wonder in wonder victory mode. What would be some good parameters to use for headquarters.findBestTrainableUnit() for this? (The units would then be assigned to the wonder with assignGuardToCriticalEnt).

comment:34 Changed 10 months ago by mimo

In 19163:

petra: cleanup and improve regicide support, patch by Sandarac, refs #4142

comment:35 Changed 10 months ago by mimo

Thanks for the last modifs. I'm still worried by the getPopulationMax(). Although the queueManager is supposed to take care of the resource sharings, it is not very clever and has to be guided. Allowing it to build PopMax?/10 = 30 healers (if we have several critical entities) as soon as it has a temple, even if its current pop is only 60 (which may happen) has the risk that it devotes too much resources for healers instead of strenghthening its army. I've no a priori idea on the needed number of healers, so if you think that Pop/10 is not enough, maybe something like min(PopMax?/10, Pop/4) would be ok? but i think it is important to have a limitation depending on its current pop. Another way to "guide" the queueManager would be to change the priority of the healer queue when numberOfhealer > pop/10 (see how it is done in headquarter/buildMarket with the reset of the priority at its default value once the entity has been funded). And finally, it may be better to add a new queue instead of reusing the villager one so that both productions can go in parallel.

I have nonetheless committed the current patch which is good now so that new changes can be a small patch (I've removed the useless continue in lines 149 and 210 compared to your patch v1.8).

comment:36 Changed 10 months ago by Sandarac

Keywords: rfc removed

comment:37 Changed 9 months ago by Sandarac

Wonder guards patch: https://code.wildfiregames.com/D140 Regicide military guards patch: https://code.wildfiregames.com/D157

comment:38 Changed 9 months ago by Sandarac

Resolution: fixed
Status: newclosed

Regicide support for Petra is largely complete as of r19241

Note: See TracTickets for help on using tickets.