Skip to content

Conversation

bmgxyz
Copy link

@bmgxyz bmgxyz commented Feb 5, 2025

  • Add SettlerTileAdjustments as a child of Civilization
    • Encodes the previously hard-coded values as defaults for public fields
    • Uses lambda functions to take values at runtime for adjustments that take arguments; this allows future users to supply arbitrary logic
  • Replace hard-coded bonus values in SettlerLocationAI.cs with calls and references to SettlerTileAdjustments via the supplied Player
    • Notably, DistancePenalty is now a negative value by default, which I think is more clear because now adjustments are always added to the tile score
  • Make settler tile score a float instead of int, which gives more precision for multiplicative adjustments
  • Apply lots of lints from Rider
    • Identifier case tweaks (this caused most of the changes in other files)
    • Using ternary operators to make code a little smaller when it's still clear
    • Some grammar problems (Rider is picky about these for some reason)
    • Lint ignores in cases where it makes sense (to me)
    • I'm willing to undo these if they're too aggressive or unwanted

Closes #92

- Add `SettlerTileAdjustments` as a child of `Civilization`
  - Encodes the previously hard-coded values as defaults for public fields
  - Uses lambda functions to take values at runtime for adjustments that take
    arguments; this allows future users to supply arbitrary logic
- Replace hard-coded bonus values in `SettlerLocationAI.cs` with calls and
  references to `SettlerTileAdjustments` via the supplied `Player`
  - Notably, `DistancePenalty` is now a negative value by default, which I
    think is more clear because now adjustments are always added to the tile
    score
- Make settler tile score a `float` instead of `int`, which gives more
  precision for multiplicative adjustments
- Apply lots of lints from Rider
  - Identifier case tweaks (this caused most of the changes in other files)
  - Using ternary operators to make code a little smaller when it's still clear
  - Some grammar problems (Rider is picky about these for some reason)
  - Lint ignores in cases where it makes sense (to me)
  - I'm willing to undo these if they're too aggressive or unwanted
Copy link
Contributor

@TomWerner TomWerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and the detailed description!

Would it be possible to split these apart into a couple of PRs? In particular, I think a PR that only fixes linting issues would be ideal. The linting issues touch so many files that its hard to see the actually interesting changes, so reducing diff noise would make it much easier to review the PR confidently.

Also it looks like C7/C7.sln.DotSettings is a new file, was that intended to be included in this PR?

@bmgxyz
Copy link
Author

bmgxyz commented Feb 7, 2025

Would it be possible to split these apart into a couple of PRs? In particular, I think a PR that only fixes linting issues would be ideal.

Makes sense to me. I'll pull out the lint changes into a new PR and then go through the (many) other ones on that separate branch. It will take some time but will make for a cleaner development experience because the UI will quiet down. Only the parts that are directly related to #92 will remain here.

Also it looks like C7/C7.sln.DotSettings is a new file, was that intended to be included in this PR?

This is part of the linting changes. Rider applies spelling and grammar linting in comments 🙄 and some of the words that appear in comments aren't in its dictionary (e.g., "civilopedia"). It's possible to add the unrecognized words to a dictionary so Rider stops flagging them, either locally (untracked) or "team-shared" (tracked). The file you mentioned is where Rider stores this configuration. I think tracking this file is a good idea because it should prevent this kind of linter noise in all Rider installations for this project.

A better option would probably be to find a way to disable this kind of linting entirely, but that should be a discussion elsewhere. I hadn't used Rider before this project, so I'm sure there's a lot I don't know here.


Do you have any thoughts about my approach related to #92? I'm still unfamiliar with the project internals and C# in general, so it wouldn't surprise me if it can be improved.

@bmgxyz bmgxyz marked this pull request as draft February 7, 2025 02:51
@TomWerner
Copy link
Contributor

Do you have any thoughts about my approach related to #92? I'm still unfamiliar with the project internals and C# in general, so it wouldn't surprise me if it can be improved.

If you're talking about the changes to Civilization.cs and SettlerLocationAI.cs, yeah, that seems good. Do you happen to know if the tests pass successfully? I'm curious as to how the new class of constants is handled with JSON serialization.

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.

Settler AI - Refactor so weights are not constants

2 participants