AI: fetch the land colour it is actually short of

forge-ai/src/main/java/forge/ai/AiController.java

@@ -30,7 +30,6 @@
 import forge.card.CardType;
 import forge.card.ColorSet;
 import forge.card.MagicColor;
-import forge.card.mana.ManaAtom;
 import forge.card.mana.ManaCost;
 import forge.deck.Deck;
 import forge.deck.DeckSection;
@@ -618,23 +617,6 @@ private Card chooseBestLandToPlay(CardCollection landList) {
         final Set<String> basics = Sets.newHashSet();
 
         // what colors are available?
-        int[] counts = new int[6]; // in WUBRGC order
-
-        for (Card c : player.getCardsIn(ZoneType.Battlefield)) {
-            for (SpellAbility m: c.getManaAbilities()) {
-                m.setActivatingPlayer(c.getController());
-                for (AbilityManaPart mp : m.getAllManaParts()) {
-                    for (String part : mp.mana(m).split(" ")) {
-                        // TODO handle any
-                        int index = ManaAtom.getIndexFromName(part);
-                        if (index != -1) {
-                            counts[index] += 1;
-                        }
-                    }
-                }
-            }
-        }
-
         // what types can I go get?
         for (final String name : MagicColor.Constant.BASIC_LANDS) {
             if (landList.stream().anyMatch(c -> c.getType().hasSubtype(name)) &&
@@ -657,27 +639,9 @@ private Card chooseBestLandToPlay(CardCollection landList) {
                     }
 
                     // TODO handle fetchlands and what they can fetch for
-                    // determine new color pips
-                    int[] card_counts = new int[6]; // in WUBRGC order
-                    for (SpellAbility m: card.getManaAbilities()) {
-                        m.setActivatingPlayer(card.getController());
-                        for (AbilityManaPart mp : m.getAllManaParts()) {
-                            for (String part : mp.mana(m).split(" ")) {
-                                // TODO handle any
-                                int index = ManaAtom.getIndexFromName(part);
-                                if (index != -1) {
-                                    card_counts[index] += 1;
-                                }
-                            }
-                        }
-                    }
-
-                    // use 1 / x+1 for diminishing returns
-                    // TODO use max pips of each color in the deck from deck statistics to weight this
-                    for (int i = 0; i < card_counts.length; i++) {
-                        int diff = (card_counts[i] * 50) / (counts[i] + 1);
-                        score += diff;
-                    }
+                    // depth in colours we are thin on, with diminishing returns, plus what this
+                    // land would unblock outright - the two say different things, so they add
+                    score += ComputerUtilCard.getColorFixingValue(player, card);
 
                     // TODO utility lands only if we have enough to pay their costs
                     // TODO Tron lands and other lands that care about land counts

forge-ai/src/main/java/forge/ai/ComputerUtilCard.java

@@ -1,6 +1,7 @@
 package forge.ai;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.IdentityHashMap;
@@ -30,6 +31,7 @@
 import forge.card.MagicColor;
 import forge.card.MagicColor.Constant;
 import forge.card.mana.ManaCost;
+import forge.card.mana.ManaCostShard;
 import forge.deck.CardPool;
 import forge.deck.Deck;
 import forge.deck.DeckSection;
@@ -211,6 +213,91 @@ public static Card getBestEnchantmentAI(final List<Card> list, final SpellAbilit
         return cardStream.max(Comparator.comparing(Card::getCMC)).orElse(null);
     }
 
+    /** Score per source fixed, on the scale AiController's land scoring already uses. */
+    public static final int COLOR_FIXING_WEIGHT = 50;
+
+    /** How many missing sources these extra ones supply across the player's hand and board. */
+    private static int countSourcesFixed(final Player benefits, final int[] have, final int[] with) {
+        if (Arrays.equals(have, with)) {
+            return 0;
+        }
+
+        int fixed = 0;
+        for (Card c : benefits.getCardsIn(ZoneType.Hand)) {
+            fixed += countSourcesFixed(c.getManaCost(), have, with);
+        }
+        for (Card c : benefits.getCardsIn(ZoneType.Battlefield)) {
+            // only what can be activated from here, not the permanent's own casting cost
+            for (SpellAbility ab : c.getNonManaAbilities()) {
+                if (!ab.isActivatedAbility() || ab.getPayCosts() == null
+                        || ab.getPayCosts().getCostMana() == null) {
+                    continue;
+                }
+                fixed += countSourcesFixed(ab.getPayCosts().getCostMana().getMana(), have, with);
+            }
+        }
+        return fixed;
+    }
+
+    /** What this land is worth to the player's mana: what it makes payable, plus spare depth. */
+    public static int getColorFixingValue(final Player benefits, final Card candidate) {
+        if (benefits == null || candidate == null) {
+            return 0;
+        }
+        // one pass over the battlefield; the candidate only ever adds to what is already there
+        final int[] have = ComputerUtilCost.getManaSourceCounts(benefits);
+        final int[] with = have.clone();
+        ComputerUtilCost.addManaSources(candidate, with);
+        return COLOR_FIXING_WEIGHT * countSourcesFixed(benefits, have, with) + evaluateSpareSources(have, with);
+    }
+
+    /** Value of a spare source of a color nothing needs yet, falling off as sources accumulate. */
+    private static int evaluateSpareSources(final int[] have, final int[] with) {
+        int value = 0;
+        for (int i = 0; i < have.length; i++) {
+            if (with[i] > have[i]) {
+                // 1/(x+1) for diminishing returns, matching the land scoring this replaces
+                value += ((with[i] - have[i]) * COLOR_FIXING_WEIGHT) / (have[i] + 1);
+            }
+        }
+        return value;
+    }
+
+    /** How many more colored sources this cost still wants; zero once it is castable. */
+    private static int countMissingSources(final ManaCost cost, final int[] counts) {
+        if (cost == null || cost.isNoCost()) {
+            return 0;
+        }
+        byte mask = 0;
+        for (MagicColor.Color color : MagicColor.Color.values()) {
+            if (counts[color.ordinal()] > 0) {
+                mask |= color.getColorMask();
+            }
+        }
+        int[] need = new int[counts.length];
+        int missing = 0;
+        for (ManaCostShard shard : cost) {
+            if (shard.isPhyrexian()) {
+                continue;
+            }
+            if (shard.isMonoColor() && !shard.isOr2Generic()) {
+                // a plain colored pip wants a source of its own
+                need[MagicColor.Color.fromByte(shard.getColorMask()).ordinal()]++;
+            } else if (!shard.canBePaidWithManaOfColor(mask)) {
+                missing++;                   // hybrid and generic keep the looser check
+            }
+        }
+        for (int i = 0; i < need.length; i++) {
+            missing += Math.max(0, need[i] - counts[i]);
+        }
+        return missing;
+    }
+
+    /** How many of one cost's missing sources these extra ones supply, never negative. */
+    private static int countSourcesFixed(final ManaCost cost, final int[] have, final int[] with) {
+        return Math.max(0, countMissingSources(cost, have) - countMissingSources(cost, with));
+    }
+
     /**
      * <p>
      * getBestLandAI.
@@ -220,6 +307,10 @@ public static Card getBestEnchantmentAI(final List<Card> list, final SpellAbilit
      * @return a {@link forge.game.card.Card} object.
      */
     public static Card getBestLandAI(final Iterable<Card> list) {
+        return getBestLandAI(null, list);
+    }
+
+    public static Card getBestLandAI(final Player benefits, final Iterable<Card> list) {
         final List<Card> land = CardLists.filter(list, CardPredicates.LANDS);
         if (land.isEmpty()) {
             return null;
@@ -249,7 +340,9 @@ public static Card getBestLandAI(final Iterable<Card> list) {
                 }
             }
 
-            return Aggregates.random(nbLand);
+            // a colour we are short of outranks raw land value; ties fall through to it
+            return Aggregates.itemWithMax(nbLand,
+                    c -> getColorFixingValue(benefits, c) + landEvaluator.apply(c));
         }
 
         // if no non-basic lands, target the least represented basic land type

forge-ai/src/main/java/forge/ai/ComputerUtilCost.java

@@ -5,6 +5,7 @@
 import java.util.Set;
 import java.util.function.Predicate;
 
+import forge.card.MagicColor;
 import forge.game.GameObject;
 import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -647,16 +648,41 @@ public static Set<String> getAvailableManaColors(Player ai, List<Card> additiona
         }
 
         for (Card c : cardsToConsider) {
-            for (SpellAbility sa : c.getManaAbilities()) {
-                if (sa.getManaPart() != null) {
-                    colorsAvailable.add(sa.getManaPart().getOrigProduced());
-                }
+            // the raw Produced$ is a script string, and every caller runs this through
+            // ColorSet.fromNames, which drops anything that is not a colour name - so an "Any"
+            // source used to contribute nothing at all
+            colorsAvailable.addAll(c.getProducibleColors());
+            if (colorsAvailable.size() == MagicColor.Constant.COLORS_AND_COLORLESS.size()) {
+                break; // nothing left for a further source to add
             }
         }
 
         return colorsAvailable;
     }
 
+    /**
+     * How many sources of each color the player's board could produce, indexed by
+     * {@link MagicColor.Color#ordinal()}. Counted per card rather than per ability, since a land
+     * with two mana abilities still only taps once.
+     */
+    public static int[] getManaSourceCounts(Player ai) {
+        int[] counts = new int[MagicColor.Color.values().length];
+        for (Card c : ai.getCardsIn(ZoneType.Battlefield)) {
+            addManaSources(c, counts);
+        }
+        return counts;
+    }
+
+    /** Adds what one card could produce to counts from {@link #getManaSourceCounts}. */
+    public static void addManaSources(Card c, int[] counts) {
+        final Set<String> producible = c.getProducibleColors();
+        for (MagicColor.Color color : MagicColor.Color.values()) {
+            if (producible.contains(color.getName())) {
+                counts[color.ordinal()] += 1;
+            }
+        }
+    }
+
     public static boolean isFreeCastAllowedByPermanent(Player player, String altCost) {
         Game game = player.getGame();
         for (Card cardInPlay : game.getCardsIn(ZoneType.Battlefield)) {

forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java

@@ -537,8 +537,31 @@ private static AiAbilityDecision hiddenTriggerAI(final Player ai, final SpellAbi
      *            a List<Card> object.
      * @return a {@link forge.game.card.Card} object.
      */
-    private static Card basicManaFixing(final Player ai, List<Card> list) { // Search for a Basic Land
-        final CardCollectionView combined = CardCollection.combine(ai.getCardsIn(ZoneType.Battlefield), ai.getCardsIn(ZoneType.Hand));
+    private static Card basicManaFixing(final Player decider, final Player owner, List<Card> list) { // Search for a Basic Land
+        // the land ends up with its owner, so the colours that matter are theirs, not the
+        // chooser's. No card in the pool currently chooses for another player here, so rather
+        // than guess at that case it is left on the existing behaviour untouched.
+        if (!decider.isOpponentOf(owner)) {
+            // narrow to the lands worth the most, then let the basic-type spread and the
+            // dual-land preference below break the tie as they already did
+            final List<Card> best = new ArrayList<>();
+            int most = 0; // only worth narrowing for lands that improve the mana at all
+            for (Card c : list) {
+                final int value = ComputerUtilCard.getColorFixingValue(owner, c);
+                if (value > most) {
+                    most = value;
+                    best.clear();
+                }
+                if (value == most && most > 0) {
+                    best.add(c);
+                }
+            }
+            if (!best.isEmpty()) {
+                list = best;
+            }
+        }
+
+        final CardCollectionView combined = CardCollection.combine(owner.getCardsIn(ZoneType.Battlefield), owner.getCardsIn(ZoneType.Hand));
         final List<String> basics = new ArrayList<>();
 
         // what types can I go get?
@@ -1572,7 +1595,7 @@ public static Card chooseCardToHiddenOriginChangeZone(ZoneType destination, List
         } else if (origin.contains(ZoneType.Library) && (type.contains("Basic") || areAllBasics(type))) {
             if (keycardFound != null) return keycardFound;
 
-            c = basicManaFixing(decider, fetchList);
+            c = basicManaFixing(decider, player, fetchList);
         } else if (ZoneType.Hand.equals(destination) && CardLists.getNotType(fetchList, "Creature").isEmpty()) {
             if (keycardFound != null) return keycardFound;
 
@@ -1600,12 +1623,12 @@ public static Card chooseCardToHiddenOriginChangeZone(ZoneType destination, List
             CardCollectionView hand = decider.getCardsIn(ZoneType.Hand);
             if (!hand.anyMatch(CardPredicates.LANDS) && CardLists.count(decider.getCardsIn(ZoneType.Battlefield), CardPredicates.LANDS) < 4 &&
                     !hand.anyMatch(crd -> ComputerUtilMana.hasEnoughManaSourcesToCast(crd.getFirstSpellAbility(), decider))) {
-                c = basicManaFixing(decider, fetchList);
+                c = basicManaFixing(decider, player, fetchList);
             }
             if (c == null) {
                 if (fetchList.allMatch(CardPredicates.LANDS)) {
                     // we're only choosing from lands, so get the best land
-                    c = ComputerUtilCard.getBestLandAI(fetchList);
+                    c = ComputerUtilCard.getBestLandAI(player, fetchList);
                 } else {
                     fetchList = CardLists.getNotType(fetchList, "Land");
                     // Prefer to pull a creature, generally more useful for AI.
@@ -2090,7 +2113,7 @@ private static Card considerRamp(Player ai, SpellAbility sa, CardCollection choi
 
         // If we are below the threshold, look for a land in the available choices and prefer it
         if (totalManaSources < threshold) {
-            Card manaFixing = basicManaFixing(ai, choices);
+            Card manaFixing = basicManaFixing(ai, ai, choices);
             if (manaFixing != null) {
                 return manaFixing;
             }

forge-ai/src/main/java/forge/ai/ability/ControlGainAi.java

@@ -211,7 +211,7 @@ protected AiAbilityDecision canPlay(final Player ai, final SpellAbility sa) {
                 } else if (artifacts > 0) {
                     t = ComputerUtilCard.getBestArtifactAI(list);
                 } else if (lands > 0) {
-                    t = ComputerUtilCard.getBestLandAI(list);
+                    t = ComputerUtilCard.getBestLandAI(ai, list);
                 } else if (enchantments > 0) {
                     t = ComputerUtilCard.getBestEnchantmentAI(list, sa, false);
                 } else {

forge-game/src/main/java/forge/game/card/Card.java

@@ -3351,19 +3351,32 @@ public final boolean canProduceColorMana(final Set<String> colors) {
         return false;
     }
 
-    public final boolean canProduceSameManaTypeWith(final Card c) {
-        if (getManaAbilities().isEmpty()) {
-            return false;
-        }
+    /** Every color this card could produce, walking its mana abilities once. */
+    public final Set<String> getProducibleColors() {
         Set<String> colors = new HashSet<>();
-        for (final SpellAbility ab : c.getManaAbilities()) {
+        for (final SpellAbility ab : getManaAbilities()) {
+            // without an activating player canProduce falls into a much more expensive path, so
+            // fill it in - but only when missing, so a payment simulation keeps the payer it set
+            if (ab.getActivatingPlayer() == null) {
+                ab.setActivatingPlayer(getController());
+            }
             if (ab.getApi() == ApiType.ManaReflected) {
                 colors.addAll(CardUtil.getReflectableManaColors(ab));
             } else {
                 colors = CardUtil.canProduce(6, ab, colors);
             }
+            if (colors.size() == MagicColor.Constant.COLORS_AND_COLORLESS.size()) {
+                break; // nothing left for a further ability to add
+            }
+        }
+        return colors;
+    }
+
+    public final boolean canProduceSameManaTypeWith(final Card c) {
+        if (getManaAbilities().isEmpty()) {
+            return false;
         }
-        return canProduceColorMana(colors);
+        return canProduceColorMana(c.getProducibleColors());
     }
 
     public final int getMaxManaProduced() {