Merge Experimental AI candidate actions into the default AI #4600
Conversation
|
A few days ago I thought I saw something weird in the first commit, but I can't find it anymore, so I guess it's all good! |
|
Hmm. I really don't know a lot about how the AI works. But labels like "old default AI" make me nervious since they presume no further development and lead to "old old default" and "new old default". Perhaps what you're wanting is enabling and disabling constants/macros/whatever to control backward compatability? Do we even need backward compatability? |
|
Might I drop the suggestion of "1.14 default" here? |
|
@CelticMinstrel Is it the fact that the old default recruitment CA is still in the AI configuration, but commented out? While that's unnecessary, it's intentional (and not new, as you can see) to emphasize that it is replaced by the new recruitment CA. @GregoryLundberg @AI0867 Good, thanks for the comments. I don't like "old default" either, but couldn't come up with something I do like at the time. So I just put "old default" in there to provoke other suggestions 😉 I think "1.14 default" is good, so unless there are objections or other ideas, I'll use that. And yes, we absolutely need backwards compatibility. Although it's not really compatibility, but balance. As I wrote in #4598, the new AI will likely not have a significant impact on balance for the vast majority of scenarios, but it might for a few. And springing that on UMC creators on short notice without a fallback option would not be fair. |
It might have been that? I'm not sure. (But wasn't the new default supposed to enable both recruitment CAs in case there were recruitment instructions present, or something?) |
It is, and it will be. But you can't just slap the two into the AI together. There needs to be a mechanism to switch between them. I'll get to that as I work myself through the tasks of #4598. |
|
Just a small issue with the cost function, you seem to be assigning some global variables on lines 1624-26. Those should be declared local, shouldn't they? |
|
Yes, they should. Thanks for pointing that out. And for going through that code! |
mattsc
force-pushed
the
expai_ca_merge
branch
from
4e27134
to
27baf69
Compare
|
FTR: since there is no notification when one does this, I just squashed everything that needs to be squashed. Next big thing to do: figure out how to deal with the recruitment aspects. |
In the same way as they are set up for the default AI
... and call it the "1.14 default AI".
It is now identical to the default AI, so we only show it in debug mode in the MP computer player selection menu. It will be moved back to be always available when there is new development happening.
This adds the correct behavior for the castle_switch, move_to_any_enemy and place_healers CAs. It was already in place for the other former ExpAI CAs (except for recruiting and spread_poison, for which it does not apply). This automatically also adds this behavior to the Healer Support Micro AI.
This lets the AI find paths around areas defined by [avoid] tags, rather than being stopped dead by them. See comments in the code for details. The relevant custom cost function, custom_cost_with_avoid(), can be accessed directly as well.
mattsc
force-pushed
the
expai_ca_merge
branch
from
27baf69
to
3f6ea2e
Compare
|
I like the fan-out improvement to pathfinding, but doesn't it make more sense to make a sort of distance-map to the target (instead of the waypoint) and move to the lowest-distance unoccupied hex that is reachable this turn? I'm not sure what the actual distance metric should be. The number of turns needed to reach the target is important of course, but can be an underestimate due to more blocking later. Similarly, the number of movement points for the unit's movetype is imperfect due to rounding of multi-point moves. In theory, further improvements could be possible by considering every unit at the same time, but sounds complex, expensive and very susceptible to not being an improvement at all in the case of uncertainty. |
|
You're right on the principle, of course. The problem is that the only way to do this "right", I think, would be to do path finding from every hex to the target, and that's just prohibitively expensive. Besides that, the way point in this case is the next step on the shortest route toward the target for this unit, if there weren't an allied unit on in, taking terrain, [avoid] tags etc. into account. And the latter things are the problem, when it's not just open terrain, making sure this also works around bends etc. Thus, being the fewest movement point from the next waypoint is the best approximation I could come up with. I'm open for suggestions if somebody has a better idea. In fact, I think going one hex beyond this waypoint might be better, but I think that's as far as we can go. Even going two hexes farther can risk the unit going to the wrong side of an impassable wall. (I have tested this quite a bit by the way, and it works pretty well both on open terrain and going around obstacles.) One of the things I've learned with working on AIs is that it is rarely possible to do the job "right", and it's a constant struggle to figure out what counts as good enough. In that sense, I also don't think that the rounding errors etc. you mention are too important, but that's just a side note. The important thing is to find something that works pretty well in "all" cases, rather than something that works perfectly most of the time and screws up royally sometimes. Anyway, just rambling about things you know anyway... Not directly related: While thinking about what you wrote I noticed a different problem with the new path finding with avoid function. It currently fully takes enemies (blocked hexes and ZoC) into account - which is what you want in some cases, and not in others (e.g. when you want to find a path through the enemies). So I guess I need to add an ignore_enemies option to that one too. Sigh. Yet another option ... |
data/ai/lua/ai_helper.lua
Outdated
| @@ -89,11 +89,15 @@ function ai_helper.put_labels(map, cfg) | |||
| -- - keys: (array) if the value to be displayed is a subelement of the LS data, | |||
| -- use these keys to access it. For example, if we want to display data[3] | |||
| -- set keys = { 3 }, if it's data.arg[3], set keys = { 'arg', 3 } | |||
| -- - no_clear=false: (boolean) if 'true', do not clear existing labels | |||
There was a problem hiding this comment.
I'd rather see this called clear rather than dealing in double negatives.
You can still make it default to true by adding a line like if no_clear == nil then no_clear = true end.
data/ai/lua/ai_helper.lua
Outdated
| @@ -1410,6 +1410,12 @@ function ai_helper.next_hop(unit, x, y, cfg) | |||
| -- path: if given, find the next hop along this path, rather than doing new path finding | |||
| -- In this case, it is assumed that the path is possible, in other words, that cost has been checked | |||
| -- avoid_map: a location set with the hexes the unit is not allowed to step on | |||
| -- no_fan_out: prior to Wesnoth 1.16, the unit strictly followed the path, which can lead to | |||
There was a problem hiding this comment.
Again, I'd prefer this to be called fan_out to avoid dealing in double negatives.
I missed the context of the fan_out when I made my original comment. I didn't realize it was a parameter for a function that finds the next waypoint. Re-running A* at that point will probably indeed skyrocket the complexity, but there has to be a way (by describing a cost-function or something else) to make the pathfinder realize that a different path is currently better due to blocked hexes.
Assume the following map: Two large chambers, with two-hex-wide tunnels between them, one slightly longer than the other. |
And that's what By contrast, if you use the default path finder, There are still some limitations to this. If the move cost on the terrain within the same tunnel varies, there are situations when the unit would not find its way around other own units within that tunnel. This is due to fundamental limitations of the A* algorithm (or at least of the Wesnoth A* algorithm), and that's why the fan-out option to So maybe some of the confusion was due to the fact that I added two new pieces of functionality: one for path finding, one for finding the next hop along (or around) the found path. They are not to be used in place of each other, but complementary. |
It is now superseded by the retreat_injured CA (of the former Experimental AI).
The old villages CA is quite a bit better at distributing multiple units across multiple villages. The advantage of the new grab_villages CA is that it has a variable score, sometimes grabbing villages before, and sometimes after attacks. This does not outweigh its shortcomings though. So for now, the default AI will continue to use the previous CA, and the Experimental AI will use the new one. Thus, the two AIs are not quite identical any more (but still very similar). I also added a todo comment that the grab_villages CA might be reinstated if it is improved.
Most of the former Experimental AI CAs did already (or don't use the leader in the first place), but not quite all of them.
Previously the CA would already move multiple leaders if all leaders were to be moved, but it would abandon moving any leader after finding one that should not move.
If the units previously on the keep moved off, we don't have to use the low score any more.
Recruiting itself still only handles one leader. This will be taken care of later.
mattsc
force-pushed
the
expai_ca_merge
branch
from
827bd47
to
2cc8cce
Compare
battle_calcs.attack_rating() includes most of the contributions to the previous rating, plus a lot more.
In the same way as it is done in the combat CA
The algorithm used in this CA is too simple to work reliably in a general setting, it tends to send whole groups of units toward small numbers of villages, or even individual ones. In its current version, it should not be used at all, not even in the Experimental AI. The recommended way to emphasize village hunting is to set the village_value aspect to a larger-than-default value and let the move-to-targets CA take care of it. We are, however, leaving the CA code and the macros in place for potential future work.
With the merge of Experimental AI candidate actions into the default AI, a spread_poison CA is now included by default.
With the merge of Experimental AI candidate actions into the default AI, the retreat CA is now included by default.
This scenario has a custom CA for attacks, so all other attack CAs need to be removed.
The high_xp_attack CA is deleted when adding the Fast MAI, but not added back in when deleting the MAI.
This has no impact on functionality or use, it is simply done for consistency with other CAs and to indicate that these CAs are now part of the default AI.
It is too different from previous default recruiting and would significantly affect scenario balance. It also currently does not work with multiple leaders and many of the aspects. It will be made available as an option (and is already in the Experimental AI) after these shortcomings have been fixed.
mattsc
changed the title
|
I think this is done, so I removed the "WIP" from the title. I will let this sit here for a couple days for people to comment while I work on the wiki etc. There will be a number of followup tasks:
All of these will be done separately, so they do not impact merging this PR or closing #4598. Well, I guess, backporting bug fixes is listed as a task in the issue, but that one is easy. |
|
This is rather off-topic, but after looking at the most recent diff I noticed the |
|
Yeah ... Both those functions are quite old. I do go through the ai_helper functions occasionally, specifically also with the goal of eliminating slow stuff like |
See Feature Request #4598 for details.