fix: using cachedRGB value directly causing incorrect serialization in ChromaColour

Author: lineargraph

build-src/src/main/kotlin/moulconfig.test.gradle.kts

@@ -0,0 +1,8 @@
+tasks.withType<Test> {
+    useJUnitPlatform()
+}
+dependencies {
+    val junit5Version = "5.8.1"
+    "testImplementation"("org.junit.jupiter:junit-jupiter-api:$junit5Version")
+    "testRuntimeOnly"("org.junit.jupiter:junit-jupiter-engine:$junit5Version")
+}

common/build.gradle.kts

@@ -4,6 +4,7 @@ plugins {
     id("moulconfig.dokka")
     `maven-publish`
 	id("moulconfig.base")
+    id("moulconfig.test")
 }
 
 java.toolchain.languageVersion.set(JavaLanguageVersion.of(8))

common/src/main/java/io/github/notenoughupdates/moulconfig/ChromaColour.kt

@@ -3,6 +3,7 @@ package io.github.notenoughupdates.moulconfig
 import com.google.gson.annotations.Expose
 import java.awt.Color
 import kotlin.math.abs
+import kotlin.math.roundToInt
 
 @Suppress("DeprecatedCallableAddReplaceWith", "DEPRECATION")
 data class ChromaColour(
@@ -35,8 +36,8 @@ data class ChromaColour(
 ) {
 
     private fun evaluateColourWithShift(hueShift: Double): Int {
-        if (abs(cachedRGBHueOffset - hueShift) < 1/360.0)return cachedRGB
-        val effectiveHue = ((hue.toDouble() + hueShift)%1).toFloat()
+        if (abs(cachedRGBHueOffset - hueShift) < 1 / 360.0) return cachedRGB
+        val effectiveHue = ((hue.toDouble() + hueShift) % 1).toFloat()
         val ret = (Color.HSBtoRGB(effectiveHue, saturation, brightness) and 0x00FFFFFF) or (alpha shl 24)
         cachedRGBHueOffset = hueShift
         cachedRGB = ret
@@ -68,6 +69,7 @@ data class ChromaColour(
         effectiveHueOffset += offset
         return evaluateColourWithShift(effectiveHueOffset)
     }
+
     /**
      * @param offset offset the colour by a hue amount.
      * @return the colour, at the current time if this is a chrome colour
@@ -106,12 +108,12 @@ data class ChromaColour(
 
     @Deprecated("")
     fun toLegacyString(): String {
-        val timeInSeconds = timeForFullRotationInMillis / 1000
         val namedSpeed =
-        if (timeInSeconds == 0) 0 else (255 - (timeInSeconds - MIN_CHROMA_SECS) * 254f / (MAX_CHROMA_SECS - MIN_CHROMA_SECS)).toInt()
-        val red = cachedRGB shr 16 and 0xFF
-        val green = cachedRGB shr 8 and 0xFF
-        val blue = cachedRGB and 0xFF
+            if (timeForFullRotationInMillis == 0) 0 else getSpeedForMillis(timeForFullRotationInMillis / 1000f)
+        val rgb = evaluateColourWithShift(.0)
+        val red = rgb shr 16 and 0xFF
+        val green = rgb shr 8 and 0xFF
+        val blue = rgb and 0xFF
         return special(namedSpeed, alpha, red, green, blue)
     }
 
@@ -172,6 +174,11 @@ data class ChromaColour(
         @Deprecated("")
         fun getSecondsForSpeed(speed: Int): Float = (255 - speed) / 254f * (MAX_CHROMA_SECS - MIN_CHROMA_SECS) + MIN_CHROMA_SECS
 
+        @Deprecated("")
+        fun getSpeedForMillis(seconds: Float): Int {
+            return (255 - ((seconds - MIN_CHROMA_SECS) / (MAX_CHROMA_SECS - MIN_CHROMA_SECS) * 254)).roundToInt()
+        }
+
         @JvmStatic
         @Deprecated("")
         fun specialToChromaRGB(special: String): Int {

common/src/test/java/io/github/notenoughupdates/moulconfig/ChromaColourTest.kt

@@ -0,0 +1,36 @@
+package io.github.notenoughupdates.moulconfig
+
+import org.junit.jupiter.api.Assertions
+import org.junit.jupiter.api.Disabled
+import org.junit.jupiter.api.Test
+import java.awt.Color
+
+/**
+ * Test to verfiy that [ChromaColour.toLegacyString] and [ChromaColour.forLegacyString] both work as expected, serializing the RGB value.
+ */
+class ChromaColourTest {
+    fun testWithSpeed(jColor: Color, speed: Int) {
+        val stringRepr = "$speed:${jColor.alpha}:${jColor.red}:${jColor.green}:${jColor.blue}"
+        val time = if (speed > 0) (ChromaColour.getSecondsForSpeed(speed) * 1000).toInt() else 0
+        val fromStatic = ChromaColour.fromRGB(jColor.red, jColor.green, jColor.blue, time, jColor.alpha)
+        if (speed == 0) // Can't test colour accuracy given that the system time influences this
+            Assertions.assertEquals(jColor.rgb, fromStatic.getEffectiveColourRGB())
+        Assertions.assertEquals(stringRepr, fromStatic.toLegacyString())
+        Assertions.assertEquals(fromStatic, ChromaColour.forLegacyString(stringRepr))
+    }
+
+    @Disabled("This test takes around ~8 seconds to run, we don't need that overhead")
+    @Test
+    fun testAllColours() {
+        yieldAllColours()
+            .forEach {
+                testWithSpeed(it, 0)
+                testWithSpeed(it, 10)
+                testWithSpeed(it, 255)
+            }
+    }
+
+    fun yieldAllColours() = (0..0xFFFFFF)
+        .asSequence()
+        .map { Color(it) }
+}