The current AI behoviour has only be set to add variety in the way the AI would react, it has never been tuned to be consistent: for example the defensive and agressive behaviour are just used by the AI in some conditions but are in no way some consistent strategical behaviour, and it has no sense to restrict it as done in the patch. (lines 130-132 of config.js). We need to keep a true "random" level as it was before.
All that patch was a good first step by Sandarac as he was commited to work on it, by commiting it like that with nobody working on its tuning has no sense to me. The technical part is good, but not the interface with the ai.

I submitted the previous comment too fast.

In rP20646#25976, @mimo wrote:

it has never been tuned to be consistent, in no way some consistent strategical behaviour

Nor is it claimed to be. It is claimed that we now have a GUI dialog to select one and that this behavior setting does influence the AI.
Whenever a new AI patch is added, one has the possibility to add some conditions. It's degree of freedom, not a necessity to rush fine tuning.

and it has no sense to restrict it as done in the patch. (lines 130-132 of config.js). We need to keep a true "random" level as it was before.

no sense are hard words. See inline comment there for an explain which sense I see in Sandaracs patch.

All that patch was a good first step by Sandarac

Agree, and as a first step it was committed (Maybe a second rather since there are already few config variables influencing the behavior).

commiting it like that with nobody working on its tuning has no sense to me

As mentioned in the review, in an 8 player game with 7 defensive and 1 aggressive AI, only the aggressive one did rushes.
So there's sense number one to me. The function of the button is already paid with that.
Hence nobody has to commit to fine-tuning AI behaviors.
It's a degree of freedom to be able to add behavior conditions, similar to the ones in this patch and similar to the difficulty conditions when writing new AI patches.

No because that patch changes the AI behaviour in an undefined way, which i don't want to spend time to understand.
You must restore the current behaviour (a fully random one) on which we can base something and try to improve the behaviour, not force everybody to adapt to 3 behaviours which have never been tested. Then if there are more options i don't care, but allowing players to choose options which have never been tested nor tuned can only lead to disapointment.

Futhermore i disagree that your test was a right one to justify that patch (you prevented all AIs except one to attack and concluded that indeed that change give consistent behaviour because only one was attacking)

My two cents regarding the review status of this commit (I don't have a lot of knowledge of the code here): @elexis if you need to change a patch, you have to commandeer it and then ask for review(s). "Based on patch by" is not one of the possibilities I listed on the staff forums when we discussed commit messages. I will try and update the ReviewingPatches wiki page as soon as I can.

If we ever want to be able to select an AI behavior then we do need the C++ and GUI part of the patch.
Since it's already possible to notice a difference, the player already has a benefit from the patch.
Since it's possible to build upon the behavior ID, the AI developer has a benefit from the patch too.
Even if Sandarac isn't going to provide more patches.

Just like the AI difficulties medium and hard, the types "aggressive" and "defensive" are whatever any AI developer wants it to be (balancing).
Noone is obligated to do anything with it, but it's a degree of freedom for most future AI patches.
As I stated I did watch the AI in maybe 5 matches (about 2h gameplay time with the 20x gamespeed) and notice the different behaviors.

There are three prototypes General Aggressive and Defensive.
The meaning relates to the aggressive and defensive AI properties, so it's not completely bogus.
They are only a name, two numbers and one condition right now, it can change arbitrarily.
The first patch is inherently not meant to be a final tuning, but just adding the infrastructure to build changes upon.

If one selects random, any configuration value can appear, so maybe that is already what you want with regards to the "random" part, or do you prefer the General AI to be possibly compeltely defensive (0) or completely aggressive (1)?
Adding a new character only requires a string and testing for that (In the tickets there were more ideas mentioned).
Editing a character also requires a line or two to begin with.

In rP20646#25995, @Itms wrote:

My two cents regarding the review status of this commit (I don't have a lot of knowledge of the code here): @elexis if you need to change a patch, you have to commandeer it and then ask for review(s). "Based on patch by" is not one of the possibilities I listed on the staff forums when we discussed commit messages. I will try and update the ReviewingPatches wiki page as soon as I can.

The only changes I did were gamesetup ones that noone complained about.

Commiting the c++ part was completely fine (and good), but

• the gui dropdown to select the behaviour should have been kept hidden, waiting for somebody to do some work on the ai side to use it
• the default behaviour of the ai should have been kept at its current value (all 3 behaviour chosen randomly).

As i said, i don't know what these settings do, nor how do they interfere with the difficulty. Some combination of them lead to very strong Ais (even in easy level) while others lead to dumb AI. With everything random, on average, the required difficulty is satisfied, but i guess we all experienced games where the AI was really very strong even on lower levels, and others where it was really weak on harder levels. We must understand (and that requires work, tunings and tests) to understand how all these settings interfere with each other before making them available. Otherwise, we don't know what we provide: it's very possible that what you've defined as a defensive AI is quite weak, and that even in hardest level, every beginner with a minimum of experience will beat it. That's what i call inconsitent behaviour, and not one i want to propose to players (except if somebody commits himself to do some checks and stats to tune them).

The gamesetup default is random (defaults.json), can be changed in the options, same as rP20566. (player_defaults.json is / should be used in Atlas and Gamesetup.cpp.)
Besides the attack plan diff, the patch didn't introduce aggressive or defensive AI behavior, only reduces the possible value range of aggressive and defensive which exist since rP14865, it shouldn't improve nor worsen the described effect.
Perhaps it's not so bad to be able to chose, even if the characters aren't as pronounced as they could be.

It is not the same as what was prior to the patch: random before meant completely random, while now it is one of the 3 predefined behaviour is chosen randomly. And as i said previously, switching to these 3 only behaviour is at best premature, or could even be inconsistent in some conditions. So you must revert back the ai to its previous behaviour.
In addition, i'm more than surprised that you could have commited that patch without any prior discussions with the guy who worked on the ai since several years (the fact that i've not commited that patch from sandarac while i've done it for all its other ai patches should already have given you a hint).

In rP20646#26014, @mimo wrote:

It is not the same as what was prior to the patch: random before meant completely random, while now it is one of the 3 predefined behaviour is chosen randomly. And as i said previously, switching to these 3 only behaviour is at best premature, or could even be inconsistent in some conditions. So you must revert back the ai to its previous behaviour.

Choosing different types AI behaviours is a nice feature (IMO) and doing that with setting some values like aggressiveness and stuff seems like the way to go. But as mimo suggests a true random behaviour (as in all values random, not a randomly chosen one) is very nice too. So why not add such in the list? (or wasn't the generalist meant to be like that?) That way we still have the behaviour we can build on and the various types can be a substraction of that.

In addition, i'm more than surprised that you could have commited that patch without any prior discussions with the guy who worked on the ai since several years (the fact that i've not commited that patch from sandarac while i've done it for all its other ai patches should already have given you a hint).

I have to side with elexis here, since the patch has been online for about 5 month, and I perfectly understand you didn't have time for a full review (nor I want to force you in looking at every single patch that is related to AI, since that is undo-able), but when you have such a strong opinion against the patch posting that to the revision doesn't seem that much of a time drain. Also not posting comments is not the same as being against. f.e. example you might have missed it or didn't have time. However, that point I must give you, as the patch is so closely connected to the AI in general, and not being able to give your sight of things, is not the way we (should) work together.

In rP20646#26001, @mimo wrote:

Otherwise, we don't know what we provide: it's very possible that what you've defined as a defensive AI is quite weak, and that even in hardest level, every beginner with a minimum of experience will beat it. That's what i call inconsitent behaviour, and not one i want to propose to players (except if somebody commits himself to do some checks and stats to tune them).

So we end up with some more different things, so the player can figure out what suits him/her best. Looks like a feature to me. Ofc letting the AI do what is says it is doing, is a must too, but that comes from tuning and stuff. I do understand that you do not want to do that, but without a tentative support there wouldn't be any tuning ever.

I propose to fix the various inlines and add a true random behaviour, opinions on that?

In rP20646#26015, @bb wrote:

In rP20646#26014, @mimo wrote:

It is not the same as what was prior to the patch: random before meant completely random, while now it is one of the 3 predefined behaviour is chosen randomly. And as i said previously, switching to these 3 only behaviour is at best premature, or could even be inconsistent in some conditions. So you must revert back the ai to its previous behaviour.

Choosing different types AI behaviours is a nice feature (IMO) and doing that with setting some values like aggressiveness and stuff seems like the way to go. But as mimo suggests a true random behaviour (as in all values random, not a randomly chosen one) is very nice too. So why not add such in the list? (or wasn't the generalist meant to be like that?) That way we still have the behaviour we can build on and the various types can be a substraction of that.

The problem is not the feature (who would be against it) but because using it as is done, without checking what the code does and testing different configurations is not the way to develop the ai. The variables used, while connected to the wanted, behaviour were not used for that in the first place, but only to have a different behaviour according to some random.

In addition, i'm more than surprised that you could have commited that patch without any prior discussions with the guy who worked on the ai since several years (the fact that i've not commited that patch from sandarac while i've done it for all its other ai patches should already have given you a hint).

I have to side with elexis here, since the patch has been online for about 5 month, and I perfectly understand you didn't have time for a full review (nor I want to force you in looking at every single patch that is related to AI, since that is undo-able), but when you have such a strong opinion against the patch posting that to the revision doesn't seem that much of a time drain. Also not posting comments is not the same as being against. f.e. example you might have missed it or didn't have time. However, that point I must give you, as the patch is so closely connected to the AI in general, and not being able to give your sight of things, is not the way we (should) work together.

I had no strong opinion, as i still expected that sandarac would come back, and i would have trusted him to work on that to solve the problematic points. That's completely different to commit it without anybody commited to work on it.

In rP20646#26001, @mimo wrote:

Otherwise, we don't know what we provide: it's very possible that what you've defined as a defensive AI is quite weak, and that even in hardest level, every beginner with a minimum of experience will beat it. That's what i call inconsitent behaviour, and not one i want to propose to players (except if somebody commits himself to do some checks and stats to tune them).

So we end up with some more different things, so the player can figure out what suits him/her best. Looks like a feature to me. Ofc letting the AI do what is says it is doing, is a must too, but that comes from tuning and stuff. I do understand that you do not want to do that, but without a tentative support there wouldn't be any tuning ever.

I propose to fix the various inlines and add a true random behaviour, opinions on that?

No. as said above, this is still too premature to have it with the current code.

In rP20646#26022, @mimo wrote:

The problem is not the feature (who would be against it) but because using it as is done, without checking what the code does and testing different configurations is not the way to develop the ai.

Most likely things could improve in this way...

The variables used, while connected to the wanted, behaviour were not used for that in the first place, but only to have a different behaviour according to some random.

I don't really see the problem in setting those values a little less random with some player's setting as long as the fully random is still available. Ofc when the values does not do what the are supposed to do that is another story, and indeed if that is the case (I certainly didn't test the patch :P) we indeed run into some bugs that have to be fixed. But wouldn't that already be a bug now? only just revealed by this commit?

I had no strong opinion, as i still expected that sandarac would come back, and i would have trusted him to work on that to solve the problematic points. That's completely different to commit it without anybody commited to work on it.

It is indeed sad @Sandarac is not here to fix some things... but should that block us on developing?

In rP20646#26001, @mimo wrote:

Otherwise, we don't know what we provide: it's very possible that what you've defined as a defensive AI is quite weak, and that even in hardest level, every beginner with a minimum of experience will beat it. That's what i call inconsitent behaviour, and not one i want to propose to players (except if somebody commits himself to do some checks and stats to tune them).

So we end up with some more different things, so the player can figure out what suits him/her best. Looks like a feature to me. Ofc letting the AI do what is says it is doing, is a must too, but that comes from tuning and stuff. I do understand that you do not want to do that, but without a tentative support there wouldn't be any tuning ever.

I propose to fix the various inlines and add a true random behaviour, opinions on that?

No. as said above, this is still too premature to have it with the current code.

Also here I don't really see why, I certainly believe you on that the support of this feature in the ai code could be better, but as a first go it doesn't seem that bad. Also wouldn't this commit be a great startingpoint of making the code less premature? Having the option already available to the players would then probably highlight some of the points that can be improved and make it for us as developers easier to test.

This discussion is becoming ridiculous. Usually, people nitpick at length to commit a patch when a comma is wrong, and here you say that it is good to commit something untested, without knowing what it really does, just as a starting point for something that nobody is working on. I'm tired and won't touch a line of ai code as long as this patch is not reverted.

I agree to revert it. None of it really matters.

Thanks for the revert.