From a53eb66b58cde18e900d58a915aa9474c33fee18 Mon Sep 17 00:00:00 2001 From: Oleg Agafonov Date: Wed, 7 May 2025 17:34:36 +0400 Subject: [PATCH] other: reworked target selection (now it use same logic and methods in all places), fixed AI and selection freezes in some use cases (related to #13606, #11285) --- .../java/mage/player/ai/ComputerPlayer.java | 19 +++--- .../cards/abilities/keywords/SaddleTest.java | 52 +++++++++------- .../oneshot/exile/SearchNameExileTests.java | 2 + .../test/cards/triggers/SacredGroundTest.java | 2 + .../java/org/mage/test/player/TestPlayer.java | 7 ++- Mage/src/main/java/mage/target/Target.java | 13 +++- .../main/java/mage/target/TargetAmount.java | 7 ++- .../src/main/java/mage/target/TargetImpl.java | 62 +++++++++++++++---- Mage/src/main/java/mage/target/Targets.java | 11 +++- .../target/common/TargetCardInLibrary.java | 11 ++-- 10 files changed, 133 insertions(+), 53 deletions(-) diff --git a/Mage.Server.Plugins/Mage.Player.AI/src/main/java/mage/player/ai/ComputerPlayer.java b/Mage.Server.Plugins/Mage.Player.AI/src/main/java/mage/player/ai/ComputerPlayer.java index 7238c0ab417..a5a99306423 100644 --- a/Mage.Server.Plugins/Mage.Player.AI/src/main/java/mage/player/ai/ComputerPlayer.java +++ b/Mage.Server.Plugins/Mage.Player.AI/src/main/java/mage/player/ai/ComputerPlayer.java @@ -251,6 +251,7 @@ public class ComputerPlayer extends PlayerImpl { for (Permanent permanent : targets) { if (target.canTarget(abilityControllerId, permanent.getId(), source, game) && !target.getTargets().contains(permanent.getId())) { // stop to add targets if not needed and outcome is no advantage for AI player + // TODO: need research and improve - is it good to check target.isChosen(game) instead return true? if (target.getMinNumberOfTargets() == target.getTargets().size()) { if (outcome.isGood() && hasOpponent(permanent.getControllerId(), game)) { return true; @@ -2044,19 +2045,20 @@ public class ComputerPlayer extends PlayerImpl { // we still use playerId when getting cards even if they don't control the search List cardChoices = new ArrayList<>(cards.getCards(target.getFilter(), playerId, source, game)); - while (!target.doneChoosing(game)) { + do { Card card = selectCardTarget(abilityControllerId, cardChoices, outcome, target, source, game); if (card != null) { target.addTarget(card.getId(), source, game); cardChoices.remove(card); } else { // We don't have any valid target to choose so stop choosing - return target.getTargets().size() >= target.getMinNumberOfTargets(); + return target.isChosen(game); } + // try to fill as much as possible for good effect (see while end) or half for bad (see if) if (outcome == Outcome.Neutral && target.getTargets().size() > target.getMinNumberOfTargets() + (target.getMaxNumberOfTargets() - target.getMinNumberOfTargets()) / 2) { - return true; + return target.isChosen(game); } - } + } while (target.getTargets().size() < target.getMaxNumberOfTargets()); return true; } @@ -2075,19 +2077,20 @@ public class ComputerPlayer extends PlayerImpl { } List cardChoices = new ArrayList<>(cards.getCards(target.getFilter(), abilityControllerId, source, game)); - while (!target.doneChoosing(game)) { + do { Card card = selectCard(abilityControllerId, cardChoices, outcome, target, game); if (card != null) { target.add(card.getId(), game); cardChoices.remove(card); // selectCard don't remove cards (only on second+ tries) } else { // We don't have any valid target to choose so stop choosing - return target.getTargets().size() >= target.getMinNumberOfTargets(); + return target.isChosen(game); } + // try to fill as much as possible for good effect (see while end) or half for bad (see if) if (outcome == Outcome.Neutral && target.getTargets().size() > target.getMinNumberOfTargets() + (target.getMaxNumberOfTargets() - target.getMinNumberOfTargets()) / 2) { - return true; + return target.isChosen(game); } - } + } while (target.getTargets().size() < target.getMaxNumberOfTargets()); return true; } diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/abilities/keywords/SaddleTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/abilities/keywords/SaddleTest.java index 1a09b791aab..33051479103 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/abilities/keywords/SaddleTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/abilities/keywords/SaddleTest.java @@ -15,6 +15,11 @@ import org.mage.test.serverside.base.CardTestPlayerBase; */ public class SaddleTest extends CardTestPlayerBase { + /** + * Whenever Quilled Charger attacks while saddled, it gets +1/+2 and gains menace until end of turn. + *

+ * Saddle 2 + */ private static final String charger = "Quilled Charger"; private static final String bear = "Grizzly Bears"; @@ -47,29 +52,35 @@ public class SaddleTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerA, charger); addCard(Zone.BATTLEFIELD, playerA, bear); - setChoice(playerA, bear); + // turn 1 - saddle and trigger on attack activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Saddle"); - + setChoice(playerA, bear); attack(1, playerA, charger, playerB); + runCode("on saddle", 1, PhaseStep.POSTCOMBAT_MAIN, playerA, (info, player, game) -> { + assertTapped(bear, true); + assertTapped(charger, true); + assertSaddled(charger, true); + assertAbility(playerA, charger, new MenaceAbility(false), true); + assertLife(playerB, 20 - 4 - 1); + }); setStrictChooseMode(true); - setStopAt(1, PhaseStep.END_TURN); - execute(); - - assertTapped(bear, true); - assertTapped(charger, true); - assertSaddled(charger, true); - assertAbility(playerA, charger, new MenaceAbility(false), true); - assertLife(playerB, 20 - 4 - 1); - setStopAt(2, PhaseStep.UPKEEP); execute(); + // turn 2 - saddle ends assertSaddled(charger, false); } + /** + * Whenever Rambling Possum attacks while saddled, it gains +1/+2 until end of turn. Then you may return any number + * of creatures that saddled it this turn to their owner's hand. + *

+ * Saddle 1 + */ private static final String possum = "Rambling Possum"; private static final String lion = "Silvercoat Lion"; + private static final String elf = "Arbor Elf"; @Test public void testSaddledThisTurn() { @@ -77,11 +88,11 @@ public class SaddleTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerA, bear); addCard(Zone.BATTLEFIELD, playerA, lion); - setChoice(playerA, bear); activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Saddle"); + setChoice(playerA, bear); // to saddle cost attack(1, playerA, possum, playerB); - setChoice(playerA, bear); + setChoice(playerA, bear); // to return setStrictChooseMode(true); setStopAt(1, PhaseStep.END_TURN); @@ -100,19 +111,17 @@ public class SaddleTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerA, possum); addCard(Zone.BATTLEFIELD, playerA, bear); addCard(Zone.BATTLEFIELD, playerA, lion); + addCard(Zone.BATTLEFIELD, playerA, elf); - setChoice(playerA, bear); + // turn 1 - saddle x2 and trigger on attack activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Saddle"); - - setStrictChooseMode(true); - setStopAt(1, PhaseStep.PRECOMBAT_MAIN); - execute(); + setChoice(playerA, bear + "^" + lion); attack(1, playerA, possum, playerB); - setChoice(playerA, lion); + setChoice(playerA, elf); // to return (try to choose a wrong creature, so game must not allow to choose it) + setStrictChooseMode(true); setStopAt(1, PhaseStep.END_TURN); - // TODO: test framework must have tools to check targeting (as workaround try to check it by look at test command error) try { execute(); @@ -123,7 +132,8 @@ public class SaddleTest extends CardTestPlayerBase { } assertTapped(bear, true); - assertTapped(lion, false); + assertTapped(lion, true); + assertTapped(elf, false); assertTapped(possum, true); assertSaddled(possum, true); } diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/abilities/oneshot/exile/SearchNameExileTests.java b/Mage.Tests/src/test/java/org/mage/test/cards/abilities/oneshot/exile/SearchNameExileTests.java index 55b4465a293..71e83ef6543 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/abilities/oneshot/exile/SearchNameExileTests.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/abilities/oneshot/exile/SearchNameExileTests.java @@ -136,6 +136,8 @@ public class SearchNameExileTests extends CardTestPlayerBase { castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "fused Ready // Willing"); castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Test of Talents", "Ready // Willing", "Ready // Willing"); + // TODO: a non strict cause a good AI choice test - make strict and duplicate as really AI test? + // in non strict mode AI must choose as much as possible in good "up to" target and half in bad target setStrictChooseMode(false); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/SacredGroundTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/SacredGroundTest.java index 6e4aefc1cf7..ee676c58ac4 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/SacredGroundTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/SacredGroundTest.java @@ -144,7 +144,9 @@ public class SacredGroundTest extends CardTestPlayerBase { castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Molten Rain", "Caves of Koilos"); waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, 1); castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Surgical Extraction", "Caves of Koilos"); + setChoice(playerA, true); // Pay 2 life instead of {B} + //setStrictChooseMode(true); TODO: good example of AI choices, so add new AI test instead non strict here setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); diff --git a/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java b/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java index 4f5c180df56..62b2826a382 100644 --- a/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java +++ b/Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java @@ -2335,12 +2335,17 @@ public class TestPlayer implements Player { } } + if (!targetFound) { + //failOnLastBadChoice(game, source, target, choiceRecord, "unknown or can't target"); + } + try { if (target.isChosen(game)) { return true; } else { + // TODO: move check above and fix all fail tests (not after target.isChosen) if (!targetFound) { - failOnLastBadChoice(game, source, target, choiceRecord, "unknown or can't target"); + failOnLastBadChoice(game, source, target, choiceRecord, "selected, but not all required targets"); } } } finally { diff --git a/Mage/src/main/java/mage/target/Target.java b/Mage/src/main/java/mage/target/Target.java index 3a29a88a474..c639fdf9f0b 100644 --- a/Mage/src/main/java/mage/target/Target.java +++ b/Mage/src/main/java/mage/target/Target.java @@ -7,6 +7,7 @@ import mage.constants.Zone; import mage.filter.Filter; import mage.game.Game; import mage.players.Player; +import mage.util.Copyable; import java.io.Serializable; import java.util.Collection; @@ -17,14 +18,23 @@ import java.util.UUID; /** * @author BetaSteward_at_googlemail.com */ -public interface Target extends Serializable { +public interface Target extends Copyable, Serializable { + /** + * All targets selected by a player + *

+ * Warning, for "up to" targets it will return true all the time, so make sure your dialog + * use do-while logic and call "choose" one time min or use doneChoosing + */ boolean isChosen(Game game); + // TODO: combine code or research usages (doneChoosing must be in while cycles, isChosen in other places, see #13606) boolean doneChoosing(Game game); void clearChosen(); + boolean isChoiceSelected(); + boolean isNotTarget(); /** @@ -161,6 +171,7 @@ public interface Target extends Serializable { UUID getFirstTarget(); + @Override Target copy(); // some targets are chosen from players that are not the controller of the ability (e.g. Pandemonium) diff --git a/Mage/src/main/java/mage/target/TargetAmount.java b/Mage/src/main/java/mage/target/TargetAmount.java index d128d38ec9b..37f84122b81 100644 --- a/Mage/src/main/java/mage/target/TargetAmount.java +++ b/Mage/src/main/java/mage/target/TargetAmount.java @@ -109,17 +109,20 @@ public abstract class TargetAmount extends TargetImpl { if (!amountWasSet) { setAmount(source, game); } - chosen = isChosen(game); + chosen = false; while (remainingAmount > 0) { if (!player.canRespond()) { + chosen = isChosen(game); return chosen; } if (!getTargetController(game, playerId).chooseTargetAmount(outcome, this, source, game)) { + chosen = isChosen(game); return chosen; } chosen = isChosen(game); } - return chosen; + + return isChosen(game); } @Override diff --git a/Mage/src/main/java/mage/target/TargetImpl.java b/Mage/src/main/java/mage/target/TargetImpl.java index 046bf9fa203..f6bd03a7ede 100644 --- a/Mage/src/main/java/mage/target/TargetImpl.java +++ b/Mage/src/main/java/mage/target/TargetImpl.java @@ -31,7 +31,16 @@ public abstract class TargetImpl implements Target { protected int minNumberOfTargets; protected boolean required = true; protected boolean requiredExplicitlySet = false; + /** + * Simple chosen state due selected and require targets count + * Complex targets must override isChosen method or set chosen value manually + * For "up to" targets it will be false before first choose dialog: + * - chosen = false - player do not make a choice yet + * - chosen = true, targets.size = 0 - player choose 0 targets in "up to" and it's valid + * - chosen = true, targets.size >= 1 - player choose some targets and it's valid + */ protected boolean chosen = false; + // is the target handled as targeted spell/ability (notTarget = true is used for not targeted effects like e.g. sacrifice) protected boolean notTarget = false; protected boolean atRandom = false; // for inner choose logic @@ -229,15 +238,28 @@ public abstract class TargetImpl implements Target { @Override public boolean isChosen(Game game) { + // min = max = 0 - for abilities with X=0, e.g. nothing to choose if (getMaxNumberOfTargets() == 0 && getMinNumberOfTargets() == 0) { return true; } - return getMaxNumberOfTargets() != 0 && targets.size() == getMaxNumberOfTargets() || chosen; + + // limit by max amount + if (getMaxNumberOfTargets() > 0 && targets.size() > getMaxNumberOfTargets()) { + return chosen; + } + + // limit by min amount + if (getMinNumberOfTargets() > 0 && targets.size() < getMinNumberOfTargets()) { + return chosen; + } + + // all fine + return chosen || (targets.size() >= getMinNumberOfTargets() && targets.size() <= getMaxNumberOfTargets()); } @Override public boolean doneChoosing(Game game) { - return getMaxNumberOfTargets() != 0 && targets.size() == getMaxNumberOfTargets(); + return isChoiceSelected() && isChosen(game); } @Override @@ -247,13 +269,19 @@ public abstract class TargetImpl implements Target { chosen = false; } + @Override + public boolean isChoiceSelected() { + // min = max = 0 - for abilities with X=0, e.g. nothing to choose + return chosen || getMaxNumberOfTargets() == 0 && getMinNumberOfTargets() == 0; + } + @Override public void add(UUID id, Game game) { if (getMaxNumberOfTargets() == 0 || targets.size() < getMaxNumberOfTargets()) { if (!targets.containsKey(id)) { targets.put(id, 0); rememberZoneChangeCounter(id, game); - chosen = targets.size() >= getMinNumberOfTargets(); + chosen = isChosen(game); } } } @@ -280,7 +308,7 @@ public abstract class TargetImpl implements Target { if (!game.replaceEvent(new TargetEvent(id, source))) { targets.put(id, 0); rememberZoneChangeCounter(id, game); - chosen = targets.size() >= getMinNumberOfTargets(); + chosen = isChosen(game); if (!skipEvent && shouldReportEvents) { game.addSimultaneousEvent(GameEvent.getEvent(GameEvent.EventType.TARGETED, id, source, source.getControllerId())); } @@ -318,14 +346,16 @@ public abstract class TargetImpl implements Target { if (!game.replaceEvent(GameEvent.getEvent(GameEvent.EventType.TARGET, id, source, source.getControllerId()))) { targets.put(id, amount); rememberZoneChangeCounter(id, game); - chosen = targets.size() >= getMinNumberOfTargets(); + chosen = isChosen(game); if (!skipEvent && shouldReportEvents) { game.fireEvent(GameEvent.getEvent(GameEvent.EventType.TARGETED, id, source, source.getControllerId())); } } } else { + // AI targets simulation targets.put(id, amount); rememberZoneChangeCounter(id, game); + chosen = isChosen(game); } } @@ -336,17 +366,20 @@ public abstract class TargetImpl implements Target { return false; } - chosen = targets.size() >= getMinNumberOfTargets(); + chosen = false; do { if (!targetController.canRespond()) { + chosen = isChosen(game); return chosen; } if (!targetController.choose(outcome, this, source, game)) { + chosen = isChosen(game); return chosen; } - chosen = targets.size() >= getMinNumberOfTargets(); - } while (!isChosen(game) && !doneChoosing(game)); - return chosen; + chosen = isChosen(game); + } while (!doneChoosing(game)); + + return isChosen(game); } @Override @@ -358,13 +391,15 @@ public abstract class TargetImpl implements Target { List possibleTargets = new ArrayList<>(possibleTargets(playerId, source, game)); - chosen = targets.size() >= getMinNumberOfTargets(); + chosen = false; do { if (!targetController.canRespond()) { + chosen = isChosen(game); return chosen; } if (isRandom()) { if (possibleTargets.isEmpty()) { + chosen = isChosen(game); return chosen; } // find valid target @@ -384,13 +419,14 @@ public abstract class TargetImpl implements Target { if (autoChosenId != null) { addTarget(autoChosenId, source, game); } else if (!targetController.chooseTarget(outcome, this, source, game)) { // If couldn't autochoose ask player + chosen = isChosen(game); return chosen; } } - chosen = targets.size() >= getMinNumberOfTargets(); - } while (!isChosen(game) && !doneChoosing(game)); + chosen = isChosen(game); + } while (!doneChoosing(game)); - return chosen; + return isChosen(game); } @Override diff --git a/Mage/src/main/java/mage/target/Targets.java b/Mage/src/main/java/mage/target/Targets.java index 2d337ffbabd..ed25b10207e 100644 --- a/Mage/src/main/java/mage/target/Targets.java +++ b/Mage/src/main/java/mage/target/Targets.java @@ -4,6 +4,7 @@ import mage.abilities.Ability; import mage.constants.Outcome; import mage.game.Game; import mage.game.events.GameEvent; +import mage.players.Player; import mage.util.Copyable; import java.util.*; @@ -38,7 +39,11 @@ public class Targets extends ArrayList implements Copyable { } public List getUnchosen(Game game) { - return stream().filter(target -> !target.isChosen(game)).collect(Collectors.toList()); + return stream().filter(target -> !target.isChoiceSelected()).collect(Collectors.toList()); + } + + public boolean doneChoosing(Game game) { + return stream().allMatch(t -> t.doneChoosing(game)); } public void clearChosen() { @@ -67,7 +72,7 @@ public class Targets extends ArrayList implements Copyable { if (!target.choose(outcome, playerId, sourceId, source, game)) { return false; } - } + } while (!doneChoosing(game)); } return true; } @@ -112,7 +117,7 @@ public class Targets extends ArrayList implements Copyable { //game.restoreState(state, "Targets"); clearChosen(); } - } + } while (!doneChoosing(game)); } return true; } diff --git a/Mage/src/main/java/mage/target/common/TargetCardInLibrary.java b/Mage/src/main/java/mage/target/common/TargetCardInLibrary.java index 0b4735a40c6..e3a37ebb666 100644 --- a/Mage/src/main/java/mage/target/common/TargetCardInLibrary.java +++ b/Mage/src/main/java/mage/target/common/TargetCardInLibrary.java @@ -76,17 +76,20 @@ public class TargetCardInLibrary extends TargetCard { Cards cardsId = new CardsImpl(); cards.forEach(cardsId::add); - chosen = targets.size() >= getMinNumberOfTargets(); + chosen = false; do { if (!player.canRespond()) { + chosen = isChosen(game); return chosen; } if (!player.chooseTarget(outcome, cardsId, this, source, game)) { + chosen = isChosen(game); return chosen; } - chosen = targets.size() >= getMinNumberOfTargets(); - } while (!isChosen(game) && !doneChoosing(game)); - return chosen; + chosen = isChosen(game); + } while (!doneChoosing(game)); + + return isChosen(game); } @Override