Add a field for global unit uniques

[SeventhM]

Sorry for the delay on this one. Been real busy

Replaces #12690. Rather than doing things as a special unit type, this creates a separate unique list for global unit uniques and dumps them directly into the uniques for each unit type. I'm not sure if this is the correct approach (rather than dumping the uniques into the unit itself)

Also adds in a number of helper functions needed to make these sorts of things easier to work with

Keep in mind, this code does not go through validation as of the writing of this PR description. Not sure if that should be in this PR or a sort of next PR

[SeventhM]

Also, side question, what's stopping us from doing something similar for base units in the first place (dump the uniques directly into the unit and don't bother checking unit types again)?

[yairm210]

There's no reason for this to be a function on HasUniques, this is just UniqueMap().apply{addUniques(uniqueObjects)}?
I mean if you had logic here like if (uniqueObjects.isEmpty()) return UniqueMap.EMPTY then I would understand, but currently seems removable

[SeventhM]

I mean... I guess? Function was more or less added for consistency with the other function what was already there (since this solution does need to be able to reference specifically that group of uniqueObjects. But also... I mean, I kinda feel like that would've been true before too, no?

[SeventhM]

Also, technically this is safer than doing if (uniqueObjects.isEmpty()) return UniqueMap.EMPTY since UniqueMaps aren't inherently or even implicitly immutable. We would need to make a separate type for that or hope it never comes up

[yairm210]

I really don't like functions masquerading as fields, but I see why you felt you had to do this, this to me is a prime indication this is also the wrong approach :/

[SeventhM]

Well, I kinda feel like no matter how you do it (unless it's a promotion, obviously), you need to do something like this or eat the performance loss calculating it. We need a way to use the field before the ruleset is initialized, and we need a way to use it after. And we can't use a lazy because that's not resettable (and the options to make a resettable lazy is possible, but clunky). This is more or less just 2 lazys stapled to one field, which I would argue is fine (besides concerns about memory. I;m not sure the best way to solve that without writing, say, iUniqueMap to uniqueMap's field each time when it's called)

[yairm210]

RulesetObjects are supposed to be basically immutable
This change says "the unit type from ruleset A can return uniques from ruleset B if they're both from different mods in the same game", which to me is a clear indication that this is not the way to go

Units, on the other hand, are not ruleset objects, so when building their uniquemap I have no problem taking the uniques

Putting the unit uniques inside the global uniques IS DEFINITELY the way to go though 👍🏿

[SeventhM]

Units, on the other hand, are not ruleset objects

When we say "units", what are we referring to?

If we're talking about BaseUnit, BaseUnit is a ruleset object that basically did more or less a similar thing to what this PR does (redirect all usage of their maps to use the unit type's uniques once the ruleset is initialized). It just wasn't a blatant and obvious as what I'm doing here. I'm not going to entirely disagree with the idea of "it's supposed to be immutable, why does it get it's reference from global", but I feel like we're already doing that.

If we're talking about MapUnit... I mean, I guess. But because of how we're dancing around the code, it likely means we store the uniqueMap/uniqueObjects in the global uniques and update them into the mapUnit when we set transients. I'm not entirely against in theory, but it kinda comes back to "base units (and unit types for that matter) should already know this information, why do mapUnits gotta build this map as well?" Actually, I have in part of my local build this same sort of hack for base units as well, and this:

        val unitUniqueSources =
            baseUnit.uniqueObjects.asSequence() +
                type.uniqueObjects
        val otherUniqueSources = promotions.getPromotions().flatMap { it.uniqueObjects } +
            statuses.flatMap { it.uniques }
        val uniqueSources = unitUniqueSources + otherUniqueSources

simplfies to this:

        val otherUniqueSources = promotions.getPromotions().flatMap { it.uniqueObjects } +
            statuses.flatMap { it.uniques }
        val uniqueSources = baseUnit.uniqueObjects.asSequence() + otherUniqueSources

No need to bother doing the extra work if the baseUnit can and probably should do the work for you

[yairm210]

No, you're right dammit. We already do that with BaseUnit->UnitType references across origins.

Even so, get/set is not the right pattern.
Just like BaseUnit does distinguish between "my uniques" and "uniques from the type", it should also do so in relation to uniques from the ruleset.
That's why it overrides hasUnique and hasTagUnique etc.
So e.g. || ::ruleset.isInitialized && type.hasTagUnique(tagUnique) -> || ::ruleset.isInitialized && ruleset.globalUniques.unitUniqueshasTagUnique(tagUnique)

And yes, I meant MapUnits. What's important is that all unit uniques get put into the MapUnit UniqueMap because that's the single source of querying
Regarding performance: What's important is performance of MapUnit unique checking, which happens extremely often, and not of the UnitType.
Looking back at 12690 I see I misunderstood - that would not even have worked, since the actual UniqueMap being checked for MapUnit wouldn't have even contained the unique since it only checks tempUniquesMap

---

IN CONCLUSION

There are 2 main use cases for unit uniques

• Checking uniques on MapUnit - performance critical - done via tempUniquesMap
• Checking uniques on UnitType (when e.g. deciding what units are buildable or which construction to build) - not performance critical - currently done by checking both BaseUnit and UnitType

So, we need to ensure that global unit uniques are

• Added to the MapUnit.tempUniquesMap
• Returned correctly by BaseUnit functions

And now the question needs to be: Do we check the UnitType for uniques anywhere on its own? I'm pretty sure we don't, except for displaying in civilopedia, which means that the global uniques should be returned by BaseUnit
Adding it as another option in BaseUnit.hasUnique() and friends sounds to me like the correct solution

(I'm sorry if this sounds nitpicky but if we build this wrong we will come to regret it and will have to rebuild it anyway -_-)

[SeventhM]

There are 2 main use cases for unit uniques

I would like to point your attention at #12404. There, the primary concern is that if we were to check the temp map for filters, we would end up checking the filters twice. If I remember correctly, that wasn't just a performance concern, though I forget why or when the implications came up. That's why that PR explicitly uses a different uniqueMap for specifically filter checks. So there are at least some performance concerns regarding the functions for base units, still

And now the question needs to be: Do we check the UnitType for uniques anywhere on its own? I'm pretty sure we don't, except for displaying in civilopedia, which means that the global uniques should be returned by BaseUnit
Adding it as another option in BaseUnit.hasUnique() and friends sounds to me like the correct solution

As for this, I think there's a second question. Do we care about the unique objects at all after the ruleset transient has been set? And do we care from the perspective of wanting to know what ruleset those uniques came from? If we did care about it solely from the perspective of the civilopedia, I could take aspects of this PR as is, having the "canonical" uniqueMaps and Objects use super's lazy pattern, but also overriding the IHasUniques functions to check for the existence of a ruleset instead and redirect to the internal maps there.

Unfortunately, there's a number of places where we expect ruleset objects, BaseUnit included, to have up to date maps. For instance, BaseUnit only checks its own unique objects and the unique objects of the promotion it expects to have for evaluating its unit force. It doesn't even check unit type uniques like it should. So ideally the idea would likely be that we have some way to indicate to other programmers that the other object list exists

[yairm210]

This is a fantastic idea, and of course you're correct - if we already know this will be reused in this ruleset, why not?
What I'm worried about is that baseUnit is copied by reference, and since this is lazy, it "locks in" the first evaluation

So given this case, where a baseunit is in mod A:

• I start Unciv
• I load game 1 which has mods A and B
• Baseunit gets global unit uniques from B "locked in"
• I load game 2 with mods A and C
• rulesetUniqueMap still has uniques from mod B even though it's no longer in the game!

This is the immutability assumption again :/

If we want to mitigate this we'll need to clone the BaseUnit when adding it to a mod, instead of adding the original

[yairm210]

This is the problem you tried to avoid in UnitType using getters/setters
TBH if we already need to handle this in both places, I think "possible mutability in UniqueObjects but we need to clone them when adding to rulesets" is cleaner than getters and setters

[SeventhM]

• rulesetUniqueMap still has uniques from mod B even though it's no longer in the game!

Huh. I kinda would've expected that we by default copy, but I never paid enough attention to that. Should we do a PR for that before merging this?