From 591b68d7be1582794c741705360b31be80a856ba Mon Sep 17 00:00:00 2001 From: asvitkine Date: Sun, 27 Aug 2023 17:09:04 -0400 Subject: [PATCH] Speed up canal validation code. (#11915) No functional changes. This was showing up in profiles. --- .../triplea/attachments/CanalAttachment.java | 53 +++++++++---------- .../move/validation/MoveValidator.java | 40 +++++++------- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java index 2b9ca38ad5e..d0a8443eb0e 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java @@ -12,7 +12,9 @@ import games.strategy.triplea.Constants; import games.strategy.triplea.delegate.Matches; import java.util.HashSet; +import java.util.List; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import javax.annotation.Nullable; import org.triplea.java.collections.CollectionUtils; @@ -34,23 +36,34 @@ public CanalAttachment(final String name, final Attachable attachable, final Gam super(name, attachable, gameData); } + private static boolean hasCanal(final Territory t, final String canalName) { + return !get(t, canalAttachment -> canalAttachment.getCanalName().equals(canalName)).isEmpty(); + } + + public static List get(final Territory t, final Route onRoute) { + return get(t, attachment -> isCanalOnRoute(attachment.getCanalName(), onRoute)); + } + + private static List get(final Territory t, Predicate cond) { + return t.getAttachments().values().stream() + .filter(attachment -> attachment.getName().startsWith(Constants.CANAL_ATTACHMENT_PREFIX)) + .map(CanalAttachment.class::cast) + .filter(cond) + .collect(Collectors.toList()); + } + + static CanalAttachment get(final Territory t, final String nameOfAttachment) { + return getAttachment(t, nameOfAttachment, CanalAttachment.class); + } + /** * Checks if the route contains both territories to pass through the given canal. If route is null * returns true. */ - public static boolean isCanalOnRoute(final String canalName, final Route route) { - if (route == null) { - return true; - } + private static boolean isCanalOnRoute(final String canalName, final Route route) { boolean previousTerritoryHasCanal = false; for (final Territory t : route) { - boolean currentTerritoryHasCanal = false; - for (final CanalAttachment canalAttachment : get(t)) { - if (canalAttachment.getCanalName().equals(canalName)) { - currentTerritoryHasCanal = true; - break; - } - } + boolean currentTerritoryHasCanal = hasCanal(t, canalName); if (previousTerritoryHasCanal && currentTerritoryHasCanal) { return true; } @@ -59,17 +72,6 @@ public static boolean isCanalOnRoute(final String canalName, final Route route) return false; } - public static Set get(final Territory t) { - return t.getAttachments().values().stream() - .filter(attachment -> attachment.getName().startsWith(Constants.CANAL_ATTACHMENT_PREFIX)) - .map(CanalAttachment.class::cast) - .collect(Collectors.toSet()); - } - - static CanalAttachment get(final Territory t, final String nameOfAttachment) { - return getAttachment(t, nameOfAttachment, CanalAttachment.class); - } - private void setCanalName(final String name) { canalName = name.intern(); } @@ -162,11 +164,8 @@ public void validate(final GameState data) throws GameParseException { } final Set territories = new HashSet<>(); for (final Territory t : data.getMap()) { - final Set canalAttachments = get(t); - for (final CanalAttachment canalAttachment : canalAttachments) { - if (canalAttachment.getCanalName().equals(canalName)) { - territories.add(t); - } + if (hasCanal(t, canalName)) { + territories.add(t); } } if (territories.size() != 2) { diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java index fc11c5a4103..78af3dfaac8 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java @@ -193,10 +193,9 @@ private MoveValidationResult validateFirst( .negate()); if (matches.isEmpty() || !matches.stream().allMatch(Matches.unitIsOwnedBy(player))) { result.setError( - "Player, " - + player.getName() - + ", is not owner of all the units: " - + MyFormatter.unitsToTextNoOwner(units)); + String.format( + "Player, %s, is not owner of all the units: %s", + player.getName(), MyFormatter.unitsToTextNoOwner(units))); return result; } } @@ -216,9 +215,8 @@ private MoveValidationResult validateFirst( final Collection landOnRoute = route.getMatches(Matches.territoryIsLand()); if (!landOnRoute.isEmpty()) { // TODO: if this ever changes, we need to also update getBestRoute(), because getBestRoute is - // also checking to - // make sure we avoid land territories owned by nations with these 2 relationship type - // attachment options + // also checking to make sure we avoid land territories owned by nations with these 2 + // relationship type attachment options for (final Territory t : landOnRoute) { if (units.stream().anyMatch(Matches.unitIsLand()) && !data.getRelationshipTracker().canMoveLandUnitsOverOwnedLand(player, t.getOwner())) { @@ -291,7 +289,7 @@ private MoveValidationResult validateCanal( */ public @Nullable String validateCanal( final Route route, @Nullable final Collection units, final GamePlayer player) { - return validateCanal(route, units, new HashMap<>(), player); + return validateCanal(route, units, Map.of(), player); } @Nullable @@ -300,6 +298,20 @@ String validateCanal( @Nullable final Collection units, final Map> airTransportDependents, final GamePlayer player) { + Map> territoryCanals = new HashMap<>(); + int numCanals = 0; + for (Territory t : route.getAllTerritories()) { + // Only check canals that are on the route + var canals = CanalAttachment.get(t, route); + territoryCanals.put(t, canals); + numCanals += canals.size(); + } + if (numCanals == 0) { + return null; + } + final boolean mustControlAllCanals = + Properties.getControlAllCanalsBetweenTerritoriesToPass(data.getProperties()); + // Check each unit 1 by 1 to see if they can move through necessary canals on route String result = null; final Set unitsThatFailCanal = new HashSet<>(); @@ -310,14 +322,9 @@ String validateCanal( for (final Unit unit : unitsWithoutDependents) { for (final Territory t : route.getAllTerritories()) { Optional failureMessage = Optional.empty(); - for (final CanalAttachment canalAttachment : CanalAttachment.get(t)) { - if (!CanalAttachment.isCanalOnRoute(canalAttachment.getCanalName(), route)) { - continue; // Only check canals that are on the route - } + for (CanalAttachment canalAttachment : territoryCanals.get(t)) { failureMessage = canPassThroughCanal(canalAttachment, unit, player); final boolean canPass = failureMessage.isEmpty(); - final boolean mustControlAllCanals = - Properties.getControlAllCanalsBetweenTerritoriesToPass(data.getProperties()); if (mustControlAllCanals != canPass) { // If need to control any canal and can pass OR need to control all and can't pass. break; @@ -365,10 +372,7 @@ public boolean canAnyUnitsPassCanal( final GamePlayer player) { boolean canPass = true; final Route route = new Route(start, end); - for (final CanalAttachment canalAttachment : CanalAttachment.get(start)) { - if (!CanalAttachment.isCanalOnRoute(canalAttachment.getCanalName(), route)) { - continue; // Only check canals that are on the route - } + for (final CanalAttachment canalAttachment : CanalAttachment.get(start, route)) { final Collection unitsWithoutDependents = findNonDependentUnits(units, route, Map.of()); canPass = canAnyPassThroughCanal(canalAttachment, unitsWithoutDependents, player).isEmpty(); final boolean mustControlAllCanals =