Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Personality Behaviors Template #11165

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tuvus
Copy link
Collaborator

@tuvus tuvus commented Feb 20, 2024

This PR is just a discussion of values that should go in personalities #10939 to affect the AI behaviours. There is no actual implementation yet.

I suggest these 6 attributes that can influence most of the AI's behaviours without being too specific.
This PR is just a discussion of values that should go in personalities to affect the AI behaviours. There is no actual implementation yet. My goal is to change the intention of wantsToFocusOn() to shouldFocusOn() by replacing many of the uses with these new personality behaviours. Hardcoding a boolean value for the AI is almost never a good thing when it is used to directly limit what actions the AI can take.

I suggest these 6 attributes to influence most of the AI's behaviours without being too specific.

  • Militaristic: Determines how much does the civilization prioritizes building a military, but not necessarily using it. A higher value means more focus on military, a lower value means it is likely more peaceful.
  • WarMongering: Determines how likely the civilization is to declare war. A higher value means the civilization is more aggressive, a lower value means it is more defensive.
  • Commerce: Determines how open the civilization is to trade, value open borders, and liberate city-states. A higher value means more trading frequency even with civilizations they don't like.
  • Diplomatic: Determines how likely the civilization is to declare friendship, defensive pact, peace treaty, or other diplomatic actions.
  • Loyalty: Determines how much the civilization values a long-lasting alliance, how willing they are to join wars with them, and how much they despise other unreliable civilizations.
  • Expansion: Determines how focused the civilization is on founding or capturing new cities. A lower value means they might focus on culture more.

The idea comes from http://civdata.com/, however, I don't agree with having so many variables that they don't mean much. These attributes need to combine enough uses so that they are actually meaningful while still allowing for a variety of play styles.

These attributes are not hardcoded to a specific victory type, but instead focus on the more general behaviour. The behaviour needs to be general in order to account for mods which may change the victory type.

Any thoughts?

@SeventhM
Copy link
Collaborator

My couple of notes:

  1. I feel like this should be an issue, not a PR, if it's primarily for a discussion
  2. While the idea of a separate section in the docs for these is a good idea, I'd personally advise against them currently (or would at least advise commenting them out). This would be to allow flexibility moving forwards without giving some expectation of what will or won't happen
  3. I'm not sure I get the renaming of "Military" to "Militaristic". At best, it seems like a minor change that doesn't really matter too much but imo should be given an actual discussion (either here or on discord). At worst... It's a discussion about naming conventions. Most of the use cases of these hardcoded atributes is about setting how much the personality should focus on that subject. So maybe, each one should read well as "[value] focus". "Military focus" reads fine, "militaristic focus"... Ehhhhh... (Maybe we can rename PersonalityValue to PersonalityFocus or Personality.Focus) for similar reasons
  4. Expansion 100% sounds like something better served with uniques. I already have one have of cultural AI's city limits with Add unique for Personality to avoid building object #11160, and the only thing needed for it would be a conditional for city count. Something to limit settling could probably also be done via uniques. I kinda feel like I'd need some code example to understand why it's needed as a focus. This is why I very much feel notes 1 and 2 here are the most important
  5. A similar story goes to Diplomatic and/or Commerce, though these I'd be willing to give a pass on if we can find a suitable way to implement them
  6. Loyalty seems like the one I could give the least amount of opinion to. You're the main one working on the AI, so I'll definitely leave that to you. But ideally we should probably stick to uniques where possible unless it's something that makes the most sense starting as a variable

@tuvus
Copy link
Collaborator Author

tuvus commented Feb 21, 2024

Expansion 100% sounds like something better served with unique ... a conditional for city count

This would once again be hardcoding, maybe you can add some unique for a mod. But I don't think we should have some single if statement with no leniency for expansion as a core feature of the AI. Keep in mind that I don't know that much about the unique system.
This kind of the same to me wanting to remove
if (civInfo.wantsToFocusOn(Victory.Focus.Culture) && civInfo.cities.size > 3 && civInfo.getPersonality().isNeutralPersonality)) at NextTurnAutomation line 465 in trainSettler().

(Maybe we can rename PersonalityValue to PersonalityFocus or Personality.Focus)

I was thinking something like that as well. We might want to move the behaviors out of PersonalityValue to separate them from the stats.

From my experience of working on with the AI all six of these are relatively easy to implement.

I feel like this should be an issue, not a PR, if it's primarily for a discussion

This PR is more of a check-in before I actually work on the changes. I thought about some others like offensive/defensive but that would require much more of a military unit AI rework.

As for naming, I am good for whatever. Warmongering focus sounds a little weird, maybe conquest focus?

@SeventhM
Copy link
Collaborator

This would once again be hardcoding, maybe you can add some unique for a mod

Again, I'd like to point to 11160. Personalities naturally support uniques. As currently implemented, the line in NextTurnAutomation does not function with custom personalities, only neutral ones (even a blank custom personality doesn't count). And 11160 already allows for denying building settlers if it doesn't fit the conditionals. I don't see how this would add any more hard coding over what is currently available, and especially don't see how adding a specific focus for this would make anything better. Again, I kinda would rather an example of what the intent was before I could say it was a good idea. Otherwise, a move away from variables and towards uniques is generally a better idea

This PR is more of a check-in before I actually work on the changes.

Still, it looks a bit off here as a PR. Personal preference, maybe Yairm or Trog but it feels like this should at least be a draft, if not an issue, and it seems a bit presumptuous as a non draft PR.

Warmongering focus sounds a little weird, maybe conquest focus?

Maybe... Though the focus is more on declaring war, not so much as how you use your troops, so Idk if that's the best name for it

@tuvus
Copy link
Collaborator Author

tuvus commented Feb 21, 2024

I don't see how this would add any more hard coding over what is currently available

I will probably change how it works. Using an integer to take multiple factors into account has worked to improve a lot of other AI functions. And having an expansion focus/behavior int would work well with that.

There is no problem with a modder telling a civ it can't build more than X cities under some condition. However, I would like the base AI to be responsive (by using the integer valuing system) and expand based on its surroundings and game settings. The additions from 11160 seem like they would be a good override for modders, but not the best for AI design.

also, I think I used hardcoding to refer to something more like static too much

Though the focus is more on declaring war, not so much as how you use your troops

Good point

@SeventhM
Copy link
Collaborator

Well you're the AI guy. My main concern mostly stems from wanting to see implementation alongside the variables inside a PR, if you can provide that implementation, I won't have an issue

Can we still convert this to a draft, though?

@tuvus tuvus marked this pull request as draft February 21, 2024 14:29
@SomeTroglodyte
Copy link
Collaborator

  1. should be an issue, not a PR, if it's primarily for a discussion

Or - should use the 'discussions' feature of github? Or even projects, where you can also structure decisions, goals and assignments? Joking - I can open them to see what they look like on my fork, not here.

@tuvus
Copy link
Collaborator Author

tuvus commented Feb 21, 2024

My main concern mostly stems from wanting to see implementation alongside the variables inside a PR, if you can provide that implementation

I am a little confused by this, I would prefer to have this PR go through and then make incremental changes to implement the features.

Commenting out the docs until it is implemented is a good idea though.

@SeventhM
Copy link
Collaborator

I would prefer to have this PR go through and then make incremental changes to implement the features.

I would prefer the opposite tbh: add in slow incremental changes implementing parts of this along the way. We can use this as reference, but we are hamstrung into saying "this must be how we do that". Avoids needing to reedit stuff like names over and over as the kinks are ironed out

@tuvus tuvus marked this pull request as ready for review February 23, 2024 14:39
@tuvus
Copy link
Collaborator Author

tuvus commented Feb 23, 2024

Implementing the behaviors will go in a different PR. We also need this PR to go through since it deletes the Military PersonalityValue and since the Military PersonalityValue was publicly displayed on the docs, it might cause problems since a modder could then try and use it.

We can change the names as we go since we aren't showing them in the docs yet. Having to edit them is probably unavoidable, and putting everything in one PR is not a good practice.

I probably won't start implementing these behaviors until after this goes through in order to prevent vcs conflicts and simplify the workflow.

@SeventhM
Copy link
Collaborator

We also need this PR to go through since it deletes the Military PersonalityValue

And again, as I stated prior, I'm not sure the value of doing that. From a name perspective it "militaristic" seems worse than "military" here, and the only change there is a rename, not a functional change that justifies it. Again, imo this should imo either be a draft or be an issue with a checklist, with the bulk of this discussion happening on discord or through implementation, not through here. But I could definitely be outvoted by @yairm210

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

# Conflicts:
#	core/src/com/unciv/models/ruleset/nation/Personality.kt
Copy link

Conflicts have been resolved.

@yairm210
Copy link
Owner

What's the actual diff here...? I see 'all green' and 'all red' but the before and after look the same...?

@tuvus
Copy link
Collaborator Author

tuvus commented Feb 28, 2024

Interesting how that happened. I added these 6 behavior attributes in PersonalityValue, Personality, and nameToVariable:
Militaristic, WarMongering, Commerce, Diplomatic, Loyalty, Expansion
and changed the docs for them as well. Currently, I have the docs commented out until we settle on the names.
It also changes the previous Military value to Militaristic which won't be shown to modders until the name is decided upon.

@yairm210
Copy link
Owner

I have no cost wonder on Military vs Militaristic tbh.
Since this is a @SeventhM area, I feel he should be the arbiter of this PR - I'll accept his approval or disapproval.
I can't find anything wrong here, though.

@SeventhM
Copy link
Collaborator

SeventhM commented Feb 29, 2024

Since this is a @SeventhM area, I feel he should be the arbiter of this PR - I'll accept his approval or disapproval.
I can't find anything wrong here, though.

My personal viewpoint is

  1. "Military" vs "Militaristic" at best should be polled on discord, at worst is a worse name overall
  2. This PR adds more stub values beyond just "warmongerer", which already was a bit presumptuous in some fashion. I feel like this should be either an issue (akin to Missing features from Civ V - G&K #4697) or a draft PR. As is, I don't exactly see the value. If the PR was to stay, I'm pretty sure it should be a draft

Copy link

github-actions bot commented Mar 9, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

# Conflicts:
#	docs/Modders/Mod-file-structure/2-Civilization-related-JSON-files.md
Copy link

Conflicts have been resolved.

Copy link

github-actions bot commented Apr 4, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

# Conflicts:
#	core/src/com/unciv/logic/automation/city/ConstructionAutomation.kt
#	core/src/com/unciv/models/ruleset/nation/Personality.kt
@github-actions github-actions bot removed the Conflicts label May 3, 2024
Copy link

github-actions bot commented May 3, 2024

Conflicts have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants