[dgant]

Distinct BWEM chokepoints connecting the same two areas currently compare and hash as equal.

Discovered on Katrina, where one of the main bases appears to have no walkable choke leading out of it.

[JasperGeurtz]

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

[JasperGeurtz]

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);
     }

[JasperGeurtz]

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));
+                        }
+                    }
+                }
+            }
         }
     }
 }