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

Move capitalCityIndicator #12504

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SeventhM
Copy link
Collaborator

@SeventhM SeventhM commented Nov 20, 2024

PR is to move capitalCityIndicator, as it previously doesn't make sense in the city code. Starting as a draft because there are many, many, many questions. I wish I PR'ed this a while ago before "uniqueTo" became something you can filter from as many aspects made more sense before then

  1. Currently, the IndicatesCapital unique does not use conditionals. City.isCapital uses getUniqiues instead of getMatchingUniques, which inherently ignores condtionals. CityConquestFunctions.destroyBuildingsOnCapture uses hasUnique, but this is not given a state, so it uses EmptyState instead of IgnoreConditionals, auto-failing any civ/city check. previously, this function also used hasUnique without a state, thus having the same problem. This PR changes it to specifically use IgnoreConditionals for consistency with isCapital
  2. Since uniqueTo uses filters and filters can now have conditionals and uniqueTo can multiFilter, there is a question of which state this should use. I would assume IgnoreConditionals here as well, not the civ's state or the city's state like I erroneously used prior. This PR as of writing this keeps the incorrect behavior of the previous PR, but I can easily change it back
  3. If there is an assumption that this should be consistent all game, then this information should be cached. This would change the answer of point 2 to be "the civ state at the time of caching", same as the uniqueBulidings cache. This isn't a particularly complicated bit, but it does force the question of when we want to check the filter as what to cache changes based on when we want to check for filters

Side note, regardless of those points, the changes to Civ.moveCapitalTo to keep the code functioning as it did prior. kotlin's smart casting isn't recognizing that city can't be null if the indicator isn't null

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.

1 participant