From 83f7ae377ae43df8aba79113c02a138ba1f76826 Mon Sep 17 00:00:00 2001 From: Oleg Agafonov Date: Thu, 6 Aug 2020 22:17:37 +0400 Subject: [PATCH] Refactored restore state code, added additional error check for mana undo param; --- Mage.Sets/src/mage/cards/m/Metalworker.java | 6 ++-- .../src/mage/cards/s/SacellumGodspeaker.java | 4 ++- .../abilities/costs/mana/ManaCostsImpl.java | 15 +++++---- .../effects/common/DoIfCostPaid.java | 2 +- .../effects/common/DoWhenCostPaid.java | 2 +- .../keyword/CumulativeUpkeepAbility.java | 2 +- .../mana/ActivatedManaAbilityImpl.java | 17 ++++------ Mage/src/main/java/mage/cards/CardImpl.java | 32 ++++++++++++++----- Mage/src/main/java/mage/game/GameImpl.java | 8 ++++- Mage/src/main/java/mage/players/Player.java | 7 ++++ .../main/java/mage/players/PlayerImpl.java | 7 ---- Mage/src/main/java/mage/util/ManaUtil.java | 2 +- 12 files changed, 64 insertions(+), 40 deletions(-) diff --git a/Mage.Sets/src/mage/cards/m/Metalworker.java b/Mage.Sets/src/mage/cards/m/Metalworker.java index 5a363c383da..ad178780970 100644 --- a/Mage.Sets/src/mage/cards/m/Metalworker.java +++ b/Mage.Sets/src/mage/cards/m/Metalworker.java @@ -35,8 +35,10 @@ public final class Metalworker extends CardImpl { this.power = new MageInt(1); this.toughness = new MageInt(2); - // {tap}: Reveal any number of artifact cards in your hand. Add {C}{C} for each card revealed this way. - this.addAbility(new SimpleManaAbility(Zone.BATTLEFIELD, new MetalworkerManaEffect(), new TapSourceCost())); + // {T}: Reveal any number of artifact cards in your hand. Add {C}{C} for each card revealed this way. + SimpleManaAbility ability = new SimpleManaAbility(Zone.BATTLEFIELD, new MetalworkerManaEffect(), new TapSourceCost()); + ability.setUndoPossible(false); + this.addAbility(ability); } public Metalworker(final Metalworker card) { diff --git a/Mage.Sets/src/mage/cards/s/SacellumGodspeaker.java b/Mage.Sets/src/mage/cards/s/SacellumGodspeaker.java index dae62820251..bed2265a6fc 100644 --- a/Mage.Sets/src/mage/cards/s/SacellumGodspeaker.java +++ b/Mage.Sets/src/mage/cards/s/SacellumGodspeaker.java @@ -33,7 +33,9 @@ public final class SacellumGodspeaker extends CardImpl { this.toughness = new MageInt(2); // {T}: Reveal any number of creature cards with power 5 or greater from your hand. Add {G} for each card revealed this way. - this.addAbility(new SimpleManaAbility(Zone.BATTLEFIELD, new SacellumGodspeakerEffect(), new TapSourceCost())); + SimpleManaAbility ability = new SimpleManaAbility(Zone.BATTLEFIELD, new SacellumGodspeakerEffect(), new TapSourceCost()); + ability.setUndoPossible(false); + this.addAbility(ability); } public SacellumGodspeaker(final SacellumGodspeaker card) { diff --git a/Mage/src/main/java/mage/abilities/costs/mana/ManaCostsImpl.java b/Mage/src/main/java/mage/abilities/costs/mana/ManaCostsImpl.java index 7618ad20251..9b2848230ea 100644 --- a/Mage/src/main/java/mage/abilities/costs/mana/ManaCostsImpl.java +++ b/Mage/src/main/java/mage/abilities/costs/mana/ManaCostsImpl.java @@ -150,13 +150,16 @@ public class ManaCostsImpl extends ArrayList implements M */ @Override public boolean payOrRollback(Ability ability, Game game, UUID sourceId, UUID payingPlayerId) { - int bookmark = game.bookmarkState(); - handlePhyrexianManaCosts(payingPlayerId, ability, game); - if (pay(ability, game, sourceId, payingPlayerId, false, null)) { - game.removeBookmark(bookmark); - return true; + Player player = game.getPlayer(payingPlayerId); + if (player != null) { + int bookmark = game.bookmarkState(); + handlePhyrexianManaCosts(payingPlayerId, ability, game); + if (pay(ability, game, sourceId, payingPlayerId, false, null)) { + game.removeBookmark(bookmark); + return true; + } + player.restoreState(bookmark, ability.getRule(), game); } - game.restoreState(bookmark, ability.getRule()); return false; } diff --git a/Mage/src/main/java/mage/abilities/effects/common/DoIfCostPaid.java b/Mage/src/main/java/mage/abilities/effects/common/DoIfCostPaid.java index 1d95942ebc4..7f6d13dc605 100644 --- a/Mage/src/main/java/mage/abilities/effects/common/DoIfCostPaid.java +++ b/Mage/src/main/java/mage/abilities/effects/common/DoIfCostPaid.java @@ -108,7 +108,7 @@ public class DoIfCostPaid extends OneShotEffect { player.resetStoredBookmark(game); // otherwise you can e.g. undo card drawn with Mentor of the Meek } else { // Paying cost was cancels so try to undo payment so far - game.restoreState(bookmark, DoIfCostPaid.class.getName()); + player.restoreState(bookmark, DoIfCostPaid.class.getName(), game); if (!otherwiseEffects.isEmpty()) { for (Effect effect : otherwiseEffects) { effect.setTargetPointer(this.targetPointer); diff --git a/Mage/src/main/java/mage/abilities/effects/common/DoWhenCostPaid.java b/Mage/src/main/java/mage/abilities/effects/common/DoWhenCostPaid.java index 71602a0099e..0697ec3beca 100644 --- a/Mage/src/main/java/mage/abilities/effects/common/DoWhenCostPaid.java +++ b/Mage/src/main/java/mage/abilities/effects/common/DoWhenCostPaid.java @@ -60,7 +60,7 @@ public class DoWhenCostPaid extends OneShotEffect { player.resetStoredBookmark(game); return true; } - game.restoreState(bookmark, DoWhenCostPaid.class.getName()); + player.restoreState(bookmark, DoWhenCostPaid.class.getName(), game); return true; } diff --git a/Mage/src/main/java/mage/abilities/keyword/CumulativeUpkeepAbility.java b/Mage/src/main/java/mage/abilities/keyword/CumulativeUpkeepAbility.java index 6436544db8a..cafd1a867b1 100644 --- a/Mage/src/main/java/mage/abilities/keyword/CumulativeUpkeepAbility.java +++ b/Mage/src/main/java/mage/abilities/keyword/CumulativeUpkeepAbility.java @@ -103,7 +103,7 @@ class CumulativeUpkeepEffect extends OneShotEffect { game.fireEvent(new GameEvent(EventType.PAID_CUMULATIVE_UPKEEP, permanent.getId(), permanent.getId(), player.getId(), ageCounter, false)); return true; } else { - game.restoreState(bookmark, source.getRule()); + player.restoreState(bookmark, source.getRule(), game); } } game.fireEvent(new GameEvent(EventType.DIDNT_PAY_CUMULATIVE_UPKEEP, permanent.getId(), permanent.getId(), player.getId(), ageCounter, false)); diff --git a/Mage/src/main/java/mage/abilities/mana/ActivatedManaAbilityImpl.java b/Mage/src/main/java/mage/abilities/mana/ActivatedManaAbilityImpl.java index a2aa47149f6..46b9a085c7f 100644 --- a/Mage/src/main/java/mage/abilities/mana/ActivatedManaAbilityImpl.java +++ b/Mage/src/main/java/mage/abilities/mana/ActivatedManaAbilityImpl.java @@ -1,25 +1,17 @@ package mage.abilities.mana; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; -import java.util.UUID; import mage.Mana; -import mage.abilities.Ability; import mage.abilities.ActivatedAbilityImpl; import mage.abilities.costs.Cost; import mage.abilities.effects.Effect; import mage.abilities.effects.common.ManaEffect; -import mage.constants.AbilityType; -import mage.constants.AsThoughEffectType; -import mage.constants.ManaType; -import mage.constants.TimingRule; -import mage.constants.Zone; +import mage.constants.*; import mage.game.Game; import mage.game.stack.Spell; import mage.game.stack.StackObject; +import java.util.*; + /** * @author BetaSteward_at_googlemail.com */ @@ -134,6 +126,9 @@ public abstract class ActivatedManaAbilityImpl extends ActivatedAbilityImpl impl * Is it allowed to undo the mana creation. It's e.g. not allowed if some * game revealing information is related (like reveal the top card of the * library) + *

+ * TODO: it helps with single mana activate for mana pool, but will not work while activates on paying for casting + * (e.g. user can cheats to see next draw card) * * @return */ diff --git a/Mage/src/main/java/mage/cards/CardImpl.java b/Mage/src/main/java/mage/cards/CardImpl.java index f66a4023329..24f34599fe5 100644 --- a/Mage/src/main/java/mage/cards/CardImpl.java +++ b/Mage/src/main/java/mage/cards/CardImpl.java @@ -1,12 +1,6 @@ package mage.cards; import com.google.common.collect.ImmutableList; -import java.lang.reflect.Constructor; -import java.lang.reflect.InvocationTargetException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.UUID; import mage.MageObject; import mage.MageObjectImpl; import mage.Mana; @@ -33,6 +27,10 @@ import mage.util.SubTypeList; import mage.watchers.Watcher; import org.apache.log4j.Logger; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.util.*; + public abstract class CardImpl extends MageObjectImpl implements Card { private static final long serialVersionUID = 1L; @@ -352,6 +350,24 @@ public abstract class CardImpl extends MageObjectImpl implements Card { for (Ability subAbility : ability.getSubAbilities()) { abilities.add(subAbility); } + + // verify check: draw effect can't be rollback after mana usage (example: Chromatic Sphere) + // (player can cheat with cancel button to see next card) + // verify test will catch that errors + if (ability instanceof ActivatedManaAbilityImpl) { + ActivatedManaAbilityImpl manaAbility = (ActivatedManaAbilityImpl) ability; + String rule = manaAbility.getRule().toLowerCase(Locale.ENGLISH); + if (manaAbility.getEffects().stream().anyMatch(e -> e.getOutcome().equals(Outcome.DrawCard)) + || rule.contains("reveal ") + || rule.contains("draw ")) { + if (manaAbility.isUndoPossible()) { + throw new IllegalArgumentException("Ability contains draw/reveal effect, but isUndoPossible is true. Ability: " + + ability.getClass().getSimpleName() + "; " + ability.getRule()); + } + } + } + + } protected void addAbilities(List abilities) { @@ -395,10 +411,10 @@ public abstract class CardImpl extends MageObjectImpl implements Card { * Dynamic cost modification for card (process only own abilities). Example: * if it need stack related info (like real targets) then must check two * states (game.inCheckPlayableState): - * + *

* 1. In playable state it must check all possible use cases (e.g. allow to * reduce on any available target and modes) - * + *

* 2. In real cast state it must check current use case (e.g. real selected * targets and modes) * diff --git a/Mage/src/main/java/mage/game/GameImpl.java b/Mage/src/main/java/mage/game/GameImpl.java index eebae0d7168..a7d8590b986 100644 --- a/Mage/src/main/java/mage/game/GameImpl.java +++ b/Mage/src/main/java/mage/game/GameImpl.java @@ -699,6 +699,12 @@ public abstract class GameImpl implements Game, Serializable { return savedStates.size(); } + /** + * Warning, for inner usage only, use player.restoreState as much as possible instead + * + * @param bookmark + * @param context additional information for error message + */ @Override public void restoreState(int bookmark, String context) { if (!simulation && !this.hasEnded()) { // if player left or game is over no undo is possible - this could lead to wrong winner @@ -1247,7 +1253,7 @@ public abstract class GameImpl implements Game, Serializable { if (player != null) { int bookmark = player.getStoredBookmark(); if (bookmark != -1) { - restoreState(bookmark, "undo"); + player.restoreState(bookmark, "undo", this); player.setStoredBookmark(-1); fireUpdatePlayersEvent(); } diff --git a/Mage/src/main/java/mage/players/Player.java b/Mage/src/main/java/mage/players/Player.java index 90405edeee4..1da17ef95f0 100644 --- a/Mage/src/main/java/mage/players/Player.java +++ b/Mage/src/main/java/mage/players/Player.java @@ -451,6 +451,13 @@ public interface Player extends MageItem, Copyable { void resetStoredBookmark(Game game); + default void restoreState(int bookmark, String text, Game game) { + game.restoreState(bookmark, text); + if (getStoredBookmark() >= bookmark) { + resetStoredBookmark(game); + } + } + void revealCards(Ability source, Cards cards, Game game); void revealCards(String titelSuffix, Cards cards, Game game); diff --git a/Mage/src/main/java/mage/players/PlayerImpl.java b/Mage/src/main/java/mage/players/PlayerImpl.java index f827fb9a2cd..339c7e0a497 100644 --- a/Mage/src/main/java/mage/players/PlayerImpl.java +++ b/Mage/src/main/java/mage/players/PlayerImpl.java @@ -1385,13 +1385,6 @@ public abstract class PlayerImpl implements Player, Serializable { return false; } - public void restoreState(int bookmark, String text, Game game) { - game.restoreState(bookmark, text); - if (storedBookmark >= bookmark) { - resetStoredBookmark(game); - } - } - @Override public boolean activateAbility(ActivatedAbility ability, Game game) { if (ability == null) { diff --git a/Mage/src/main/java/mage/util/ManaUtil.java b/Mage/src/main/java/mage/util/ManaUtil.java index 44689f497e7..1d606b33cd3 100644 --- a/Mage/src/main/java/mage/util/ManaUtil.java +++ b/Mage/src/main/java/mage/util/ManaUtil.java @@ -549,7 +549,7 @@ public final class ManaUtil { } if (!payed) { - game.restoreState(bookmark, restoreContextName); + player.restoreState(bookmark, restoreContextName, game); game.fireUpdatePlayersEvent(); } else { game.removeBookmark(bookmark);