Round of fixes for jbwapi

src/main/java/bwapi/BuildingPlacer.java

@@ -243,9 +243,8 @@ private static void ReserveAllStructures(final PlacementReserve reserve, final U
         reserve.restoreIfInvalid();
     }
 
-    private static void ReserveExistingAddonPlacement(final PlacementReserve reserve, final TilePosition desiredPosition, final Game game) {
-        final TilePosition start = desiredPosition.subtract(new TilePosition(MAX_RANGE, MAX_RANGE)).divide(2);
-
+    static void ReserveExistingAddonPlacement(final PlacementReserve reserve, final TilePosition desiredPosition, final Game game) {
+        final TilePosition start = desiredPosition.subtract(new TilePosition(MAX_RANGE, MAX_RANGE).divide(2));
         //Exclude addon placement locations
         reserve.backup();
         game.self().getUnits().stream()
@@ -259,7 +258,7 @@ private static void ReserveExistingAddonPlacement(final PlacementReserve reserve
         reserve.restoreIfInvalid();
     }
 
-    private static void ReserveDefault(final PlacementReserve reserve, final UnitType type, final TilePosition desiredPosition, final Game game) {
+    static void ReserveDefault(final PlacementReserve reserve, final UnitType type, final TilePosition desiredPosition, final Game game) {
         reserve.backup();
         final PlacementReserve original = reserve;
 
@@ -274,15 +273,14 @@ private static void ReserveDefault(final PlacementReserve reserve, final UnitTyp
                 case Terran_Missile_Turret:
                 case Protoss_Robotics_Facility:
                 case Protoss_Gateway:
-                case Protoss_Photon_Cannon:
+                default:
+                    ReserveStructure(reserve, it, 2, type, desiredPosition);
+                    break;
                 case Terran_Barracks:
                 case Terran_Bunker:
                 case Zerg_Creep_Colony:
                     ReserveStructure(reserve, it, 1, type, desiredPosition);
                     break;
-                default:
-                    ReserveStructure(reserve, it, 2, type, desiredPosition);
-                    break;
             }
         }
 

src/main/java/bwapi/Game.java

@@ -16,7 +16,7 @@
  * game state information from Starcraft Broodwar. Game state information includes all units,
  * resources, players, forces, bullets, terrain, fog of war, regions, etc.
  */
-public final class Game {
+public class Game {
     private static final int[][] damageRatio = {
             // Ind, Sml, Med, Lrg, Non, Unk
             {0, 0, 0, 0, 0, 0}, // Independent
@@ -680,10 +680,12 @@ public List<Unit> getUnitsOnTile(final int tileX, final int tileY) {
      * given build tile.
      */
     public List<Unit> getUnitsOnTile(final int tileX, final int tileY, final UnitFilter pred) {
-        return getAllUnits().stream().filter(u -> {
-            final TilePosition tp = u.getTilePosition();
-            return tp.x == tileX && tp.y == tileY && pred.test(u);
-        }).collect(Collectors.toList());
+        final TilePosition tile = new TilePosition(tileX, tileY);
+        if (!tile.isValid(this)) {
+            return Collections.emptyList();
+        }
+        final Position p = tile.toPosition();
+        return getUnitsInRectangle(p.x, p.y, p.x + 32, p.y + 32, pred);
     }
 
     public List<Unit> getUnitsInRectangle(final int left, final int top, final int right, final int bottom) {
@@ -740,7 +742,7 @@ public List<Unit> getUnitsInRadius(final Position center, final int radius) {
 
     public List<Unit> getUnitsInRadius(final Position center, final int radius, final UnitFilter pred) {
         return getAllUnits().stream()
-                .filter(u -> center.getApproxDistance(u.getPosition()) <= radius && pred.test(u))
+                .filter(u -> u.getDistance(center) <= radius && pred.test(u))
                 .collect(Collectors.toList());
     }
 
@@ -2380,10 +2382,9 @@ public int getLastEventTime() {
      * parameter.
      */
     final public boolean setMap(final String mapFileName) {
-        if (mapFileName == null || mapFileName.length() >= 260 || mapFileName.charAt(0) == 0) {
+        if (mapFileName == null || mapFileName.isEmpty() || mapFileName.length() >= 260 || mapFileName.charAt(0) == 0) {
             return false;
         }
-
         addCommand(CommandType.SetMap, mapFileName, 0);
         return true;
     }

src/main/java/bwapi/Player.java

@@ -544,14 +544,14 @@ public Color getColor() {
      * @return character code to use for text in Broodwar.
      */
     public Text getTextColor() {
-        switch (color.id) {
+        switch (getColor().id) {
             case 111: // red
                 return Text.BrightRed;
             case 165: // blue
                 return Text.Blue;
             case 159: // teal
                 return Text.Teal;
-            case 164: // purp
+            case 164: // purple
                 return Text.Purple;
             case 156: // orange with fix from @n00byEdge
                 return Text.Orange;
@@ -561,7 +561,7 @@ public Text getTextColor() {
                 return Text.PlayerWhite;
             case 135: // yellow
                 return Text.PlayerYellow;
-            case 185: // green p9
+            case 117: // green
                 return Text.DarkGreen;
             case 136: // p10
                 return Text.LightYellow;

src/main/java/bwapi/Unit.java

@@ -1270,7 +1270,7 @@ public List<Unit> getUnitsInRadius(final int radius, final UnitFilter pred) {
                 getTop() - radius,
                 getRight() + radius,
                 getBottom() + radius,
-                u -> getDistance(u) <= radius && pred.test(u));
+                u -> u != this && getDistance(u) <= radius && pred.test(u));
     }
 
     public List<Unit> getUnitsInWeaponRange(final WeaponType weapon) {
@@ -1301,30 +1301,29 @@ public List<Unit> getUnitsInWeaponRange(final WeaponType weapon, final UnitFilte
                 getRight() + max,
                 getBottom() + max,
                 u -> {
-                    if (!pred.test(u)) {
-                        return false;
-                    }
                     // Unit check and unit status
                     if (u == this || u.isInvincible()) {
                         return false;
                     }
-
                     // Weapon distance check
                     final int dist = getDistance(u);
                     if ((weapon.minRange() != 0 && dist < weapon.minRange()) || dist > max) {
                         return false;
                     }
-
                     // Weapon behavioural checks
+                    // NOTE: The target flag checks below (after targetsOwn) are inverted, matching an upstream bug in C++ BWAPI's UnitInterface::getUnitsInWeaponRange (BWAPILIB/Source/Unit.cpp).
                     final UnitType ut = u.getType();
-                    return (!weapon.targetsOwn() || u.getPlayer().equals(getPlayer())) &&
-                            (weapon.targetsAir() || u.isFlying()) &&
-                            (weapon.targetsGround() || !u.isFlying()) &&
-                            (!weapon.targetsMechanical() || !ut.isMechanical()) &&
-                            (!weapon.targetsOrganic() || !ut.isOrganic()) &&
-                            (!weapon.targetsNonBuilding() || ut.isBuilding()) &&
-                            (!weapon.targetsNonRobotic() || ut.isRobotic()) &&
-                            (!weapon.targetsOrgOrMech() || (!ut.isOrganic() && !ut.isMechanical()));
+                    if ((weapon.targetsOwn() && !u.getPlayer().equals(getPlayer())) ||
+                            (!weapon.targetsAir() && !u.isFlying()) ||
+                            (!weapon.targetsGround() && u.isFlying()) ||
+                            (weapon.targetsMechanical() && ut.isMechanical()) ||
+                            (weapon.targetsOrganic() && ut.isOrganic()) ||
+                            (weapon.targetsNonBuilding() && !ut.isBuilding()) ||
+                            (weapon.targetsNonRobotic() && !ut.isRobotic()) ||
+                            (weapon.targetsOrgOrMech() && (ut.isOrganic() || ut.isMechanical()))) {
+                        return false;
+                    }
+                    return pred.test(u);
                 });
     }
 
@@ -3913,7 +3912,7 @@ public boolean canAttackUnitGrouped(Unit targetUnit, boolean checkCanTargetUnit,
             return false;
         }
 
-        if (isInvincible()) {
+        if (targetUnit.isInvincible()) {
             return false;
         }
 
@@ -4717,16 +4716,14 @@ public boolean canGather(Unit targetUnit, boolean checkCanTargetUnit, boolean ch
             return false;
         }
 
-        if (!isCompleted()) {
+        if (!targetUnit.isCompleted()) {
             return false;
         }
 
-        if (!hasPath(getPosition())) {
+        if (!hasPath(targetUnit.getPosition())) {
             return false;
         }
-
-        return !uType.isRefinery() || getPlayer().equals(game.self());
-
+        return !uType.isRefinery() || targetUnit.getPlayer().equals(game.self());
     }
 
     public boolean canReturnCargo() {
@@ -6171,7 +6168,7 @@ public boolean canUseTechUnit(TechType tech, Unit targetUnit, boolean checkCanTa
                 if (!targetType.isOrganic() || targetType.isFlyer()) {
                     return false;
                 }
-                if (!targetUnit.getPlayer().isNeutral() && getPlayer().isEnemy(getPlayer())) {
+                if (!targetUnit.getPlayer().isNeutral() && getPlayer().isEnemy(targetUnit.getPlayer())) {
                     return false;
                 }
                 break;

src/main/java/bwapi/UnitFilter.java

@@ -3,7 +3,9 @@
 import java.util.function.Predicate;
 
 public interface UnitFilter extends Predicate<Unit> {
-    UnitFilter IsTransPort = u -> u.getType().spaceProvided() > 0 && u.getType() != UnitType.Terran_Bunker;
+    UnitFilter IsTransport = u -> u.getType().spaceProvided() > 0 && u.getType() != UnitType.Terran_Bunker;
+    @Deprecated // backwards compatible
+    UnitFilter IsTransPort = IsTransport;
     UnitFilter CanProduce = u -> u.getType().canProduce();
     UnitFilter CanAttack = u -> u.getType().canAttack();
     UnitFilter CanMove = u -> u.getType().canMove();

src/test/java/bwapi/BuildingPlacerTest.java

@@ -0,0 +1,63 @@
+package bwapi;
+
+import org.junit.Test;
+
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class BuildingPlacerTest {
+
+    @Test
+    public void reserveExistingAddonPlacementShouldUseHalfMaxRangeOffset() {
+        Game game = mock(Game.class);
+        Player self = mock(Player.class);
+        when(game.self()).thenReturn(self);
+
+        TilePosition desired = new TilePosition(40, 40);
+        // start should be desired - (64, 64)/2 = (8, 8).
+        // Factory at (40, 40) has its 2x2 addon at world tiles (44, 41)..(45, 42),
+        // which corresponds to reserve coordinates (36, 33)..(37, 34).
+        Unit factory = mock(Unit.class);
+        when(factory.exists()).thenReturn(true);
+        when(factory.getType()).thenReturn(UnitType.Terran_Factory);
+        when(factory.getTilePosition()).thenReturn(new TilePosition(40, 40));
+        when(self.getUnits()).thenReturn(Collections.singletonList(factory));
+
+        BuildingPlacer.PlacementReserve reserve = new BuildingPlacer.PlacementReserve(64);
+        reserve.setRange(0, 0, 64, 64, (byte) 1);
+
+        BuildingPlacer.ReserveExistingAddonPlacement(reserve, desired, game);
+
+        assertEquals(0, reserve.getValue(36, 33));
+        assertEquals(0, reserve.getValue(37, 34));
+    }
+
+    @Test
+    public void reserveDefaultShouldUsePaddingTwoForFactory() {
+        Game game = mock(Game.class);
+        Player self = mock(Player.class);
+        when(game.self()).thenReturn(self);
+
+        TilePosition desired = new TilePosition(40, 40);
+        Unit factory = mock(Unit.class);
+        when(factory.exists()).thenReturn(true);
+        when(factory.getType()).thenReturn(UnitType.Terran_Factory);
+        when(factory.getTilePosition()).thenReturn(new TilePosition(40, 40));
+        when(factory.getPosition()).thenReturn(new TilePosition(40, 40).toPosition());
+        when(self.getUnits()).thenReturn(Collections.singletonList(factory));
+
+        BuildingPlacer.PlacementReserve reserve = new BuildingPlacer.PlacementReserve(64);
+        reserve.setRange(0, 0, 64, 64, (byte) 1);
+
+        BuildingPlacer.ReserveDefault(reserve, UnitType.Terran_Supply_Depot, desired, game);
+
+        // For a 4x3 Factory at (40, 40) (reserve 32, 32) and a 3x2 Supply Depot:
+        // padding=1 reserves x in [25, 35), y in [27, 35)
+        // padding=2 reserves x in [24, 36), y in [26, 36)
+        assertEquals(0, reserve.getValue(24, 26));
+        assertEquals(0, reserve.getValue(35, 35));
+    }
+}

src/test/java/bwapi/FilterTest.java

@@ -39,4 +39,20 @@ public void testFilterCombinations() {
         assertTrue(units.stream().noneMatch(IsMineralField.and(IsRefinery)));
         assertTrue(units.stream().noneMatch(IsMineralField.negate()));
     }
+
+    @Test
+    public void isTransportShouldMatchDropshipAndExcludeBunker() {
+        Unit dropship = mock(Unit.class);
+        when(dropship.getType()).thenReturn(UnitType.Terran_Dropship);
+
+        Unit bunker = mock(Unit.class);
+        when(bunker.getType()).thenReturn(UnitType.Terran_Bunker);
+
+        Unit marine = mock(Unit.class);
+        when(marine.getType()).thenReturn(UnitType.Terran_Marine);
+
+        assertTrue(IsTransport.test(dropship));
+        assertFalse(IsTransport.test(bunker));
+        assertFalse(IsTransport.test(marine));
+    }
 }

src/test/java/bwapi/GameTest.java

@@ -13,10 +13,15 @@
 import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
 
 @RunWith(Theories.class)
 public class GameTest {
@@ -116,4 +121,57 @@ public void hasPowerOnTile() throws IOException {
         // tile (31, 15) is pixel (992, 480), right next to the pylon
         assertTrue(game.hasPower(31, 15));
     }
+
+    @Test
+    public void getUnitsOnTileShouldUseUnitBoundingBoxAndValidateTile() {
+        Game game = spy(new Game());
+        doReturn(128 * 32).when(game).mapPixelWidth();
+        doReturn(128 * 32).when(game).mapPixelHeight();
+
+        // Command Center at top-left tile (10, 10) spans pixels [320..447, 320..415] (tiles 10..13, 10..12)
+        Unit commandCenter = mock(Unit.class);
+        when(commandCenter.getTilePosition()).thenReturn(new TilePosition(10, 10));
+        when(commandCenter.getLeft()).thenReturn(320);
+        when(commandCenter.getTop()).thenReturn(320);
+        when(commandCenter.getRight()).thenReturn(447);
+        when(commandCenter.getBottom()).thenReturn(415);
+
+        game.setAllUnits(Collections.singletonList(commandCenter));
+
+        // Tile (11, 11) is covered by the Command Center even though its top-left tile is (10, 10)
+        List<Unit> onTile = game.getUnitsOnTile(11, 11);
+        assertEquals(Collections.singletonList(commandCenter), onTile);
+
+        // Invalid tile should return empty list
+        assertTrue(game.getUnitsOnTile(-1, -1).isEmpty());
+    }
+
+    @Test
+    public void getUnitsInRadiusShouldUseUnitEdgeDistance() {
+        Game game = new Game();
+
+        // Unit centered at (100, 100) with bounding box [80..120, 80..120]
+        Unit unit = mock(Unit.class);
+        Position unitCenter = new Position(100, 100);
+        Position queryCenter = new Position(150, 100);
+        when(unit.getPosition()).thenReturn(unitCenter);
+        when(unit.getLeft()).thenReturn(80);
+        when(unit.getRight()).thenReturn(120);
+        when(unit.getTop()).thenReturn(80);
+        when(unit.getBottom()).thenReturn(120);
+        when(unit.exists()).thenReturn(true);
+        when(unit.getDistance(queryCenter)).thenCallRealMethod();
+
+        game.setAllUnits(Collections.singletonList(unit));
+
+        // Center-to-center distance is 50 (> 35), but edge-to-point distance is 30 (<= 35)
+        List<Unit> result = game.getUnitsInRadius(queryCenter, 35);
+        assertEquals(Collections.singletonList(unit), result);
+    }
+
+    @Test
+    public void setMapWithEmptyStringShouldReturnFalseInsteadOfThrowing() {
+        Game game = new Game();
+        assertFalse(game.setMap(""));
+    }
 }

src/test/java/bwapi/PlayerTest.java

@@ -2,6 +2,7 @@
 
 import org.junit.Test;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -18,4 +19,13 @@ public void neutralIsNotEnemy() {
 
         assertFalse(self.isEnemy(neutral));
     }
+
+    @Test
+    public void getTextColorShouldReturnDarkGreenForColorGreen() {
+        Player player = mock(Player.class);
+        when(player.getColor()).thenReturn(Color.Green); // Color(117)
+        when(player.getTextColor()).thenCallRealMethod();
+
+        assertEquals(Text.DarkGreen, player.getTextColor());
+    }
 }