Merge pull request #28: the Yahtzee dice fixes

Holding dice works whatever order they are tapped in, and the hold survives
the throw. Verified on dev by ci.yml runs 118 and 121 and published as
v1.2.123-dev from this tree.

app/src/main/java/org/prolibertate/games/game/yahtzee/YahtzeeAi.kt

@@ -28,9 +28,8 @@ object YahtzeeAi : GameAi<YahtzeeState, YahtzeeMove> {
         if (rolls.isEmpty()) return best ?: writes.first()
         if (bestScore >= WORTH_KEEPING) return best!!
 
-        val keep = whatToKeep(state.dice)
+        val keep = whatToKeep(state.dice).toSet()
-        return rolls.firstOrNull { it.keep.sorted() == keep.sorted() }
+        return rolls.firstOrNull { it.keep == keep } ?: rolls.first()
-            ?: rolls.first()
     }
 
     /**

app/src/main/java/org/prolibertate/games/game/yahtzee/YahtzeeModel.kt

@@ -132,10 +132,16 @@ sealed interface YahtzeeMove
  * Positions rather than faces: two fives are two different dice, and a player
  * who keeps one of them and rerolls the other is doing something a set of faces
  * could not express.
+ *
+ * A set rather than a list, because holding the third die and then the first is
+ * the same act as holding the first and then the third. As a list those were two
+ * different moves, and only the ascending one was ever offered as legal — so
+ * keeping dice in any other order was silently dropped and the throw did
+ * nothing.
  */
 @Serializable
 @SerialName("roll")
-data class RollDice(val keep: List<Int> = emptyList()) : YahtzeeMove
+data class RollDice(val keep: Set<Int> = emptySet()) : YahtzeeMove
 
 @Serializable
 @SerialName("score")

app/src/main/java/org/prolibertate/games/game/yahtzee/YahtzeeRules.kt

@@ -53,7 +53,7 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
             // The first throw of a turn is all five dice: there is nothing on
             // the table yet to keep.
             moves += if (!state.hasRolled) {
-                listOf(RollDice(emptyList()))
+                listOf(RollDice(emptySet()))
             } else {
                 keepSubsets().map { RollDice(it) }
             }
@@ -67,9 +67,9 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
     }
 
     /** Every set of dice positions a player might hold back — thirty-two of them. */
-    private fun keepSubsets(): List<List<Int>> =
+    private fun keepSubsets(): List<Set<Int>> =
         (0 until (1 shl DICE_COUNT)).map { mask ->
-            (0 until DICE_COUNT).filter { mask and (1 shl it) != 0 }
+            (0 until DICE_COUNT).filter { mask and (1 shl it) != 0 }.toSet()
         }
 
     override fun applyMove(state: YahtzeeState, seat: Int, move: YahtzeeMove): YahtzeeState {
@@ -83,7 +83,6 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
 
     private fun applyRoll(state: YahtzeeState, move: RollDice): YahtzeeState {
         require(state.rollsUsed < ROLLS_PER_TURN) { "No rolls left this turn" }
-        require(move.keep.distinct().size == move.keep.size) { "Cannot keep a die twice" }
         require(move.keep.all { it in 0 until DICE_COUNT }) { "No such die" }
         require(state.hasRolled || move.keep.isEmpty()) { "Nothing to keep before the first roll" }
         return state.copy(
@@ -92,7 +91,7 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
         )
     }
 
-    private fun throwDice(state: YahtzeeState, keep: List<Int>): List<Int> {
+    private fun throwDice(state: YahtzeeState, keep: Set<Int>): List<Int> {
         val random = Random(
             state.seed +
                 state.round * 10_000L +

app/src/main/java/org/prolibertate/games/ui/game/YahtzeeScreen.kt

@@ -20,6 +20,7 @@ import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Text
 import androidx.compose.material3.TextButton
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.collectAsState
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateListOf
@@ -82,6 +83,11 @@ fun YahtzeeScreen(
         return
     }
 
+    // A turn starts with the dice back in the cup, and its first throw must
+    // keep nothing — a selection carried over from the last turn would be
+    // refused, and the Throw button would look broken again.
+    LaunchedEffect(current.turn, current.round) { kept.clear() }
+
     val yours = current.turn == localSeat && !current.isOver
     val canRoll = legal.any { it is RollDice }
     val writable = legal.filterIsInstance<ScoreIn>().map { it.category }.toSet()
@@ -113,10 +119,12 @@ fun YahtzeeScreen(
             if (yours && canRoll) {
                 Button(
                     onClick = {
-                        controller.submit(RollDice(kept.toList()))
+                        controller.submit(RollDice(kept.toSet()))
-                        // What was kept applied to the throw just made; the next
+                        // The selection stays. A kept die does not move or
-                        // one starts from whatever is on the table now.
+                        // change, so the hold a player just made is still the
-                        kept.clear()
+                        // hold they want — asking them to pick the same three
+                        // again between every throw is work the screen can do
+                        // for them.
                     },
                 ) {
                     Text(if (current.hasRolled) "Throw the rest" else "Throw")

app/src/test/java/org/prolibertate/games/game/yahtzee/YahtzeeRulesTest.kt

@@ -118,7 +118,7 @@ class YahtzeeRulesTest {
         val state = YahtzeeRules.initialState(config())
         assertEquals(0, YahtzeeRules.currentSeat(state))
         val legal = YahtzeeRules.legalMoves(state, 0)
-        assertEquals(listOf(RollDice(emptyList())), legal)
+        assertEquals(listOf(RollDice(emptySet())), legal)
         // Nothing may be written in before the dice have been thrown.
         assertThrows { YahtzeeRules.applyMove(state, 0, ScoreIn(YahtzeeCategory.CHANCE)) }
     }
@@ -142,15 +142,15 @@ class YahtzeeRulesTest {
         val again = YahtzeeRules.applyMove(state, 0, RollDice())
         assertEquals(once.dice, again.dice)
 
-        val keptOnce = YahtzeeRules.applyMove(once, 0, RollDice(listOf(0, 1)))
+        val keptOnce = YahtzeeRules.applyMove(once, 0, RollDice(setOf(0, 1)))
-        val keptAgain = YahtzeeRules.applyMove(once, 0, RollDice(listOf(0, 1)))
+        val keptAgain = YahtzeeRules.applyMove(once, 0, RollDice(setOf(0, 1)))
         assertEquals(keptOnce.dice, keptAgain.dice)
     }
 
     @Test
     fun `kept dice stay put and the rest are thrown again`() {
         val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
-        val kept = YahtzeeRules.applyMove(state, 0, RollDice(listOf(0, 2, 4)))
+        val kept = YahtzeeRules.applyMove(state, 0, RollDice(setOf(0, 2, 4)))
         assertEquals(state.dice[0], kept.dice[0])
         assertEquals(state.dice[2], kept.dice[2])
         assertEquals(state.dice[4], kept.dice[4])
@@ -169,10 +169,40 @@ class YahtzeeRulesTest {
         assertTrue(YahtzeeRules.legalMoves(state, 0).all { it is ScoreIn })
     }
 
+    @Test
+    fun `the order dice were tapped in does not matter`() {
+        // The screen adds each die as it is tapped, so holding the third and
+        // then the first once gave [2, 0]. As a list that was a different move
+        // from [0, 2], and only the ascending one was ever offered as legal, so
+        // the controller dropped it and the throw did nothing at all.
+        val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
+        assertEquals(RollDice(setOf(2, 0)), RollDice(setOf(0, 2)))
+
+        val legal = YahtzeeRules.legalMoves(state, 0)
+        assertTrue(
+            "a keep must be legal however it was built up",
+            legal.contains(RollDice(setOf(2, 0))),
+        )
+        // And it has to actually hold those dice.
+        val thrown = YahtzeeRules.applyMove(state, 0, RollDice(setOf(2, 0)))
+        assertEquals(state.dice[0], thrown.dice[0])
+        assertEquals(state.dice[2], thrown.dice[2])
+        assertEquals(2, thrown.rollsUsed)
+    }
+
+    @Test
+    fun `every way of keeping dice is on offer`() {
+        // Thirty-two subsets of five dice, and each of them exactly once.
+        val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
+        val rolls = YahtzeeRules.legalMoves(state, 0).filterIsInstance<RollDice>()
+        assertEquals(32, rolls.size)
+        assertEquals(32, rolls.map { it.keep }.distinct().size)
+    }
+
     @Test
     fun `nothing may be kept before the first throw`() {
         val state = YahtzeeRules.initialState(config())
-        assertThrows { YahtzeeRules.applyMove(state, 0, RollDice(listOf(0))) }
+        assertThrows { YahtzeeRules.applyMove(state, 0, RollDice(setOf(0))) }
     }
 
     @Test
@@ -277,7 +307,7 @@ class YahtzeeRulesTest {
         val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
         assertEquals(state, YahtzeeRules.decodeState(YahtzeeRules.encodeState(state)))
 
-        val roll: YahtzeeMove = RollDice(listOf(0, 3))
+        val roll: YahtzeeMove = RollDice(setOf(0, 3))
         assertEquals(roll, YahtzeeRules.decodeMove(YahtzeeRules.encodeMove(roll)))
         val write: YahtzeeMove = ScoreIn(YahtzeeCategory.LARGE_STRAIGHT)
         assertEquals(write, YahtzeeRules.decodeMove(YahtzeeRules.encodeMove(write)))