From 5b49fa4cc2264508d21d76530c655db10ac06685 Mon Sep 17 00:00:00 2001 From: Oleg Agafonov Date: Thu, 2 Nov 2023 17:57:55 +0400 Subject: [PATCH] choose mode improves: - fixed broken cards with once per turn choose (example: Galadriel, Light of Valinor, closes #11362); - fixed cheat to skip required mode by cancel button (example: Black Market Connections, closes #11149, closes #10611); - fixed empty modes list if nothing available to choose; - improved compatibility with max modes and other modification effects; - fixed that non-valid modes can be selected in some use cases; --- .../src/mage/player/human/HumanPlayer.java | 127 +++++++++--------- .../test/cards/modal/OnlyOnceModeTest.java | 106 +++++++++++++++ Mage/src/main/java/mage/abilities/Modes.java | 73 ++++++---- 3 files changed, 219 insertions(+), 87 deletions(-) create mode 100644 Mage.Tests/src/test/java/org/mage/test/cards/modal/OnlyOnceModeTest.java diff --git a/Mage.Server.Plugins/Mage.Player.Human/src/mage/player/human/HumanPlayer.java b/Mage.Server.Plugins/Mage.Player.Human/src/mage/player/human/HumanPlayer.java index d5866a4ac33..1530a0e189d 100644 --- a/Mage.Server.Plugins/Mage.Player.Human/src/mage/player/human/HumanPlayer.java +++ b/Mage.Server.Plugins/Mage.Player.Human/src/mage/player/human/HumanPlayer.java @@ -18,8 +18,6 @@ import mage.cards.decks.Deck; import mage.choices.Choice; import mage.choices.ChoiceImpl; import mage.constants.*; -import static mage.constants.PlayerAction.REQUEST_AUTO_ANSWER_RESET_ALL; -import static mage.constants.PlayerAction.TRIGGER_AUTO_ORDER_RESET_ALL; import mage.filter.StaticFilters; import mage.filter.common.FilterAttackingCreature; import mage.filter.common.FilterBlockingCreature; @@ -56,6 +54,9 @@ import java.util.*; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.stream.Collectors; +import static mage.constants.PlayerAction.REQUEST_AUTO_ANSWER_RESET_ALL; +import static mage.constants.PlayerAction.TRIGGER_AUTO_ORDER_RESET_ALL; + /** * @author BetaSteward_at_googlemail.com */ @@ -2428,9 +2429,17 @@ public class HumanPlayer extends PlayerImpl { return null; } - if (modes.size() > 1) { - // done option for up to choices - boolean canEndChoice = modes.getSelectedModes().size() >= modes.getMinModes() || modes.isMayChooseNone(); + if (modes.size() == 0) { + return null; + } + + if (modes.size() == 1) { + return modes.getMode(); + } + + boolean done = false; + while (!done && canRespond()) { + // prepare modes list MageObject obj = game.getObject(source); Map modeMap = new LinkedHashMap<>(); int modeIndex = 0; @@ -2468,65 +2477,61 @@ public class HumanPlayer extends PlayerImpl { } } - if (!modeMap.isEmpty()) { - - // can done for up to - if (canEndChoice) { - modeMap.put(Modes.CHOOSE_OPTION_DONE_ID, "Done"); - } - modeMap.put(Modes.CHOOSE_OPTION_CANCEL_ID, "Cancel"); - - boolean done = false; - while (!done && canRespond()) { - - String message = "Choose mode (selected " + modes.getSelectedModes().size() + " of " + modes.getMaxModes(game, source) - + ", min " + modes.getMinModes() + ")"; - if (obj != null) { - message = message + "
" + obj.getLogName(); - } - - updateGameStatePriority("chooseMode", game); - prepareForResponse(game); - if (!isExecutingMacro()) { - game.fireGetModeEvent(playerId, message, modeMap); - } - waitForResponse(game); - - UUID responseId = getFixedResponseUUID(game); - if (responseId != null) { - for (Mode mode : modes.getAvailableModes(source, game)) { - if (mode.getId().equals(responseId)) { - // TODO: add checks on 2x selects (cheaters can rewrite client side code and select same mode multiple times) - // reason: wrong setup eachModeMoreThanOnce and eachModeOnlyOnce in many cards - return mode; - } - } - - // end choice by done option in ability pickup dialog - if (canEndChoice && Modes.CHOOSE_OPTION_DONE_ID.equals(responseId)) { - done = true; - } - - // cancel choice (remove all selections) - if (Modes.CHOOSE_OPTION_CANCEL_ID.equals(responseId)) { - modes.clearSelectedModes(); - } - } else if (canEndChoice) { - // end choice by done button in feedback panel - // disable after done option implemented - // done = true; - } - - // triggered abilities can't be skipped by cancel or wrong answer - if (source.getAbilityType() != AbilityType.TRIGGERED) { - done = true; - } - } + // done button for "for up" choices only + boolean canEndChoice = modes.getSelectedModes().size() >= modes.getMinModes() || modes.isMayChooseNone(); + if (canEndChoice) { + modeMap.put(Modes.CHOOSE_OPTION_DONE_ID, "Done"); + } + modeMap.put(Modes.CHOOSE_OPTION_CANCEL_ID, "Cancel"); + + // prepare dialog + String message = "Choose mode (selected " + modes.getSelectedModes().size() + " of " + modes.getMaxModes(game, source) + + ", min " + modes.getMinModes() + ")"; + if (obj != null) { + message = message + "
" + obj.getLogName(); + } + + updateGameStatePriority("chooseMode", game); + prepareForResponse(game); + if (!isExecutingMacro()) { + game.fireGetModeEvent(playerId, message, modeMap); + } + waitForResponse(game); + + // process choice + UUID responseId = getFixedResponseUUID(game); + if (responseId != null) { + for (Mode mode : modes.getAvailableModes(source, game)) { + if (mode.getId().equals(responseId)) { + // TODO: add checks on 2x selects (cheaters can rewrite client side code and select same mode multiple times) + // reason: wrong setup eachModeMoreThanOnce and eachModeOnlyOnce in many cards + return mode; + } + } + + // end choice by done option in ability pickup dialog + if (canEndChoice && Modes.CHOOSE_OPTION_DONE_ID.equals(responseId)) { + done = true; + } + + // cancel choice (remove all selections) + if (Modes.CHOOSE_OPTION_CANCEL_ID.equals(responseId)) { + modes.clearSelectedModes(); + } + } else if (canEndChoice) { + // end choice by done button in feedback panel + // disable after done option implemented + // done = true; + } + + // triggered abilities can't be skipped by cancel or wrong answer + if (source.getAbilityType() != AbilityType.TRIGGERED) { + done = true; } - return null; } - return modes.getMode(); + // user disconnected, press cancel, press done or something else + return null; } @Override diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/modal/OnlyOnceModeTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/modal/OnlyOnceModeTest.java new file mode 100644 index 00000000000..56d3300a0f7 --- /dev/null +++ b/Mage.Tests/src/test/java/org/mage/test/cards/modal/OnlyOnceModeTest.java @@ -0,0 +1,106 @@ +package org.mage.test.cards.modal; + +import mage.constants.PhaseStep; +import mage.constants.Zone; +import mage.counters.CounterType; +import org.junit.Test; +import org.mage.test.serverside.base.CardTestPlayerBase; + +/** + * @author JayDi85 + */ +public class OnlyOnceModeTest extends CardTestPlayerBase { + + @Test + public void test_OncePerGame() { + // {2}, {T}: Choose one that hasn't been chosen + // Three Bowls of Porridge deals 2 damage to target creature. + // Tap target creature. + // Sacrifice Three Bowls of Porridge. You gain 3 life. + addCard(Zone.BATTLEFIELD, playerA, "Three Bowls of Porridge"); + // + addCard(Zone.BATTLEFIELD, playerA, "Spectral Bears", 1); // 3/3 + addCard(Zone.BATTLEFIELD, playerA, "Forest", 2); + + // each mode usage must be removed from a list, so all mode choices must be 1 + + // turn 1 - first mode available + // choose 1: damage 2 to target + checkDamage("before mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", 0); + activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{2}, {T}"); + setModeChoice(playerA, "1"); + addTarget(playerA, "Spectral Bears"); // to damage + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN); + checkDamage("before mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", 2); + + // turn 3 - first mode doesn't restore, so mode 2 will be used + // choose 2: Tap target creature + checkPermanentTapped("before mode 2", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", false, 1); + activateAbility(3, PhaseStep.PRECOMBAT_MAIN, playerA, "{2}, {T}"); + setModeChoice(playerA, "1"); + addTarget(playerA, "Spectral Bears"); // to tap + waitStackResolved(3, PhaseStep.PRECOMBAT_MAIN); + checkPermanentTapped("after mode 2", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "Spectral Bears", true, 1); + + setStrictChooseMode(true); + setStopAt(3, PhaseStep.END_TURN); + execute(); + } + + @Test + public void test_OncePerTurn() { + int triggerCalls = 5; + + // Whenever another creature enters the battlefield under your control, choose one that hasn't been chosen this turn + // Add {G}{G}{G}. + // Put a +1/+1 counter on each creature you control. + // Scry 2, then draw a card. + addCard(Zone.BATTLEFIELD, playerA, "Galadriel, Light of Valinor"); + // + addCard(Zone.HAND, playerA, "Grizzly Bears", triggerCalls); // {1}{G} + addCard(Zone.BATTLEFIELD, playerA, "Forest", 2 * triggerCalls); + // + addCard(Zone.LIBRARY, playerA, "Mountain", 10); + + // each mode usage must be removed from a list, so all mode choices must be 1 + + // do trigger 1: Add {G}{G}{G} + checkManaPool("before mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 0); + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears"); + setModeChoice(playerA, "1"); + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN); + checkManaPool("after mode 1", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 3); + + // do trigger 2: Put a +1/+1 counter + checkPermanentCounters("before mode 2", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", CounterType.P1P1, 0); + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears"); + setModeChoice(playerA, "1"); + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN); + checkPermanentCounters("after mode 2", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", CounterType.P1P1, 1); + + // do trigger 3: Scry 2, then draw a card + checkHandCardCount("before mode 3", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Mountain", 0); + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears"); + setModeChoice(playerA, "1"); + addTarget(playerA, "Mountain"); // scry 2, one to bottom, one to top + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN); + checkHandCardCount("after mode 3", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Mountain", 1); + + // do trigger 4: nothing + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears"); + + // next turn triggers works again + // do trigger 5: Add {G}{G}{G} + checkManaPool("before mode 1 on turn 3", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 0); + castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears"); + setModeChoice(playerA, "1"); + waitStackResolved(3, PhaseStep.PRECOMBAT_MAIN); + checkManaPool("after mode 1 on turn 3", 3, PhaseStep.PRECOMBAT_MAIN, playerA, "G", 3); + + setStrictChooseMode(true); + setStopAt(3, PhaseStep.END_TURN); + execute(); + + assertPermanentCount(playerA, "Grizzly Bears", triggerCalls); + } +} diff --git a/Mage/src/main/java/mage/abilities/Modes.java b/Mage/src/main/java/mage/abilities/Modes.java index 4a17f4588ff..9bd8390f76c 100644 --- a/Mage/src/main/java/mage/abilities/Modes.java +++ b/Mage/src/main/java/mage/abilities/Modes.java @@ -45,7 +45,7 @@ public class Modes extends LinkedHashMap implements Copyable private TargetController chooseController; private boolean mayChooseSameModeMoreThanOnce = false; // example: choose three... you may choose the same mode more than once private boolean mayChooseNone = false; - private boolean isRandom = false; + private boolean isRandom = false; // random from available modes, not modes TODO: research rules of Cult of Skaro after WHO release (is it random from all modes or from available/valid) public Modes() { // add default mode @@ -287,19 +287,41 @@ public class Modes extends LinkedHashMap implements Copyable && getOnceTurnNum(game, source) != game.getTurnNum(); } + private boolean isSelectedValid(Ability source, Game game) { + if (isLimitUsageByOnce()) { + setOnceSelectedModes(source, game); + } + return this.selectedModes.size() >= this.getMinModes() + || (this.selectedModes.size() == 0 && mayChooseNone); + } + public boolean choose(Game game, Ability source) { if (isAlreadySelectedModesOutdated(game, source)) { this.clearAlreadySelectedModes(source, game); } - if (this.size() > 1) { - this.clearSelectedModes(); - if (this.isRandom) { - List modes = getAvailableModes(source, game); - this.addSelectedMode(modes.get(RandomUtil.nextInt(modes.size())).getId()); - return true; - } + this.clearSelectedModes(); - // check if mode modifying abilities exist + // runtime check + if (this.isRandom && limitUsageByOnce) { + // non-tested use case, if you catch this error then disable and manually test, if fine then that check can be removed + throw new IllegalStateException("Wrong code usage: random modes are not support with once usage"); + } + + // 700.2b + // The controller of a modal triggered ability chooses the mode(s) as part of putting that ability + // on the stack. If one of the modes would be illegal (due to an inability to choose legal targets, for + // example), that mode can’t be chosen. If no mode is chosen, the ability is removed from the + // stack. (See rule 603.3c.) + List availableModes = getAvailableModes(source, game); + if (availableModes.size() == 0) { + return isSelectedValid(source, game); + } + + // modal spells must show choose dialog even for 1 option, so check this.size instead evailableModes.size here + if (this.size() > 1) { + // multiple modes + + // modes modifications, e.g. choose max modes instead single Card card = game.getCard(source.getSourceId()); if (card != null) { for (Ability modeModifyingAbility : card.getAbilities(game)) { @@ -310,7 +332,14 @@ public class Modes extends LinkedHashMap implements Copyable } } - // check if all modes can be activated automatically + // choose random + if (this.isRandom) { + // TODO: research rules of Cult of Skaro after WHO release (is it random from all modes or from available/valid) + this.addSelectedMode(availableModes.get(RandomUtil.nextInt(availableModes.size())).getId()); + return isSelectedValid(source, game); + } + + // UX: check if all modes can be activated automatically if (this.size() == this.getMinModes() && !isMayChooseSameModeMoreThanOnce()) { Set onceSelectedModes = null; if (isLimitUsageByOnce()) { @@ -322,10 +351,7 @@ public class Modes extends LinkedHashMap implements Copyable this.addSelectedMode(mode.getId()); } } - if (isLimitUsageByOnce()) { - setOnceSelectedModes(source, game); - } - return !selectedModes.isEmpty(); + return isSelectedValid(source, game); } // 700.2d @@ -352,20 +378,15 @@ public class Modes extends LinkedHashMap implements Copyable while (this.selectedModes.size() < currentMaxModes) { Mode choice = player.chooseMode(this, source, game); if (choice == null) { - if (isLimitUsageByOnce()) { - setOnceSelectedModes(source, game); - } - return this.selectedModes.size() >= this.getMinModes() - || (this.selectedModes.size() == 0 && mayChooseNone); + // user press cancel/stop in choose dialog or nothing to choose + return isSelectedValid(source, game); } this.addSelectedMode(choice.getId()); if (currentMode == null) { currentMode = choice; } } - if (isLimitUsageByOnce()) { - setOnceSelectedModes(source, game); - } + // effects helper (keep real choosing player) if (chooseController == TargetController.OPPONENT) { selectedModes .stream() @@ -374,13 +395,13 @@ public class Modes extends LinkedHashMap implements Copyable .forEach(effects -> effects.setValue("choosingPlayer", playerId)); } } else { - // only one mode available - this.clearSelectedModes(); + // only one mode Mode mode = this.values().iterator().next(); this.addSelectedMode(mode.getId()); this.setActiveMode(mode); } - return true; + + return isSelectedValid(source, game); } /** @@ -449,7 +470,7 @@ public class Modes extends LinkedHashMap implements Copyable Set res = new HashSet<>(); // if selected modes is not for current turn, so we ignore any value that may be there - if (!ignoreOutdatedData && isAlreadySelectedModesOutdated(game, source)) { + if (ignoreOutdatedData && isAlreadySelectedModesOutdated(game, source)) { return res; }