From 83017e3c51a1b13ea073a236887e0d49525d2dc2 Mon Sep 17 00:00:00 2001 From: Oleg Agafonov Date: Sun, 5 Sep 2021 18:44:50 +0400 Subject: [PATCH] Devs: added docs and todo about short living LKI, LKI and current problems; --- .../src/mage/cards/b/BlessingOfFrost.java | 2 +- .../src/mage/cards/b/BlimComedicGenius.java | 2 +- .../src/mage/cards/s/SelflessExorcist.java | 2 +- .../src/mage/cards/t/ThievingSkydiver.java | 2 +- .../cards/single/ulg/GoblinWelderTest.java | 13 +++++--- .../cards/triggers/dies/BloodArtistTest.java | 33 +++++++++++++++---- .../triggers/dies/OmnathLocusOfRageTest.java | 10 ++++-- .../mage/abilities/TriggeredAbilityImpl.java | 28 ++++++++++++++++ Mage/src/main/java/mage/game/GameImpl.java | 3 +- .../main/java/mage/players/PlayerImpl.java | 3 ++ 10 files changed, 79 insertions(+), 19 deletions(-) diff --git a/Mage.Sets/src/mage/cards/b/BlessingOfFrost.java b/Mage.Sets/src/mage/cards/b/BlessingOfFrost.java index 204ccb6230..1deee6907f 100644 --- a/Mage.Sets/src/mage/cards/b/BlessingOfFrost.java +++ b/Mage.Sets/src/mage/cards/b/BlessingOfFrost.java @@ -89,7 +89,7 @@ class BlessingOfFrostEffect extends OneShotEffect { permanent.addCounters(CounterType.P1P1.createInstance(target.getTargetAmount(targetId)), source.getControllerId(), source, game); } } - game.applyEffects(); + game.getState().processAction(game); player.drawCards(game.getBattlefield().count( filter, source.getSourceId(), source.getControllerId(), game ), source, game); diff --git a/Mage.Sets/src/mage/cards/b/BlimComedicGenius.java b/Mage.Sets/src/mage/cards/b/BlimComedicGenius.java index bc1bb2af16..5b1782303c 100644 --- a/Mage.Sets/src/mage/cards/b/BlimComedicGenius.java +++ b/Mage.Sets/src/mage/cards/b/BlimComedicGenius.java @@ -86,7 +86,7 @@ class BlimComedicGeniusEffect extends OneShotEffect { game.addEffect(new GainControlTargetEffect( Duration.Custom, true, targetPointer.getFirst(game, source) ).setTargetPointer(new FixedTarget(source.getFirstTarget(), game)), source); - game.applyEffects(); + game.getState().processAction(game); Map cardsMap = new HashMap<>(); for (UUID playerId : game.getState().getPlayersInRange(source.getControllerId(), game)) { Player player = game.getPlayer(playerId); diff --git a/Mage.Sets/src/mage/cards/s/SelflessExorcist.java b/Mage.Sets/src/mage/cards/s/SelflessExorcist.java index 24c1e648e9..6461f6bf3a 100644 --- a/Mage.Sets/src/mage/cards/s/SelflessExorcist.java +++ b/Mage.Sets/src/mage/cards/s/SelflessExorcist.java @@ -76,7 +76,7 @@ class SelflessExorcistEffect extends OneShotEffect { return false; } player.moveCards(card, Zone.EXILED, source, game); - game.applyEffects(); + game.getState().processAction(game); Permanent permanent = source.getSourcePermanentIfItStillExists(game); if (permanent == null) { return true; diff --git a/Mage.Sets/src/mage/cards/t/ThievingSkydiver.java b/Mage.Sets/src/mage/cards/t/ThievingSkydiver.java index 8fa7e69d36..e67dc43906 100644 --- a/Mage.Sets/src/mage/cards/t/ThievingSkydiver.java +++ b/Mage.Sets/src/mage/cards/t/ThievingSkydiver.java @@ -109,7 +109,7 @@ class ThievingSkydiverEffect extends OneShotEffect { || !artifact.hasSubtype(SubType.EQUIPMENT, game)) { return false; } - game.applyEffects(); + game.getState().processAction(game); permanent.addAttachment(artifact.getId(), source, game); return true; } diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/single/ulg/GoblinWelderTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/single/ulg/GoblinWelderTest.java index b7fad55f84..272573b623 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/single/ulg/GoblinWelderTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/single/ulg/GoblinWelderTest.java @@ -17,7 +17,7 @@ public class GoblinWelderTest extends CardTestPlayerBase { private static final String relic = "Darksteel Relic"; private static final String aspirant = "Blood Aspirant"; - @Ignore + @Ignore // TODO: related to problems with dies triggers and short living LKI, see TriggeredAbilityImpl for details @Test public void testSacrificeDiesTrigger() { addCard(Zone.BATTLEFIELD, playerA, welder); @@ -25,10 +25,15 @@ public class GoblinWelderTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerA, aspirant); addCard(Zone.GRAVEYARD, playerA, relic); - addTarget(playerA, relic); - addTarget(playerA, wurmcoil); activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}:"); + addTarget(playerA, wurmcoil); + addTarget(playerA, relic); + waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN, true); + // must have 2 dies triggers on stack: from source and from another, but it have only from another + setChoice(playerA, "Whenever you sacrifice a permanent"); // select from 2 triggers + checkStackSize("check triggers", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 2); + setStrictChooseMode(true); setStopAt(1, PhaseStep.END_TURN); execute(); assertAllCommandsUsed(); @@ -36,6 +41,6 @@ public class GoblinWelderTest extends CardTestPlayerBase { assertGraveyardCount(playerA, wurmcoil, 1); assertPermanentCount(playerA, relic, 1); assertCounterCount(aspirant, CounterType.P1P1, 1); - assertPermanentCount(playerA, "Wurm", 2); // TODO: currently fails here + assertPermanentCount(playerA, "Wurm", 2); } } diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/BloodArtistTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/BloodArtistTest.java index 8498f3595f..976289bdf0 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/BloodArtistTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/BloodArtistTest.java @@ -1,4 +1,3 @@ - package org.mage.test.cards.triggers.dies; import mage.constants.PhaseStep; @@ -7,9 +6,8 @@ import org.junit.Test; import org.mage.test.serverside.base.CardTestPlayerBase; /** - * * @author noxx - * + *

* Whenever Blood Artist or another creature dies, target player loses 1 life * and you gain 1 life. */ @@ -30,11 +28,20 @@ public class BloodArtistTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerB, "Bloodflow Connoisseur", 1); + // 2x blood artist, kill one of it and get 3x dies triggers + // from living artist: 2x triggers + // from killed artist: 1x trigger castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Lightning Bolt", "Blood Artist"); + setChoice(playerA, "Whenever {this} or another creature"); // 2x dies triggers + addTarget(playerA, playerB, 2); // targets for 2x triggers castSpell(1, PhaseStep.POSTCOMBAT_MAIN, playerA, "Lightning Bolt", "Bloodflow Connoisseur"); + addTarget(playerA, playerB); // targets for 1x trigger (from living artist) + + setStrictChooseMode(true); setStopAt(1, PhaseStep.END_TURN); execute(); + assertAllCommandsUsed(); assertLife(playerA, 23); assertLife(playerB, 17); @@ -53,11 +60,15 @@ public class BloodArtistTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerB, "Pillarfield Ox", 1); + // sac 2x and gen 2x dies triggers castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Bone Splinters", "Pillarfield Ox"); - setChoice(playerA, "Silvercoat Lion"); + setChoice(playerA, "Silvercoat Lion"); // sacrifice for cost + addTarget(playerA, playerB, 2); + setStrictChooseMode(true); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); + assertAllCommandsUsed(); assertGraveyardCount(playerA, "Bone Splinters", 1); assertGraveyardCount(playerA, "Silvercoat Lion", 1); @@ -77,11 +88,15 @@ public class BloodArtistTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerB, "Pillarfield Ox", 1); + // sac blood artist as a cost and trigger 1x castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Bone Splinters", "Pillarfield Ox"); - setChoice(playerA, "Blood Artist"); + setChoice(playerA, "Blood Artist"); // sacrifice for cost + addTarget(playerA, playerB); // dies trigger with lose life + setStrictChooseMode(true); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); + assertAllCommandsUsed(); assertGraveyardCount(playerA, "Bone Splinters", 1); assertGraveyardCount(playerA, "Blood Artist", 1); @@ -104,13 +119,17 @@ public class BloodArtistTest extends CardTestPlayerBase { addCard(Zone.BATTLEFIELD, playerB, "Pillarfield Ox", 1); addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion", 1); + // sac blood artist for cost, trigger 1x BUT remove blood artist before trigger resolve castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Bone Splinters", "Pillarfield Ox"); - setChoice(playerA, "Blood Artist"); - // Blood Artist may no longer trigger from destroyed creature because already in the graveyard + setChoice(playerA, "Blood Artist"); // sacrifice for cost + addTarget(playerA, playerB); // dies trigger with lose life, but it will be fizzled + // remove boold artist first - Blood Artist may no longer trigger from destroyed creature because already in the graveyard castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Terror", "Silvercoat Lion", "Bone Splinters"); + setStrictChooseMode(true); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); + assertAllCommandsUsed(); assertGraveyardCount(playerA, "Bone Splinters", 1); assertGraveyardCount(playerA, "Terror", 1); diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/OmnathLocusOfRageTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/OmnathLocusOfRageTest.java index 63969189fc..e3d6882a22 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/OmnathLocusOfRageTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/OmnathLocusOfRageTest.java @@ -27,8 +27,12 @@ public class OmnathLocusOfRageTest extends CardTestPlayerBase { addCard(Zone.HAND, playerB, "Diabolic Edict", 1); // {1}{B} addCard(Zone.BATTLEFIELD, playerB, "Swamp", 2); - castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "Diabolic Edict", playerA); + castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "Diabolic Edict"); + addTarget(playerB, playerA); + addTarget(playerA, "Omnath, Locus of Rage"); // sacrifice target + addTarget(playerA, playerB); // target for dies trigger with damage + setStrictChooseMode(true); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); assertAllCommandsUsed(); @@ -55,7 +59,9 @@ public class OmnathLocusOfRageTest extends CardTestPlayerBase { castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "Lightning Bolt", "Lightning Elemental"); // Dying Lightning Elemental does no longer trigger ability of Omnath castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "Blastfire Bolt", "Omnath, Locus of Rage"); + addTarget(playerA, playerB); // target for dies trigger with damage + setStrictChooseMode(true); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); assertAllCommandsUsed(); @@ -67,7 +73,6 @@ public class OmnathLocusOfRageTest extends CardTestPlayerBase { assertLife(playerA, 20); assertLife(playerB, 17); - } @Test @@ -84,6 +89,7 @@ public class OmnathLocusOfRageTest extends CardTestPlayerBase { setChoice(playerB, "Green"); castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerB, "Stave Off", "Silvercoat Lion"); + setStrictChooseMode(true); setStopAt(1, PhaseStep.BEGIN_COMBAT); execute(); assertAllCommandsUsed(); diff --git a/Mage/src/main/java/mage/abilities/TriggeredAbilityImpl.java b/Mage/src/main/java/mage/abilities/TriggeredAbilityImpl.java index 4db1e5a241..3b1a46dab9 100644 --- a/Mage/src/main/java/mage/abilities/TriggeredAbilityImpl.java +++ b/Mage/src/main/java/mage/abilities/TriggeredAbilityImpl.java @@ -296,6 +296,34 @@ public abstract class TriggeredAbilityImpl extends AbilityImpl implements Trigge if (game.getState().getZone(source.getSourceId()) == Zone.BATTLEFIELD) { sourceObject = game.getPermanent(source.getSourceId()); } else { + // TODO: multiple calls of ApplyEffects all around the code are breaking a short living lki idea + // (PlayerImpl's call to move to battlefield do the worse thing) + // - + // Original idea: short living LKI must help to find a moment in the inner of resolve + // - + // Example: + // --!---------------!-------------!-----!-----------! + // - ! steps ! perm zone ! LKI ! short LKI ! + // --!---------------!-------------!-----!-----------! + // - ! resolve start ! battlefield ! no ! no ! + // - ! step 1 ! battlefield ! no ! no ! permanent moving to graveyard by step's command + // - ! step 2 ! graveyard ! yes ! yes ! other commands + // - ! step 3 ! graveyard ! yes ! yes ! other commands + // - ! raise triggers! graveyard ! yes ! yes ! handle and put triggers that was raised in resolve steps + // - ! resolve end ! graveyard ! yes ! no ! + // - ! resolve next ! graveyard ! yes ! no ! resolve next spell + // - ! empty stack ! graveyard ! no ! no ! no more to resolve + // --!---------------!-------------!-----!-----------! + // - + // - Problem 1: move code (well, not only move) calls ApplyEffects in the middle of the resolve + // - and reset short LKI (after short LKI reset dies trigger will not work) + // - Example: Goblin Welder calls sacrifice and card move in the same effect - but move call do + // - a reset and dies trigger ignored (trigger thinks that permanent already dies) + // - + // - Possible fix: + // - replace ApplyEffects in the move code by game.getState().processAction(game); + // - check and fix many broken (is it was a false positive test or something broken) + //sourceObject = (Permanent) game.getLastKnownInformation(source.getSourceId(), Zone.BATTLEFIELD); if (game.getShortLivingLKI(source.getSourceId(), Zone.BATTLEFIELD)) { sourceObject = (Permanent) game.getLastKnownInformation(source.getSourceId(), Zone.BATTLEFIELD); } diff --git a/Mage/src/main/java/mage/game/GameImpl.java b/Mage/src/main/java/mage/game/GameImpl.java index 6ee75983ff..925f2e5789 100644 --- a/Mage/src/main/java/mage/game/GameImpl.java +++ b/Mage/src/main/java/mage/game/GameImpl.java @@ -1984,8 +1984,7 @@ public abstract class GameImpl implements Game { break; } } - state.handleSimultaneousEvent(this); - applyEffects(); // needed e.g if boost effects end and cause creatures to die + this.getState().processAction(this); // needed e.g if boost effects end and cause creatures to die somethingHappened = true; } checkConcede(); diff --git a/Mage/src/main/java/mage/players/PlayerImpl.java b/Mage/src/main/java/mage/players/PlayerImpl.java index 9797958726..6be8a2d328 100644 --- a/Mage/src/main/java/mage/players/PlayerImpl.java +++ b/Mage/src/main/java/mage/players/PlayerImpl.java @@ -4484,6 +4484,9 @@ public abstract class PlayerImpl implements Player, Serializable { } } } + // TODO: must be replaced by game.getState().processAction(game), see isInUseableZoneDiesTrigger comments + // about short living LKI problem + //game.getState().processAction(game); game.applyEffects(); break; case HAND: