Skip to content

Conversation

SimonCeder
Copy link
Collaborator

Relates to #4602
This PR adds Porcelain and Jewelry for Mercantile city states, and a resource overview for the city-state diplomacy screen. They are luxuries created out of thin air in the city state, and are extra incentives to ally them.

So how do you find out which is created in which city-state? With the new resource overview line:
diploresources

Added unique: "Can only be created by Mercantile City-States"

for (cityStateName in availableCityStatesNames.take(newGameParameters.numberOfCityStates)) {
val civ = CivilizationInfo(cityStateName)
civ.cityStatePersonality = CityStatePersonality.values().random()
if (ruleset.nations[cityStateName]?.cityStateType == CityStateType.Mercantile) {
if (!ruleset.tileResources.values.any { it.unique == "Can only be created by Mercantile City-States" }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe cache unusedMercantileResources.isEmpty() outside the loop? Two uses inside the loop...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure what you mean by this? Do you think I'm using an inappropriate data structure here? I wasn't bothered about optimizing this bit too heavily as it only gets called once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just saw like an obvious small optimization that would also improve readability.. But you're right nothing critical here. And I was wrong as well as the second reuse is not the bool 'existence' but the complete list.

stringBuilder.appendLine("Improved by [$improvement]".tr())
stringBuilder.appendLine("{Bonus stats for improvement}: ".tr() + "$improvementStats".tr())

if (terrainsCanBeBuiltOnString.isNotEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conflicts with #4628 - purely FYI. The function is gone in that PR and replaced. I get the reason - I'll adapt #4628 in any case as such checks wouldn't hurt even if this doesn't make it. (actually I have a gut feeling I already had such tuning in before stashing and splitting the 'pedia project - because I tested the thing with some major mods long ago...? Did I miss something?)

SomeTroglodyte added a commit to SomeTroglodyte/Unciv that referenced this pull request Jul 25, 2021
Copy link
Collaborator

@SomeTroglodyte SomeTroglodyte left a comment

Choose a reason for hiding this comment

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

👍

@ajustsomebody
Copy link
Contributor

ajustsomebody commented Jul 26, 2021

great work, how are they implemented? are they a tile resource or just appear out of thin air, they also shouldnt be buildings that happen to provide these as it shouldnt provide those luxuries when conquered

edit: actually this seems to be different from a post i found in civfanatics, i still support it being implemented as a non-tile based luxury

" The merchantile CS luxuries don't spawn next to the city-states, rather they spawn under the CS after the settler has settled.
This also happens when a merchantile CS conquers a city, a new porcelain or jewelry spawns under the city they conquered. As they usually raze their new cities, the luxury will stay behind.
Settling on top of the lux will give it to you, but I doubt great improvements like citadel work."

https://forums.civfanatics.com/threads/porcelain-available-but-how-to-access.468484/

@SomeTroglodyte
Copy link
Collaborator

how are they implemented

See files changed link above. Even if you don't 'speak' kotlin, just looking at the file names and reading the json will often give you a clear idea how.

"but I doubt great improvements like citadel work"

Great improvements don't "work" for luxury resources anyway, and citadels don't "work" for any resource (at least it shouldn't)... right? That just might make that source a little questionable. I'm not criticizing your research, just saying to take it with a grain of salt.

yairm210 pushed a commit that referenced this pull request Jul 27, 2021
* Civilopedia phase 7 - Terrain

* Civilopedia phase 7 - Terrain - tuning

* Civilopedia phase 7 - Terrain - patch1 "for"

* Civilopedia phase 7 - Terrain - patch2 for #4641
@ajustsomebody
Copy link
Contributor

ajustsomebody commented Jul 27, 2021

how are they implemented

See files changed link above. Even if you don't 'speak' kotlin, just looking at the file names and reading the json will often give you a clear idea how.

"but I doubt great improvements like citadel work"

Great improvements don't "work" for luxury resources anyway, and citadels don't "work" for any resource (at least it shouldn't)... right? That just might make that source a little questionable. I'm not criticizing your research, just saying to take it with a grain of salt.

if (civInfo.isCityState() && isCapital() && civInfo.cityStateResource != null) { cityResources.add(getRuleset().tileResources[civInfo.cityStateResource]!!, 1, "Mercantile
City-State")
`
yep i think i understood it. now ill see whether the resource monitor shows strategic resources as well

@yairm210
Copy link
Owner

You seem to have conflicts with another PR that was merged

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Jul 27, 2021

That's likely the getDescription() change here and the getDescription() removal in Civilopedia phase 7 #4628 .

Verified. A simple 'accept theirs' on a merge of master into Simon's branch gives:
image

... looks fine to me.

... Except, who's getting those 2 Gold?

@yairm210 yairm210 merged commit 1f6f49e into yairm210:master Jul 27, 2021
@SimonCeder SimonCeder deleted the mercantile branch July 27, 2021 20:18
@ajustsomebody ajustsomebody mentioned this pull request Aug 1, 2021
26 tasks
@ajustsomebody
Copy link
Contributor

@SimonCeder @SomeTroglodyte does anything wrong happen when a city state has negative resources? does the player receive -2 resource as well?

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.

4 participants