From 5e7a88b6039820c859b52bff8b6410da517b0447 Mon Sep 17 00:00:00 2001 From: liamiak Date: Mon, 3 Aug 2026 19:43:22 -0600 Subject: [PATCH 1/8] AI: fetch the land colour it is actually short of Land searches picked by list order. basicManaFixing chose the basic type the player had fewest of, then took list.get(0) from whatever survived that filter, and getBestLandAI ended in Aggregates.random. Neither asked which colours were actually blocking anything. Every fetchland comes through here - areAllBasics("Plains,Island") is true - and a "Plains" search matches every dual carrying the Plains type. Measured over 12 seeded AI-vs-AI games (deck 260613, three seeds), basicManaFixing fires 46 times, 44 of them with a real choice, once over 31 candidates. One observed decision offered Tundra, Underground Sea, Volcanic Island, Tropical Island, Raffine's Tower and Ketria Triome among others - all carrying Island, all different beside it - and it took Tundra because Tundra was first. getColorFixingNeed counts how many cards go from unpayable to payable if that player had this land, across their hand and the activatable abilities on their permanents. It reuses ComputerUtilCost.getAvailableManaColors, which already takes an "if I also had this land" argument, and canBePaidWithAvailable. Ordering turned out to matter more than the metric. Colour need is asked before the basic-type count, because that count cannot tell a colour that is missing from one that is merely uncommon: with three Islands and a hand wanting black it concluded it needed Plains, having none, and fetched a Plains-Island. Asking first also means the whole candidate list is still in front of it rather than the remains of a filter. Where a measure cannot separate the candidates it returns null and the caller keeps what it was already doing, so nothing decides while blind - evaluateLand is never asked to rank a utility land against a basic, and the existing fallbacks stand. Of the 44 real decisions, colour need has signal in 33 and changes the pick in 25. Also fixes what the comment above the old call site suspected: basicManaFixing read the decider's board while searching someone else's library. It now works from the owner's side, and inverts every layer when an opponent is the one choosing. That inversion is covered by a unit test but never ran in the measured games - deck 260613 has no Chooser$ cards - so it is the least exercised part of this. Co-Authored-By: Claude Opus 5 --- .../main/java/forge/ai/ComputerUtilCard.java | 86 ++++++++++++++++++- .../java/forge/ai/ability/ChangeZoneAi.java | 28 ++++-- .../java/forge/ai/ability/ControlGainAi.java | 2 +- 3 files changed, 106 insertions(+), 10 deletions(-) diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java index 5545667c3884..53ec28d533db 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java +++ b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java @@ -211,6 +211,81 @@ public static Card getBestEnchantmentAI(final List list, final SpellAbilit return cardStream.max(Comparator.comparing(Card::getCMC)).orElse(null); } + /** + * How many cards a player cannot currently pay for, in hand or as an ability on their + * permanents, would become payable if they also had this land. + */ + public static int getColorFixingNeed(final Player benefits, final Card candidate) { + if (benefits == null || candidate == null) { + return 0; + } + final byte have = ColorSet.fromNames(ComputerUtilCost.getAvailableManaColors(benefits, (List) null)).getColor(); + final byte with = ColorSet.fromNames(ComputerUtilCost.getAvailableManaColors(benefits, candidate)).getColor(); + if (have == with) { + return 0; + } + + int unblocked = 0; + for (Card c : benefits.getCardsIn(ZoneType.Hand)) { + if (unblocksWith(c.getManaCost(), have, with)) { + unblocked++; + } + } + for (Card c : benefits.getCardsIn(ZoneType.Battlefield)) { + for (SpellAbility ab : c.getAllSpellAbilities()) { + if (ab.isManaAbility() || ab.getPayCosts() == null || ab.getPayCosts().getCostMana() == null) { + continue; + } + if (unblocksWith(ab.getPayCosts().getCostMana().getMana(), have, with)) { + unblocked++; + } + } + } + return unblocked; + } + + private static boolean unblocksWith(final ManaCost cost, final byte have, final byte with) { + return cost != null && !cost.isNoCost() && !cost.canBePaidWithAvailable(have) + && cost.canBePaidWithAvailable(with); + } + + /** + * The candidate that best covers colors the player receiving the land is short of, or null when + * none of them stands out - so this never decides while it is blind, and the caller keeps + * whatever it was already doing. A player choosing for an opponent wants the least helpful one. + */ + public static Card getBestLandToGainAI(final Player benefits, final List candidates, final boolean hostile) { + return pickStandout(candidates, c -> getColorFixingNeed(benefits, c), hostile); + } + + /** the single best candidate by this measure, or null if the measure cannot separate them */ + private static Card pickStandout(final List candidates, final Function score, + final boolean invert) { + if (candidates.isEmpty()) { + return null; + } + final List best = bestBy(candidates, score, invert); + return best.size() == candidates.size() ? null : best.get(0); + } + + /** every candidate tied for the best score, or the whole list when the measure sees no difference */ + private static List bestBy(final List candidates, final Function score, + final boolean invert) { + List best = Lists.newArrayList(); + int bestScore = Integer.MIN_VALUE; + for (Card c : candidates) { + int s = invert ? -score.apply(c) : score.apply(c); + if (s > bestScore) { + bestScore = s; + best.clear(); + best.add(c); + } else if (s == bestScore) { + best.add(c); + } + } + return best; + } + /** *

* getBestLandAI. @@ -220,6 +295,10 @@ public static Card getBestEnchantmentAI(final List list, final SpellAbilit * @return a {@link forge.game.card.Card} object. */ public static Card getBestLandAI(final Iterable list) { + return getBestLandAI(null, list); + } + + public static Card getBestLandAI(final Player benefits, final Iterable list) { final List land = CardLists.filter(list, CardPredicates.LANDS); if (land.isEmpty()) { return null; @@ -249,7 +328,12 @@ public static Card getBestLandAI(final Iterable list) { } } - return Aggregates.random(nbLand); + // a colour we are short of first, then raw land value, and only then give up and guess + Card ranked = getBestLandToGainAI(benefits, nbLand, false); + if (ranked == null) { + ranked = pickStandout(nbLand, landEvaluator::apply, false); + } + return ranked != null ? ranked : Aggregates.random(nbLand); } // if no non-basic lands, target the least represented basic land type diff --git a/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java b/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java index c5e628117815..33025620eaec 100644 --- a/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java +++ b/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java @@ -537,8 +537,20 @@ private static AiAbilityDecision hiddenTriggerAI(final Player ai, final SpellAbi * a List object. * @return a {@link forge.game.card.Card} object. */ - private static Card basicManaFixing(final Player ai, List 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 list) { // Search for a Basic Land + // the land ends up with its owner, so the choice is made from their side of the table - and + // an opponent doing the choosing wants the least helpful land instead + final boolean hostile = decider.isOpponentOf(owner); + + // a colour they are actually waiting on beats evening out their basic types, so ask that + // first - the type count below cannot tell a colour that is missing from one that is merely + // uncommon, and several lands of the right type carry different colours beside it + final Card needed = ComputerUtilCard.getBestLandToGainAI(owner, list, hostile); + if (needed != null) { + return needed; + } + + final CardCollectionView combined = CardCollection.combine(owner.getCardsIn(ZoneType.Battlefield), owner.getCardsIn(ZoneType.Hand)); final List basics = new ArrayList<>(); // what types can I go get? @@ -550,12 +562,12 @@ private static Card basicManaFixing(final Player ai, List list) { // Searc // Which basic land is least available from hand and play, that I still // have in my deck - int minSize = Integer.MAX_VALUE; + int minSize = hostile ? Integer.MIN_VALUE : Integer.MAX_VALUE; String minType = null; for (String b : basics) { final int num = CardLists.getType(combined, b).size(); - if (num < minSize) { + if (hostile ? num > minSize : num < minSize) { minType = b; minSize = num; } @@ -1572,7 +1584,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 +1612,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 +2102,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; } diff --git a/forge-ai/src/main/java/forge/ai/ability/ControlGainAi.java b/forge-ai/src/main/java/forge/ai/ability/ControlGainAi.java index 822e10c8ca35..1c29cabe3bcc 100644 --- a/forge-ai/src/main/java/forge/ai/ability/ControlGainAi.java +++ b/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 { From 7b98fd3c5d6c3d7e58df0111c3d1c1fffe1969e9 Mon Sep 17 00:00:00 2001 From: liamiak Date: Mon, 3 Aug 2026 19:43:22 -0600 Subject: [PATCH 2/8] Add regression tests for colour-aware land searching Covers the decision directly and through a real Flooded Strand activation, which is how it is reached in a game: two duals both carrying the searched-for type, only one of which unblocks the hand. Also pins the two ways it declines to act - an opponent choosing gives the least useful land, and identical candidates leave the caller's own ordering alone. The fetchland case fails without the fix. Drop this commit if you would rather not carry the tests. Co-Authored-By: Claude Opus 5 --- .../forge/ai/ability/LandColorNeedAiTest.java | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java diff --git a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java new file mode 100644 index 000000000000..42c02c51c85f --- /dev/null +++ b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java @@ -0,0 +1,136 @@ +package forge.ai.ability; + +import java.util.List; + +import org.testng.annotations.Test; + +import com.google.common.collect.Lists; + +import forge.ai.AITest; +import forge.ai.ComputerUtilCard; +import forge.game.Game; +import forge.game.card.Card; +import forge.game.player.Player; +import forge.game.zone.ZoneType; + +import static org.testng.AssertJUnit.assertEquals; +import static org.testng.AssertJUnit.assertNull; + +/** + * Land searches used to pick by list order, so a fetchland that had settled on the right colour + * still chose its second colour arbitrarily. The candidates are ranked by the colours the player + * receiving the land is actually short of. + */ +public class LandColorNeedAiTest extends AITest { + + /** two duals sharing the searched-for type, differing in the colour beside it */ + private List twoDuals(Player owner) { + return Lists.newArrayList( + addCardToZone("Tundra", owner, ZoneType.Library), // Plains Island + addCardToZone("Underground Sea", owner, ZoneType.Library) // Island Swamp + ); + } + + @Test + public void picksTheColorTheHandIsWaitingOn() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + addCards("Island", 3, ai); + // castable the moment we can make black, and not before + addCardToZone("Sign in Blood", ai, ZoneType.Hand); + addCardToZone("Ravenous Chupacabra", ai, ZoneType.Hand); + game.getAction().checkStateEffects(true); + + Card chosen = ComputerUtilCard.getBestLandToGainAI(ai, twoDuals(ai), false); + assertEquals("Underground Sea", chosen.getName()); + } + + @Test + public void picksTheOtherColorWhenTheHandChanges() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + addCards("Island", 3, ai); + addCardToZone("Wrath of God", ai, ZoneType.Hand); + addCardToZone("Swords to Plowshares", ai, ZoneType.Hand); + game.getAction().checkStateEffects(true); + + Card chosen = ComputerUtilCard.getBestLandToGainAI(ai, twoDuals(ai), false); + assertEquals("Tundra", chosen.getName()); + } + + @Test + public void anOpponentChoosingGivesTheLeastUsefulLand() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + addCards("Island", 3, ai); + addCardToZone("Sign in Blood", ai, ZoneType.Hand); + addCardToZone("Ravenous Chupacabra", ai, ZoneType.Hand); + game.getAction().checkStateEffects(true); + + Card chosen = ComputerUtilCard.getBestLandToGainAI(ai, twoDuals(ai), true); + assertEquals("Tundra", chosen.getName()); + } + + /** with nothing to separate them the caller keeps whatever it was already doing */ + @Test + public void staysOutOfTheWayWhenItCannotTell() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + addCards("Island", 3, ai); + game.getAction().checkStateEffects(true); + + List identical = Lists.newArrayList( + addCardToZone("Tundra", ai, ZoneType.Library), + addCardToZone("Tundra", ai, ZoneType.Library)); + assertNull(ComputerUtilCard.getBestLandToGainAI(ai, identical, false)); + } + + /** + * The same decision through a real fetchland, which is how this is reached in a game: + * Flooded Strand searches for a Plains or an Island, and every dual carrying one of those + * types is a legal target. + */ + @Test + public void aFetchlandBringsBackTheColorWeAreShortOf() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + Card strand = addCard("Flooded Strand", ai); + strand.setSickness(false); + addCards("Island", 3, ai); + // only castable once we can make black + addCardToZone("Sign in Blood", ai, ZoneType.Hand); + addCardToZone("Ravenous Chupacabra", ai, ZoneType.Hand); + // both carry Island, so both are legal fetches; only one gives us black + addCardToZone("Tundra", ai, ZoneType.Library); + addCardToZone("Underground Sea", ai, ZoneType.Library); + fillLibrary(ai, 10); + game.getAction().checkStateEffects(true); + + moveToMain2(game, ai); + playUntilStackClear(game); + + assertEquals("fetched the land that unblocks our hand", + 1, countCardsWithName(game, "Underground Sea")); + assertEquals(0, countCardsWithName(game, "Tundra")); + } + + /** callers with no player to ask still get land value instead of a coin flip */ + @Test + public void fallsBackToLandValueWithNoPlayer() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + List mixed = Lists.newArrayList( + addCardToZone("Tundra", ai, ZoneType.Library), + addCardToZone("Raffine's Tower", ai, ZoneType.Library)); // triome, produces more colours + + assertNull("nothing to say without a player to ask", + ComputerUtilCard.getBestLandToGainAI(null, mixed, false)); + assertEquals("Raffine's Tower", ComputerUtilCard.getBestLandAI(null, mixed).getName()); + } +} From ade9089d944bd6bc3d0b8469aebdc472ae92c9f2 Mon Sep 17 00:00:00 2001 From: liamiak Date: Tue, 4 Aug 2026 21:08:38 -0600 Subject: [PATCH 3/8] Consolidate land colour choice around one shared measure Reworked after review. The colour logic now lives in one place and every caller ranks by the same number, rather than the fetch path carrying a second implementation beside the one in chooseBestLandToPlay. ComputerUtilCost.getManaSourceCounts is the shared primitive: how many sources of each colour a player could produce, optionally counting one more card. It uses canProduce rather than reading the produced-mana string, so "any colour" and choice-of-colour lands are counted - Mana Confluence previously scored below a basic Plains. ComputerUtilCard.getColorFixingValue is the single number lands are ranked by, combining what the land lets us pay for with the depth it adds in colours we are thin on. chooseBestLandToPlay, basicManaFixing and getBestLandAI all use it, so its two hand-rolled colour scans are gone (+3/-39 there). Demand is counted in pips, not whole cards: a colour mask cannot tell one source of a colour from two, so it thought BB was payable off a single Swamp. Counting the shortfall also credits a first source for the progress it makes rather than only the source that completes a cost. Two things the mask version got wrong, both now covered by tests: counts come from what the board can produce rather than what is untapped, so holding a land for main 2 does not change the answer; and only activated abilities on permanents count, since getAllSpellAbilities also returns a permanent's own casting cost and the far face of an MDFC or Adventure. Measured on a 40-permanent board: getColorFixingValue is 86us, so a land drop with eight candidates costs 0.69ms once per turn, and the shared scan is cheaper than the getAvailableManaColors call it partly replaces. 12-game seeded mirror sim finished 6-6 with no exceptions. Co-Authored-By: Claude Opus 5 --- .../src/main/java/forge/ai/AiController.java | 42 +----- .../main/java/forge/ai/ComputerUtilCard.java | 142 +++++++++++------- .../main/java/forge/ai/ComputerUtilCost.java | 48 +++++- .../java/forge/ai/ability/ChangeZoneAi.java | 28 ++-- .../forge/ai/ability/LandColorNeedAiTest.java | 114 ++++++++------ 5 files changed, 224 insertions(+), 150 deletions(-) diff --git a/forge-ai/src/main/java/forge/ai/AiController.java b/forge-ai/src/main/java/forge/ai/AiController.java index 9b888cbbee43..406f9ba0a129 100644 --- a/forge-ai/src/main/java/forge/ai/AiController.java +++ b/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 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 diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java index 53ec28d533db..e218bcbfacd9 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java +++ b/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; @@ -29,7 +30,9 @@ import forge.card.ColorSet; import forge.card.MagicColor; import forge.card.MagicColor.Constant; +import forge.card.mana.ManaAtom; import forge.card.mana.ManaCost; +import forge.card.mana.ManaCostShard; import forge.deck.CardPool; import forge.deck.Deck; import forge.deck.DeckSection; @@ -212,78 +215,108 @@ public static Card getBestEnchantmentAI(final List list, final SpellAbilit } /** - * How many cards a player cannot currently pay for, in hand or as an ability on their - * permanents, would become payable if they also had this land. + * Score per pip of progress, and per first source of a color. Matches the scale the land + * scoring in AiController already uses, so {@link #getColorFixingValue} can be added to it. */ - public static int getColorFixingNeed(final Player benefits, final Card candidate) { - if (benefits == null || candidate == null) { - return 0; - } - final byte have = ColorSet.fromNames(ComputerUtilCost.getAvailableManaColors(benefits, (List) null)).getColor(); - final byte with = ColorSet.fromNames(ComputerUtilCost.getAvailableManaColors(benefits, candidate)).getColor(); - if (have == with) { + public static final int COLOR_FIXING_WEIGHT = 50; + + /** + * How much closer these extra sources get the player to paying for what they are holding, + * counted in pips across their hand and the abilities on their permanents. Counting pips + * rather than whole cards means a second source of a color is credited for the double-pip + * costs a single source cannot pay, and a first source is credited for the progress it makes. + */ + private static int fixingNeed(final Player benefits, final int[] have, final int[] with) { + if (Arrays.equals(have, with)) { return 0; } - int unblocked = 0; + int progress = 0; for (Card c : benefits.getCardsIn(ZoneType.Hand)) { - if (unblocksWith(c.getManaCost(), have, with)) { - unblocked++; - } + progress += progressOn(c.getManaCost(), have, with); } for (Card c : benefits.getCardsIn(ZoneType.Battlefield)) { - for (SpellAbility ab : c.getAllSpellAbilities()) { - if (ab.isManaAbility() || ab.getPayCosts() == null || ab.getPayCosts().getCostMana() == null) { + // only what can actually be activated from here: getAllSpellAbilities would also hand + // back the permanent's own casting cost, and the far face of an MDFC or Adventure + for (SpellAbility ab : c.getNonManaAbilities()) { + if (!ab.isActivatedAbility() || ab.getPayCosts() == null + || ab.getPayCosts().getCostMana() == null) { continue; } - if (unblocksWith(ab.getPayCosts().getCostMana().getMana(), have, with)) { - unblocked++; - } + progress += progressOn(ab.getPayCosts().getCostMana().getMana(), have, with); } } - return unblocked; - } - - private static boolean unblocksWith(final ManaCost cost, final byte have, final byte with) { - return cost != null && !cost.isNoCost() && !cost.canBePaidWithAvailable(have) - && cost.canBePaidWithAvailable(with); + return progress; } /** - * The candidate that best covers colors the player receiving the land is short of, or null when - * none of them stands out - so this never decides while it is blind, and the caller keeps - * whatever it was already doing. A player choosing for an opponent wants the least helpful one. + * What this land is worth to the player's mana overall: what it makes payable, plus the depth + * it adds in colors they are thin on. The single number every caller ranks lands by. */ - public static Card getBestLandToGainAI(final Player benefits, final List candidates, final boolean hostile) { - return pickStandout(candidates, c -> getColorFixingNeed(benefits, c), hostile); + public static int getColorFixingValue(final Player benefits, final Card candidate) { + if (benefits == null || candidate == null) { + return 0; + } + // counted once and shared: both halves ask the same two questions of the board + final int[] have = ComputerUtilCost.getManaSourceCounts(benefits, null); + final int[] with = ComputerUtilCost.getManaSourceCounts(benefits, candidate); + return COLOR_FIXING_WEIGHT * fixingNeed(benefits, have, with) + depthValue(have, with); } - /** the single best candidate by this measure, or null if the measure cannot separate them */ - private static Card pickStandout(final List candidates, final Function score, - final boolean invert) { - if (candidates.isEmpty()) { - return null; + /** + * Depth this land adds beyond anything it unblocks outright. A second source of a color is + * worth having even when nothing needs it yet - it is what lets two spells of that color be + * cast in a turn - so this falls off as sources accumulate. + */ + private static int depthValue(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); + } } - final List best = bestBy(candidates, score, invert); - return best.size() == candidates.size() ? null : best.get(0); + return value; } - /** every candidate tied for the best score, or the whole list when the measure sees no difference */ - private static List bestBy(final List candidates, final Function score, - final boolean invert) { - List best = Lists.newArrayList(); - int bestScore = Integer.MIN_VALUE; - for (Card c : candidates) { - int s = invert ? -score.apply(c) : score.apply(c); - if (s > bestScore) { - bestScore = s; - best.clear(); - best.add(c); - } else if (s == bestScore) { - best.add(c); + /** + * How many more sources of a color this cost still wants. Zero once it is castable; two for a + * {@code BB} cost off a board with no black, so that a first black source is credited with the + * progress it makes rather than only the source that finally completes it. + */ + private static int shortfall(final ManaCost cost, final int[] counts) { + if (cost == null || cost.isNoCost()) { + return 0; + } + byte mask = 0; + for (int i = 0; i < counts.length; i++) { + if (counts[i] > 0) { + mask |= ManaAtom.MANATYPES[i]; } } - return best; + int[] need = new int[counts.length]; + int missing = 0; + for (ManaCostShard shard : cost) { + if (shard.isPhyrexian()) { + continue; + } + int index = shard.isMonoColor() && !shard.isOr2Generic() + ? ManaAtom.getIndexFromName(MagicColor.toShortString(shard.getColorMask())) : -1; + if (index != -1) { + need[index]++; // a plain colored pip wants a source of its own + } 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; + } + + /** pips of progress this land makes towards a cost, never negative */ + private static int progressOn(final ManaCost cost, final int[] have, final int[] with) { + return Math.max(0, shortfall(cost, have) - shortfall(cost, with)); } /** @@ -328,12 +361,9 @@ public static Card getBestLandAI(final Player benefits, final Iterable lis } } - // a colour we are short of first, then raw land value, and only then give up and guess - Card ranked = getBestLandToGainAI(benefits, nbLand, false); - if (ranked == null) { - ranked = pickStandout(nbLand, landEvaluator::apply, false); - } - return ranked != null ? ranked : 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 diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java index 392f42d03b6e..085a99078880 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java +++ b/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java @@ -639,7 +639,53 @@ public static Set getAvailableManaColors(Player ai, Card additionalLand) return getAvailableManaColors(ai, Lists.newArrayList(additionalLand)); } public static Set getAvailableManaColors(Player ai, List additionalLands) { - CardCollection cardsToConsider = CardLists.filter(ai.getCardsIn(ZoneType.Battlefield), CardPredicates.UNTAPPED); + return manaColors(ai, additionalLands, true); + } + + /** + * Which colors the board could produce given time, ignoring what happens to be tapped right + * now. That is what choosing a land cares about, where {@link #getAvailableManaColors} answers + * the different question of what can be paid for this turn. + */ + public static Set getProducibleManaColors(Player ai, Card additionalLand) { + return manaColors(ai, additionalLand == null ? null : Lists.newArrayList(additionalLand), false); + } + + /** WUBRGC, matching the index order callers use for these counts */ + private static final String[] MANA_SHORT_NAMES = { "W", "U", "B", "R", "G", "C" }; + + /** + * How many sources of each color the player could produce, in WUBRGC order, optionally counting + * one extra card. Unlike {@link #getProducibleManaColors} this keeps the count rather than the + * set, which is what tells a second source of a color from a first. + */ + public static int[] getManaSourceCounts(Player ai, Card additional) { + CardCollection cardsToConsider = new CardCollection(ai.getCardsIn(ZoneType.Battlefield)); + if (additional != null) { + cardsToConsider.add(additional); + } + + int[] counts = new int[MANA_SHORT_NAMES.length]; + for (Card c : cardsToConsider) { + for (SpellAbility m : c.getManaAbilities()) { + m.setActivatingPlayer(c.getController()); + for (int i = 0; i < MANA_SHORT_NAMES.length; i++) { + // canProduce rather than reading the produced string: it is what understands + // "any colour" and a choice of colours, which Command Tower and the like need + if (m.canProduce(MANA_SHORT_NAMES[i])) { + counts[i] += 1; + } + } + } + } + return counts; + } + + private static Set manaColors(Player ai, List additionalLands, boolean untappedOnly) { + CardCollection cardsToConsider = new CardCollection(ai.getCardsIn(ZoneType.Battlefield)); + if (untappedOnly) { + cardsToConsider = CardLists.filter(cardsToConsider, CardPredicates.UNTAPPED); + } Set colorsAvailable = Sets.newHashSet(); if (additionalLands != null) { diff --git a/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java b/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java index 33025620eaec..4d299d078aa9 100644 --- a/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java +++ b/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java @@ -538,16 +538,20 @@ private static AiAbilityDecision hiddenTriggerAI(final Player ai, final SpellAbi * @return a {@link forge.game.card.Card} object. */ private static Card basicManaFixing(final Player decider, final Player owner, List list) { // Search for a Basic Land - // the land ends up with its owner, so the choice is made from their side of the table - and - // an opponent doing the choosing wants the least helpful land instead - final boolean hostile = decider.isOpponentOf(owner); - - // a colour they are actually waiting on beats evening out their basic types, so ask that - // first - the type count below cannot tell a colour that is missing from one that is merely - // uncommon, and several lands of the right type carry different colours beside it - final Card needed = ComputerUtilCard.getBestLandToGainAI(owner, list, hostile); - if (needed != null) { - return needed; + // 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)) { + int most = 0; // only worth narrowing for lands that improve the mana at all + for (Card c : list) { + most = Math.max(most, ComputerUtilCard.getColorFixingValue(owner, c)); + } + // 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 + if (most > 0) { + final int best = most; + list = CardLists.filter(list, c -> ComputerUtilCard.getColorFixingValue(owner, c) == best); + } } final CardCollectionView combined = CardCollection.combine(owner.getCardsIn(ZoneType.Battlefield), owner.getCardsIn(ZoneType.Hand)); @@ -562,12 +566,12 @@ private static Card basicManaFixing(final Player decider, final Player owner, Li // Which basic land is least available from hand and play, that I still // have in my deck - int minSize = hostile ? Integer.MIN_VALUE : Integer.MAX_VALUE; + int minSize = Integer.MAX_VALUE; String minType = null; for (String b : basics) { final int num = CardLists.getType(combined, b).size(); - if (hostile ? num > minSize : num < minSize) { + if (num < minSize) { minType = b; minSize = num; } diff --git a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java index 42c02c51c85f..f5e78b266d21 100644 --- a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java +++ b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java @@ -1,7 +1,5 @@ package forge.ai.ability; -import java.util.List; - import org.testng.annotations.Test; import com.google.common.collect.Lists; @@ -14,25 +12,25 @@ import forge.game.zone.ZoneType; import static org.testng.AssertJUnit.assertEquals; -import static org.testng.AssertJUnit.assertNull; +import static org.testng.AssertJUnit.assertTrue; /** * Land searches used to pick by list order, so a fetchland that had settled on the right colour - * still chose its second colour arbitrarily. The candidates are ranked by the colours the player - * receiving the land is actually short of. + * still chose its second colour arbitrarily. Both the play and the search path now rank candidates + * by what the colours they add would let us pay for, plus depth in the colours we are thin on. */ public class LandColorNeedAiTest extends AITest { /** two duals sharing the searched-for type, differing in the colour beside it */ - private List twoDuals(Player owner) { - return Lists.newArrayList( - addCardToZone("Tundra", owner, ZoneType.Library), // Plains Island - addCardToZone("Underground Sea", owner, ZoneType.Library) // Island Swamp - ); + private Card[] twoDuals(Player owner) { + return new Card[] { + addCardToZone("Tundra", owner, ZoneType.Library), // Plains Island + addCardToZone("Underground Sea", owner, ZoneType.Library) // Island Swamp + }; } @Test - public void picksTheColorTheHandIsWaitingOn() { + public void scoresTheColorTheHandIsWaitingOn() { Game game = initAndCreateGame(); Player ai = game.getPlayers().get(1); @@ -42,12 +40,24 @@ public void picksTheColorTheHandIsWaitingOn() { addCardToZone("Ravenous Chupacabra", ai, ZoneType.Hand); game.getAction().checkStateEffects(true); - Card chosen = ComputerUtilCard.getBestLandToGainAI(ai, twoDuals(ai), false); - assertEquals("Underground Sea", chosen.getName()); + Card[] duals = twoDuals(ai); + int white = ComputerUtilCard.getColorFixingValue(ai, duals[0]); + int black = ComputerUtilCard.getColorFixingValue(ai, duals[1]); + assertTrue("black unblocks the hand, white does not", black > white); + + // whether our sources happen to be tapped says nothing about which colours the board can + // make, so holding the land drop until main 2 must not change the answer + for (Card c : ai.getCardsIn(ZoneType.Battlefield)) { + c.setTapped(true); + } + game.getAction().checkStateEffects(true); + assertEquals("tapping out must not change the measure", + white, ComputerUtilCard.getColorFixingValue(ai, duals[0])); + assertEquals(black, ComputerUtilCard.getColorFixingValue(ai, duals[1])); } @Test - public void picksTheOtherColorWhenTheHandChanges() { + public void scoresTheOtherColorWhenTheHandChanges() { Game game = initAndCreateGame(); Player ai = game.getPlayers().get(1); @@ -56,37 +66,72 @@ public void picksTheOtherColorWhenTheHandChanges() { addCardToZone("Swords to Plowshares", ai, ZoneType.Hand); game.getAction().checkStateEffects(true); - Card chosen = ComputerUtilCard.getBestLandToGainAI(ai, twoDuals(ai), false); - assertEquals("Tundra", chosen.getName()); + Card[] duals = twoDuals(ai); + assertTrue("now it is white we are waiting on", + ComputerUtilCard.getColorFixingValue(ai, duals[0]) + > ComputerUtilCard.getColorFixingValue(ai, duals[1])); } + /** a permanent has already been paid for, so it must not make its own colours look needed */ @Test - public void anOpponentChoosingGivesTheLeastUsefulLand() { + public void aResolvedPermanentDoesNotLookLikeDemand() { Game game = initAndCreateGame(); Player ai = game.getPlayers().get(1); addCards("Island", 3, ai); - addCardToZone("Sign in Blood", ai, ZoneType.Hand); - addCardToZone("Ravenous Chupacabra", ai, ZoneType.Hand); + // already resolved, so neither its casting cost nor its Adventure half is waiting on a + // colour - a permanent must not make its own colours look needed + addCard("Bonecrusher Giant", ai); game.getAction().checkStateEffects(true); - Card chosen = ComputerUtilCard.getBestLandToGainAI(ai, twoDuals(ai), true); - assertEquals("Tundra", chosen.getName()); + // nothing is in hand, so a Mountain is worth depth on one new colour and nothing else. + // If the Giant's casting cost or its Adventure half counted, this would be far higher. + assertEquals("only depth on the new colour, no demand from the Giant", + ComputerUtilCard.COLOR_FIXING_WEIGHT, + ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Mountain", ai, ZoneType.Library))); + + // and with no player to ask, ranking falls through to land value rather than a coin flip + assertEquals("Raffine's Tower", ComputerUtilCard.getBestLandAI(null, Lists.newArrayList( + addCardToZone("Tundra", ai, ZoneType.Library), + addCardToZone("Raffine's Tower", ai, ZoneType.Library))).getName()); } - /** with nothing to separate them the caller keeps whatever it was already doing */ + /** + * A colour mask cannot tell one source of a colour from two, so it thinks {@code BB} is + * payable off a single Swamp. Counting pips is what makes the second source worth having. + */ @Test - public void staysOutOfTheWayWhenItCannotTell() { + public void aSecondSourceCountsForDoublePips() { Game game = initAndCreateGame(); Player ai = game.getPlayers().get(1); addCards("Island", 3, ai); + addCards("Swamp", 1, ai); // one black source only + addCardToZone("Sign in Blood", ai, ZoneType.Hand); // BB, so it wants a second game.getAction().checkStateEffects(true); - List identical = Lists.newArrayList( - addCardToZone("Tundra", ai, ZoneType.Library), - addCardToZone("Tundra", ai, ZoneType.Library)); - assertNull(ComputerUtilCard.getBestLandToGainAI(ai, identical, false)); + assertTrue("a second black source is progress towards BB, a fourth blue source is not", + ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Swamp", ai, ZoneType.Library)) + > ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Island", ai, ZoneType.Library))); + } + + /** with nothing waiting on a colour, depth still prefers the colours we are thin on */ + @Test + public void depthPrefersTheColorWeAreThinnestOn() { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + + addCards("Island", 3, ai); + addCards("Plains", 1, ai); + game.getAction().checkStateEffects(true); // empty hand: nothing to unblock at all + + int firstSwamp = ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Swamp", ai, ZoneType.Library)); + int secondPlains = ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Plains", ai, ZoneType.Library)); + int fourthIsland = ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Island", ai, ZoneType.Library)); + + assertTrue("a colour we have none of beats a second source", firstSwamp > secondPlains); + assertTrue("and a second source still beats a fourth", secondPlains > fourthIsland); + assertTrue("but a fourth is not worthless", fourthIsland > 0); } /** @@ -118,19 +163,4 @@ public void aFetchlandBringsBackTheColorWeAreShortOf() { 1, countCardsWithName(game, "Underground Sea")); assertEquals(0, countCardsWithName(game, "Tundra")); } - - /** callers with no player to ask still get land value instead of a coin flip */ - @Test - public void fallsBackToLandValueWithNoPlayer() { - Game game = initAndCreateGame(); - Player ai = game.getPlayers().get(1); - - List mixed = Lists.newArrayList( - addCardToZone("Tundra", ai, ZoneType.Library), - addCardToZone("Raffine's Tower", ai, ZoneType.Library)); // triome, produces more colours - - assertNull("nothing to say without a player to ask", - ComputerUtilCard.getBestLandToGainAI(null, mixed, false)); - assertEquals("Raffine's Tower", ComputerUtilCard.getBestLandAI(null, mixed).getName()); - } } From 3d9dba9e7b078a8464580ae10a6aed321e330f18 Mon Sep 17 00:00:00 2001 From: liamiak Date: Wed, 5 Aug 2026 17:55:32 -0600 Subject: [PATCH 4/8] Simplify the colour measure per review Five things from the review, all in one pass since they turned out to share a cause. The hand written {"W","U","B","R","G","C"} is gone; MagicColor.Color.values() is already exactly that, in that order, and its ordinal indexes the counts. shortfall now uses the same enum rather than ManaAtom, so there is one source of truth for the ordering instead of two that happened to agree. getProducibleManaColors is deleted. It had no callers left once the counts replaced it, and reading getOrigProduced was the less accurate check anyway - it is what missed "any colour" lands. getAvailableManaColors goes back to exactly what it was. Colour collection now goes through Card.getProducibleColors, extracted from canProduceSameManaTypeWith, which already walked the mana abilities this way using CardUtil.canProduce and handled ManaReflected. Both callers share it. getColorFixingValue makes one pass over the battlefield instead of two: the candidate can only add to what is already there, so the second set of counts is a clone plus that one card. basicManaFixing scores each candidate once and keeps the best as it goes, rather than finding the maximum and then filtering by recomputing it. One thing worth recording: getProducibleColors sets the activating player on each mana ability first, as getMaxManaProduced already does. Without it every canProduce falls into a far more expensive path - measured on a 40 permanent board, the scan is 30us with it and 1191us without. Same board as before, so getColorFixingValue stays around 86us and a land drop with eight candidates under a millisecond. 357 tests, 0 failures. Co-Authored-By: Claude Opus 5 --- .../main/java/forge/ai/ComputerUtilCard.java | 21 +++--- .../main/java/forge/ai/ComputerUtilCost.java | 72 +++++++------------ .../java/forge/ai/ability/ChangeZoneAi.java | 19 +++-- .../src/main/java/forge/game/card/Card.java | 20 ++++-- 4 files changed, 62 insertions(+), 70 deletions(-) diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java index e218bcbfacd9..134b82fb3f09 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java +++ b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java @@ -30,7 +30,6 @@ import forge.card.ColorSet; import forge.card.MagicColor; import forge.card.MagicColor.Constant; -import forge.card.mana.ManaAtom; import forge.card.mana.ManaCost; import forge.card.mana.ManaCostShard; import forge.deck.CardPool; @@ -257,9 +256,10 @@ public static int getColorFixingValue(final Player benefits, final Card candidat if (benefits == null || candidate == null) { return 0; } - // counted once and shared: both halves ask the same two questions of the board - final int[] have = ComputerUtilCost.getManaSourceCounts(benefits, null); - final int[] with = ComputerUtilCost.getManaSourceCounts(benefits, candidate); + // 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 * fixingNeed(benefits, have, with) + depthValue(have, with); } @@ -289,9 +289,9 @@ private static int shortfall(final ManaCost cost, final int[] counts) { return 0; } byte mask = 0; - for (int i = 0; i < counts.length; i++) { - if (counts[i] > 0) { - mask |= ManaAtom.MANATYPES[i]; + for (MagicColor.Color color : MagicColor.Color.values()) { + if (counts[color.ordinal()] > 0) { + mask |= color.getColorMask(); } } int[] need = new int[counts.length]; @@ -300,10 +300,9 @@ private static int shortfall(final ManaCost cost, final int[] counts) { if (shard.isPhyrexian()) { continue; } - int index = shard.isMonoColor() && !shard.isOr2Generic() - ? ManaAtom.getIndexFromName(MagicColor.toShortString(shard.getColorMask())) : -1; - if (index != -1) { - need[index]++; // a plain colored pip wants a source of its own + 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 } diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java index 085a99078880..e7bcc52195d1 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java +++ b/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; @@ -639,53 +640,7 @@ public static Set getAvailableManaColors(Player ai, Card additionalLand) return getAvailableManaColors(ai, Lists.newArrayList(additionalLand)); } public static Set getAvailableManaColors(Player ai, List additionalLands) { - return manaColors(ai, additionalLands, true); - } - - /** - * Which colors the board could produce given time, ignoring what happens to be tapped right - * now. That is what choosing a land cares about, where {@link #getAvailableManaColors} answers - * the different question of what can be paid for this turn. - */ - public static Set getProducibleManaColors(Player ai, Card additionalLand) { - return manaColors(ai, additionalLand == null ? null : Lists.newArrayList(additionalLand), false); - } - - /** WUBRGC, matching the index order callers use for these counts */ - private static final String[] MANA_SHORT_NAMES = { "W", "U", "B", "R", "G", "C" }; - - /** - * How many sources of each color the player could produce, in WUBRGC order, optionally counting - * one extra card. Unlike {@link #getProducibleManaColors} this keeps the count rather than the - * set, which is what tells a second source of a color from a first. - */ - public static int[] getManaSourceCounts(Player ai, Card additional) { - CardCollection cardsToConsider = new CardCollection(ai.getCardsIn(ZoneType.Battlefield)); - if (additional != null) { - cardsToConsider.add(additional); - } - - int[] counts = new int[MANA_SHORT_NAMES.length]; - for (Card c : cardsToConsider) { - for (SpellAbility m : c.getManaAbilities()) { - m.setActivatingPlayer(c.getController()); - for (int i = 0; i < MANA_SHORT_NAMES.length; i++) { - // canProduce rather than reading the produced string: it is what understands - // "any colour" and a choice of colours, which Command Tower and the like need - if (m.canProduce(MANA_SHORT_NAMES[i])) { - counts[i] += 1; - } - } - } - } - return counts; - } - - private static Set manaColors(Player ai, List additionalLands, boolean untappedOnly) { - CardCollection cardsToConsider = new CardCollection(ai.getCardsIn(ZoneType.Battlefield)); - if (untappedOnly) { - cardsToConsider = CardLists.filter(cardsToConsider, CardPredicates.UNTAPPED); - } + CardCollection cardsToConsider = CardLists.filter(ai.getCardsIn(ZoneType.Battlefield), CardPredicates.UNTAPPED); Set colorsAvailable = Sets.newHashSet(); if (additionalLands != null) { @@ -703,6 +658,29 @@ private static Set manaColors(Player ai, List additionalLands, boo 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 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)) { diff --git a/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java b/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java index 4d299d078aa9..ba740605db99 100644 --- a/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java +++ b/forge-ai/src/main/java/forge/ai/ability/ChangeZoneAi.java @@ -542,15 +542,22 @@ private static Card basicManaFixing(final Player decider, final Player owner, Li // 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 best = new ArrayList<>(); int most = 0; // only worth narrowing for lands that improve the mana at all for (Card c : list) { - most = Math.max(most, ComputerUtilCard.getColorFixingValue(owner, c)); + final int value = ComputerUtilCard.getColorFixingValue(owner, c); + if (value > most) { + most = value; + best.clear(); + } + if (value == most && most > 0) { + best.add(c); + } } - // 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 - if (most > 0) { - final int best = most; - list = CardLists.filter(list, c -> ComputerUtilCard.getColorFixingValue(owner, c) == best); + if (!best.isEmpty()) { + list = best; } } diff --git a/forge-game/src/main/java/forge/game/card/Card.java b/forge-game/src/main/java/forge/game/card/Card.java index 5249b18cd2aa..0097faf8d2fb 100644 --- a/forge-game/src/main/java/forge/game/card/Card.java +++ b/forge-game/src/main/java/forge/game/card/Card.java @@ -3351,19 +3351,27 @@ public final boolean canProduceColorMana(final Set 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 getProducibleColors() { Set colors = new HashSet<>(); - for (final SpellAbility ab : c.getManaAbilities()) { + for (final SpellAbility ab : getManaAbilities()) { + // as getMaxManaProduced does: without an activating player each canProduce falls into + // a much more expensive path, and this is hot enough for that to matter + ab.setActivatingPlayer(getController()); if (ab.getApi() == ApiType.ManaReflected) { colors.addAll(CardUtil.getReflectableManaColors(ab)); } else { colors = CardUtil.canProduce(6, ab, colors); } } - return canProduceColorMana(colors); + return colors; + } + + public final boolean canProduceSameManaTypeWith(final Card c) { + if (getManaAbilities().isEmpty()) { + return false; + } + return canProduceColorMana(c.getProducibleColors()); } public final int getMaxManaProduced() { From 0dfc4f18c2ef456c99e3abe9516d58553333347c Mon Sep 17 00:00:00 2001 From: liamiak Date: Wed, 5 Aug 2026 18:11:57 -0600 Subject: [PATCH 5/8] Only fill in the activating player when it is missing ComputerUtilMana sets a payer on mana abilities during payment simulation, and that payer is not always the card's controller. getProducibleColors was overwriting it unconditionally, so a call landing mid-simulation could clobber the state that simulation was relying on. Filling it in only when null keeps the cheap path for cold abilities without touching a payment in progress. Faster too, since abilities keep whatever was already set: getColorFixingValue is 58us on the same 40 permanent board, against 86us before this review. Co-Authored-By: Claude Opus 5 --- forge-game/src/main/java/forge/game/card/Card.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/forge-game/src/main/java/forge/game/card/Card.java b/forge-game/src/main/java/forge/game/card/Card.java index 0097faf8d2fb..1eaf080446ca 100644 --- a/forge-game/src/main/java/forge/game/card/Card.java +++ b/forge-game/src/main/java/forge/game/card/Card.java @@ -3355,9 +3355,12 @@ public final boolean canProduceColorMana(final Set colors) { public final Set getProducibleColors() { Set colors = new HashSet<>(); for (final SpellAbility ab : getManaAbilities()) { - // as getMaxManaProduced does: without an activating player each canProduce falls into - // a much more expensive path, and this is hot enough for that to matter - ab.setActivatingPlayer(getController()); + // Without an activating player each canProduce falls into a much more expensive path, + // and this is hot enough for that to matter. Only fill it in when it is missing, so a + // payment simulation that has already set a payer keeps it. + if (ab.getActivatingPlayer() == null) { + ab.setActivatingPlayer(getController()); + } if (ab.getApi() == ApiType.ManaReflected) { colors.addAll(CardUtil.getReflectableManaColors(ab)); } else { From ddbc0564511c2ac1613be72d8eebb5c0d6d79fea Mon Sep 17 00:00:00 2001 From: liamiak Date: Thu, 6 Aug 2026 20:31:18 -0600 Subject: [PATCH 6/8] Name the colour-fixing helpers for what they count countMissingSources / countSourcesFixed / evaluateSpareSources, so the family reads off getColorFixingValue. Stop getProducibleColors once every colour is present, and trim the commentary back towards the rate the rest of forge-ai runs at. Co-Authored-By: Claude Opus 5 --- .../main/java/forge/ai/ComputerUtilCard.java | 54 ++++++------------- .../src/main/java/forge/game/card/Card.java | 8 +-- 2 files changed, 22 insertions(+), 40 deletions(-) diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java index 134b82fb3f09..8391f16b9097 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java +++ b/forge-ai/src/main/java/forge/ai/ComputerUtilCard.java @@ -213,45 +213,33 @@ public static Card getBestEnchantmentAI(final List list, final SpellAbilit return cardStream.max(Comparator.comparing(Card::getCMC)).orElse(null); } - /** - * Score per pip of progress, and per first source of a color. Matches the scale the land - * scoring in AiController already uses, so {@link #getColorFixingValue} can be added to it. - */ + /** Score per source fixed, on the scale AiController's land scoring already uses. */ public static final int COLOR_FIXING_WEIGHT = 50; - /** - * How much closer these extra sources get the player to paying for what they are holding, - * counted in pips across their hand and the abilities on their permanents. Counting pips - * rather than whole cards means a second source of a color is credited for the double-pip - * costs a single source cannot pay, and a first source is credited for the progress it makes. - */ - private static int fixingNeed(final Player benefits, final int[] have, final int[] with) { + /** 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 progress = 0; + int fixed = 0; for (Card c : benefits.getCardsIn(ZoneType.Hand)) { - progress += progressOn(c.getManaCost(), have, with); + fixed += countSourcesFixed(c.getManaCost(), have, with); } for (Card c : benefits.getCardsIn(ZoneType.Battlefield)) { - // only what can actually be activated from here: getAllSpellAbilities would also hand - // back the permanent's own casting cost, and the far face of an MDFC or Adventure + // 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; } - progress += progressOn(ab.getPayCosts().getCostMana().getMana(), have, with); + fixed += countSourcesFixed(ab.getPayCosts().getCostMana().getMana(), have, with); } } - return progress; + return fixed; } - /** - * What this land is worth to the player's mana overall: what it makes payable, plus the depth - * it adds in colors they are thin on. The single number every caller ranks lands by. - */ + /** 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; @@ -260,15 +248,11 @@ public static int getColorFixingValue(final Player benefits, final Card candidat final int[] have = ComputerUtilCost.getManaSourceCounts(benefits); final int[] with = have.clone(); ComputerUtilCost.addManaSources(candidate, with); - return COLOR_FIXING_WEIGHT * fixingNeed(benefits, have, with) + depthValue(have, with); + return COLOR_FIXING_WEIGHT * countSourcesFixed(benefits, have, with) + evaluateSpareSources(have, with); } - /** - * Depth this land adds beyond anything it unblocks outright. A second source of a color is - * worth having even when nothing needs it yet - it is what lets two spells of that color be - * cast in a turn - so this falls off as sources accumulate. - */ - private static int depthValue(final int[] have, final int[] 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]) { @@ -279,12 +263,8 @@ private static int depthValue(final int[] have, final int[] with) { return value; } - /** - * How many more sources of a color this cost still wants. Zero once it is castable; two for a - * {@code BB} cost off a board with no black, so that a first black source is credited with the - * progress it makes rather than only the source that finally completes it. - */ - private static int shortfall(final ManaCost cost, final int[] counts) { + /** 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; } @@ -313,9 +293,9 @@ private static int shortfall(final ManaCost cost, final int[] counts) { return missing; } - /** pips of progress this land makes towards a cost, never negative */ - private static int progressOn(final ManaCost cost, final int[] have, final int[] with) { - return Math.max(0, shortfall(cost, have) - shortfall(cost, with)); + /** 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)); } /** diff --git a/forge-game/src/main/java/forge/game/card/Card.java b/forge-game/src/main/java/forge/game/card/Card.java index 1eaf080446ca..5298ce007e7c 100644 --- a/forge-game/src/main/java/forge/game/card/Card.java +++ b/forge-game/src/main/java/forge/game/card/Card.java @@ -3355,9 +3355,8 @@ public final boolean canProduceColorMana(final Set colors) { public final Set getProducibleColors() { Set colors = new HashSet<>(); for (final SpellAbility ab : getManaAbilities()) { - // Without an activating player each canProduce falls into a much more expensive path, - // and this is hot enough for that to matter. Only fill it in when it is missing, so a - // payment simulation that has already set a payer keeps it. + // 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()); } @@ -3366,6 +3365,9 @@ public final Set getProducibleColors() { } 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; } From a6898ad5dd693908306cef421da3222f239fb66e Mon Sep 17 00:00:00 2001 From: liamiak Date: Thu, 6 Aug 2026 20:49:04 -0600 Subject: [PATCH 7/8] Make the double-pip test actually test the double-pip credit It passed off three Islands and one Swamp even with the pip counting removed, because the depth term alone separated the two candidates. One of each leaves the pip counting as the only thing that can. Also rank through getBestLandAI, which nothing exercised with a real player. Co-Authored-By: Claude Opus 5 --- .../forge/ai/ability/LandColorNeedAiTest.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java index f5e78b266d21..b4b6130cf596 100644 --- a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java +++ b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java @@ -45,6 +45,11 @@ public void scoresTheColorTheHandIsWaitingOn() { int black = ComputerUtilCard.getColorFixingValue(ai, duals[1]); assertTrue("black unblocks the hand, white does not", black > white); + // and that is what ranking picks: the two duals are worth the same as lands, so the + // colour we are waiting on is the only thing separating them + assertEquals("Underground Sea", + ComputerUtilCard.getBestLandAI(ai, Lists.newArrayList(duals)).getName()); + // whether our sources happen to be tapped says nothing about which colours the board can // make, so holding the land drop until main 2 must not change the answer for (Card c : ai.getCardsIn(ZoneType.Battlefield)) { @@ -105,12 +110,14 @@ public void aSecondSourceCountsForDoublePips() { Game game = initAndCreateGame(); Player ai = game.getPlayers().get(1); - addCards("Island", 3, ai); - addCards("Swamp", 1, ai); // one black source only - addCardToZone("Sign in Blood", ai, ZoneType.Hand); // BB, so it wants a second + // one of each, so depth alone cannot tell the two candidates apart and only the pip + // counting can - a colour mask would call BB payable off the single Swamp + addCards("Island", 1, ai); + addCards("Swamp", 1, ai); + addCardToZone("Sign in Blood", ai, ZoneType.Hand); // BB, so it wants a second black game.getAction().checkStateEffects(true); - assertTrue("a second black source is progress towards BB, a fourth blue source is not", + assertTrue("a second black source is progress towards BB, a second blue source is not", ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Swamp", ai, ZoneType.Library)) > ComputerUtilCard.getColorFixingValue(ai, addCardToZone("Island", ai, ZoneType.Library))); } From fe85a2260cc660e4087c362e2f9957f493632d47 Mon Sep 17 00:00:00 2001 From: liamiak Date: Thu, 6 Aug 2026 21:17:57 -0600 Subject: [PATCH 8/8] Count an "Any" source as the colours it can actually make getAvailableManaColors collected the raw Produced$ string, and every caller runs that through ColorSet.fromNames, which keeps only colour names - so "Any" contributed nothing and a board of City of Brass read as unable to cast anything coloured. Ask getProducibleColors instead, which resolves it, and stop once every colour is present. Co-Authored-By: Claude Opus 5 --- .../main/java/forge/ai/ComputerUtilCost.java | 10 +++--- .../forge/ai/ability/LandColorNeedAiTest.java | 32 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java b/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java index e7bcc52195d1..1401db4148fc 100644 --- a/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java +++ b/forge-ai/src/main/java/forge/ai/ComputerUtilCost.java @@ -648,10 +648,12 @@ public static Set getAvailableManaColors(Player ai, List 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 } } diff --git a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java index b4b6130cf596..3545a5c8cba7 100644 --- a/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java +++ b/forge-gui-desktop/src/test/java/forge/ai/ability/LandColorNeedAiTest.java @@ -4,14 +4,19 @@ import com.google.common.collect.Lists; +import java.util.List; + import forge.ai.AITest; import forge.ai.ComputerUtilCard; +import forge.ai.ComputerUtilCost; +import forge.card.ColorSet; import forge.game.Game; import forge.game.card.Card; import forge.game.player.Player; import forge.game.zone.ZoneType; import static org.testng.AssertJUnit.assertEquals; +import static org.testng.AssertJUnit.assertFalse; import static org.testng.AssertJUnit.assertTrue; /** @@ -141,6 +146,33 @@ public void depthPrefersTheColorWeAreThinnestOn() { assertTrue("but a fourth is not worthless", fourthIsland > 0); } + /** + * Every caller runs getAvailableManaColors through ColorSet.fromNames, which keeps only colour + * names - so a source whose script says {@code Produced$ Any} used to contribute nothing, and + * a board of nothing but City of Brass read as unable to cast anything coloured. + */ + @Test + public void anyColorSourcesOfferEveryColor() { + assertTrue("an any-colour source offers white", canPayWhiteOff("City of Brass")); + assertTrue(canPayWhiteOff("Mana Confluence")); + // and it still says no when the colour really is absent + assertFalse("a blue source does not offer white", canPayWhiteOff("Island")); + } + + private boolean canPayWhiteOff(String landName) { + Game game = initAndCreateGame(); + Player ai = game.getPlayers().get(1); + for (int i = 0; i < 3; i++) { + addCard(landName, ai).setSickness(false); + } + Card swords = addCardToZone("Swords to Plowshares", ai, ZoneType.Hand); + game.getAction().checkStateEffects(true); + + ColorSet available = ColorSet.fromNames( + ComputerUtilCost.getAvailableManaColors(ai, (List) null)); + return swords.getManaCost().canBePaidWithAvailable(available.getColor()); + } + /** * The same decision through a real fetchland, which is how this is reached in a game: * Flooded Strand searches for a Plains or an Island, and every dual carrying one of those