Fix bugs with dies triggers due to short living LKI reset (#12438)

* replace applyEffects() with processAction() for card usages

* update Goblin Welder and test

* add test for Historian's Wisdom

* enable other related tests

* only reset short living LKI for process action, not all apply effects

* update docs

* remove applyEffects from condition in Historian's Wisdom

* add another test case

Mage.Sets/src/mage/cards/e/EvolvedSleeper.java

@@ -119,7 +119,7 @@ class EvolvedSleeperPhyrexianEffect extends OneShotEffect {
             permanent.addCounters(CounterType.P1P1.createInstance(), source.getControllerId(), source, game);
         }
         Player player = game.getPlayer(source.getControllerId());
-        game.applyEffects();
+        game.getState().processAction(game);
         player.drawCards(1, source, game);
         player.loseLife(1, game, source, false);
         return true;

Mage.Sets/src/mage/cards/e/ExtraordinaryJourney.java

@@ -92,7 +92,7 @@ class ExtraordinaryJourneyEffect extends OneShotEffect {
         if (!effect.apply(game, source)) {
             return false;
         }
-        game.getState().applyEffects(game);
+        game.getState().processAction(game);
 
         Set<Card> cards = permanents
                 .stream()

Mage.Sets/src/mage/cards/g/GoblinWelder.java

@@ -85,7 +85,6 @@ public final class GoblinWelder extends CardImpl {
                 return false;
             }
             boolean sacrifice = artifact.sacrifice(source, game);
-            game.getState().processAction(game); // bug #7672
             boolean putOnBF = owner.moveCards(card, Zone.BATTLEFIELD, source, game);
             if (sacrifice || putOnBF) {
                 return true;

Mage.Sets/src/mage/cards/h/HistoriansWisdom.java

@@ -75,7 +75,8 @@ enum HistoriansWisdomCondition implements Condition {
 
     @Override
     public boolean apply(Game game, Ability source) {
-        game.applyEffects(); // Make sure +2/+1 buff gets applied first
+        //game.applyEffects(); // Make sure +2/+1 buff gets applied first
+        // TODO: not appropriate to call here - investigate where in the engine to apply effects to solve bug
         Permanent enchantment = source.getSourcePermanentIfItStillExists(game);
         if (enchantment == null) {
             return false;

Mage.Sets/src/mage/cards/i/IndustrialAdvancement.java

@@ -70,7 +70,7 @@ class IndustrialAdvancementEffect extends OneShotEffect {
             return false;
         }
         TargetSacrifice target = new TargetSacrifice(0, 1, StaticFilters.FILTER_PERMANENT_CREATURE);
-        player.choose(outcome, target, source, game);
+        player.choose(Outcome.Sacrifice, target, source, game);
         Permanent permanent = game.getPermanent(target.getFirstTarget());
         if (permanent == null || !permanent.sacrifice(source, game)) {
             return false;
@@ -82,7 +82,7 @@ class IndustrialAdvancementEffect extends OneShotEffect {
         TargetCard targetCard = new TargetCardInLibrary(
                 0, 1, StaticFilters.FILTER_CARD_CREATURE
         );
-        player.choose(outcome, cards, targetCard, source, game);
+        player.choose(Outcome.PutCreatureInPlay, cards, targetCard, source, game);
         player.moveCards(game.getCard(targetCard.getFirstTarget()), Zone.BATTLEFIELD, source, game);
         cards.retainZone(Zone.LIBRARY, game);
         player.putCardsOnBottomOfLibrary(cards, game, source, false);

Mage.Sets/src/mage/cards/m/MasterOfCeremonies.java

@@ -10,7 +10,6 @@ import mage.cards.CardImpl;
 import mage.cards.CardSetInfo;
 import mage.choices.Choice;
 import mage.choices.ChoiceImpl;
-import mage.choices.VoteHandler;
 import mage.constants.CardType;
 import mage.constants.Outcome;
 import mage.constants.SubType;
@@ -114,7 +113,7 @@ class MasterOfCeremoniesChoiceEffect extends OneShotEffect {
             Token treasureOpponent = new TreasureToken();
             treasureOpponent.putOntoBattlefield(1, game, source, opponentId);
         }
-        game.applyEffects();
+        game.getState().processAction(game);
 
         // Friends - You and that player each create a 1/1 green and white Citizen creature token.
         for (UUID opponentId : friendChoosers) {
@@ -124,7 +123,7 @@ class MasterOfCeremoniesChoiceEffect extends OneShotEffect {
             Token citizenOpponent = new CitizenGreenWhiteToken();
             citizenOpponent.putOntoBattlefield(1, game, source, opponentId);
         }
-        game.applyEffects();
+        game.getState().processAction(game);
 
         // Secrets - You and that player each draw a card.
         for (UUID opponentId : secretsChoosers) {
@@ -148,4 +147,4 @@ class MasterOfCeremoniesChoiceEffect extends OneShotEffect {
     public MasterOfCeremoniesChoiceEffect copy() {
         return new MasterOfCeremoniesChoiceEffect(this);
     }
-}
+}

Mage.Sets/src/mage/cards/n/Necromentia.java

@@ -116,7 +116,7 @@ class NecromentiaEffect extends OneShotEffect {
             targetPlayer.shuffleLibrary(source, game);
 
             if (numberOfCardsExiledFromHand > 0) {
-                game.getState().applyEffects(game);
+                game.getState().processAction(game);
                 Token zombieToken = new ZombieToken();
                 zombieToken.putOntoBattlefield(numberOfCardsExiledFromHand, game, source, targetPlayer.getId());
             }
@@ -124,4 +124,4 @@ class NecromentiaEffect extends OneShotEffect {
         }
         return false;
     }
-}
+}

Mage.Sets/src/mage/cards/p/PhabineBosssConfidant.java

@@ -5,8 +5,6 @@ import mage.abilities.Ability;
 import mage.abilities.common.BeginningOfCombatTriggeredAbility;
 import mage.abilities.common.SimpleStaticAbility;
 import mage.abilities.dynamicvalue.common.ParleyCount;
-import mage.abilities.effects.ContinuousEffect;
-import mage.abilities.effects.ContinuousEffectImpl;
 import mage.abilities.effects.Effect;
 import mage.abilities.effects.OneShotEffect;
 import mage.abilities.effects.common.DrawCardAllEffect;
@@ -109,7 +107,7 @@ class PhabineBosssConfidantParleyEffect extends OneShotEffect {
         if (landCount > 0) {
             Token citizenToken = new CitizenGreenWhiteToken();
             citizenToken.putOntoBattlefield(landCount, game, source, source.getControllerId(), false, false);
-            game.applyEffects();
+            game.getState().processAction(game);
         }
 
         if (nonLandCount > 0) {
@@ -124,4 +122,4 @@ class PhabineBosssConfidantParleyEffect extends OneShotEffect {
     public PhabineBosssConfidantParleyEffect copy() {
         return new PhabineBosssConfidantParleyEffect(this);
     }
-}
+}

Mage.Sets/src/mage/cards/p/PheliaExuberantShepherd.java

@@ -97,7 +97,7 @@ class PheliaExuberantShepherdEffect extends OneShotEffect {
         }
         Set<Card> cards = exileZone.getCards(game);
         player.moveCards(cards, Zone.BATTLEFIELD, source, game, false, false, true, null);
-        game.getState().applyEffects(game);
+        game.getState().processAction(game);
 
         Permanent phelia = source.getSourcePermanentIfItStillExists(game);
         if (phelia == null) {
@@ -118,4 +118,4 @@ class PheliaExuberantShepherdEffect extends OneShotEffect {
         phelia.addCounters(CounterType.P1P1.createInstance(), source, game);
         return true;
     }
-}
+}

Mage.Sets/src/mage/cards/s/ShowstoppingSurprise.java

@@ -61,7 +61,7 @@ class ShowstoppingSurpriseEffect extends OneShotEffect {
         }
         if (permanent.isFaceDown(game)) {
             permanent.turnFaceUp(source, game, source.getControllerId());
-            game.applyEffects();
+            game.getState().processAction(game);
         }
         int power = permanent.getPower().getValue();
         if (power < 1) {

Mage.Sets/src/mage/cards/t/TheStoneBrain.java

@@ -123,7 +123,7 @@ class TheStoneBrainEffect extends OneShotEffect {
                 targetPlayer.shuffleLibrary(source, game);
 
                 if (numberOfCardsExiledFromHand > 0) {
-                    game.getState().applyEffects(game);
+                    game.getState().processAction(game);
                     targetPlayer.drawCards(numberOfCardsExiledFromHand, source, game);
                 }
             }

Mage.Sets/src/mage/cards/u/UnmooredEgo.java

@@ -110,7 +110,7 @@ class UnmooredEgoEffect extends OneShotEffect {
                 targetPlayer.shuffleLibrary(source, game);
 
                 if (numberOfCardsExiledFromHand > 0) {
-                    game.getState().applyEffects(game);
+                    game.getState().processAction(game);
                     targetPlayer.drawCards(numberOfCardsExiledFromHand, source, game);
                 }
             }

Mage.Tests/src/test/java/org/mage/test/cards/single/neo/HistoriansWisdomTest.java

@@ -0,0 +1,48 @@
+package org.mage.test.cards.single.neo;
+
+import mage.constants.PhaseStep;
+import mage.constants.Zone;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.mage.test.serverside.base.CardTestPlayerBase;
+
+/**
+ * @author xenohedron
+ */
+public class HistoriansWisdomTest extends CardTestPlayerBase {
+
+    /** 2G Aura
+     Enchant artifact or creature
+     When Historian’s Wisdom enters the battlefield, if enchanted permanent is a creature
+            with the greatest power among creatures on the battlefield, draw a card.
+     As long as enchanted permanent is a creature, it gets +2/+1.
+     */
+
+    private static final String hw = "Historian's Wisdom";
+    private static final String bear = "Runeclaw Bear"; // 2/2
+    private static final String chimera = "Horizon Chimera"; // 3/2 Whenever you draw a card, you gain 1 life.
+
+    @Test
+    @Ignore // apply effects in condition is not an appropriate solution
+    public void testTrigger() {
+        addCard(Zone.BATTLEFIELD, playerA, bear);
+        addCard(Zone.BATTLEFIELD, playerA, chimera);
+        addCard(Zone.HAND, playerA, hw);
+        addCard(Zone.BATTLEFIELD, playerA, "Forest", 3);
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, hw, bear);
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+
+        assertPowerToughness(playerA, bear, 4, 3);
+        assertPowerToughness(playerA, chimera, 3, 2);
+        assertPermanentCount(playerA, hw, 1);
+        assertAttachedTo(playerA, hw, bear, true);
+        assertHandCount(playerA, 1);
+        assertLife(playerA, 21);
+        assertLife(playerB, 20);
+    }
+
+}

Mage.Tests/src/test/java/org/mage/test/cards/single/tsp/ChromaticStarTest.java

@@ -62,7 +62,6 @@ public class ChromaticStarTest extends CardTestPlayerBase {
         assertHandCount(playerA, 1);
     }
 
-    @Ignore // short living LKI bug -- see #12385
     @Test
     public void test_Star_ChainMana_Auto() {
         setStrictChooseMode(true);

Mage.Tests/src/test/java/org/mage/test/cards/single/ulg/GoblinWelderTest.java

@@ -3,7 +3,6 @@ package org.mage.test.cards.single.ulg;
 import mage.constants.PhaseStep;
 import mage.constants.Zone;
 import mage.counters.CounterType;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.mage.test.serverside.base.CardTestPlayerBase;
 
@@ -17,7 +16,6 @@ public class GoblinWelderTest extends CardTestPlayerBase {
     private static final String relic = "Darksteel Relic";
     private static final String aspirant = "Blood Aspirant";
 
-    @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);
@@ -29,7 +27,7 @@ public class GoblinWelderTest extends CardTestPlayerBase {
         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
+        // must have 2 dies triggers on stack: from source and from another
         setChoice(playerA, "Whenever you sacrifice a permanent"); // select from 2 triggers
         checkStackSize("check triggers", 1, PhaseStep.PRECOMBAT_MAIN, playerA, 2);
 
@@ -40,6 +38,6 @@ public class GoblinWelderTest extends CardTestPlayerBase {
         assertGraveyardCount(playerA, wurmcoil, 1);
         assertPermanentCount(playerA, relic, 1);
         assertCounterCount(aspirant, CounterType.P1P1, 1);
-        assertPermanentCount(playerA, "Wurm Token", 2);
+        assertPermanentCount(playerA, "Phyrexian Wurm Token", 2);
     }
 }

Mage.Tests/src/test/java/org/mage/test/cards/triggers/dies/SacrificeDiesTriggerTest.java

@@ -2,7 +2,6 @@ package org.mage.test.cards.triggers.dies;
 
 import mage.constants.PhaseStep;
 import mage.constants.Zone;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.mage.test.serverside.base.CardTestPlayerBase;
 
@@ -378,7 +377,6 @@ public class SacrificeDiesTriggerTest extends CardTestPlayerBase {
     }
 
     @Test
-    @Ignore // TODO: enable after shortLKI and move to battlefield will be fixed
     public void test_DiesTriggerWhileMultiStepsEffect_ShortLKI() {
         // see details on shortLKI problems in isInUseableZoneDiesTrigger
 
@@ -414,4 +412,33 @@ public class SacrificeDiesTriggerTest extends CardTestPlayerBase {
         // from end step trigger
         assertPermanentCount(playerA, "Silvercoat Lion", 1);
     }
+
+    // bug #9688
+    @Test
+    public void testIndustrialAdvancement() {
+        skipInitShuffling();
+        addCard(Zone.BATTLEFIELD, playerA, "Industrial Advancement");
+        // At the beginning of your end step, you may sacrifice a creature. If you do, look at the top X cards of your
+        // library, where X is that creature’s mana value. You may put a creature card from among them onto the
+        // battlefield. Put the rest on the bottom of your library in a random order.
+        addCard(Zone.BATTLEFIELD, playerA, "Guardian Automaton"); // 3/3 gain 3 life when it dies
+        addCard(Zone.LIBRARY, playerA, "Lone Missionary"); // etb gain 4 life
+        addCard(Zone.LIBRARY, playerA, "Horned Turtle");
+        addCard(Zone.LIBRARY, playerA, "Maritime Guard");
+        addCard(Zone.LIBRARY, playerA, "Kraken Hatchling");
+
+        setChoice(playerA, "Guardian Automaton"); // sacrifice on end step
+        setChoice(playerA, "Lone Missionary"); // put onto battlefield
+        setChoice(playerA, "When {this} dies"); // choose trigger order
+
+        setStrictChooseMode(true);
+        setStopAt(2, PhaseStep.PRECOMBAT_MAIN);
+        execute();
+
+        assertPermanentCount(playerA, "Lone Missionary", 1);
+        assertGraveyardCount(playerA, "Guardian Automaton", 1);
+        assertLife(playerA, 27);
+
+    }
+
 }

Mage/src/main/java/mage/abilities/TriggeredAbilityImpl.java

@@ -436,10 +436,7 @@ 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
+            //  The idea: short living LKI must help to find a moment in the inner of resolve
             //  -
             //  Example:
             //   --!---------------!-------------!-----!-----------!
@@ -455,15 +452,6 @@ public abstract class TriggeredAbilityImpl extends AbilityImpl implements Trigge
             //   - ! 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);
             }

Mage/src/main/java/mage/game/GameImpl.java

@@ -1929,7 +1929,6 @@ public abstract class GameImpl implements Game {
 
     @Override
     public synchronized void applyEffects() {
-        resetShortLivingLKI();
         state.applyEffects(this);
     }
 

Mage/src/main/java/mage/game/GameState.java

@@ -677,11 +677,12 @@ public class GameState implements Serializable, Copyable<GameState> {
      */
     public void processAction(Game game) {
         game.getState().handleSimultaneousEvent(game);
+        game.resetShortLivingLKI();
         game.applyEffects();
         game.getState().getTriggers().checkStateTriggers(game);
     }
 
-    public void applyEffects(Game game) {
+    void applyEffects(Game game) {
         applyEffectsCounter++;
         for (Player player : players.values()) {
             player.reset();

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

@@ -691,7 +691,7 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
                 + CardUtil.getSourceLogName(game, source, this.getId()));
         this.setTransformed(!this.transformed);
         this.transformCount++;
-        game.applyEffects();
+        game.applyEffects(); // not process action - no firing of simultaneous events yet
         this.replaceEvent(EventType.TRANSFORMING, game);
         game.addSimultaneousEvent(GameEvent.getEvent(EventType.TRANSFORMED, this.getId(), this.getControllerId()));
         return true;

Mage/src/main/java/mage/game/permanent/token/TokenImpl.java

@@ -454,7 +454,7 @@ public abstract class TokenImpl extends MageObjectImpl implements Token {
         }
         CreatedTokensEvent.addEvents(allAddedTokens, source, game);
 
-        game.getState().applyEffects(game); // Needed to do it here without LKIReset i.e. do get SwordOfTheMeekTest running correctly.
+        game.applyEffects(); // without LKI reset
     }
 
     @Override

Mage/src/main/java/mage/players/PlayerImpl.java

@@ -4779,10 +4779,7 @@ 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();
+                game.applyEffects(); // without LKI reset
                 break;
             case HAND:
                 for (Card card : cards) {