From be2336102635aa788409ea5cbcf4a877ca2f320d Mon Sep 17 00:00:00 2001 From: jje4th Date: Tue, 3 May 2016 17:25:13 +0000 Subject: [PATCH] A fix for OncePerEffect script ability applying too much when triggered by costs. Affects The Gitrog Monster and Sidisi, Brood Tyrant. Before the fix if multiple lands are discarded/sacrificed to a single cost (e.g Keldon Arzonist, Zombie Infestatio, etc) it would trigger the Girtog Monster's card draw ability multiple times. (Similar for Sidisi milling multiple creatures as part of a cost. Don't know a card that has mill multiple cards as a cost.) It implements a new stack for tracking cost payments and a wrapper to track the cost payment instance. Change zone triggers use this information to de-dupe multiple triggers firing from the same cost. --- .gitattributes | 2 + forge-game/src/main/java/forge/game/Game.java | 19 ++----- .../src/main/java/forge/game/GameAction.java | 1 + .../java/forge/game/cost/CostPayment.java | 24 +++++++-- .../cost/IndividualCostPaymentInstance.java | 21 ++++++++ .../game/trigger/TriggerChangesZone.java | 50 ++++++++++++++++--- .../forge/game/zone/CostPaymentStack.java | 37 ++++++++++++++ 7 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 forge-game/src/main/java/forge/game/cost/IndividualCostPaymentInstance.java create mode 100644 forge-game/src/main/java/forge/game/zone/CostPaymentStack.java diff --git a/.gitattributes b/.gitattributes index d8eba6a5f8f..a277cddcabe 100644 --- a/.gitattributes +++ b/.gitattributes @@ -519,6 +519,7 @@ forge-game/src/main/java/forge/game/cost/CostUnattach.java -text forge-game/src/main/java/forge/game/cost/CostUntap.java -text forge-game/src/main/java/forge/game/cost/CostUntapType.java -text forge-game/src/main/java/forge/game/cost/ICostVisitor.java -text +forge-game/src/main/java/forge/game/cost/IndividualCostPaymentInstance.java -text forge-game/src/main/java/forge/game/cost/PaymentDecision.java -text forge-game/src/main/java/forge/game/cost/package-info.java svneol=native#text/plain forge-game/src/main/java/forge/game/event/Event.java -text @@ -719,6 +720,7 @@ forge-game/src/main/java/forge/game/trigger/TriggerVote.java -text forge-game/src/main/java/forge/game/trigger/TriggerWaiting.java -text forge-game/src/main/java/forge/game/trigger/WrappedAbility.java -text forge-game/src/main/java/forge/game/trigger/package-info.java svneol=native#text/plain +forge-game/src/main/java/forge/game/zone/CostPaymentStack.java -text forge-game/src/main/java/forge/game/zone/MagicStack.java svneol=native#text/plain forge-game/src/main/java/forge/game/zone/PlayerZone.java svneol=native#text/plain forge-game/src/main/java/forge/game/zone/PlayerZoneBattlefield.java svneol=native#text/plain diff --git a/forge-game/src/main/java/forge/game/Game.java b/forge-game/src/main/java/forge/game/Game.java index 760ab13f64f..f4d546eeec2 100644 --- a/forge-game/src/main/java/forge/game/Game.java +++ b/forge-game/src/main/java/forge/game/Game.java @@ -17,16 +17,7 @@ */ package forge.game; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Random; -import java.util.Set; -import java.util.TreeMap; +import java.util.*; import com.google.common.base.Predicate; import com.google.common.base.Predicates; @@ -46,6 +37,7 @@ import forge.game.card.CardUtil; import forge.game.card.CardView; import forge.game.combat.Combat; import forge.game.cost.Cost; +import forge.game.cost.IndividualCostPaymentInstance; import forge.game.event.Event; import forge.game.event.GameEventGameOutcome; import forge.game.phase.Phase; @@ -64,16 +56,12 @@ import forge.game.spellability.SpellAbilityStackInstance; import forge.game.spellability.SpellAbilityView; import forge.game.trigger.TriggerHandler; import forge.game.trigger.TriggerType; -import forge.game.zone.MagicStack; -import forge.game.zone.PlayerZone; -import forge.game.zone.Zone; -import forge.game.zone.ZoneType; +import forge.game.zone.*; import forge.trackable.Tracker; import forge.util.Aggregates; import forge.util.collect.FCollection; import forge.util.collect.FCollectionView; import forge.util.Visitor; -import java.util.HashMap; /** * Represents the state of a single game, a new instance is created for each game. @@ -91,6 +79,7 @@ public class Game { public final Untap untap; public final Upkeep upkeep; public final MagicStack stack; + public final CostPaymentStack costPaymentStack = new CostPaymentStack(); private final PhaseHandler phaseHandler; private final StaticEffects staticEffects = new StaticEffects(); private final TriggerHandler triggerHandler = new TriggerHandler(this); diff --git a/forge-game/src/main/java/forge/game/GameAction.java b/forge-game/src/main/java/forge/game/GameAction.java index ade1814ad31..488b56d4a08 100644 --- a/forge-game/src/main/java/forge/game/GameAction.java +++ b/forge-game/src/main/java/forge/game/GameAction.java @@ -288,6 +288,7 @@ public class GameAction { runParams.put("Origin", zoneFrom != null ? zoneFrom.getZoneType().name() : null); runParams.put("Destination", zoneTo.getZoneType().name()); runParams.put("SpellAbilityStackInstance", game.stack.peek()); + runParams.put("IndividualCostPaymentInstance", game.costPaymentStack.peek()); game.getTriggerHandler().runTrigger(TriggerType.ChangesZone, runParams, false); if (zoneFrom != null && zoneFrom.is(ZoneType.Battlefield)) { final HashMap runParams2 = new HashMap(); diff --git a/forge-game/src/main/java/forge/game/cost/CostPayment.java b/forge-game/src/main/java/forge/game/cost/CostPayment.java index 9830454a69e..b9e0c3cb8af 100644 --- a/forge-game/src/main/java/forge/game/cost/CostPayment.java +++ b/forge-game/src/main/java/forge/game/cost/CostPayment.java @@ -134,12 +134,18 @@ public class CostPayment { public boolean payCost(final CostDecisionMakerBase decisionMaker) { final List costParts = this.getCost().getCostPartsWithZeroMana(); for (final CostPart part : costParts) { + // Wrap the cost and push onto the cost stack + decisionMaker.getPlayer().getGame().costPaymentStack.push(new IndividualCostPaymentInstance(part)); + PaymentDecision pd = part.accept(decisionMaker); if (pd == null || !part.payAsDecided(decisionMaker.getPlayer(), pd, ability)) { + decisionMaker.getPlayer().getGame().costPaymentStack.pop(); // cost is resolved return false; } this.paidCostParts.add(part); + + decisionMaker.getPlayer().getGame().costPaymentStack.pop(); // cost is resolved } // this clears lists used for undo. @@ -148,6 +154,7 @@ public class CostPayment { ((CostPartWithList) part1).resetLists(); } } + return true; } @@ -164,21 +171,32 @@ public class CostPayment { for (final CostPart part : this.cost.getCostParts()) { PaymentDecision decision = part.accept(decisionMaker); if (null == decision) return false; - - if (decisionMaker.paysRightAfterDecision() && !part.payAsDecided(decisionMaker.getPlayer(), decision, ability)) + + // wrap the payment and push onto the cost stack + decisionMaker.getPlayer().getGame().costPaymentStack.push(new IndividualCostPaymentInstance(part)); + if (decisionMaker.paysRightAfterDecision() && !part.payAsDecided(decisionMaker.getPlayer(), decision, ability)) { + decisionMaker.getPlayer().getGame().costPaymentStack.pop(); // cost is resolved return false; - + } + + decisionMaker.getPlayer().getGame().costPaymentStack.pop(); // cost is either paid or deferred decisions.put(part, decision); } for (final CostPart part : this.cost.getCostParts()) { + // wrap the payment and push onto the cost stack + decisionMaker.getPlayer().getGame().costPaymentStack.push(new IndividualCostPaymentInstance(part)); + if (!part.payAsDecided(decisionMaker.getPlayer(), decisions.get(part), this.ability)) { + decisionMaker.getPlayer().getGame().costPaymentStack.pop(); // cost is resolved return false; } // abilities care what was used to pay for them if( part instanceof CostPartWithList ) { ((CostPartWithList) part).resetLists(); } + + decisionMaker.getPlayer().getGame().costPaymentStack.pop(); // cost is resolved } return true; } diff --git a/forge-game/src/main/java/forge/game/cost/IndividualCostPaymentInstance.java b/forge-game/src/main/java/forge/game/cost/IndividualCostPaymentInstance.java new file mode 100644 index 00000000000..ab99faac732 --- /dev/null +++ b/forge-game/src/main/java/forge/game/cost/IndividualCostPaymentInstance.java @@ -0,0 +1,21 @@ +package forge.game.cost; + +import forge.game.IIdentifiable; + +public class IndividualCostPaymentInstance implements IIdentifiable { + private static int maxId = 0; + private static int nextId() { return ++maxId; } + + private final int id; + private final CostPart cost; + + public IndividualCostPaymentInstance(final CostPart cost) { + id = nextId(); + this.cost = cost; + } + + public int getId() { return id; } + + public CostPart getCost() { return cost; } + +} diff --git a/forge-game/src/main/java/forge/game/trigger/TriggerChangesZone.java b/forge-game/src/main/java/forge/game/trigger/TriggerChangesZone.java index 9629a8f1981..028af3ea97f 100644 --- a/forge-game/src/main/java/forge/game/trigger/TriggerChangesZone.java +++ b/forge-game/src/main/java/forge/game/trigger/TriggerChangesZone.java @@ -20,11 +20,15 @@ package forge.game.trigger; import forge.game.ability.AbilityUtils; import forge.game.card.Card; import forge.game.card.CardFactoryUtil; +import forge.game.cost.CostPayment; +import forge.game.cost.IndividualCostPaymentInstance; import forge.game.spellability.SpellAbility; import forge.game.spellability.SpellAbilityStackInstance; import forge.util.Expressions; +import java.util.HashSet; import java.util.Map; +import java.util.Set; /** *

@@ -36,6 +40,10 @@ import java.util.Map; */ public class TriggerChangesZone extends Trigger { + // stores the costs when this trigger has already been run (to prevent multiple card draw triggers for single + // discard event of multiple cards on the Gitrog Moster for instance) + private Set processedCostEffects = new HashSet(); + /** *

* Constructor for Trigger_ChangesZone. @@ -131,14 +139,33 @@ public class TriggerChangesZone extends Trigger { if (this.mapParams.containsKey("OncePerEffect")) { // A "once per effect" trigger will only trigger once regardless of how many things the effect caused - // to change zones. The SpellAbilityStackInstance keeps track of which host cards with "OncePerEffect" - // triggers already fired as a result of that effect. - // - // TODO This isn't quite ideal, since it really should be keeping track of the SpellAbility of the host - // card, rather than keeping track of the host card itself - but it's good enough for now - since there are - // no cards with multiple different OncePerEffect triggers. - SpellAbilityStackInstance si = (SpellAbilityStackInstance) runParams2.get("SpellAbilityStackInstance"); - return si == null || si.attemptOncePerEffectTrigger(this.getHostCard()); + // to change zones. + + // check if this is triggered by a cost payment & only fire if it isn't a duplicate trigger + IndividualCostPaymentInstance currentPayment = (IndividualCostPaymentInstance) runParams2.get("IndividualCostPaymentInstance"); + if (currentPayment != null) { // only if there is an active cost + + // each cost in a payment can trigger the effect for example Sinsiter Concoction has five costs: + // {B}, Pay one life, Mill a card, Discard a Card, and sacrifice Sinister Concoction + // If you mill a land and discard a land, The Gitrog Moster should trigger twice since each of these + // costs is an independent action + // however, due to forge implementation multiple triggers may be created for a single cost. For example, + // Zombie Infestation has a cost of "Discard two cards". If you discard two lands, The Gitrog Moster + // should only trigger once because discarding two lands is a single action. + return this.processedCostEffects.add(currentPayment.getId()); + } + // otherwise use the stack ability + else { + // The SpellAbilityStackInstance keeps track of which host cards with "OncePerEffect" + // triggers already fired as a result of that effect. + // TODO This isn't quite ideal, since it really should be keeping track of the SpellAbility of the host + // card, rather than keeping track of the host card itself - but it's good enough for now - since there + // are no cards with multiple different OncePerEffect triggers. + SpellAbilityStackInstance si = (SpellAbilityStackInstance) runParams2.get("SpellAbilityStackInstance"); + + // si == null means the stack is empty + return si == null || si.attemptOncePerEffectTrigger(this.getHostCard()); + } } /* this trigger can only be activated once per turn, verify it hasn't already run */ @@ -161,4 +188,11 @@ public class TriggerChangesZone extends Trigger { sb.append("Zone Changer: ").append(sa.getTriggeringObject("Card")); return sb.toString(); } + + @Override + // Resets the state stored each turn for per-instance restriction + public void resetTurnState() { + super.resetTurnState(); + this.processedCostEffects.clear(); + } } diff --git a/forge-game/src/main/java/forge/game/zone/CostPaymentStack.java b/forge-game/src/main/java/forge/game/zone/CostPaymentStack.java new file mode 100644 index 00000000000..91e32f20e5c --- /dev/null +++ b/forge-game/src/main/java/forge/game/zone/CostPaymentStack.java @@ -0,0 +1,37 @@ +package forge.game.zone; + +import forge.game.cost.IndividualCostPaymentInstance; + +import java.util.Stack; + +/* + * simple stack wrapper class for tracking cost payments (mainly for triggers to use) + */ +public class CostPaymentStack { + + private Stack stack; + + public CostPaymentStack() { + stack = new Stack(); + } + + public IndividualCostPaymentInstance push(IndividualCostPaymentInstance costPaymentInstance) { + return stack.push(costPaymentInstance); + } + + public IndividualCostPaymentInstance pop(){ + return stack.pop(); + } + + public IndividualCostPaymentInstance peek() { + if (stack.empty()) { + return null; + } + + return stack.peek(); + } + + public void clear() { + stack.clear(); + } +}