3 fixes (#1925)

* Improve logic / avoid cheating

* Fix Aggregates.random on FCollection disaster

* Fix finding wrong SA

Co-authored-by: tool4EvEr <tool4EvEr@192.168.0.59>

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

@@ -2282,23 +2282,31 @@ public class ComputerUtil {
         if (goodChoices.isEmpty()) {
             goodChoices = validCards;
         }
-        final CardCollection dChoices = new CardCollection();
+
+        if (min == 1 && max == 1) {
             if (sa.hasParam("DiscardValid")) {
                 final String validString = sa.getParam("DiscardValid");
                 if (validString.contains("Creature") && !validString.contains("nonCreature")) {
                     final Card c = ComputerUtilCard.getBestCreatureAI(goodChoices);
                     if (c != null) {
-                    dChoices.add(c);
+                        return new CardCollection(c);
                     }
                 }
             }
+        }
+
+        // not enough good choices, need to fill the rest
+        int minDiff = min - goodChoices.size();
+        if (minDiff > 0) {
+            goodChoices.addAll(Aggregates.random(CardLists.filter(validCards, Predicates.not(Predicates.in(goodChoices))), minDiff));
+            return goodChoices;
+        }
 
         Collections.sort(goodChoices, CardLists.TextLenComparator);
 
         CardLists.sortByCmcDesc(goodChoices);
-        dChoices.add(goodChoices.get(0));
 
-        return Aggregates.random(goodChoices, min, new CardCollection());
+        return new CardCollection(Aggregates.random(goodChoices, max));
     }
 
     public static CardCollection getCardsToDiscardFromFriend(Player aiChooser, Player p, SpellAbility sa, CardCollection validCards, int min, int max) {

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

@@ -59,7 +59,7 @@ public class ChooseCardEffect extends SpellAbilityEffect {
         final Card host = sa.getHostCard();
         final Player activator = sa.getActivatingPlayer();
         final Game game = activator.getGame();
-        final CardCollection chosen = new CardCollection();
+        CardCollection chosen = new CardCollection();
 
         final TargetRestrictions tgt = sa.getTargetRestrictions();
         final List<Player> tgtPlayers = getTargetPlayers(sa);
@@ -221,7 +221,8 @@ public class ChooseCardEffect extends SpellAbilityEffect {
 
             } else if ((tgt == null) || p.canBeTargetedBy(sa)) {
                 if (sa.hasParam("AtRandom") && !choices.isEmpty()) {
-                    Aggregates.random(choices, validAmount, chosen);
+                    // don't pass FCollection for direct modification, the Set part would get messed up
+                    chosen = new CardCollection(Aggregates.random(choices, validAmount));
                     dontRevealToOwner = false;
                 } else {
                     String title = sa.hasParam("ChoiceTitle") ? sa.getParam("ChoiceTitle") : Localizer.getInstance().getMessage("lblChooseaCard") + " ";

forge-game/src/main/java/forge/game/ability/effects/ChooseGenericEffect.java

@@ -77,7 +77,7 @@ public class ChooseGenericEffect extends SpellAbilityEffect {
             }
 
             for (SpellAbility chosenSA : chosenSAs) {
-                if (sa.hasParam("AtRandom") && sa.getParam("AtRandom").equals("Urza") && chosenSA.usesTargeting()) {
+                if (random && sa.getParam("AtRandom").equals("Urza") && chosenSA.usesTargeting()) {
                     List<Card> validTargets = CardUtil.getValidCardsToTarget(chosenSA.getTargetRestrictions(), sa);
                     if (validTargets.isEmpty()) {
                         List <SpellAbility> newChosenSAs = Lists.newArrayList();

forge-game/src/main/java/forge/game/ability/effects/DiscardEffect.java

@@ -119,7 +119,7 @@ public class DiscardEffect extends SpellAbilityEffect {
             }
         }
         return sb.toString();
-    } // discardStackDescription()
+    }
 
     @Override
     public void resolve(SpellAbility sa) {
@@ -314,5 +314,5 @@ public class DiscardEffect extends SpellAbilityEffect {
 
         // run trigger if something got milled
         table.triggerChangesZoneAll(game, sa);
-    } // discardResolve()
+    }
 }

forge-game/src/main/java/forge/game/ability/effects/SacrificeEffect.java

@@ -152,7 +152,7 @@ public class SacrificeEffect extends SpellAbilityEffect {
                     }
 
                     if (sa.hasParam("Random")) {
-                        choosenToSacrifice = Aggregates.random(validTargets, Math.min(amount, validTargets.size()), new CardCollection());
+                        choosenToSacrifice = new CardCollection(Aggregates.random(validTargets, Math.min(amount, validTargets.size())));
                     } else if (optional && !p.getController().confirmAction(sa, null, Localizer.getInstance().getMessage("lblDoYouWantSacrifice"), null)) {
                         choosenToSacrifice = CardCollection.EMPTY;
                     } else {

forge-gui/res/cardsfolder/h/hallow.txt

@@ -1,7 +1,7 @@
 Name:Hallow
 ManaCost:W
 Types:Instant
-A:SP$ Effect | Cost$ W | ValidTgts$ Card.inZoneStack | TgtZone$ Stack | TgtPrompt$ Select target spell to prevent damage from | ReplacementEffects$ PreventDmg | ExileOnMoved$ Stack | RememberObjects$ TargetedSource | SpellDescription$ Prevent all damage target spell would deal this turn. You gain life equal to the damage prevented this way.
+A:SP$ Effect | Cost$ W | ValidTgts$ Card | TargetType$ Spell | TgtZone$ Stack | TgtPrompt$ Select target spell to prevent damage from | ReplacementEffects$ PreventDmg | ExileOnMoved$ Stack | RememberObjects$ TargetedSource | SpellDescription$ Prevent all damage target spell would deal this turn. You gain life equal to the damage prevented this way.
 SVar:PreventDmg:Event$ DamageDone | ValidSource$ Card.IsRemembered | ReplaceWith$ GainLifeYou | PreventionEffect$ True | Description$ Prevent all damage that would be dealt by targeted spell this turn. You gain life equal to the damage prevented this way.
 SVar:GainLifeYou:DB$ GainLife | Defined$ You | LifeAmount$ X
 SVar:X:ReplaceCount$DamageAmount

forge-gui/res/cardsfolder/s/shieldmage_elder.txt

@@ -3,7 +3,7 @@ ManaCost:5 W
 Types:Creature Human Cleric Wizard
 PT:2/3
 A:AB$ Pump | Cost$ tapXType<2/Cleric> | ValidTgts$ Creature | KW$ Prevent all damage that would be dealt by CARDNAME. | IsCurse$ True | TgtPrompt$ Select target creature | SpellDescription$ Prevent all damage target creature would deal this turn.
-A:AB$ Effect | Cost$ tapXType<2/Wizard> | ValidTgts$ Card.inZoneStack | TgtZone$ Stack | IsCurse$ True | TgtPrompt$ Select target spell | RememberObjects$ TargetedSource | StaticAbilities$ STNoDmg | SubAbility$ DBCleanup | SpellDescription$ Prevent all damage target spell would deal this turn.
+A:AB$ Effect | Cost$ tapXType<2/Wizard> | ValidTgts$ Card | TargetType$ Spell | TgtZone$ Stack | IsCurse$ True | TgtPrompt$ Select target spell | RememberObjects$ TargetedSource | StaticAbilities$ STNoDmg | SubAbility$ DBCleanup | SpellDescription$ Prevent all damage target spell would deal this turn.
 SVar:STNoDmg:Mode$ Continuous | EffectZone$ Command | AffectedZone$ Battlefield,Stack | Affected$ Card.IsRemembered | AddKeyword$ Prevent all damage that would be dealt by CARDNAME. | Description$ Prevent all damage target spell would deal this turn.
 SVar:DBCleanup:DB$ Cleanup | ClearRemembered$ True
 AI:RemoveDeck:All

forge-gui/res/cardsfolder/v/vexing_shusher.txt

@@ -3,6 +3,6 @@ ManaCost:RG RG
 Types:Creature Goblin Shaman
 PT:2/2
 K:This spell can't be countered.
-A:AB$ Pump | Cost$ RG | ValidTgts$ Card.inZoneStack | TgtZone$ Stack | PumpZone$ Stack | KW$ HIDDEN CARDNAME can't be countered. | SpellDescription$ Target spell can't be countered.
+A:AB$ Pump | Cost$ RG | ValidTgts$ Card | TgtZone$ Stack | TargetType$ Spell | PumpZone$ Stack | KW$ HIDDEN CARDNAME can't be countered. | SpellDescription$ Target spell can't be countered.
 AI:RemoveDeck:All
 Oracle:This spell can't be countered.\n{R/G}: Target spell can't be countered.

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

@@ -114,7 +114,7 @@ public class HumanCostDecision extends CostDecisionMakerBase {
         int c = cost.getAbilityAmount(ability);
 
         if (discardType.equals("Random")) {
-            CardCollectionView randomSubset = Aggregates.random(hand, c, new CardCollection());
+            CardCollectionView randomSubset = new CardCollection(Aggregates.random(hand, c));
             if (randomSubset.size() > 1 && ability.getActivatingPlayer() != null) {
                 randomSubset = ability.getActivatingPlayer().getController().orderMoveToZoneList(randomSubset, ZoneType.Graveyard, ability);
             }

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

@@ -477,7 +477,7 @@ public class HumanPlay {
                         return false;
                     }
 
-                    ((CostDiscard)part).payAsDecided(p, PaymentDecision.card(Aggregates.random(p.getCardsIn(ZoneType.Hand), amount, new CardCollection())), sourceAbility, true);
+                    ((CostDiscard)part).payAsDecided(p, PaymentDecision.card(Aggregates.random(p.getCardsIn(ZoneType.Hand), amount)), sourceAbility, true);
                 } else {
                     CardCollectionView list = CardLists.getValidCards(p.getCardsIn(ZoneType.Hand), part.getType(), p, source, sourceAbility);
                     boolean hasPaid = payCostPart(controller, p, sourceAbility, hcd.isEffect(), (CostPartWithList)part, amount, list, Localizer.getInstance().getMessage("lbldiscard") + orString);

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

@@ -298,6 +298,10 @@ public class TargetSelection {
                         // By peeking at stack item, target is set to its SI state. So set it back before adding targets
                         ability.resetTargets();
                     }
+                    // make sure we're not accidentally finding a cast trigger of this card first
+                    if (!abilityOnStack.isSpell()) {
+                        continue;
+                    }
                     if (abilityOnStack.getHostCard().getView().equals(chosen)) {
                         ability.getTargets().add(abilityOnStack);
                         break;