Unciv icon indicating copy to clipboard operation
Unciv copied to clipboard

Buying Castles and associated ideas

Open SomeTroglodyte opened this issue 1 year ago • 7 comments

Before creating

  • [X] This is NOT a gameplay feature from Civ VI, BNW, or outside - see Roadmap
  • [X] This is NOT a gameplay feature from Vanilla Civ V or from G&K - If so, it should be a comment in https://github.com/yairm210/Unciv/issues/4697

Problem Description

As we know, buying Happiness buildings takes a lot of time compared to other buildings - ~because someone whined about starving cities~ because the limit for nullifies growth may be passed and cities might need food again, so all of them may perhaps need their worked tiles re-optimized. That may well be several seconds on a fast phone and ~20 cities.

Apart from optimizing that code (e.g. intelligently parse mods to see where happiness limits affecting nullify growth really are) -

  • Doesn't happen for Castles and their follow-ups with Professional Army and/or Neuschweinstein: The code might need to go and scan all StatsFromObject uniques from the Civ too!

Related Issue Links

#9799, #9425, #9238, #10397 - PR #9395 introduced the re-eval

Desired Solution

Advice... How can we best deal with that

Alternative Approaches

  • Could that recalculation be done in a Coroutine while the user stays in the CityScreen, complete with preemption when they buy another Happiness building or queue reordering when they change to view one of the affected cities? And background processing continuing to run to finish while user changes views, potentially updating WorldScreen map tiles overlays while the user is looking at them?

Additional Context

  • I see a StatsFromBuildings unique where a StatsFromObject should suffice from an implementation viewpoint - is that for prettier language only? (Sovereignty policy)

SomeTroglodyte avatar Nov 21 '23 15:11 SomeTroglodyte

So would an extra

        if (getMatchingUniques(UniqueType.StatsFromObject, StateForConditionals.IgnoreConditionals).any {
                it.stats[stat] > 0 && matchesFilter(it.params[1])
            }) return true

...in Building.isStatRelated be the ticket? Could take ages to test properly.

And as for the monster 'hanging' delays - maybe rethink the line #10397 points out - "Note that negative food will also be nullified. Pretty sure that's conform civ V, but haven't checked." - (or decide to tune the Unique to allow nullifying positive growth and/or starvation independently, so mods or our base rulesets can control the delay), because if starvation isn't nullified, the entire reason for reallocating citizens is moot?

SomeTroglodyte avatar Nov 21 '23 16:11 SomeTroglodyte

Buying castles should change the happiness is the civ which would trigger recalc for all cities If we manually check for happiness buildings, we can get rid of that check

yairm210 avatar Nov 21 '23 17:11 yairm210

That's an extra recalc we can throw away

yairm210 avatar Nov 21 '23 17:11 yairm210

Not sure I get what you mean.

Also - that code up there would in fact extend the meaning of "Science" and "Culture" buildings (in fact I think that would be correct and is slightly wrong now), making Castles meet the Culture building filter after Neuschwanstein is owned - but not make Neuschwanstein itself a Happiness/Culture building which it theoretically should be if and only if the player already has at least one Castle... But then it can't be bought. But mods could create such a situation.

SomeTroglodyte avatar Nov 21 '23 17:11 SomeTroglodyte

...in Building.isStatRelated be the ticket?

I'm not so sure about doing that tbh

  1. Building.getMatchingUniques only checks the uniques of the object itself, not any uniques the civ may have by this point. E.g., Neuschwanstein wouldn't make castles into a culture building, but "[stats] from every [Castle]" on the building itself would. You would need to pass in the city/civ into Building.isStatRelated to do what you want
  2. From what I'm aware, Civ 5 itself wouldn't consider Castles as culture buildings with Neuschwanstein, so this seems slightly wrong, not what we currently do

SeventhM avatar Nov 22 '23 22:11 SeventhM

  1. Neuschwanstein wouldn't male

Yeah you're right, I actually noticed later my last statement was wrong, inverted actually, but I was at the moment too far away from a browser[^1] to post, then forgot. Treating Castles as Culture buildings would indeed feel wrong. But still - there's a very small window to still get the "starving after buying" effect despite the fix we attempted for #9238.

[^1]: Yeah, I started dreaming about Unciv code some while ago. Horribly embarrassing and a true deep horror, the "I have no mouth but I must scream" kind...

SomeTroglodyte avatar Nov 23 '23 01:11 SomeTroglodyte

I'm still in favour of a rule change - make unhappiness not nullify starvation, then throw away that forced worker reassignment...

SomeTroglodyte avatar Nov 23 '23 10:11 SomeTroglodyte

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

github-actions[bot] avatar Mar 14 '24 20:03 github-actions[bot]

Can't find the PR Yairm did to reduce the amount of recalculations, but this should be solved by now. Does this need to stay open?

SeventhM avatar Mar 16 '24 22:03 SeventhM

Apart from optimizing that code (e.g. intelligently parse mods to see where happiness limits affecting nullify growth really are)

Yair did that nicely. I remember that too.... git blame on allHappinessLevelsThatAffectUniques says #11034.

Doesn't happen for Castles and their follow-ups with Professional Army and/or Neuschweinstein

Don't know - but looking at the critical path now it seems there is no problem. Interactive buying always ends up in CityConstructionsTable.purchaseConstruction which always calls reassignPopulation, which should take care of happier city needing more food??? Anyway, if there's a window someone else will notice sooner or later.

SomeTroglodyte avatar Apr 03 '24 12:04 SomeTroglodyte