Initial Civ Personality implementation

[SeventhM]

The is a PR based on combining elements of #10791 and #10717 (sorry @MioBestWaifu, but I kinda got impatient) as well as somewhat closely working with Victory.Focus. There are some obvious questions, notes, and other aspects to consider from this PR

• The most obvious question is which things should have focus. Currently, it seems to me that the obvious considerations is stats. Production and happiness is probably the only ones that's even remotely questionable. Even food has an obvious usecase for a civ like India whose whole gimmick is growing as much as possible. Next would be how often they build units, then how often they declare war, and lastly possibly considerations for how often it should trade or how often it train settler units
• The second most obvious question is scale. While it was sometimes inconsistent at it, Implementing basic civ personalities in diplomacy #10717 seems to have focused its scale at 1-100. I'm not sure if that level of granularity is necessary, so for now I've chosen to focus in on a 1-10 scale, while allowing for floats for additional granularity if needed. That said, regardless of the scale, I'm not sure how a mod creator might interpret this numbers or how well they might be felt moving forwards
• There's very obvious oddities with Victory.Focus. I'm not sure how much of that sure get fixed with this PR or how much should be split down into other PRs. Given that some of the work on this is to also base some of the work on Victory.Focus, it seems worthwhile to fix some of its details as well
  • ConstructionAutomation adds modifiers to buildings based on whether they provide a stat rather rather how much of the stat it provides, which seems odd to me
  • There's numerous times where we treat something as if there can only be one Focus when that's not how Victory.Focus works. I cut down some of the if-else chains for supporting city states, but haven't touched them elsewhere yet
  • Why does Victory.Focus.Production even exists? Shouldn't we focus on production solely for building the victory buildings? Seems kinda overkill in all other situations
  • Victory.Focus.Faith currently does not have an implementation. This PR does give it a minor one, though the Faith focus likely should also prioritize buildings that spread religion faster as well
  • There's obvious weirdo behaviors, particularly from Focus.Culture, that are pretty unnecessary with the extra granularity of the other Personality variables. This PR is disabling them if it detects a functioning AI personality
• I haven't implemented it as of currently, but I'm planning on moving the preferred victory type to the Personality instead of the Nation, since that is what it's more closely aligned with
• Given that there's a variable, a float no less, for every stat, it may make sense for this to be a RulesetStatsObject rather than a RulesetObject. I haven't put too much time in seeing which one makes more sense here.
• There's a broad question of how uniques should play into this. I know Yairm has stated ideally new stuff like this should be in uniques rather than hardcoded values/variables, but I do this there is a bit of an exception here. That said, there may be room for personality specific uniques for focusing efforts on specific buildings/units or increasing or decreasing the focus as conditions change. One of the main advantages of this being a RulesetObject is that it does still have a functioning uniqueMap if we want to use it.
• There's also a question of if it should even return a Personality if a player is playing. Afterall, the player should choose what to focus on/ prioritize in that case

I would definitely like it if I can get help from @tuvus on what should be affected by this PR and where things should split into separate PRs/what should be adjusted regarding what already exist for Victory.Focus

[tuvus]

Some comments since they were asked for:
(civInfo.getPersonality()?.happiness ?: 5f).toInt() - 5 Could we change this and variations to be a function in Personality.kt so we don't have to deal with it in the other classes? In my opinion, a value around 1.00 so we can multiply something by it so we can see all the integer values in one place, seems best for encapsulation. Like 376 in ConstructionAutomation.kt.

What are the reasons for having a hardcoded preferred policy? If we improve our policy picker to convert each policy branch to its stats, we could maybe use the other personality fields instead. It would take a lot more work, but we need to find a way to rank uniques anyway if there isn't one I think.

Note that I have limited knowledge of how wantsToFocusOn works but I don't think hardcoding preferred victory type is the way to go. I think it might be easier if each victory had its own int value since a civ might be eligible for multiple victory types and generally, we only care about one at a time.
For anyone who doesn't know http://civdata.com/ has the personality information. We certainly don't want to have all of that, though. (The Gandhi nuke thing is kind of annoying)

A question that we are likely to come across in the future: Is there a way for us to make the Civ personalities changeable during the game? It might be an interesting way for the modders to interact with the AI through fancy building events and policies.

There's also a question of if it should even return a Personality if a player is playing

My idea would be that players always have a personality and can customize the basics in the AutoPlay tab or something. We will almost always need to do some work of automation for the player so this may be able to give them more control again.

For a disclaimer, I haven't looked into ConstructionAutomation very much yet, but I plan to look into it after working on the Worker AI.

[SeventhM]

What are the reasons for having a hardcoded preferred policy?

Just a way of working with what was already there. Currently, it's already hardcoded to work based on the preferred victory type, all this would do is add in the preferred policy alongside it. And I can see a mod creator wanting an AI to focus on a policy branch over working for what's best for those stats

A question that we are likely to come across in the future: Is there a way for us to make the Civ personalities changeable

Actually something I was thinking about too. Actually, that also leans into my questions about possible uniques going forwards, but that should probably be in another PR. It could slot into getPersonality pretty nicely, though

[SeventhM]

After taking a break from this PR, I'll try to at least get it to a ready level. Minimum, this is to split some extra elements of the AI's behavior I want to look at into its own PR. Question, btw: why was the old culture AI adverse to building defensive stuff? I can maybe, possibly, not really understand why it didn't attack, but this has nothing to do with offense

Interesting strange thing: Victory.Focus.Military doesn't declare war more often than everyone else. You would expect that it does, but the only reason it declares war more often is it has a larger military.

[yairm210]

Sounds like getPersonalityValue(personality) one liner, you have this 5 time

[SeventhM]

I'm down for getting rid of these parenthesis, but now that I look at it, wouldn't using .get be better than that? Actually, on second look, I don't need the parenthesis in the first place

[yairm210]

*=, I assume, also for the rest of this function

[SomeTroglodyte]

A kotlin quirk that throwing away the results of an expression does not get a warning.
(Is city.civ.getPersonality() expensive? val it if so - this is automation and runs middlish-often I would expect)

Also - this is rankStatsForCityWork, while worker automation is controlled by Automation.rankStatsValue - see also #11082 - you have considered it I assume?

[SeventhM]

Most of the cost of city.civ.getPersonality is looking up the ruleset... but, hmm, yeah, probably should be a val

Haven't considered worker automation (I mostly looked at where the existing victory focus code was, none is there to my knowledge. Would rather work on it as a separate PR

[tuvus]

Can we change these civPersonality[PersonalityValue.Culture].toInt() - 5 lines to use something like civPersonality.getNormalizedValue(range = 5) to be more readable. The important thing is the range parameter shows here what values it will give out for readability.

[SeventhM]

I know Yairm mentioned something similar, but my issue is kinda simple: wouldn't that read as civPersonality.getNormalizedValue(PersonalityValue.Culture, range = 5). Which means

1. There's no real shortening of lines, nor any real simplification. If anything, it makes lines longer
2. It's only better for readability if you know what a "nornalizedValue" should mean in this case. Sure, kdocs, but I'm pretty sure we could address questions here with kdocs as well
3. You're suggesting a more expensive version of .get or (.scaledFocus * 5).toInt()... For just this?
4. Why not just create an overload for scaledFocus? I'm all for doing this, but then my question is "why are we using that as a replacement for square brackets/.get"

[tuvus]

Why not just create an overload for scaledFocus? I'm all for doing this, but then my question is "why are we using that as a replacement for square brackets/.get"

The main goal is abstraction, encapsulation and lower coupling. If I am accessing a certain civPersonality in some other file, the reader shouldn't have to know about how the actual personality data structure is implemented. (ie: is it stored in a hashmap and what is the range of the personality values?) It doesn't have to be shorter and the performance cost here is not an issue.

Also (range = 5) and getNormalizedValue were just a suggestion.

[SeventhM]

Ok, but I'm not sure where the benefits of encapsulation is here, especially since there already is a getter/setter that already handles things pretty cleanly. Like, the code will always have to read personality.getterFunction(PersonalityValue, whateverArgument), with the toInt likely being ideal as well

Again, I can totally seeing adjusting scaledFocus to cover what you want. But the only thing your suggestion addresses assuming the scale is 0-10 vs any other set of values... Which shouldn't matter here anyways because it's still going to be communicated to the modder as 0-10, and this usecase in particular is still using specifically that assumption

[SomeTroglodyte]

That -5 discussion prompted me to actually read the new class. Food for thought:

• I dislike hybrid files that show the 'can't classify' icon in Studio, but I like namespaces...
  - PersonalityValuecould become Personality.Value with very little work
• Space after type colon and before opening curlies and before inheritance colon...
• Writing fun... : Type { return as fun... = very often reads better
• basePersonality as both kinda-instance name and field name? I'd name them getNeutralPersonality and isNeutralPersonality
• the field has a special role, it's not read from json, but you want nobody outside to mutate it. Put it in a private primary constructor val parameter with default, and only the companion can instantiate with a true value?
• the companion offers a factory for that neutral Personality - but a single instance would be enough, no need to allocate each and every defaults-to - that is, if they run often enough, as in at least once per game.
• ... that could be even academically safe if Personality were immutable to the compiler. Possible, as Json inserts values via reflection. I might be wrong, it runs, but as the branch has no test data, that change is not tested by a simple start.
• the set operator is never used - together with the fun syntax that makes the change to immutable trivial.
• OK, immutable goes too far, we have runtime-immutable var's all over the place in our RulesetObjects because so far we do NOT rely on Json setting val's through reflection.

As you can see abstracting the scale of those values didn't even cross my mind. As they are meant to be mostly gleaned from public sources on the original, documentation could be enough. Having a public API that normalizes 0..10 to, e.g., -1..+1 and then have AI code that maps those back to 0..10 weights is academical standard, but also a bit silly, compromises are allowed imho.

[SeventhM]

• PersonalityValuecould become Personality.Value with very little work

Not at computer, some of this message is academically above my head, especially without a computer to get a feel for exactly what you mean (I'm, like 90% self taught, 10% learned the basics of c++ in highschool, only learned java by tearing apart a friend's project), but this statement I 100% can comment on. On paper this sounds right, in practice... How do you import this?

• You import Personality and use Personality.Value? This introduces a new standard for just this one enum that we basically never use elsewhere. It's easy to believe an unknowing developer not knowing the intent and instead just importing Personality.Value
• Import Personality.Value? Now you have something like Value.Food. To vague imo, compared to say Stat.Food
• Introduce some easy way to use Stat here to avoid weird vagueness? Idk, I can't see a way of handling that wouldn't be a massive hassle

If the insistence is not having multiple classes/enum s in the same file, then fine (though I don't think it feels particularly worthwhile here besides looking at Studio's interface). But renaming it to that in particular would be pretty awful

Better suggestion is Personality.PersonalityValue. Been away from a computer all day, but I'm not so sure that's too great either

• the set operator is never used

I'd be fine with removing that and making it immutable, though imo seems a bit early to rule out

[SomeTroglodyte]

Was just ideas. Yes, you got it - import Personality and use Personality.Value. Not a new standard - I do it, Gdx does it. You're right about the naming - Gdx does it like: Label.LabelStyle... And not only unknowing developers might ruin that, Studio's newer aggressive autoimport too with default settings (heck it imports ImageGetter.ruleset unasked if you start to use the name without declaring the parameter first)...

As I said, just ideas.

But "bit early to rule out a mutable Personality" sounds good, like - changing personality = Education = you can teach Gandhi to be nice and not nuke everybody... 💣 💥 👼

[SeventhM]

• the field has a special role, it's not read from json, but you want nobody outside to mutate it. Put it in a private primary constructor val parameter with default, and only the companion can instantiate with a true value?

Like StateForConditionals.IgnoreConditionals? I guess that works, but Idk. Pretty much everything else here isn't in the primary constructor which seems inconsistent? Idk, maybe I'm worrying about formatting a bit too much here, just seems like it'll look odd

• the companion offers a factory for that neutral Personality - but a single instance would be enough, no need to allocate each and every defaults-to - that is, if they run often enough, as in at least once per game.

Now that... that is possible. I wonder if I need a clone function with that. Well, if it's expected to be immutable, it shouldn't be an issue, but...

[SeventhM]

import Personality and use Personality.Value

Oh, huh. Studio's aggressive auto imports actually imports like this. Neat

Still not sure addressing a quirk in Studio is reason enough that this is worthwhile, but I guess my import concerns isn't as valid as I thought

[SomeTroglodyte]

Like StateForConditionals.IgnoreConditionals

Not quite. StateForConditionals is a data class, immutable, never read from json. I mean

class Demo private constructor (val isSpecialDefault: Boolean = false) {
    var someField = ""
    var anotherField = 0
    companion object {
        val default = Demo(true)
    }
}

Now Demo.default is the only way to get an instance which is Demo.isSpecialDefault == true, any instance from json deserialization will have the flag off.

look odd

Look different, which could be the point. Tons of possible permutations, up to using a class hierarchy as "type" marker... I just thought about setting apart visually that some fields are json-controlled others not, and since there's no @Transient because it's not serialized, only deserialized... As I said, only ideas, go with what you like best.

[yairm210]

I think there have been a lot of discussions about the future, but I want to know if I can merge this PR :)
I personally don't have any remaining issues that would prevent me from merging this, looking at Tuvus he doesn't seem to either (it's just changes on existing structure, not restructure)

@SomeTroglodyte - it's hard to separate your "maybe think of" comments from "I think this needs to be different" comments.
Are there any remaining points that would in your eyes prevent this being merged?

[SomeTroglodyte]

in your eyes prevent

No! If in doubt, always assume my comments are food for thought only.
The only reason not to would be that we can't yet play-test it as there is no data. But that's planned that way.

[tuvus]

What is the purpose of neuteralPersonality? I don't understand yet what Civs this should apply to. It seems to break a few things right now and maybe more in the future, but I want to know if there was a purpose for incorporating it.

[SeventhM]

Things being broken is a bug. See #11158. neutralPersonality otherwise is supposed to detect a personality isn't implemented and use assumptions from before this PR