improve random choice of N cards

remove gui calls from choosePile effect

forge-core/src/main/java/forge/util/Aggregates.java

@@ -79,8 +79,7 @@ public class Aggregates {
         if( null == source ) 
             return null;
         Random rnd = MyRandom.getRandom(); 
-        if ( source instanceof List<?> )
+        if ( source instanceof List<?> ) {
-        {
             List<T> src = (List<T>)source;
             int len = src.size();
             switch(len) {
@@ -88,33 +87,43 @@ public class Aggregates {
                 case 1: return src.get(0);
                 default: return src.get(rnd.nextInt(len));
             }
-
         }
         
-        int n = 0;
         T candidate = null;
+        int lowest = Integer.MAX_VALUE;
         for (final T item : source) {
-            if ((rnd.nextDouble() * ++n) < 1) {
+            int next = rnd.nextInt();
+            if(next < lowest) {
+                lowest = next;
                 candidate = item;
             }
         }
         return candidate;
     }
 
-    // Get several random values
-    // should improve to make 1 pass over source and track N candidates at once
     public static final <T> List<T> random(final Iterable<T> source, final int count) {
         final List<T> result = new ArrayList<T>();
-        for (int i = 0; i < count; ++i) {
+        final int[] randoms = new int[count];
-            final T toAdd = Aggregates.random(source);
+        for(int i = 0; i < count; i++) {
-            if (toAdd == null) {
+            randoms[i] = Integer.MAX_VALUE;
-                break;
+            result.add(null);
+        }
+        
+        Random rnd = MyRandom.getRandom(); 
+        for(T item : source) {
+            int next = rnd.nextInt();
+            for(int i = 0; i < count; i++) {
+                if(next < randoms[i]) {
+                    randoms[i] = next;
+                    result.set(i, item);
+                    break;
+                }
             }
-            result.add(toAdd);
         }
         return result;
     }
 
+
     public static final <K, U> Iterable<U> uniqueByLast(final Iterable<U> source, final Function<U, K> fnUniqueKey) { // this might be exotic
         final Map<K, U> uniques = new Hashtable<K, U>();
         for (final U c : source) {

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

@@ -956,5 +956,47 @@ public class AiController {
         return result;
     }
 
+    public List<Card> chooseCardsForEffect(List<Card> pool, SpellAbility sa, int min, int max, boolean isOptional) {
+        if( sa == null || sa.getApi() == null ) {
+            throw new UnsupportedOperationException();
+        }
+        List<Card> result = new ArrayList<>();
+        switch( sa.getApi()) {
+            case TwoPiles:
+                Card biggest = null;
+                Card smallest = null;
+                biggest = pool.get(0);
+                smallest = pool.get(0);
+
+                for (Card c : pool) {
+                    if (c.getCMC() >= biggest.getCMC()) {
+                        biggest = c;
+                    } else if (c.getCMC() <= smallest.getCMC()) {
+                        smallest = c;
+                    }
+                }
+                result.add(biggest);
+
+                if (max >= 3 && !result.contains(smallest)) {
+                    result.add(smallest);
+                }
+                
+            default:
+                for (int i = 0; i < max; i++) {
+                    Card c = player.getController().chooseSingleCardForEffect(pool, sa, null, isOptional);
+                    if (c != null) {
+                        result.add(c);
+                        pool.remove(c);
+                    } else {
+                        break;
+                    }
+                }
+                
+        
+        }
+        return result;
+        
+    }
+
 }
 

forge-gui/src/main/java/forge/game/ability/effects/ChooseCardEffect.java

@@ -36,7 +36,7 @@ public class ChooseCardEffect extends SpellAbilityEffect {
         final Card host = sa.getSourceCard();
         final Player activator = sa.getActivatingPlayer();
         final Game game = activator.getGame();
-        final List<Card> chosen = new ArrayList<Card>();
+        List<Card> chosen = new ArrayList<Card>();
 
         final TargetRestrictions tgt = sa.getTargetRestrictions();
         final List<Player> tgtPlayers = getTargetPlayers(sa);
@@ -79,23 +79,10 @@ public class ChooseCardEffect extends SpellAbilityEffect {
                 }
             } else if ((tgt == null) || p.canBeTargetedBy(sa)) {
                 if (sa.hasParam("AtRandom")) {
-                    for (int i = 0; i < validAmount; i++) {
+                    chosen = Aggregates.random(choices, validAmount);
-                        Card c = Aggregates.random(choices);
-                        if (c != null) {
-                            chosen.add(c);
-                            choices.remove(c);
-                        } else {
-                            break;
-                        }
-                    }
                 } else {
-                    final List<Card> choice = p.getController().chooseCardsForEffect(choices, sa, sa.hasParam("ChoiceTitle") ? 
+                    String title = sa.hasParam("ChoiceTitle") ? sa.getParam("ChoiceTitle") : "Choose a card ";
-                            sa.getParam("ChoiceTitle") : "Choose a card ", validAmount, !sa.hasParam("Mandatory"));
+                    chosen = p.getController().chooseCardsForEffect(choices, sa, title, validAmount, validAmount, !sa.hasParam("Mandatory"));
-                    for (Card c : choice) {
-                        if (c != null) {
-                            chosen.add(c);
-                        }
-                    }
                 }
             }
         }

forge-gui/src/main/java/forge/game/ability/effects/PlayEffect.java

@@ -112,7 +112,7 @@ public class PlayEffect extends SpellAbilityEffect {
                 }
                 if (sa.hasParam("ChoiceNum")) {
                     final int choicenum = AbilityUtils.calculateAmount(source, sa.getParam("ChoiceNum"), sa);
-                    tgtCards = activator.getController().chooseCardsForEffect(choice, sa, source + " - Choose up to " + Lang.nounWithNumeral(choicenum, "card"), choicenum, true);
+                    tgtCards = activator.getController().chooseCardsForEffect(choice, sa, source + " - Choose up to " + Lang.nounWithNumeral(choicenum, "card"), 0, choicenum, true);
                 } else {
                     tgtCards = choice;
                 }

forge-gui/src/main/java/forge/game/ability/effects/TwoPilesEffect.java

@@ -3,7 +3,9 @@ package forge.game.ability.effects;
 import java.util.ArrayList;
 import java.util.List;
 
-import forge.ai.ComputerUtilCard;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+
 import forge.game.ability.AbilityFactory;
 import forge.game.ability.AbilityUtils;
 import forge.game.ability.SpellAbilityEffect;
@@ -14,8 +16,6 @@ import forge.game.spellability.AbilitySub;
 import forge.game.spellability.SpellAbility;
 import forge.game.spellability.TargetRestrictions;
 import forge.game.zone.ZoneType;
-import forge.gui.GuiChoose;
-import forge.gui.GuiDialog;
 
 public class TwoPilesEffect extends SpellAbilityEffect {
 
@@ -79,8 +79,6 @@ public class TwoPilesEffect extends SpellAbilityEffect {
 
         for (final Player p : tgtPlayers) {
             if ((tgt == null) || p.canBeTargetedBy(sa)) {
-                final ArrayList<Card> pile1 = new ArrayList<Card>();
-                final ArrayList<Card> pile2 = new ArrayList<Card>();
                 List<Card> pool = new ArrayList<Card>();
                 if (sa.hasParam("DefinedCards")) {
                     pool = new ArrayList<Card>(AbilityUtils.getDefinedCards(sa.getSourceCard(), sa.getParam("DefinedCards"), sa));
@@ -91,157 +89,44 @@ public class TwoPilesEffect extends SpellAbilityEffect {
                 int size = pool.size();
 
                 // first, separate the cards into piles
-                if (separator.isHuman()) {
+                final List<Card> pile1 = separator.getController().chooseCardsForEffect(pool, sa, "Divide cards into two piles", 1, size - 1, false);
-                    final List<Card> firstPile = GuiChoose.order("Place into two piles", "Pile 1", -1, pool, null, card);
+                final List<Card> pile2 = Lists.newArrayList(pool);
-                    for (final Object o : firstPile) {
+                Iterables.removeAll(pile2, pile1);
-                        pile1.add((Card) o);
-                    }
-
-                    for (final Card c : pool) {
-                        if (!pile1.contains(c)) {
-                            pile2.add(c);
-                        }
-                    }
-                } else if (size > 0) {
-                    //computer separates
-                    Card biggest = null;
-                    Card smallest = null;
-                    biggest = pool.get(0);
-                    smallest = pool.get(0);
-
-                    for (Card c : pool) {
-                        if (c.getCMC() >= biggest.getCMC()) {
-                            biggest = c;
-                        } else if (c.getCMC() <= smallest.getCMC()) {
-                            smallest = c;
-                        }
-                    }
-                    pile1.add(biggest);
-
-                    if (size > 3 && !pile1.contains(smallest)) {
-                        pile1.add(smallest);
-                    }
-                    for (Card c : pool) {
-                        if (!pile1.contains(c)) {
-                            pile2.add(c);
-                        }
-                    }
-                }
 
                 System.out.println("Pile 1:" + pile1);
                 System.out.println("Pile 2:" + pile2);
                 card.clearRemembered();
 
-                pile1WasChosen = selectPiles(sa, pile1, pile2, chooser, card, pool);
+                pile1WasChosen = chooser.getController().chooseCardsPile(sa, pile1, pile2, !sa.hasParam("FaceDown"));
+                List<Card> chosenPile = pile1WasChosen ? pile1 : pile2;
+                List<Card> unchosenPile = !pile1WasChosen ? pile1 : pile2;
+                
+                p.getGame().getAction().nofityOfValue(sa, chooser, chooser + " chooses Pile " + (pile1WasChosen ? "1" : "2"), chooser);
 
                 // take action on the chosen pile
                 if (sa.hasParam("ChosenPile")) {
+                    for (final Card z : chosenPile) {
+                        card.addRemembered(z);
+                    }
                     final SpellAbility action = AbilityFactory.getAbility(card.getSVar(sa.getParam("ChosenPile")), card);
                     action.setActivatingPlayer(sa.getActivatingPlayer());
                     ((AbilitySub) action).setParent(sa);
-
                     AbilityUtils.resolve(action);
                 }
 
-                // take action on the chosen pile
+                // take action on the unchosen pile
                 if (sa.hasParam("UnchosenPile")) {
-                    //switch the remembered cards
                     card.clearRemembered();
-                    if (pile1WasChosen) {
+                    for (final Card z : unchosenPile) {
-                        for (final Card c : pile2) {
+                        card.addRemembered(z);
-                            card.addRemembered(c);
-                        }
-                    } else {
-                        for (final Card c : pile1) {
-                            card.addRemembered(c);
-                        }
                     }
+                    
                     final SpellAbility action = AbilityFactory.getAbility(card.getSVar(sa.getParam("UnchosenPile")), card);
                     action.setActivatingPlayer(sa.getActivatingPlayer());
                     ((AbilitySub) action).setParent(sa);
-
                     AbilityUtils.resolve(action);
                 }
             }
         }
     } // end twoPiles resolve
-
-    private boolean selectPiles(final SpellAbility sa, ArrayList<Card> pile1, ArrayList<Card> pile2,
-            Player chooser, Card card, List<Card> pool) {
-        boolean pile1WasChosen = true;
-        // then, the chooser picks a pile
-
-        if (sa.hasParam("FaceDown")) {
-            // Used for Phyrexian Portal, FaceDown Pile choosing
-            if (chooser.isHuman()) {
-                final String p1Str = String.format("Pile 1 (%s cards)", pile1.size());
-                final String p2Str = String.format("Pile 2 (%s cards)", pile2.size());
-                final String[] possibleValues = { p1Str , p2Str };
-
-                pile1WasChosen = GuiDialog.confirm(card, "Choose a Pile", possibleValues);
-            }
-            else {
-                // AI will choose the first pile if it is larger or the same
-                // TODO Improve this to be slightly more random to not be so predictable
-                pile1WasChosen = pile1.size() >= pile2.size();
-            }
-        } else {
-            if (chooser.isHuman()) {
-                final Card[] disp = new Card[pile1.size() + pile2.size() + 2];
-                disp[0] = new Card(-1);
-                disp[0].setName("Pile 1");
-                for (int i = 0; i < pile1.size(); i++) {
-                    disp[1 + i] = pile1.get(i);
-                }
-                disp[pile1.size() + 1] = new Card(-2);
-                disp[pile1.size() + 1].setName("Pile 2");
-                for (int i = 0; i < pile2.size(); i++) {
-                    disp[pile1.size() + i + 2] = pile2.get(i);
-                }
-
-                // make sure Pile 1 or Pile 2 is clicked on
-                while (true) {
-                    final Object o = GuiChoose.one("Choose a pile", disp);
-                    final Card c = (Card) o;
-                    String name = c.getName();
-
-                    if (!(name.equals("Pile 1") || name.equals("Pile 2"))) {
-                        continue;
-                    }
-
-                    pile1WasChosen = name.equals("Pile 1");
-                    break;
-                }
-            } else {
-                int cmc1 = ComputerUtilCard.evaluatePermanentList(new ArrayList<Card>(pile1));
-                int cmc2 = ComputerUtilCard.evaluatePermanentList(new ArrayList<Card>(pile2));
-                if (CardLists.getNotType(pool, "Creature").isEmpty()) {
-                    cmc1 = ComputerUtilCard.evaluateCreatureList(new ArrayList<Card>(pile1));
-                    cmc2 = ComputerUtilCard.evaluateCreatureList(new ArrayList<Card>(pile2));
-                    System.out.println("value:" + cmc1 + " " + cmc2);
-                }
-
-                // for now, this assumes that the outcome will be bad
-                // TODO: This should really have a ChooseLogic param to
-                // figure this out
-                pile1WasChosen = cmc1 >= cmc2;
-                if ("Worst".equals(sa.getParam("AILogic"))) {
-                    pile1WasChosen = !pile1WasChosen;
-                }
-                GuiDialog.message("Computer chooses the Pile " + (pile1WasChosen ? "1" : "2"));
-            }
-        }
-
-        if (pile1WasChosen) {
-            for (final Card z : pile1) {
-                card.addRemembered(z);
-            }
-        } else {
-            for (final Card z : pile2) {
-                card.addRemembered(z);
-            }
-        }
-
-        return pile1WasChosen;
-    }
 }

forge-gui/src/main/java/forge/game/ability/effects/UntapEffect.java

@@ -78,7 +78,7 @@ public class UntapEffect extends SpellAbilityEffect {
             List<Card> list = CardLists.getType(p.getCardsIn(ZoneType.Battlefield), valid);
             list = CardLists.filter(list, Presets.TAPPED);
             
-            List<Card> selected = p.getController().chooseCardsForEffect(list, sa, "Select cards to untap", num, true);
+            List<Card> selected = p.getController().chooseCardsForEffect(list, sa, "Select cards to untap", 0, num, true);
             if( selected != null )
                 for( Card c : selected ) 
                     c.untap();

forge-gui/src/main/java/forge/game/player/PlayerController.java

@@ -126,7 +126,8 @@ public abstract class PlayerController {
     // Specify a target of a spell (Spellskite)
     public abstract Pair<SpellAbilityStackInstance, GameObject> chooseTarget(SpellAbility sa, List<Pair<SpellAbilityStackInstance, GameObject>> allTargets);
 
-    public abstract List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int amount, boolean isOptional);
+    // Q: why is there min/max and optional at once? A: This is to handle cases like 'choose 3 to 5 cards or none at all'  
+    public abstract List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int min, int max, boolean isOptional);
     public final Card chooseSingleCardForEffect(Collection<Card> sourceList, SpellAbility sa, String title) { return chooseSingleCardForEffect(sourceList, sa, title, false, null); }
     public final Card chooseSingleCardForEffect(Collection<Card> sourceList, SpellAbility sa, String title, boolean isOptional) { return chooseSingleCardForEffect(sourceList, sa, title, isOptional, null); } 
     public abstract Card chooseSingleCardForEffect(Collection<Card> sourceList, SpellAbility sa, String title, boolean isOptional, Player relatedPlayer);
@@ -200,6 +201,6 @@ public abstract class PlayerController {
     public abstract void playTrigger(Card host, WrappedAbility wrapperAbility, boolean isMandatory);
 
     public abstract boolean playSaFromPlayEffect(SpellAbility tgtSA);
-
     public abstract Map<GameEntity, CounterType> chooseProliferation();
+    public abstract boolean chooseCardsPile(SpellAbility sa, List<Card> pile1,List<Card> pile2, boolean faceUp);
 }

forge-gui/src/main/java/forge/game/player/PlayerControllerAi.java

@@ -127,19 +127,8 @@ public class PlayerControllerAi extends PlayerController {
     }
 
     @Override
-    public List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int amount,
+    public List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int min, int max, boolean isOptional) {
-            boolean isOptional) {
+        return brains.chooseCardsForEffect(sourceList, sa, min, max, isOptional);
-        List<Card> chosen = new ArrayList<Card>();
-        for (int i = 0; i < amount; i++) {
-            Card c = this.chooseSingleCardForEffect(sourceList, sa, title, isOptional);
-            if (c != null) {
-                chosen.add(c);
-                sourceList.remove(c);
-            } else {
-                break;
-            }
-        }
-        return chosen;
     }
 
     @Override
@@ -672,4 +661,23 @@ public class PlayerControllerAi extends PlayerController {
         return currentAbility.doTrigger(true, player);
     }
 
+    @Override
+    public boolean chooseCardsPile(SpellAbility sa, List<Card> pile1, List<Card> pile2, boolean faceUp) {
+        if (!faceUp) {
+            // AI will choose the first pile if it is larger or the same
+            // TODO Improve this to be slightly more random to not be so predictable
+            return pile1.size() >= pile2.size();
+        } else {
+            boolean allCreatures = Iterables.all(Iterables.concat(pile1, pile2), CardPredicates.Presets.CREATURES);
+            int cmc1 = allCreatures ? ComputerUtilCard.evaluateCreatureList(pile1) : ComputerUtilCard.evaluatePermanentList(pile1);
+            int cmc2 = allCreatures ? ComputerUtilCard.evaluateCreatureList(pile2) : ComputerUtilCard.evaluatePermanentList(pile2);
+            System.out.println("value:" + cmc1 + " " + cmc2);
+
+            // for now, this assumes that the outcome will be bad
+            // TODO: This should really have a ChooseLogic param to
+            // figure this out
+            return "Worst".equals(sa.getParam("AILogic")) ^ (cmc1 >= cmc2);
+        }
+    }
+
 }

forge-gui/src/main/java/forge/gui/player/PlayerControllerHuman.java

@@ -330,12 +330,12 @@ public class PlayerControllerHuman extends PlayerController {
      * @see forge.game.player.PlayerController#chooseCardsForEffect(java.util.Collection, forge.card.spellability.SpellAbility, java.lang.String, int, boolean)
      */
     @Override
-    public List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int amount,
+    public List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int min, int max, boolean isOptional) {
-            boolean isOptional) {
         // If only one card to choose, use a dialog box.
         // Otherwise, use the order dialog to be able to grab multiple cards in one shot
-        if (amount == 1) {
+        if (max == 1) {
-            return Lists.newArrayList(chooseSingleCardForEffect(sourceList, sa, title, isOptional));
+            Card singleChosen = chooseSingleCardForEffect(sourceList, sa, title, isOptional);
+            return singleChosen == null ?  Lists.<Card>newArrayList() : Lists.newArrayList(singleChosen);
         }
         GuiUtils.setPanelSelection(sa.getSourceCard());
         
@@ -350,14 +350,14 @@ public class PlayerControllerHuman extends PlayerController {
         }
         
         if(cardsAreInMyHandOrBattlefield) {
-            InputSelectCards sc = new InputSelectCardsFromList(isOptional ? 0 : amount, amount, sourceList);
+            InputSelectCards sc = new InputSelectCardsFromList(min, max, sourceList);
             sc.setMessage(title);
             sc.setCancelAllowed(isOptional);
             sc.showAndWait();
             return sc.hasCancelled() ? Lists.<Card>newArrayList() : sc.getSelected();
         }
 
-        int remaining = isOptional ? -1 : Math.max(sourceList.size() - amount, 0);
+        int remaining = isOptional ? -1 : Math.max(sourceList.size() - max, 0);
         return GuiChoose.order(title, "Chosen Cards", remaining, sourceList, null, sa.getSourceCard());
     }
 
@@ -1025,4 +1025,39 @@ public class PlayerControllerHuman extends PlayerController {
         return select.chooseTargets(null);
         
     }
+
+    @Override
+    public boolean chooseCardsPile(SpellAbility sa, List<Card> pile1, List<Card> pile2, boolean faceUp) {
+        if (!faceUp) {
+            final String p1Str = String.format("Pile 1 (%s cards)", pile1.size());
+            final String p2Str = String.format("Pile 2 (%s cards)", pile2.size());
+            final String[] possibleValues = { p1Str , p2Str };
+            return GuiDialog.confirm(sa.getSourceCard(), "Choose a Pile", possibleValues);
+        } else {
+            final Card[] disp = new Card[pile1.size() + pile2.size() + 2];
+            disp[0] = new Card(-1);
+            disp[0].setName("Pile 1");
+            for (int i = 0; i < pile1.size(); i++) {
+                disp[1 + i] = pile1.get(i);
+            }
+            disp[pile1.size() + 1] = new Card(-2);
+            disp[pile1.size() + 1].setName("Pile 2");
+            for (int i = 0; i < pile2.size(); i++) {
+                disp[pile1.size() + i + 2] = pile2.get(i);
+            }
+
+            // make sure Pile 1 or Pile 2 is clicked on
+            while (true) {
+                final Object o = GuiChoose.one("Choose a pile", disp);
+                final Card c = (Card) o;
+                String name = c.getName();
+
+                if (!(name.equals("Pile 1") || name.equals("Pile 2"))) {
+                    continue;
+                }
+
+                return name.equals("Pile 1");
+            }
+        }
+    }
 }

forge-gui/src/test/java/forge/gamesimulationtests/util/PlayerControllerForTests.java

@@ -57,6 +57,7 @@ import forge.gamesimulationtests.util.playeractions.DeclareAttackersAction;
 import forge.gamesimulationtests.util.playeractions.DeclareBlockersAction;
 import forge.gamesimulationtests.util.playeractions.PlayerActions;
 import forge.item.PaperCard;
+import forge.util.MyRandom;
 
 /**
  * Default harmless implementation for tests.
@@ -156,8 +157,8 @@ public class PlayerControllerForTests extends PlayerController {
 	}
 
 	@Override
-	public List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int amount, boolean isOptional) {
+	public List<Card> chooseCardsForEffect(List<Card> sourceList, SpellAbility sa, String title, int min, int max, boolean isOptional) {
-		return chooseItems(sourceList, amount);
+		return chooseItems(sourceList, max);
 	}
 
 	@Override
@@ -537,4 +538,9 @@ public class PlayerControllerForTests extends PlayerController {
     public boolean chooseTargetsFor(SpellAbility currentAbility) {
         return currentAbility.doTrigger(true, player);
     }
+
+    @Override
+    public boolean chooseCardsPile(SpellAbility sa, List<Card> pile1, List<Card> pile2, boolean faceUp) {
+        return MyRandom.getRandom().nextBoolean();
+    }
 }