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

Change save promotion from unitType to just baseUnit #12960

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

Conversation

Emandac
Copy link
Contributor

@Emandac Emandac commented Feb 14, 2025

This change comes from me messing around with save promotion in promotion picker and finding weird edge cases.
Like there could be a unit in the unitType that has promotion by default when it is build that the other unit don't have,
So when saving that unit promotion, it could cause the other unit with the same unitType to have the promotion that the unit shouldn't have.

By making it BaseUnit instead of UnitType avoid units using saved promotion having promotion that they shouldn't have.

I am also really sorry that i didn't caught this edge case Pre-PR.

…of running away

Settler unit will now settle on best tile in dangerous Tiles without escort instead of running away.
with "Founds a new puppet city" in "uniques" of an unit in Units.json.
Making it so you can now settle a puppet city.
@Emandac Emandac changed the title Change save promotion from unitType to juse baseUnit Change save promotion from unitType to just baseUnit Feb 14, 2025

var cityUnitTypePromotions = HashMap<String, UnitPromotions>()
var cityUnitPromotions = HashMap<String, UnitPromotions>()
Copy link
Owner

Choose a reason for hiding this comment

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

REMOVE the City part! For the third time...
Should be called unitToPromotions

@@ -188,6 +188,8 @@ enum class UniqueType(
// Todo: Lowercase the 'U' of 'Units' in this unique
CityHealingUnits("[mapUnitFilter] Units adjacent to this city heal [amount] HP per turn when healing", UniqueTarget.Global, UniqueTarget.FollowerBelief),

ExperienceForPromotionModifier("[relativeAmount]% Experience required for promotions",UniqueTarget.Global,UniqueTarget.FollowerBelief),
Copy link
Owner

Choose a reason for hiding this comment

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

We call it "XP"
Also, how exactly is this a follower belief?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To allow modders a follower Belief with this unique

Copy link
Owner

Choose a reason for hiding this comment

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

Follower beliefs are for things that affect cities
This won't work as a follower belief
I assume you haven't tried it? If you have and it works, there's something freaky going on

Copy link
Contributor Author

@Emandac Emandac Feb 15, 2025

Choose a reason for hiding this comment

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

Ah, I am just going to remove it then 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

We call it "XP" Also, how exactly is this a follower belief?

The game is not fully consistent on this actually, it's
"Experience" in UniqueType.UnitStartingExperience
"Xp" in UniqueType.CityStateGiftedUnitsStartWithXp
"XP" in the other uniques

Copy link
Owner

Choose a reason for hiding this comment

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

It should be XP everywhere
UnitStartingExperience needs to be renamed

Copy link
Owner

Choose a reason for hiding this comment

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

Nah CityStateGiftedUnitsStartWithXp also has "XP" in the text, only the unitstarting is bad

Comment on lines +52 to +68
fun xpForNextPromotion(): Int {
// reduce or hieghter the xp for next promotion.
for (unique in unit.civ.getMatchingUniques(UniqueType.XPForPromotionModifier)) {
return Math.round(((numberOfPromotions + 1) * 10) * unique.params[0].toPercent())
}
return (numberOfPromotions + 1) * 10
}


/** @return the XP points needed to "buy" the next [count] promotions. */
fun xpForNextNPromotions(count: Int) = (1..count).sumOf { (numberOfPromotions + it) * 10 }
fun xpForNextNPromotions(count: Int): Int {
// reduce or hieghter the xp for next promotion.
for (unique in unit.civ.getMatchingUniques(UniqueType.XPForPromotionModifier)) {
return Math.round(((1..count).sumOf { (numberOfPromotions + it) * 10 } * unique.params[0].toPercent()))
}
return (1..count).sumOf { (numberOfPromotions + it) * 10 }
}
Copy link
Owner

Choose a reason for hiding this comment

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

How about we deal with this in a different PR. I don't like the duploication - we should have just "costOfPromotion(existingPromotions: Int)" and the "next N" should use that function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function would be something like this ?

        for (unique in unit.civ.getMatchingUniques(UniqueType.XPForPromotionModifier)) {
            return Math.round((existingPromotions * unique.params[0].toPercent()))
        }

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds about right

@@ -188,6 +188,8 @@ enum class UniqueType(
// Todo: Lowercase the 'U' of 'Units' in this unique
CityHealingUnits("[mapUnitFilter] Units adjacent to this city heal [amount] HP per turn when healing", UniqueTarget.Global, UniqueTarget.FollowerBelief),

XPForPromotionModifier("[relativeAmount]% Experience required for promotions",UniqueTarget.Global,UniqueTarget.FollowerBelief),
Copy link
Owner

Choose a reason for hiding this comment

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

You didn't actually change the text here -_-

@yairm210
Copy link
Owner

This is a classic example of why different issues should be in different PRs, mixing them makes the whole thing take longer and be more confusing :)

@Emandac
Copy link
Contributor Author

Emandac commented Feb 19, 2025

Agreed :d

@Emandac
Copy link
Contributor Author

Emandac commented Feb 19, 2025

Why is my build failing I only change that bit of text :(

@yairm210 yairm210 closed this Feb 19, 2025
@yairm210 yairm210 reopened this Feb 19, 2025
@Emandac
Copy link
Contributor Author

Emandac commented Feb 19, 2025

So now i create a new PR branch for promotion XP modifier or
i wait until this PR gets merge ?

@yairm210
Copy link
Owner

Whichever you want
We can keep both here if you don't mind them delaying each other

@Emandac
Copy link
Contributor Author

Emandac commented Feb 19, 2025

I think i will just let this PR get merge before doing anything else 😅

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.

3 participants