Round of fixes for jbwapi (2)

src/main/java/bwapi/BuildingPlacer.java

@@ -260,7 +260,7 @@ static void ReserveExistingAddonPlacement(final PlacementReserve reserve, final
 
     static void ReserveDefault(final PlacementReserve reserve, final UnitType type, final TilePosition desiredPosition, final Game game) {
         reserve.backup();
-        final PlacementReserve original = reserve;
+        final PlacementReserve original = new PlacementReserve(reserve);
 
         // Reserve some space around some specific units
         for (final Unit it : game.self().getUnits()) {
@@ -368,6 +368,15 @@ static class PlacementReserve {
             backup();
         }
 
+        PlacementReserve(final PlacementReserve other) {
+            maxSearch = other.maxSearch;
+            reset();
+            for (int i = 0; i < MAX_RANGE; i++) {
+                System.arraycopy(other.data[i], 0, data[i], 0, MAX_RANGE);
+                System.arraycopy(other.save[i], 0, save[i], 0, MAX_RANGE);
+            }
+        }
+
         // Checks if the given x/y value is valid for the Placement position
         static boolean isValidPos(final int x, final int y) {
             return x >= 0 && x < MAX_RANGE && y >= 0 && y < MAX_RANGE;

src/main/java/bwapi/Bullet.java

@@ -180,11 +180,17 @@ public boolean isVisible() {
      * @param player If this parameter is specified, then the Bullet's visibility to the given player is checked. If this parameter is omitted, then a default value of null is used, which will check if the BWAPI player has vision of the {@link Bullet}.
      * @return true if the {@link Bullet} is visible to the specified player, false if the {@link Bullet} is not visible to the specified player.
      */
-    public boolean isVisible(final Player player) {
-        if (player == null) {
-            return isVisible();
+    public boolean isVisible(Player player) {
+        player = player == null ? game.self() : player;
+        if (player != null) {
+            return player.hasValidId() && bulletData.isVisible(player.getID());
         }
-        return bulletData.isVisible(player.getID());
+        for (int i = 0; i < Player.MAX_VALID_ID; ++i) {
+            if (bulletData.isVisible(i)) {
+                return true;
+            }
+        }
+        return false;
     }
 
     @Override

src/main/java/bwapi/CommandTemp.java

@@ -336,7 +336,7 @@ void execute(boolean isCurrentFrame) {
 
                         // Shift training queue back one slot after the cancelled unit
                         for (int i = command.extra; i < 4; ++i) {
-                            unit.self().trainingQueue[i].set(unit.getTrainingQueue().get(i + 1), frame);
+                            unit.self().trainingQueue[i].set(unit.getTrainingQueueAt(i + 1), frame);
                         }
 
                         unit.self().trainingQueueCount.setOrAdd(-1, frame);
@@ -790,7 +790,7 @@ void execute(boolean isCurrentFrame) {
                 unit.self().remainingUpgradeTime.set(upgradeType.upgradeTime(level + 1), frame);
 
                 player.self().minerals.setOrAdd(-upgradeType.mineralPrice(level + 1), frame);
-                player.self().gas.setOrAdd(upgradeType.gasPrice(level + 1), frame);
+                player.self().gas.setOrAdd(-upgradeType.gasPrice(level + 1), frame);
 
                 player.self().isUpgrading[upgradeType.id].set(true, frame);
             }

src/main/java/bwapi/Player.java

@@ -19,6 +19,8 @@
  * @see Race
  */
 public class Player implements Comparable<Player> {
+    static final int MAX_VALID_ID = 9;
+
     private final PlayerData playerData;
     private final Game game;
     private final int id;
@@ -53,6 +55,10 @@ public int getID() {
         return id;
     }
 
+    boolean hasValidId() {
+        return id >= 0 && id < MAX_VALID_ID;
+    }
+
     /**
      * Retrieves the name of the player.
      *
@@ -141,7 +147,7 @@ public boolean isEnemy(final Player player) {
         if (player == null || isNeutral() || player.isNeutral() || isObserver() || player.isObserver()) {
             return false;
         }
-        return !playerData.isAlly(player.getID());
+        return playerData.isEnemy(player.getID());
     }
 
     /**
@@ -150,7 +156,7 @@ public boolean isEnemy(final Player player) {
      * @return true if this player is the neutral player, false if this player is any other player.
      */
     public boolean isNeutral() {
-        return equals(game.neutral());
+        return playerData.isNeutral();
     }
 
     /**
@@ -339,8 +345,9 @@ public int supplyTotal() {
      * @return The total supply available for this player and the given race.
      * @see #supplyUsed
      */
-    public int supplyTotal(final Race race) {
-        return playerData.getSupplyTotal(race.id);
+    public int supplyTotal(Race race) {
+        race = race == Race.None ? getRace() : race;
+        return race.hasValidId() ? playerData.getSupplyTotal(race.id) : 0;
     }
 
     public int supplyUsed() {
@@ -354,7 +361,11 @@ public int supplyUsed() {
      * @return The supply that is in use for this player and the given race.
      * @see #supplyTotal
      */
-    public int supplyUsed(final Race race) {
+    public int supplyUsed(Race race) {
+        race = race == Race.None ? getRace() : race;
+        if (!race.hasValidId()) {
+            return 0;
+        }
         int supplyUsed = playerData.getSupplyUsed(race.id);
         if (game.isLatComEnabled() && self().supplyUsed[race.id].valid(game.getFrameCount())) {
             return supplyUsed + self().supplyUsed[race.id].get();
@@ -544,6 +555,8 @@ public Color getColor() {
      * @return character code to use for text in Broodwar.
      */
     public Text getTextColor() {
+        // Note: C++ BWAPI 4.4.0 uses Colors::White (255) and Colors::Green (117) due to a bug,
+        // whereas Player 7 white is 84 (DirtyWhite) and Player 9 / Ice green is 185 (DarkGreen).
         switch (getColor().id) {
             case 111: // red
                 return Text.BrightRed;

src/main/java/bwapi/Race.java

@@ -58,12 +58,18 @@ public enum Race {
         Arrays.stream(Race.values()).forEach(v -> idToEnum[v.id] = v);
     }
 
+    static final int MAX_VALID_ID = 3;
+
     public final int id;
 
     Race(final int id) {
         this.id = id;
     }
 
+    boolean hasValidId() {
+        return id >= 0 && id < MAX_VALID_ID;
+    }
+
     /**
      * Retrieves the default worker type for this {@link Race}.
      * <p>

src/main/java/bwapi/Region.java

@@ -64,7 +64,7 @@ void updateNeighbours() {
             final Region region = game.getRegion(regionData.getNeighbors(i));
             neighbours.add(region);
             final int d = getDistance(region);
-            if (region.isAccessible()) {
+            if (this.getRegionGroupID() == region.getRegionGroupID()) {
                 if (d < accessibleBestDist) {
                     closestAccessibleRegion = region;
                     accessibleBestDist = d;

src/main/java/bwapi/Unit.java

@@ -2092,8 +2092,17 @@ public boolean isVisible() {
      * fog of war will be accessible, but isVisible will still return false.
      * @see #exists
      */
-    public boolean isVisible(final Player player) {
-        return unitData.isVisible(player.getID());
+    public boolean isVisible(Player player) {
+        player = player == null ? game.self() : player;
+        if (player != null) {
+            return player.hasValidId() && unitData.isVisible(player.getID());
+        }
+        for (int i = 0; i < Player.MAX_VALID_ID; ++i) {
+            if (unitData.isVisible(i)) {
+                return true;
+            }
+        }
+        return false;
     }
 
     /**
@@ -2107,6 +2116,9 @@ public boolean isTargetable() {
         if (!exists()) {
             return false;
         }
+        if (!isVisible() && !game.isFlagEnabled(Flag.CompleteMapInformation)) {
+            return false;
+        }
         final UnitType ut = getType();
         if (!isCompleted() &&
                 !ut.isBuilding() &&
@@ -2205,7 +2217,7 @@ public boolean attack(final Unit target, final boolean shiftQueueCommand) {
     }
 
     public boolean build(final UnitType type) {
-        return issueCommand(UnitCommand.train(this, type));
+        return build(type, TilePosition.None);
     }
 
     /**
@@ -2225,6 +2237,9 @@ public boolean build(final UnitType type) {
      * @see Unit#canBuild
      */
     public boolean build(final UnitType type, final TilePosition target) {
+        if (TilePosition.None.equals(target)) {
+            return train(type);
+        }
         return issueCommand(UnitCommand.build(this, target, type));
     }
 
@@ -2269,7 +2284,23 @@ public boolean buildAddon(final UnitType type) {
      * @see Unit#isTraining
      * @see Unit#canTrain
      */
-    public boolean train(final UnitType type) {
+    public boolean train() {
+        return train(UnitType.None);
+    }
+
+    public boolean train(UnitType type) {
+        if (type == UnitType.None) {
+            switch (getType()) {
+                case Protoss_Carrier:
+                case Hero_Gantrithor:
+                    type = Protoss_Interceptor;
+                    break;
+                case Protoss_Reaver:
+                case Hero_Warbringer:
+                    type = Protoss_Scarab;
+                    break;
+            }
+        }
         return issueCommand(UnitCommand.train(this, type));
     }
 
@@ -2876,6 +2907,9 @@ public boolean useTech(final TechType tech, final Position target) {
     }
 
     public boolean useTech(final TechType tech, final Unit target) {
+        if (target == null) {
+            return issueCommand(UnitCommand.useTech(this, tech));
+        }
         return issueCommand(UnitCommand.useTech(this, tech, target));
     }
 
@@ -3506,18 +3540,7 @@ public boolean canIssueCommandTypeGrouped(UnitCommandType ct, boolean checkComma
     }
 
     public boolean canTargetUnit(Unit targetUnit) {
-        if (targetUnit == null || !targetUnit.exists()) {
-            return false;
-        }
-        final UnitType targetType = targetUnit.getType();
-        if (!targetUnit.isCompleted() && !targetType.isBuilding() && !targetUnit.isMorphing() &&
-                targetType != Protoss_Archon && targetType != Protoss_Dark_Archon) {
-            return false;
-        }
-        return targetType != Spell_Scanner_Sweep &&
-                targetType != Spell_Dark_Swarm &&
-                targetType != Spell_Disruption_Web &&
-                targetType != Special_Map_Revealer;
+        return canTargetUnit(targetUnit, true);
     }
 
     /**
@@ -3534,7 +3557,7 @@ public boolean canTargetUnit(Unit targetUnit, boolean checkCommandibility) {
         if (checkCommandibility && !canCommand()) {
             return false;
         }
-        return canTargetUnit(targetUnit);
+        return targetUnit != null && targetUnit.isTargetable();
     }
 
     public boolean canAttack() {
@@ -3668,7 +3691,7 @@ public boolean canAttackGrouped(Position target, boolean checkCanTargetUnit, boo
         if (checkCommandibilityGrouped && !canCommandGrouped(false)) {
             return false;
         }
-        return canAttackMoveGrouped(false, false) || canAttackUnitGrouped(false, false);
+        return canAttackMoveGrouped(false, false);
     }
 
     public boolean canAttackGrouped(Unit target, boolean checkCanTargetUnit, boolean checkCanIssueCommandType, boolean checkCommandibilityGrouped, boolean checkCommandibility) {
@@ -4051,7 +4074,7 @@ public boolean canBuildAddon(UnitType uType, boolean checkCanIssueCommandType, b
         if (checkCommandibility && !canCommand()) {
             return false;
         }
-        if (checkCanIssueCommandType && !canBuildAddon(uType, false)) {
+        if (checkCanIssueCommandType && !canBuildAddon(false)) {
             return false;
         }
         if (!game.canMake(uType, this)) {

src/main/java/bwapi/UnitCommand.java

@@ -20,6 +20,7 @@ private UnitCommand(final Unit unit, final UnitCommandType type) {
     }
 
     private <T extends Point<T>> void assignTarget(Point<T> target) {
+        // Note: C++ BWAPI clamps target via target.makeValid() to [0, mapWidth] and [0, mapHeight].
         this.x = target.x;
         this.y = target.y;
     }
@@ -401,6 +402,7 @@ public boolean isQueued() {
                 type == Follow ||
                 type == Gather ||
                 type == Return_Cargo ||
+                type == Repair ||
                 type == Load ||
                 type == Unload_All ||
                 type == Unload_All_Position ||
@@ -409,6 +411,7 @@ public boolean isQueued() {
                 && extra != 0;
     }
 
+    // Note: JBWAPI includes `unit` in equals and hashCode, C++ BWAPI does not.
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;

src/test/java/bwapi/BulletTest.java

@@ -0,0 +1,32 @@
+package bwapi;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class BulletTest {
+
+    @Test
+    public void isVisibleShouldFallbackToAnyPlayerWhenSelfIsNullAndCheckValidPlayerId() {
+        Game game = mock(Game.class);
+        when(game.self()).thenReturn(null);
+
+        ClientData.BulletData bulletData = mock(ClientData.BulletData.class);
+        Bullet bullet = new Bullet(bulletData, 0, game);
+
+        assertFalse(bullet.isVisible());
+
+        when(bulletData.isVisible(2)).thenReturn(true);
+        assertTrue(bullet.isVisible());
+        assertTrue(bullet.isVisible(null));
+
+        ClientData.PlayerData neutralData = mock(ClientData.PlayerData.class);
+        when(neutralData.getName()).thenReturn("Neutral");
+        Player neutral = new Player(neutralData, 11, game);
+        when(bulletData.isVisible(11)).thenReturn(true);
+        assertFalse(bullet.isVisible(neutral));
+    }
+}