Stats optimization (#4830)

* Stats rework part 1

* Stats rework part 1 -patch1

* Stats rework part 1 - documentation

core/src/com/unciv/models/stats/Stats.kt

@@ -1,21 +1,69 @@
 package com.unciv.models.stats
 
 import com.unciv.models.translations.tr
+import kotlin.reflect.KMutableProperty0
 
-
+/**
-open class Stats() {
+ * A container for the seven basic ["currencies"][Stat] in Unciv,
-    var production: Float = 0f
+ * **Mutable**, allowing for easy merging of sources and applying bonuses.
-    var food: Float = 0f
+ */
-    var gold: Float = 0f
+open class Stats(
-    var science: Float = 0f
+    var production: Float = 0f,
-    var culture: Float = 0f
+    var food: Float = 0f,
-    var happiness: Float = 0f
+    var gold: Float = 0f,
+    var science: Float = 0f,
+    var culture: Float = 0f,
+    var happiness: Float = 0f,
     var faith: Float = 0f
-
+) {
-    constructor(hashMap: HashMap<Stat, Float>) : this() {
+    // This is what facilitates indexed access by [Stat] or add(Stat,Float)
-        setStats(hashMap)
+    // without additional memory allocation or expensive conditionals
+    @delegate:Transient
+    private val mapView: Map<Stat, KMutableProperty0<Float>> by lazy {
+        mapOf(
+            Stat.Production to ::production,
+            Stat.Food to ::food,
+            Stat.Gold to ::gold,
+            Stat.Science to ::science,
+            Stat.Culture to ::culture,
+            Stat.Happiness to ::happiness,
+            Stat.Faith to ::faith
+        )
     }
 
+    /** Indexed read of a value for a given [Stat], e.g. `this.gold == this[Stat.Gold]` */
+    operator fun get(stat: Stat) = mapView[stat]!!.get()
+    /** Indexed write of a value for a given [Stat], e.g. `this.gold += 1f` is equivalent to `this[Stat.Gold] += 1f` */
+    operator fun set(stat: Stat, value: Float) = mapView[stat]!!.set(value)
+
+    /** Compares two instances. Not callable via `==`. */
+    // This is an overload, not an override conforming to the kotlin conventions of `equals(Any?)`,
+    // so do not rely on it to be called for the `==` operator! A tad more efficient, though.
+    @Suppress("CovariantEquals")    // historical reasons to keep this function signature
+    fun equals(otherStats: Stats): Boolean {
+        return production == otherStats.production
+                && food == otherStats.food
+                && gold == otherStats.gold
+                && science == otherStats.science
+                && culture == otherStats.culture
+                && happiness == otherStats.happiness
+                && faith == otherStats.faith
+    }
+
+    /** @return a new instance containing the same values as `this` */
+    fun clone() = Stats(production, food, gold, science, culture, happiness, faith)
+
+    /** @return `true` if all values are zero */
+    fun isEmpty() = (
+            production == 0f
+            && food == 0f
+            && gold == 0f
+            && science == 0f
+            && culture == 0f
+            && happiness == 0f
+            && faith == 0f )
+
+    /** Reset all values to zero (in place) */
     fun clear() {
         production = 0f
         food = 0f
@@ -26,8 +74,8 @@ open class Stats() {
         faith = 0f
     }
 
+    /** Adds each value of another [Stats] instance to this one in place */
     fun add(other: Stats) {
-        // Doing this through the hashmap is nicer code but is SUPER INEFFICIENT!
         production += other.production
         food += other.food
         gold += other.gold
@@ -37,91 +85,89 @@ open class Stats() {
         faith += other.faith
     }
 
+    /** @return a new [Stats] instance containing the sum of its operands value by value */
+    operator fun plus(stats: Stats) = clone().apply { add(stats) }
 
+    /** Adds the [value] parameter to the instance value specified by [stat] in place
+     * @return `this` to allow chaining */
     fun add(stat: Stat, value: Float): Stats {
-        val hashMap = toHashMap()
+        mapView[stat]!!.set(value + mapView[stat]!!.get())
-        hashMap[stat] = hashMap[stat]!! + value
-        setStats(hashMap)
         return this
     }
 
-    operator fun plus(stat: Stats): Stats {
+    /** @return The result of multiplying each value of this instance by [number] as a new instance */
-        val clone = clone()
-        clone.add(stat)
-        return clone
-    }
-
-    fun clone(): Stats {
-        val stats = Stats()
-        stats.add(this)
-        return stats
-    }
-
     operator fun times(number: Int) = times(number.toFloat())
+    /** @return The result of multiplying each value of this instance by [number] as a new instance */
+    operator fun times(number: Float) = Stats(
+        production * number,
+        food * number,
+        gold * number,
+        science * number,
+        culture * number,
+        happiness * number,
+        faith * number
+    )
 
-    operator fun times(number: Float): Stats {
+    /** Multiplies each value of this instance by [number] in place */
-        val hashMap = toHashMap()
-        for (stat in Stat.values()) hashMap[stat] = number * hashMap[stat]!!
-        return Stats(hashMap)
-    }
-    
     fun timesInPlace(number: Float) {
-        val hashMap = toHashMap()
+        production *= number
-        for (stat in Stat.values()) hashMap[stat] = number * hashMap[stat]!!
+        food *= number
-        setStats(hashMap)
+        gold *= number
+        science *= number
+        culture *= number
+        happiness *= number
+        faith *= number
     }
 
-    fun isEmpty() = equals(Stats())
+    /** ***Not*** only a debug helper. It returns a string representing the content, already _translated_.
-
+     * 
+     * Example output: `+1 Production, -1 Food`.
+     */
     override fun toString(): String {
         return toHashMap().filter { it.value != 0f }
                 .map { (if (it.value > 0) "+" else "") + it.value.toInt() + " " + it.key.toString().tr() }.joinToString()
     }
 
+    /** @return a Map copy of the values in this instance, can be used to iterate over the values */
     fun toHashMap(): HashMap<Stat, Float> {
-        return linkedMapOf(Stat.Production to production,
+        return linkedMapOf(
-                Stat.Culture to culture,
+            Stat.Production to production,
-                Stat.Gold to gold,
             Stat.Food to food,
-                Stat.Happiness to happiness,
+            Stat.Gold to gold,
             Stat.Science to science,
+            Stat.Culture to culture,
+            Stat.Happiness to happiness,
             Stat.Faith to faith
         )
     }
 
-    fun get(stat: Stat): Float {
-        return this.toHashMap()[stat]!!
-    }
-
-    private fun setStats(hashMap: HashMap<Stat, Float>) {
-        culture = hashMap[Stat.Culture]!!
-        gold = hashMap[Stat.Gold]!!
-        production = hashMap[Stat.Production]!!
-        food = hashMap[Stat.Food]!!
-        happiness = hashMap[Stat.Happiness]!!
-        science = hashMap[Stat.Science]!!
-        faith = hashMap[Stat.Faith]!!
-    }
-
-    fun equals(otherStats: Stats): Boolean {
-        return culture == otherStats.culture
-                && gold == otherStats.gold
-                && production == otherStats.production
-                && food == otherStats.food
-                && happiness == otherStats.happiness
-                && science == otherStats.science
-                && faith == otherStats.faith
-    }
 
     companion object {
         private val allStatNames = Stat.values().joinToString("|") { it.name }
         private val statRegexPattern = "([+-])(\\d+) ($allStatNames)"
         private val statRegex = Regex(statRegexPattern)
         private val entireStringRegexPattern = Regex("$statRegexPattern(, $statRegexPattern)*")
+
+        /** Tests a given string whether it is a valid representation of [Stats],
+         * close to what [toString] would produce.
+         * - Values _must_ carry a sign - "1 Gold" tests `false`, "+1 Gold" is OK.
+         * - Separator is ", " - comma space - the space is _not_ optional.
+         * - Stat names must be untranslated and match case.
+         * - Order is not important.
+         * @see [parse]
+         */
         fun isStats(string: String): Boolean {
             if (string.isEmpty() || string[0] !in "+-") return false // very quick negative check before the heavy Regex
             return entireStringRegexPattern.matches(string)
         }
+
+        /** Parses a string to a [Stats] instance
+         * - Values _must_ carry a sign - "1 Gold" will not parse, "+1 Gold" is OK.
+         * - Separator is ", " - comma space - the space is _not_ optional.
+         * - Stat names must be untranslated and match case.
+         * - Order is not important.
+         * @see [isStats]
+         */
         fun parse(string: String): Stats {
             val toReturn = Stats()
             val statsWithBonuses = string.split(", ")

tests/src/com/unciv/testing/BasicTests.kt

@@ -14,6 +14,8 @@ import org.junit.Assert
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
+import kotlin.math.abs
+import kotlin.random.Random
 
 @RunWith(GdxTestRunner::class)
 class BasicTests {
@@ -85,4 +87,50 @@ class BasicTests {
         Assert.assertFalse(modCheck.isNotOK())
     }
 
+    //@Test  // commented so github doesn't run this
+    fun statMathStressTest() {
+        val runtime = Runtime.getRuntime()
+        runtime.gc()
+        Thread.sleep(5000) // makes timings a little more repeatable
+        val startTime = System.nanoTime()
+        statMathRunner(iterations = 1_000_000)
+        println("statMathStressTest took ${(System.nanoTime()-startTime)/1000}µs")
+    }
+
+    @Test
+    fun statMathRandomResultTest() {
+        val iterations = 42
+        val expectedStats = Stats().apply {
+            production = 12970.174f
+            food = -153216.12f
+            gold = 28614.738f
+            science = 142650.89f
+            culture = -45024.03f
+            happiness = -7081.2495f
+            faith = -14933.622f
+        }
+        val stats = statMathRunner(iterations)
+        Assert.assertTrue(stats.equals(expectedStats))
+    }
+
+    private fun statMathRunner(iterations: Int): Stats {
+        val random = Random(42)
+        val statCount = Stat.values().size
+        val stats = Stats()
+
+        for (i in 0 until iterations) {
+            val value: Float = random.nextDouble(-10.0, 10.0).toFloat()
+            stats.add( Stats(gold = value) )
+            stats.toHashMap().forEach {
+                val stat = Stat.values()[(it.key.ordinal + random.nextInt(1,statCount)).rem(statCount)]
+                stats.add(stat, -it.value)
+            }
+            val stat = Stat.values()[random.nextInt(statCount)]
+            stats.add(stat, stats.times(4).get(stat))
+            stats.timesInPlace(0.8f)
+            if (abs(stats.toHashMap().maxOfOrNull { it.value }!!) > 1000000f)
+                stats.timesInPlace(0.1f)
+        }
+        return stats
+    }
 }