From d3609b0d7206abbb680cdbb1bd32f4896de44a28 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Sat, 19 Sep 2026 08:31:38 -0700 Subject: [PATCH] fix: key UnorderedPairMap by the pair, not by the pair's hash UnorderedPairMap stored values in a HashMap keyed by the combined hash of the two keys. Two distinct pairs whose combined hashes collide therefore shared one entry: put() overwrote the earlier pair's value, get() returned the wrong pair's value, and remove() on one pair evicted the other. PhysicsWorld keeps active collisions in this map, so a hash collision between two entity pairs silently dropped a real collision and onCollision under- reported. Identity hash codes vary by JVM, platform, and allocation history, which is why this surfaced as an intermittent, platform-specific collision count rather than a deterministic failure. Key the map by an UnorderedPair that implements equals as well as hashCode, keeping the existing order-independent hash. Collisions now land in the same bucket and are separated by equals, as HashMap intends. Adds a regression with a key type whose hashCode is constant, so every pair collides, and asserts distinct pairs stay distinct through get, values and remove. Also labels the two PhysicsWorldTest collision assertions so a future failure names which strategy and which phase produced the wrong count. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VoGxx6nHtCDdeiQrTEWXFz --- .../fxgl/core/collection/UnorderedPairMap.kt | 34 +++++++++++++------ .../core/collection/UnorderedPairMapTest.kt | 28 +++++++++++++++ .../almasb/fxgl/physics/PhysicsWorldTest.kt | 4 +-- 3 files changed, 53 insertions(+), 13 deletions(-) diff --git a/fxgl-core/src/main/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMap.kt b/fxgl-core/src/main/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMap.kt index 60916af65b..8df4c327b9 100644 --- a/fxgl-core/src/main/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMap.kt +++ b/fxgl-core/src/main/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMap.kt @@ -14,7 +14,7 @@ package com.almasb.fxgl.core.collection class UnorderedPairMap @JvmOverloads constructor(capacity: Int = 16) { - private val map = HashMap(capacity) + private val map = HashMap, V>(capacity) val values get() = map.values @@ -30,31 +30,43 @@ class UnorderedPairMap * @return a value for [key1] [key2] pair or null if no such key exists */ fun get(key1: K, key2: K): V? { - return map[hash(key1, key2)] + return map[UnorderedPair(key1, key2)] } /** * Add a new mapping from [key1] [key2] to [value]. */ fun put(key1: K, key2: K, value: V) { - map[hash(key1, key2)] = value + map[UnorderedPair(key1, key2)] = value } /** * Remove an existing mapping whose key is [key1] [key2]. */ fun remove(key1: K, key2: K) { - map.remove(hash(key1, key2)) + map.remove(UnorderedPair(key1, key2)) } - private fun hash(key1: K, key2: K): Int { - val hash1 = key1.hashCode() - val hash2 = key2.hashCode() + /** + * Unordered pair key. Distinct pairs with the same hash must remain distinct. + */ + private class UnorderedPair(val a: K, val b: K) { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is UnorderedPair<*>) return false + + return (a == other.a && b == other.b) || (a == other.b && b == other.a) + } + + override fun hashCode(): Int { + val hash1 = a.hashCode() + val hash2 = b.hashCode() - return if (hash1 > hash2) { - 31 * (31 + hash1) + hash2 - } else { - 31 * (31 + hash2) + hash1 + return if (hash1 > hash2) { + 31 * (31 + hash1) + hash2 + } else { + 31 * (31 + hash2) + hash1 + } } } } \ No newline at end of file diff --git a/fxgl-core/src/test/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMapTest.kt b/fxgl-core/src/test/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMapTest.kt index ba8b1cbb28..a8f9dadfc7 100644 --- a/fxgl-core/src/test/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMapTest.kt +++ b/fxgl-core/src/test/kotlin/com/almasb/fxgl/core/collection/UnorderedPairMapTest.kt @@ -69,5 +69,33 @@ class UnorderedPairMapTest { assertThat(map2.get(key1, CustomObject()), nullValue()) } + @Test + fun `Different pairs with the same hash stay distinct`() { + class Key(val id: Int) { + override fun hashCode() = 7 + override fun equals(other: Any?) = other is Key && id == other.id + } + + val map2 = UnorderedPairMap() + val a = Key(1) + val b = Key(2) + val c = Key(3) + val d = Key(4) + + map2.put(a, b, "ab") + map2.put(c, d, "cd") + + assertThat(map2.get(a, b), `is`("ab")) + assertThat(map2.get(b, a), `is`("ab")) + assertThat(map2.get(c, d), `is`("cd")) + assertThat(map2.get(d, c), `is`("cd")) + assertThat(map2.values, containsInAnyOrder("ab", "cd")) + + map2.remove(a, b) + + assertThat(map2.get(a, b), nullValue()) + assertThat(map2.get(c, d), `is`("cd")) + } + private class CustomObject } \ No newline at end of file diff --git a/fxgl-core/src/test/kotlin/com/almasb/fxgl/physics/PhysicsWorldTest.kt b/fxgl-core/src/test/kotlin/com/almasb/fxgl/physics/PhysicsWorldTest.kt index ae10112fbb..0fb69fafd2 100644 --- a/fxgl-core/src/test/kotlin/com/almasb/fxgl/physics/PhysicsWorldTest.kt +++ b/fxgl-core/src/test/kotlin/com/almasb/fxgl/physics/PhysicsWorldTest.kt @@ -763,7 +763,7 @@ class PhysicsWorldTest { gWorld.onUpdate(0.016) pWorld.onUpdate(0.016) - assertThat(numCollisions, `is`(843)) + assertThat("unrotated collisions with $strategy", numCollisions, `is`(843)) // check with rotations now numCollisions = 0 @@ -775,7 +775,7 @@ class PhysicsWorldTest { gWorld.onUpdate(0.016) pWorld.onUpdate(0.016) - assertThat(numCollisions, `is`(970)) + assertThat("rotated collisions with $strategy", numCollisions, `is`(970)) } } } \ No newline at end of file