Improve card-based Commander deck generation legality and weighting

forge-gui/src/main/java/forge/deck/CommanderDeckGenerator.java

@@ -63,10 +63,16 @@ public static List<DeckProxy> getBrawlDecks(final DeckFormat format, boolean isF
         return uniqueCards.toFlatList().stream()
                 .filter(format.isLegalCardPredicate())
                 .filter(PaperCardPredicates.fromRules(CardRulesPredicates.CAN_BE_BRAWL_COMMANDER.and(canPlay)))
+                .filter(CommanderDeckGenerator::canGenerateBrawlDeck)
                 .map(legend -> new CommanderDeckGenerator(legend, format, isForAi, isCardGen))
                 .collect(Collectors.toList());
     }
 
+    private static boolean canGenerateBrawlDeck(final PaperCard legend) {
+        // The current random Brawl generator cannot reliably build a legal mana base for colorless commanders.
+        return !legend.getRules().getColorIdentity().isColorless();
+    }
+
     private final PaperCard legend;
     private final int index;
     private final DeckFormat format;

forge-gui/src/main/java/forge/deck/DeckgenUtil.java

@@ -678,8 +678,7 @@ public static Deck generateRandomCommanderDeck(PaperCard commander, DeckFormat f
             }else {
                 String matrixKey = (format.equals(DeckFormat.TinyLeaders) ? DeckFormat.Commander : format).toString(); //use Commander for Tiny Leaders
                 List<Map.Entry<PaperCard, Integer>> potentialCards = new ArrayList<>(CardRelationMatrixGenerator.cardPools.get(matrixKey).get(commander.getName()));
-                Collections.shuffle(potentialCards, MyRandom.getRandom());
-                for(Map.Entry<PaperCard,Integer> pair:potentialCards){
+                for(Map.Entry<PaperCard,Integer> pair:getWeightedRandomizedCardPool(potentialCards)){
                     if(format.isLegalCard(pair.getKey())) {
                         preSelectedCards.add(pair.getKey());
                     }
@@ -826,6 +825,19 @@ private static PaperCard getRandomCard(final List<PaperCard> cards) {
         return cards.isEmpty() ? null : cards.get(MyRandom.getRandom().nextInt(cards.size()));
     }
 
+    private static List<Map.Entry<PaperCard, Integer>> getWeightedRandomizedCardPool(final List<Map.Entry<PaperCard, Integer>> potentialCards) {
+        final Map<Map.Entry<PaperCard, Integer>, Double> sortKeys = new IdentityHashMap<>();
+        for (final Map.Entry<PaperCard, Integer> cardEntry : potentialCards) {
+            sortKeys.put(cardEntry, getWeightedRandomSortKey(cardEntry));
+        }
+        potentialCards.sort(Comparator.comparingDouble(sortKeys::get).reversed());
+        return potentialCards;
+    }
+    private static double getWeightedRandomSortKey(final Map.Entry<PaperCard, Integer> cardEntry) {
+        final int weight = Math.max(1, cardEntry.getValue());
+        return Math.log(MyRandom.getRandom().nextDouble()) / weight;
+    }
+
     public static Map<ManaCostShard, Integer> suggestBasicLandCount(Deck d) {
         int W=0, U=0, R=0, B=0, G=0, total=0;
         List<PaperCard> cards = d.getOrCreate(DeckSection.Main).toFlatList();

forge-gui/src/main/java/forge/gamemodes/limited/CardThemedCommanderDeckBuilder.java

@@ -1,6 +1,9 @@
 package forge.gamemodes.limited;
 
+import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.stream.Collectors;
 
 import com.google.common.collect.Lists;
@@ -20,7 +23,7 @@ public class CardThemedCommanderDeckBuilder extends CardThemedDeckBuilder {
 
     public CardThemedCommanderDeckBuilder(PaperCard commanderCard0, PaperCard partner0, final List<PaperCard> dList, boolean isForAI, DeckFormat format) {
         super(new DeckGenPool(FModel.getMagicDb().getCommonCards().getUniqueCards()), format);
-        this.availableList = dList;
+        this.availableList = new ArrayList<>(dList);
         keyCard = commanderCard0;
         secondKeyCard = partner0;
         // remove Unplayables
@@ -32,9 +35,10 @@ public CardThemedCommanderDeckBuilder(PaperCard commanderCard0, PaperCard partne
             this.aiPlayables = Lists.newArrayList(availableList);
         }
         this.availableList.removeAll(aiPlayables);
+        this.aiPlayables = uniqueCardNamesForSingletonDeck(aiPlayables);
+        this.availableList = uniqueCardNamesForSingletonDeck(availableList);
         targetSize=format.getMainRange().getMinimum();
         colors = keyCard.getRules().getColorIdentity();
-        colors = ColorSet.combine(colors, keyCard.getRules().getColorIdentity());
         if (secondKeyCard != null && !format.equals(DeckFormat.Oathbreaker)) {
             colors = ColorSet.combine(colors, secondKeyCard.getRules().getColorIdentity());
             targetSize--;
@@ -59,6 +63,13 @@ protected void addLandKeyCards(){
         //do nothing as keycards are commander/partner and are added by the DeckGenUtils
     }
 
+    @Override
+    protected void extendPlaysets(int numSpellsNeeded) {
+        // Commander-family formats are singleton except for basic lands and cards
+        // with explicit deckbuilding exceptions. Do not fill gaps by duplicating
+        // cards already selected for the main deck.
+    }
+
     @Override
     protected void addThirdColorCards(int num) {
         //do nothing as we cannot add extra colours beyond commanders
@@ -78,4 +89,25 @@ protected String generateName() {
         return keyCard.getName() +" based commander deck";
     }
 
+    private List<PaperCard> uniqueCardNamesForSingletonDeck(final List<PaperCard> cards) {
+        final List<PaperCard> result = new ArrayList<>();
+        final Map<String, Integer> countsByName = new HashMap<>();
+        countsByName.put(keyCard.getName(), 1);
+        if (secondKeyCard != null) {
+            countsByName.put(secondKeyCard.getName(), 1);
+        }
+
+        for (final PaperCard card : cards) {
+            final int maxCopies = format.getMaxCardCopies(card);
+            final String name = card.getName();
+            final int currentCount = countsByName.getOrDefault(name, 0);
+            if (currentCount < maxCopies) {
+                result.add(card);
+                countsByName.put(name, currentCount + 1);
+            }
+        }
+
+        return result;
+    }
+
 }

forge-gui/src/main/java/forge/gamemodes/limited/CardThemedDeckBuilder.java

@@ -72,7 +72,6 @@ protected final float getSpellPercentage() {
     protected static final boolean logToConsole = false;
     protected static final boolean logColorsToConsole = false;
 
-    protected Iterable<PaperCard> keyCards;
     protected Map<Integer,Integer> targetCMCs;
 
     public CardThemedDeckBuilder(IDeckGenPool pool, DeckFormat format){
@@ -326,8 +325,9 @@ public Deck buildDeck() {
 
     //Extend to playsets for non land cards to fill out deck for when no other suitable cards are available
     protected void extendPlaysets(int numSpellsNeeded){
-        Map<PaperCard,Integer> currentCounts = new HashMap<>();
-        List<PaperCard> cardsToAdd = new ArrayList<>();
+        final Map<PaperCard,Integer> currentCounts = new HashMap<>();
+        final List<PaperCard> cardsToAdd = new ArrayList<>();
+        final Map<String, Integer> countsByName = getDeckListCountsByName();
         int i=0;
         for(PaperCard card: deckList){
             if(card.getRules().getType().isLand()){
@@ -336,8 +336,8 @@ protected void extendPlaysets(int numSpellsNeeded){
             currentCounts.merge(card, 1, Integer::sum);
         }
         for(PaperCard card: currentCounts.keySet()){
-            if(currentCounts.get(card)==2 || currentCounts.get(card)==3){
-                cardsToAdd.add(card);
+            if((currentCounts.get(card)==2 || currentCounts.get(card)==3)
+                    && addCardForGeneration(card, cardsToAdd, countsByName)){
                 ++i;
                 if(i >= numSpellsNeeded ){
                     break;
@@ -375,45 +375,35 @@ private int sumMapValues(Map<Integer, Integer> integerMap){
     protected void addKeyCards(){
         // Add the first keycard if not land
         if(!keyCard.getRules().getMainPart().getType().isLand()) {
-            keyCards = IterableUtil.filter(aiPlayables, PaperCardPredicates.name(keyCard.getName()));
-            final List<PaperCard> keyCardList = Lists.newArrayList(keyCards);
-            deckList.addAll(keyCardList);
-            aiPlayables.removeAll(keyCardList);
-            rankedColorList.removeAll(keyCardList);
+            addKeyCardCopies(keyCard);
         }
         // Add the second keycard if not land
         if(secondKeyCard!=null && !secondKeyCard.getRules().getMainPart().getType().isLand()) {
-            final List<PaperCard> keyCardList = aiPlayables.stream()
-                    .filter(PaperCardPredicates.name(secondKeyCard.getName()))
-                    .collect(Collectors.toList());
-            deckList.addAll(keyCardList);
-            aiPlayables.removeAll(keyCardList);
-            rankedColorList.removeAll(keyCardList);
+            addKeyCardCopies(secondKeyCard);
         }
     }
 
     protected void addLandKeyCards(){
         // Add the deck card
         if(keyCard.getRules().getMainPart().getType().isLand()) {
-            keyCards = IterableUtil.filter(aiPlayables, PaperCardPredicates.name(keyCard.getName()));
-            final List<PaperCard> keyCardList = Lists.newArrayList(keyCards);
-            deckList.addAll(keyCardList);
-            aiPlayables.removeAll(keyCardList);
-            rankedColorList.removeAll(keyCardList);
-            landsNeeded--;
+            landsNeeded -= addKeyCardCopies(keyCard).size();
         }
         // Add the deck card
         if(secondKeyCard!=null && secondKeyCard.getRules().getMainPart().getType().isLand()) {
-            final List<PaperCard> keyCardList = aiPlayables.stream()
-                    .filter(PaperCardPredicates.name(secondKeyCard.getName()))
-                    .collect(Collectors.toList());
-            deckList.addAll(keyCardList);
-            aiPlayables.removeAll(keyCardList);
-            rankedColorList.removeAll(keyCardList);
-            landsNeeded--;
+            landsNeeded -= addKeyCardCopies(secondKeyCard).size();
         }
     }
 
+    private List<PaperCard> addKeyCardCopies(final PaperCard card) {
+        final List<PaperCard> keyCardList = limitCopiesForGeneration(aiPlayables.stream()
+                .filter(PaperCardPredicates.name(card.getName()))
+                .collect(Collectors.toList()));
+        deckList.addAll(keyCardList);
+        aiPlayables.removeAll(keyCardList);
+        rankedColorList.removeAll(keyCardList);
+        return keyCardList;
+    }
+
     public static class MatchColorIdentity implements Predicate<CardRules> {
         private final ColorSet allowedColor;
 
@@ -451,7 +441,7 @@ protected void addThirdColorCards(int num) {
             // We haven't yet ranked the off-color cards.
             // Compare them to the cards already in the deckList.
             //List<PaperCard> rankedOthers = CardRanker.rankCardsInPack(others, deckList, colors, true);
-            List<PaperCard> toAdd = new ArrayList<>();
+            final List<PaperCard> toAdd = new ArrayList<>();
             for (final PaperCard card : others) {
                 // Want a card that has just one "off" color.
                 final ColorSet off = colors.getOffColors(card.getRules().getColor());
@@ -465,16 +455,17 @@ protected void addThirdColorCards(int num) {
                     .or(DeckGeneratorBase.COLORLESS_CARDS));
             final Iterable<PaperCard> threeColorList = IterableUtil.filter(aiPlayables,
                     PaperCardPredicates.fromRules(hasColor));
+            final Map<String, Integer> countsByName = getDeckListCountsByName();
             for (final PaperCard card : threeColorList) {
-                if (num > 0) {
-                    toAdd.add(card);
+                if (num <= 0) {
+                    break;
+                }
+                if (addCardForGeneration(card, toAdd, countsByName)) {
                     num--;
                     if (logToConsole) {
                         System.out.println("Third Color[" + num + "]:" + card.getName() + "("
                                 + card.getRules().getManaCost() + ")");
                     }
-                } else {
-                    break;
                 }
             }
             deckList.addAll(toAdd);
@@ -483,8 +474,10 @@ protected void addThirdColorCards(int num) {
     }
 
     protected void addLowCMCCard(){
+        final Map<String, Integer> countsByName = getDeckListCountsByName();
         final PaperCard card = rankedColorList.stream()
                 .filter(PaperCardPredicates.IS_NON_LAND)
+                .filter(cardToAdd -> canAddCardForGeneration(cardToAdd, countsByName))
                 .findFirst().orElse(null);
         if (card != null) {
             deckList.add(card);
@@ -604,7 +597,7 @@ public boolean test(PaperCard card) {
                         && !card.getRules().getMainPart().getType().isLand();
             }
         };
-        List<PaperCard> possibleList = Lists.newArrayList(pool.getAllCards(possibleFromFullPool));
+        List<PaperCard> possibleList = limitCopiesForGeneration(Lists.newArrayList(pool.getAllCards(possibleFromFullPool)));
         //ensure we do not add more keycards in case they are commanders
         if (keyCard != null) {
             possibleList.removeAll(StaticData.instance().getCommonCards().getAllCards(keyCard));
@@ -787,7 +780,8 @@ private boolean containsTronLands(Iterable<PaperCard> cards){
      */
     private void addNonBasicLands() {
         Iterable<PaperCard> lands = IterableUtil.filter(aiPlayables, PaperCardPredicates.IS_NONBASIC_LAND);
-        List<PaperCard> landsToAdd = new ArrayList<>();
+        final List<PaperCard> landsToAdd = new ArrayList<>();
+        final Map<String, Integer> countsByName = getDeckListCountsByName();
         int minBasics;//Keep a minimum number of basics to ensure playable decks
         if(colors.isColorless()) {
             minBasics = 0;
@@ -802,8 +796,8 @@ private void addNonBasicLands() {
         for (final PaperCard card : lands) {
             if (landsNeeded > minBasics) {
                 // Use only lands that are within our colors
-                if (card.getRules().getDeckbuildingColors().hasNoColorsExcept(colors)) {
-                    landsToAdd.add(card);
+                if (card.getRules().getDeckbuildingColors().hasNoColorsExcept(colors)
+                        && addCardForGeneration(card, landsToAdd, countsByName)) {
                     landsNeeded--;
                 } else if (logToConsole) {
                     System.out.println("Excluding NonBasicLand: " + card.getName());
@@ -830,7 +824,7 @@ private void addRandomCards(int num) {
                 && !card.getRules().getAiHints().getRemRandomDecks()
                 && !card.getRules().getMainPart().getType().isLand();
 
-        List<PaperCard> possibleList = Lists.newArrayList(pool.getAllCards(possibleFromFullPool));
+        List<PaperCard> possibleList = limitCopiesForGeneration(Lists.newArrayList(pool.getAllCards(possibleFromFullPool)));
         //ensure we do not add more keycards in case they are commanders
         if (keyCard != null) {
             possibleList.removeAll(StaticData.instance().getCommonCards().getAllCards(keyCard));
@@ -844,6 +838,47 @@ private void addRandomCards(int num) {
         addManaCurveCards(possibleList, num, "Random Card");
     }
 
+    protected List<PaperCard> limitCopiesForGeneration(final List<PaperCard> cards) {
+        final Map<String, Integer> countsByName = getDeckListCountsByName();
+
+        final List<PaperCard> result = new ArrayList<>();
+        for (final PaperCard card : cards) {
+            addCardForGeneration(card, result, countsByName);
+        }
+        return result;
+    }
+
+    private Map<String, Integer> getDeckListCountsByName() {
+        final Map<String, Integer> countsByName = new HashMap<>();
+        for (final PaperCard card : deckList) {
+            addCardToGenerationCounts(card, countsByName);
+        }
+        return countsByName;
+    }
+
+    private boolean canAddCardForGeneration(final PaperCard card, final Map<String, Integer> countsByName) {
+        return countsByName.getOrDefault(card.getName(), 0) < getMaxCopiesForGeneration(card);
+    }
+
+    private boolean addCardForGeneration(final PaperCard card, final List<PaperCard> cards,
+            final Map<String, Integer> countsByName) {
+        if (!canAddCardForGeneration(card, countsByName)) {
+            return false;
+        }
+        cards.add(card);
+        addCardToGenerationCounts(card, countsByName);
+        return true;
+    }
+
+    private void addCardToGenerationCounts(final PaperCard card, final Map<String, Integer> countsByName) {
+        countsByName.merge(card.getName(), 1, Integer::sum);
+    }
+
+    private int getMaxCopiesForGeneration(final PaperCard card) {
+        final int formatMax = format.getMaxCardCopies(card);
+        return formatMax == format.getMaxCardCopies() ? maxDuplicates : formatMax;
+    }
+
     /**
      * Add creatures to the deck.
      *
@@ -853,17 +888,19 @@ private void addRandomCards(int num) {
      *            number to add
      */
     private void addCards(final Iterable<PaperCard> cards, int num) {
-        List<PaperCard> cardsToAdd = new ArrayList<>();
+        final List<PaperCard> cardsToAdd = new ArrayList<>();
+        final Map<String, Integer> countsByName = getDeckListCountsByName();
         for (final PaperCard card : cards) {
             if(card.getRules().getMainPart().getType().isLand()){
                 continue;
             }
             if (num > 0) {
-                cardsToAdd.add(card);
-                if (logToConsole) {
-                    System.out.println("Extra needed[" + num + "]:" + card.getName() + " (" + card.getRules().getManaCost() + ")");
+                if (addCardForGeneration(card, cardsToAdd, countsByName)) {
+                    if (logToConsole) {
+                        System.out.println("Extra needed[" + num + "]:" + card.getName() + " (" + card.getRules().getManaCost() + ")");
+                    }
+                    num--;
                 }
-                num--;
             } else {
                 break;
             }
@@ -896,12 +933,17 @@ private void addManaCurveCards(final Iterable<PaperCard> creatures, int num, Str
         final Map<Integer, Long> creatureCosts = deckList.stream().filter(PaperCardPredicates.IS_CREATURE)
             .collect(Collectors.groupingBy(c -> Ints.constrainToRange(c.getRules().getManaCost().getCMC(), 1, 6), Collectors.counting()));
 
-        List<PaperCard> creaturesToAdd = new ArrayList<>();
+        final List<PaperCard> creaturesToAdd = new ArrayList<>();
+        final Map<String, Integer> countsByName = getDeckListCountsByName();
         for (final PaperCard card : creatures) {
             int cmc = Ints.constrainToRange(card.getRules().getManaCost().getCMC(), 1, 6);
 
+            if (!canAddCardForGeneration(card, countsByName)) {
+                continue;
+            }
+
             if (creatureCosts.getOrDefault(cmc, 0l) < targetCMCs.get(cmc)) {
-                creaturesToAdd.add(card);
+                addCardForGeneration(card, creaturesToAdd, countsByName);
                 num--;
                 creatureCosts.merge(cmc, 1l, Long::sum);
                 if (logToConsole) {