Decrement turns of unhappiness counter per city, if positive

[zack-the-coding-actuary]

Response to @Billytifft's bug report of unhappiness due to forced labor never decrementing.

#1004 (comment)

Quick fix potentially abusing short-circuiting logic, heh. I did some homework to make sure I understood how the mechanic worked (it's already good to go as-is), I'm still learning new things about this game!

[ajhalme]

I don't think we want to update the state here. RecalculateCitizenMoods should only do what it says on the tin.

I'd look into extending HandleCityUpdates(..) in class Player, or perhaps going up a level and doing a new segment in the per-player section of AdvanceTurn in TurnHandling.cs.

On a more stylistic note: I find that doing comparisons and variable updates on the same line of source code makes the overall logic harder to parse and understand at a glance.

[zack-the-coding-actuary]

Ah yes I see this needs to be a bit more surgical. RecalculateCitizenMoods can be called from multiple function paths here so we definitely don't want to decrement on every call. Okay, I'll think about how to organize this more cleanly and propose a different fix!

[zack-the-coding-actuary]

I think something like this might be the cleanest solution. Updating the unhappiness penalty decrements in player.HandleCityUpdates means we have to account for the potential single extra turn of unhappiness in player.RecalculateCitizenMoods, which you actually don't know exists or not depending on where you're calling player.RecalculateCitizenMoods from. It can happen either during the interturn like in this turn advance step, OR in several other places.

So I think the most tractable solution is to have a small helper also in the Player class that fires exactly one time during AdvanceTurn and decrement everything there. Then all future calls to player.RecalculateCitizenMoods are valid without changing anything else in the method, which I think makes intent clearest and invasiveness minimal.

Happy to entertain any perspectives different than mine.

[ajhalme]

The one-liner is perhaps needlessly clever relative to the explicit

if (c.turnsOfUnhappinessDueToPopRushing > 0) {
    c.turnsOfUnhappinessDueToPopRushing -= 1;
}

but let us move on to greater things.

Thanks for the contribution!

[zack-the-coding-actuary]

The one-liner is perhaps needlessly clever relative to the explicit

if (c.turnsOfUnhappinessDueToPopRushing > 0) {
    c.turnsOfUnhappinessDueToPopRushing -= 1;
}

but let us move on to greater things.

Thanks for the contribution!

But a ternary conditional move saves DOZENS of cycles compared to a branch misprediction!!

cough cough sorry, that was the C / C++ Green Goblin side of me..

[ajhalme]

Well...

https://godbolt.org/z/qbPxErKbb