Skip to content

BWEM Bugfix: Different chokes receive equal hashcodes #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dgant wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

BWEM Bugfix: Different chokes receive equal hashcodes #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions src/main/java/bwem/ChokePoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,25 +313,14 @@ public boolean equals(final Object object) {
return false;
} else {
final ChokePoint that = (ChokePoint) object;
final boolean lel = this.areas.getLeft().equals(that.areas.getLeft());
final boolean ler = this.areas.getLeft().equals(that.areas.getRight());
final boolean rer = this.areas.getRight().equals(that.areas.getRight());
final boolean rel = this.areas.getRight().equals(that.areas.getLeft());
return lel && rer
|| ler && rel; /* true if area pairs are an exact match or if one pair is reversed. */
// Parallel connections can share areas or even a center, but not a graph/index.
return this.graph == that.graph && this.index == that.index;

@JasperGeurtz JasperGeurtz Nov 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointer equality for graph is wrong here i think, two the same maps in different runs "should" return an equals true, I propose the following diff:

diff --git a/src/main/java/bwem/ChokePoint.java b/src/main/java/bwem/ChokePoint.java
index 71204f43..6d91c4a2 100644
--- a/src/main/java/bwem/ChokePoint.java
+++ b/src/main/java/bwem/ChokePoint.java
@@ -43,6 +43,7 @@ public final class ChokePoint {
     private final Graph graph;
     private final boolean isPseudo;
     private final int index;
+    private final String mapHash;
     private final Pair<Area, Area> areas;
     private final WalkPosition[] nodes;
     private final List<Pair<WalkPosition, WalkPosition>> nodesInArea;
@@ -64,6 +65,7 @@ public final class ChokePoint {
 
         this.graph = graph;
         this.index = index;
+        this.mapHash = graph.getMap().game.mapHash();
         this.areas = new Pair<>(area1, area2);
         this.geometry = geometry;
 
@@ -313,25 +315,13 @@ public final class ChokePoint {
             return false;
         } else {
             final ChokePoint that = (ChokePoint) object;
-            final boolean lel = this.areas.getLeft().equals(that.areas.getLeft());
-            final boolean ler = this.areas.getLeft().equals(that.areas.getRight());
-            final boolean rer = this.areas.getRight().equals(that.areas.getRight());
-            final boolean rel = this.areas.getRight().equals(that.areas.getLeft());
-            return lel && rer
-                    || ler && rel; /* true if area pairs are an exact match or if one pair is reversed. */
+            return this.index == that.index && Objects.equals(this.mapHash, that.mapHash);
         }
     }
 
     @Override
     public int hashCode() {
-        int idLeft = areas.getLeft().getId().intValue();
-        int idRight = areas.getRight().getId().intValue();
-        if (idLeft > idRight) {
-            final int idLeftTmp = idLeft;
-            idLeft = idRight;
-            idRight = idLeftTmp;
-        }
-        return Objects.hash(idLeft, idRight);
+        return Objects.hash(this.index, this.mapHash);
     }

}
}

@Override
public int hashCode() {
int idLeft = areas.getLeft().getId().intValue();
int idRight = areas.getRight().getId().intValue();
if (idLeft > idRight) {
final int idLeftTmp = idLeft;
idLeft = idRight;
idRight = idLeftTmp;
}
return Objects.hash(idLeft, idRight);
return Objects.hash(graph, index);
}

/**
Expand Down
41 changes: 36 additions & 5 deletions src/test/java/bwapi/BWEMTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,19 @@

import bwem.BWEM;
import bwem.BWMap;
import bwta.BWTA;
import bwta.Chokepoint;
import bwta.Region;
import org.junit.Test;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.*;

public class BWEMTest {
static class BWEMMap{
Expand Down Expand Up @@ -66,13 +72,38 @@ public int hashCode() {
mapData.put("(4)Roadrunner.scx", new BWEMMap(26, 12, 35));
}

@Test
public void chokePointsFromDifferentMapsAreDistinct() throws IOException {

@JasperGeurtz JasperGeurtz Nov 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems unintuitive, if anything I would expect the opposite. These maps here (first and second) are the same map.

BWEM first = new BWEM(GameBuilder.createGame());
first.initialize();
BWEM second = new BWEM(GameBuilder.createGame());
second.initialize();
assertTrue(Collections.disjoint(first.getMap().getChokePoints(), second.getMap().getChokePoints()));
}

@Test
public void checkSSCAITMaps() throws IOException {
for (String mapName : mapData.keySet()) {

@JasperGeurtz JasperGeurtz Nov 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer testing through the BWEM API, PTAL if somehting like the following could work:

  • test two different maps have different chokepoint equality/hashcode
  • test two the same maps have equal equality hashcode
  • test that maps with chokepoints in sets (previously triggering same equals & hashcode in some situation marking them as the same, even if they are different)
diff --git a/src/test/java/bwapi/BWEMTest.java b/src/test/java/bwapi/BWEMTest.java
index 65558961..c73d8e14 100644
--- a/src/test/java/bwapi/BWEMTest.java
+++ b/src/test/java/bwapi/BWEMTest.java
@@ -1,16 +1,21 @@
 package bwapi;
 
+import bwem.Area;
 import bwem.BWEM;
 import bwem.BWMap;
+import bwem.ChokePoint;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class BWEMTest {
     static class BWEMMap{
@@ -66,12 +71,42 @@ public class BWEMTest {
         mapData.put("(4)Roadrunner.scx", new BWEMMap(26, 12, 35));
     }
 
+    @Test
+    public void chokePointsFromSameMapAreEqual() throws IOException {
+        BWEM first = new BWEM(GameBuilder.createGame("(2)Destination.scx"));
+        first.initialize();
+        BWEM second = new BWEM(GameBuilder.createGame("(2)Destination.scx"));
+        second.initialize();
+        assertEquals(first.getMap().getChokePoints(), second.getMap().getChokePoints());
+        assertEquals(new HashSet<>(first.getMap().getChokePoints()), new HashSet<>(second.getMap().getChokePoints()));
+    }
+
+    @Test
+    public void chokePointsFromDifferentMapsAreDistinct() throws IOException {
+        BWEM first = new BWEM(GameBuilder.createGame("(2)Benzene.scx"));
+        first.initialize();
+        BWEM second = new BWEM(GameBuilder.createGame("(2)Destination.scx"));
+        second.initialize();
+        assertTrue(Collections.disjoint(first.getMap().getChokePoints(), second.getMap().getChokePoints()));
+    }
+
     @Test
     public void checkSSCAITMaps() throws IOException {
         for (String mapName : mapData.keySet()) {
             Game game = GameBuilder.createGame(mapName);
             BWEM bwem = new BWEM(game);
             bwem.initialize();
-            assertEquals(new BWEMMap(bwem.getMap()), mapData.get(mapName));
+            BWMap map = bwem.getMap();
+            assertEquals(new BWEMMap(map), mapData.get(mapName));
+            assertEquals(mapName, map.getChokePoints().size(), new HashSet<>(map.getChokePoints()).size());
+
+            for (Area area : map.getAreas()) {
+                for (ChokePoint cp1 : area.getChokePoints()) {
+                    for (ChokePoint cp2 : area.getChokePoints()) {
+                        if (!cp1.isBlocked() && !cp2.isBlocked()) {
+                            assertTrue(mapName, cp1.accessibleFrom(cp2));
+                        }
+                    }
+                }
+            }
         }
     }
 }

Game game = GameBuilder.createGame(mapName);
BWEM bwem = new BWEM(game);
bwem.initialize();
assertEquals(new BWEMMap(bwem.getMap()), mapData.get(mapName));
BWTA.readMap(GameBuilder.createGame(mapName));
BWTA.analyze();
BWMap map = BWTA.getBWEM().getMap();
assertEquals(new BWEMMap(map), mapData.get(mapName));
assertEquals(mapName, map.getChokePoints().size(), new HashSet<>(map.getChokePoints()).size());

List<Chokepoint> chokes = BWTA.getChokepoints();
assertEquals(mapName, map.getChokePoints().size(), chokes.size());
for (Chokepoint choke : chokes) {
assertTrue(choke.getRegions().getLeft().getChokepoints().contains(choke));
assertTrue(choke.getRegions().getRight().getChokepoints().contains(choke));
}
for (Region region : BWTA.getRegions()) {
for (Chokepoint choke : region.getChokepoints()) {
assertNotNull(mapName, choke);
assertTrue(mapName, chokes.contains(choke));
assertTrue(choke.getRegions().getLeft().equals(region)
|| choke.getRegions().getRight().equals(region));
}
}
}
}
}
Loading