Merge branch 'wrappedAbilityCleanup' into 'master'

WrappedAbility & Trigger: some cleanup

Closes #1400

See merge request core-developers/forge!2841

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

@@ -224,15 +224,13 @@ public class PlayerControllerAi extends PlayerController {
     }
 
     @Override
-    public boolean confirmTrigger(WrappedAbility wrapper, Map<String, String> triggerParams, boolean isMandatory) {
+    public boolean confirmTrigger(WrappedAbility wrapper) {
         final SpellAbility sa = wrapper.getWrappedAbility();
         //final Trigger regtrig = wrapper.getTrigger();
         if (ComputerUtilAbility.getAbilitySourceName(sa).equals("Deathmist Raptor")) {
             return true;
         }
-        if (triggerParams.containsKey("DelayedTrigger") || isMandatory) {
+        if (wrapper.isMandatory()) {
-            //TODO: The only card with an optional delayed trigger is Shirei, Shizo's Caretaker,
-            //      needs to be expanded when a more difficult cards comes up
             return true;
         }
         // Store/replace target choices more properly to get this SA cleared.

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

@@ -46,10 +46,6 @@ public class DelayedTriggerEffect extends SpellAbilityEffect {
 
         final Trigger delTrig = TriggerHandler.parseTrigger(mapParams, sa.getHostCard(), true);
 
-        if (sa.hasParam("CopyTriggeringObjects")) {
-            delTrig.setStoredTriggeredObjects(sa.getTriggeringObjects());
-        }
-
         if (triggerRemembered != null) {
             for (final String rem : triggerRemembered.split(",")) {
                 for (final Object o : AbilityUtils.getDefinedObjects(sa.getHostCard(), rem, sa)) {
@@ -78,6 +74,11 @@ public class DelayedTriggerEffect extends SpellAbilityEffect {
             if (ApiType.SetState == overridingSA.getApi()) {
                 overridingSA.setSVar("StoredTransform", String.valueOf(sa.getHostCard().getTransformedTimestamp()));
             }
+
+            if (sa.hasParam("CopyTriggeringObjects")) {
+                overridingSA.setTriggeringObjects(sa.getTriggeringObjects());
+            }
+
             delTrig.setOverridingAbility(overridingSA);
         }
         final TriggerHandler trigHandler  = sa.getActivatingPlayer().getGame().getTriggerHandler();

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

@@ -47,10 +47,6 @@ public class ImmediateTriggerEffect extends SpellAbilityEffect {
 
         final Trigger immediateTrig = TriggerHandler.parseTrigger(mapParams, sa.getHostCard(), sa.isIntrinsic());
 
-        if (sa.hasParam("CopyTriggeringObjects")) {
-            immediateTrig.setStoredTriggeredObjects(sa.getTriggeringObjects());
-        }
-
         // Need to copy paid costs
 
         if (triggerRemembered != null) {
@@ -73,6 +69,11 @@ public class ImmediateTriggerEffect extends SpellAbilityEffect {
         if (mapParams.containsKey("Execute") || sa.hasAdditionalAbility("Execute")) {
             SpellAbility overridingSA = sa.getAdditionalAbility("Execute");
             overridingSA.setActivatingPlayer(sa.getActivatingPlayer());
+
+            if (sa.hasParam("CopyTriggeringObjects")) {
+                overridingSA.setTriggeringObjects(sa.getTriggeringObjects());
+            }
+
             immediateTrig.setOverridingAbility(overridingSA);
         }
         final TriggerHandler trigHandler  = sa.getActivatingPlayer().getGame().getTriggerHandler();

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

@@ -27,7 +27,6 @@ import forge.card.mana.ManaCost;
 import forge.game.CardTraitBase;
 import forge.game.Game;
 import forge.game.ability.AbilityFactory;
-import forge.game.ability.AbilityUtils;
 import forge.game.cost.Cost;
 import forge.game.player.Player;
 import forge.game.replacement.ReplacementEffect;
@@ -194,8 +193,8 @@ public class CardFactory {
         }
 
         final SpellAbility copySA;
-        if (sa.isTrigger()) {
+        if (sa.isTrigger() && sa.isWrapper()) {
-            copySA = getCopiedTriggeredAbility(sa);
+            copySA = getCopiedTriggeredAbility((WrappedAbility)sa, c);
         } else {
             copySA = sa.copy(c, false);
         }
@@ -214,10 +213,6 @@ public class CardFactory {
         if (!copySA.isTrigger()) {
             copySA.setPayCosts(new Cost("", sa.isAbility()));
         }
-        if (sa.getTargetRestrictions() != null) {
-            TargetRestrictions target = new TargetRestrictions(sa.getTargetRestrictions());
-            copySA.setTargetRestrictions(target);
-        }
         copySA.setActivatingPlayer(controller);
 
         if (bCopyDetails) {
@@ -587,49 +582,12 @@ public class CardFactory {
      *
      * return a wrapped ability
      */
-    public static SpellAbility getCopiedTriggeredAbility(final SpellAbility sa) {
+    public static SpellAbility getCopiedTriggeredAbility(final WrappedAbility sa, final Card newHost) {
         if (!sa.isTrigger()) {
             return null;
         }
-        // Find trigger
-        Trigger t = null;
-        if (sa.isWrapper()) {
-            // copy trigger?
-            t = sa.getTrigger();
-        } else { // some keyword ability, e.g. Exalted, Annihilator
-            return sa.copy();
-        }
-        // set up copied wrapped ability
-        SpellAbility trig = t.getOverridingAbility();
-        if (trig == null) {
-            trig = AbilityFactory.getAbility(sa.getHostCard().getSVar(t.getParam("Execute")), sa.getHostCard());
-        }
-        trig.setHostCard(sa.getHostCard());
-        trig.setTrigger(true);
-        trig.setSourceTrigger(t.getId());
-        sa.setTriggeringObjects(sa.getTriggeringObjects());
-        trig.setTriggerRemembered(t.getTriggerRemembered());
-        if (t.getStoredTriggeredObjects() != null) {
-            trig.setTriggeringObjects(t.getStoredTriggeredObjects());
-        }
 
-        trig.setActivatingPlayer(sa.getActivatingPlayer());
+        return new WrappedAbility(sa.getTrigger(), sa.getWrappedAbility().copy(newHost, false), sa.getDecider());
-        if (t.hasParam("TriggerController")) {
-            Player p = AbilityUtils.getDefinedPlayers(t.getHostCard(), t.getParam("TriggerController"), trig).get(0);
-            trig.setActivatingPlayer(p);
-        }
-
-        if (t.hasParam("RememberController")) {
-            sa.getHostCard().addRemembered(sa.getActivatingPlayer());
-        }
-
-        trig.setStackDescription(trig.toString());
-
-        WrappedAbility wrapperAbility = new WrappedAbility(t, trig, ((WrappedAbility) sa).getDecider());
-        wrapperAbility.setTrigger(true);
-        wrapperAbility.setMandatory(sa.isMandatory());
-        wrapperAbility.setDescription(wrapperAbility.getStackDescription());
-        return wrapperAbility;
     }
 
     public static CardCloneStates getCloneStates(final Card in, final Card out, final CardTraitBase sa) {

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

@@ -122,7 +122,7 @@ public abstract class PlayerController {
     public abstract boolean confirmAction(SpellAbility sa, PlayerActionConfirmMode mode, String message);
     public abstract boolean confirmBidAction(SpellAbility sa, PlayerActionConfirmMode bidlife, String string, int bid, Player winner);
     public abstract boolean confirmStaticApplication(Card hostCard, GameEntity affected, String logic, String message);
-    public abstract boolean confirmTrigger(WrappedAbility sa, Map<String, String> triggerParams, boolean isMandatory);
+    public abstract boolean confirmTrigger(WrappedAbility sa);
     public abstract Player chooseStartingPlayer(boolean isFirstGame);
 
     public abstract CardCollection orderBlockers(Card attacker, CardCollection blockers);

forge-game/src/main/java/forge/game/spellability/SpellAbility.java

@@ -998,7 +998,7 @@ public abstract class SpellAbility extends CardTraitBase implements ISpellAbilit
     }
 
     public boolean isMandatory() {
-        return false;
+        return isTrigger() && !isOptionalTrigger();
     }
 
     public final boolean canTarget(final GameObject entity) {

forge-game/src/main/java/forge/game/trigger/Trigger.java

@@ -66,40 +66,12 @@ public abstract class Trigger extends TriggerReplacementBase {
 
     private TriggerType mode;
 
-    private Map<AbilityKey, Object> storedTriggeredObjects = null;
-    
     private List<Object> triggerRemembered = Lists.newArrayList();
 
     // number of times this trigger was activated this this turn
     // used to handle once-per-turn triggers like Crawling Sensation
     private int numberTurnActivations = 0;
 
-    /**
-     * <p>
-     * Setter for the field <code>storedTriggeredObjects</code>.
-     * </p>
-     * 
-     * @param storedTriggeredObjects
-     *            a {@link java.util.HashMap} object.
-     * @since 1.0.15
-     */
-    public final void setStoredTriggeredObjects(final Map<AbilityKey, Object> storedTriggeredObjects) {
-        this.storedTriggeredObjects = AbilityKey.newMap(storedTriggeredObjects);
-    }
-
-    /**
-     * <p>
-     * Getter for the field <code>storedTriggeredObjects</code>.
-     * </p>
-     * 
-     * @return a {@link java.util.HashMap} object.
-     * @since 1.0.15
-     */
-    public final Map<AbilityKey, Object> getStoredTriggeredObjects() {
-        return this.storedTriggeredObjects;
-    }
-
-
     private Set<PhaseType> validPhases;
 
     /**

forge-game/src/main/java/forge/game/trigger/TriggerHandler.java

@@ -17,7 +17,6 @@
  */
 package forge.game.trigger;
 
-import forge.card.mana.ManaCost;
 import forge.game.Game;
 import forge.game.GlobalRuleChange;
 import forge.game.ability.AbilityFactory;
@@ -32,11 +31,9 @@ import forge.game.card.CardZoneTable;
 import forge.game.keyword.KeywordInterface;
 import forge.game.phase.PhaseType;
 import forge.game.player.Player;
-import forge.game.spellability.Ability;
 import forge.game.spellability.AbilitySub;
 import forge.game.spellability.SpellAbility;
 import forge.game.spellability.SpellAbilityStackInstance;
-import forge.game.spellability.TargetRestrictions;
 import forge.game.zone.Zone;
 import forge.game.zone.ZoneType;
 import forge.util.FileSection;
@@ -528,11 +525,7 @@ public class TriggerHandler {
         sa = regtrig.getOverridingAbility();
         if (sa == null) {
             if (!regtrig.hasParam("Execute")) {
-                sa = new Ability(host, ManaCost.ZERO) {
+                sa = new SpellAbility.EmptySa(host);
-                    @Override
-                    public void resolve() {
-                    }
-                };
             }
             else {
                 String name = regtrig.getParam("Execute");
@@ -563,9 +556,6 @@ public class TriggerHandler {
         sa.setSourceTrigger(regtrig.getId());
         regtrig.setTriggeringObjects(sa, runParams);
         sa.setTriggerRemembered(regtrig.getTriggerRemembered());
-        if (regtrig.getStoredTriggeredObjects() != null) {
-            sa.setTriggeringObjects(regtrig.getStoredTriggeredObjects());
-        }
 
         if (sa.getDeltrigActivatingPlayer() != null) {
             // make sure that the original delayed trigger activator is restored
@@ -591,33 +581,20 @@ public class TriggerHandler {
         }
 
         Player decider = null;
-        boolean mand = false;
+        boolean isMandatory = false;
         if (regtrig.hasParam("OptionalDecider")) {
             sa.setOptionalTrigger(true);
             decider = AbilityUtils.getDefinedPlayers(host, regtrig.getParam("OptionalDecider"), sa).get(0);
         }
         else if (sa instanceof AbilitySub || !sa.hasParam("Cost") || sa.getParam("Cost").equals("0")) {
-            mand = true;
+            isMandatory = true;
         }
         else { // triggers with a cost can't be mandatory
             sa.setOptionalTrigger(true);
             decider = sa.getActivatingPlayer();
         }
 
-        SpellAbility ability = sa;
-        while (ability != null) {
-            final TargetRestrictions tgt = ability.getTargetRestrictions();
-
-            if (tgt != null) {
-                tgt.setMandatory(true);
-            }
-            ability = ability.getSubAbility();
-        }
-        final boolean isMandatory = mand;
-
         final WrappedAbility wrapperAbility = new WrappedAbility(regtrig, sa, decider);
-        wrapperAbility.setTrigger(true);
-        wrapperAbility.setMandatory(isMandatory);
         //wrapperAbility.setDescription(wrapperAbility.getStackDescription());
         //wrapperAbility.setDescription(wrapperAbility.toUnsuppressedString());
 

forge-game/src/main/java/forge/game/trigger/WrappedAbility.java

@@ -33,8 +33,9 @@ public class WrappedAbility extends Ability {
     boolean mandatory = false;
 
     public WrappedAbility(final Trigger regtrig0, final SpellAbility sa0, final Player decider0) {
-        super(regtrig0.getHostCard(), ManaCost.ZERO, sa0.getView());
+        super(sa0.getHostCard(), ManaCost.ZERO, sa0.getView());
         setTrigger(regtrig0);
+        setTrigger(true);
         sa = sa0;
         decider = decider0;
         sa.setDescription(this.getStackDescription());
@@ -53,18 +54,6 @@ public class WrappedAbility extends Ability {
         return decider;
     }
 
-    public final void setMandatory(final boolean mand) {
-        this.mandatory = mand;
-    }
-
-    /**
-     * @return the mandatory
-     */
-    @Override
-    public boolean isMandatory() {
-        return mandatory;
-    }
-
     @Override
     public String getParam(String key) { return sa.getParam(key); }
 
@@ -214,7 +203,7 @@ public class WrappedAbility extends Ability {
     @Override
     public String toUnsuppressedString() {
         String desc = this.getStackDescription(); /* use augmented stack description as string for wrapped things */
-        String card = getTrigger().getHostCard().toString();
+        String card = getHostCard().toString();
         if ( !desc.contains(card) && desc.contains(" this ")) { /* a hack for Evolve and similar that don't have CARDNAME */
                 return card + ": " + desc;
         } else return desc;
@@ -446,9 +435,8 @@ public class WrappedAbility extends Ability {
     public void resolve() {
         final Game game = sa.getActivatingPlayer().getGame();
         final Trigger regtrig = getTrigger();
-        Map<String, String> triggerParams = regtrig.getMapParams();
 
-        if (!(regtrig instanceof TriggerAlways) && !triggerParams.containsKey("NoResolvingCheck")) {
+        if (!(regtrig instanceof TriggerAlways) && !regtrig.hasParam("NoResolvingCheck")) {
             // Most State triggers don't have "Intervening If"
             if (!regtrig.requirementsCheck(game)) {
                 return;
@@ -460,10 +448,10 @@ public class WrappedAbility extends Ability {
             }
         }
 
-        if (triggerParams.containsKey("ResolvingCheck")) {
+        if (regtrig.hasParam("ResolvingCheck")) {
             // rare cases: Hidden Predators (state trigger, but have "Intervening If" to check IsPresent2) etc.
             Map<String, String> recheck = new HashMap<>();
-            String key = triggerParams.get("ResolvingCheck");
+            String key = regtrig.getParam("ResolvingCheck");
             String value = regtrig.getParam(key);
             recheck.put(key, value);
             if (!meetsCommonRequirements(recheck)) {
@@ -471,29 +459,18 @@ public class WrappedAbility extends Ability {
             }
         }
 
-        TriggerHandler th = game.getTriggerHandler();
-
         // set Trigger
         sa.setTrigger(regtrig);
 
-        if (decider != null && !decider.getController().confirmTrigger(this, triggerParams, this.isMandatory())) {
+        if (decider != null && !decider.getController().confirmTrigger(this)) {
             return;
         }
 
-        if (!triggerParams.containsKey("NoTimestampCheck")) {
+        if (!regtrig.hasParam("NoTimestampCheck")) {
             timestampCheck();
         }
 
         getActivatingPlayer().getController().playSpellAbilityNoStack(sa, false);
-
-        // Add eventual delayed trigger.
-        if (triggerParams.containsKey("DelayedTrigger")) {
-            final String sVarName = triggerParams.get("DelayedTrigger");
-            final Trigger deltrig = TriggerHandler.parseTrigger(regtrig.getHostCard().getSVar(sVarName),
-                    regtrig.getHostCard(), true);
-            deltrig.setStoredTriggeredObjects(this.getTriggeringObjects());
-            th.registerDelayedTrigger(deltrig);
-        }
     }
 
     /**

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

@@ -197,7 +197,7 @@ public class PlayerControllerForTests extends PlayerController {
     }
 
     @Override
-    public boolean confirmTrigger(WrappedAbility wrapper, Map<String, String> triggerParams, boolean isMandatory) {
+    public boolean confirmTrigger(WrappedAbility wrapper) {
         return true;
     }
 

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

@@ -622,8 +622,7 @@ public class PlayerControllerHuman extends PlayerController implements IGameCont
     }
 
     @Override
-    public boolean confirmTrigger(final WrappedAbility wrapper, final Map<String, String> triggerParams,
+    public boolean confirmTrigger(final WrappedAbility wrapper) {
-            final boolean isMandatory) {
         final SpellAbility sa = wrapper.getWrappedAbility();
         final Trigger regtrig = wrapper.getTrigger();
         if (getGui().shouldAlwaysAcceptTrigger(regtrig.getId())) {
@@ -645,8 +644,7 @@ public class PlayerControllerHuman extends PlayerController implements IGameCont
             // append trigger description unless prompt is compact or detailed
             // descriptions are on
             buildQuestion.append("\n(");
-            buildQuestion.append(TextUtil.fastReplace(triggerParams.get("TriggerDescription"),
+            buildQuestion.append(regtrig.toString());
-                    "CARDNAME", regtrig.getHostCard().getName()));
             buildQuestion.append(")");
         }
         final Map<AbilityKey, Object> tos = sa.getTriggeringObjects();

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

@@ -65,6 +65,10 @@ public class TargetSelection {
 
     private boolean bTargetingDone = false;
 
+    private boolean isMandatory() {
+        return ability.isMandatory() || getTgt().getMandatory();
+    }
+
     public final boolean chooseTargets(Integer numTargets) {
         final TargetRestrictions tgt = getTgt();
         final boolean canTarget = tgt != null && tgt.doesTarget();
@@ -97,13 +101,13 @@ public class TargetSelection {
             // Cancel ability if there aren't any valid Candidates
             return false;
         }
-        if (tgt.getMandatory() && !hasCandidates && hasEnoughTargets) {
+        if (isMandatory() && !hasCandidates && hasEnoughTargets) {
             // Mandatory target selection, that has no candidates but enough targets (Min == 0, but no choices)
             return true;
         }
 
         final List<ZoneType> zones = tgt.getZone();
-        final boolean mandatory = tgt.getMandatory() && hasCandidates;
+        final boolean mandatory = isMandatory() && hasCandidates;
 
         final boolean choiceResult;
         final boolean random = tgt.isRandomTarget();