Fix SacrificeTargetCost and SacrificeAllCost activator checks (#12809)

* Fix Tergrid's Lantern and add test

* Remove custom effect, fix SacrificeTargetCost to avoid checking for activated abilities and sidestepping the controllerID

* Add test to verify change to SacrificeTargetCost

* Add special action test

* Fix canPay check for SacrificeTargetCost

* Remove activated ability check in SacrificeAllCost

* Remove cost-specific activator checks for special actions, as they are redundant

* add null check for game.getPlayer

Mage.Tests/src/test/java/org/mage/test/cards/cost/sacrifice/SacrificeTargetCostTest.java

@@ -0,0 +1,139 @@
+package org.mage.test.cards.cost.sacrifice;
+
+import mage.abilities.common.LicidAbility;
+import mage.abilities.costs.mana.ColoredManaCost;
+import mage.abilities.keyword.HasteAbility;
+import mage.constants.CardType;
+import mage.constants.ColoredManaSymbol;
+import mage.constants.PhaseStep;
+import mage.constants.Zone;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.mage.test.serverside.base.CardTestPlayerBase;
+
+/**
+ *
+ * @author jimga150
+ */
+public class SacrificeTargetCostTest extends CardTestPlayerBase {
+
+    // Tests a variety of use cases with SacrificeTargetCost, making sure the right player pays the cost
+
+    @Test
+    public void testSimpleCost() {
+        // All Rats have fear.
+        // {T}, Sacrifice a Rat: Create X 1/1 black Rat creature tokens, where X is the number of Rats you control.
+        addCard(Zone.BATTLEFIELD, playerA, "Marrow-Gnawer");
+        addCard(Zone.BATTLEFIELD, playerA, "Karumonix, the Rat King");
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}");
+        setChoice(playerA, "Karumonix, the Rat King"); // Target to sacrifice
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertPermanentCount(playerA, "Marrow-Gnawer", 1);
+        assertPermanentCount(playerA, "Rat Token", 1);
+        assertGraveyardCount(playerA, "Karumonix, the Rat King", 1);
+    }
+
+    @Test
+    public void testSimpleCostOtherPlayerActivate() {
+        // {1}, Sacrifice a land: Draw a card. Any player may activate this ability.
+        addCard(Zone.BATTLEFIELD, playerA, "Excavation");
+        addCard(Zone.BATTLEFIELD, playerB, "Forest");
+
+        // Player B activates Player A's Excavate ability
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerB, "{1}");
+        setChoice(playerB, "Forest"); // Target to sacrifice
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertHandCount(playerB, 1);
+        assertGraveyardCount(playerB, "Forest", 1);
+    }
+
+    @Test
+    public void testDoUnlessSacrificeTrigger() {
+        // When Demanding Dragon enters, it deals 5 damage to target opponent unless that player sacrifices a creature.
+        addCard(Zone.HAND, playerA, "Demanding Dragon", 2);
+        addCard(Zone.BATTLEFIELD, playerA, "Mountain", 5);
+        addCard(Zone.BATTLEFIELD, playerB, "Memnite");
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Demanding Dragon");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "No"); // Sac a creature?
+
+        castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerA, "Demanding Dragon");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "Yes"); // Sac a creature?
+        setChoice(playerB, "Memnite"); // Sac Memnite
+
+        setStrictChooseMode(true);
+        setStopAt(3, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertGraveyardCount(playerB, "Memnite", 1);
+    }
+
+    @Test
+    public void testDoUnlessSacrificeActivated() {
+        addCard(Zone.BATTLEFIELD, playerA, "Swamp", 4);
+        addCard(Zone.BATTLEFIELD, playerB, "Memnite", 1);
+        addCard(Zone.HAND, playerA, "Tergrid, God of Fright // Tergrid's Lantern");
+        addCard(Zone.HAND, playerB, "Memnarch");
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Tergrid's Lantern", true);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Target player");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "No"); // Sac or discard to avoid life loss?
+
+        activateAbility(3, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Target player");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "Yes"); // Sac or discard to avoid life loss?
+        setChoice(playerB, "Yes"); // Yes - Sacrifice, No - Discard
+        setChoice(playerB, "Memnite"); // To sacrifice
+
+        setStrictChooseMode(true);
+        setStopAt(3, PhaseStep.END_TURN);
+        execute();
+
+        assertLife(playerB, currentGame.getStartingLife() - 3);
+        assertPermanentCount(playerB, "Memnite", 0);
+        assertGraveyardCount(playerB, "Memnite", 1);
+        assertHandCount(playerB, "Memnarch", 1);
+    }
+
+    /**
+     * Use special action that has opponent sac a permanent
+     */
+    @Test
+    public void SpecialActionTest() {
+        // Enchanted creature can't attack or block, and its activated abilities can't be activated.
+        // That creature's controller may sacrifice a permanent for that player to ignore this effect until end of turn.
+        addCard(Zone.HAND, playerA, "Volrath's Curse");
+
+        addCard(Zone.BATTLEFIELD, playerA, "Island", 2);
+        addCard(Zone.BATTLEFIELD, playerB, "Memnite");
+        addCard(Zone.BATTLEFIELD, playerB, "Memnarch");
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Volrath's Curse");
+        addTarget(playerA, "Memnarch");
+
+        activateAbility(2, PhaseStep.PRECOMBAT_MAIN, playerB, "Sacrifice a ", "Volrath's Curse");
+        setChoice(playerB, "Memnite");
+
+        setStrictChooseMode(true);
+        setStopAt(2, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertPermanentCount(playerB, "Memnite", 0);
+        assertGraveyardCount(playerB, "Memnite", 1);
+        assertPermanentCount(playerB, "Memnarch", 1);
+    }
+
+}

Mage.Tests/src/test/java/org/mage/test/cards/single/khm/TergridsLanternTest.java

@@ -0,0 +1,101 @@
+package org.mage.test.cards.single.khm;
+
+import mage.constants.PhaseStep;
+import mage.constants.Zone;
+import org.junit.Test;
+import org.mage.test.serverside.base.CardTestPlayerBase;
+
+/**
+ * {@link mage.cards.t.TergridGodOfFright Tergrid, God of Fright // Tergrid's Lantern}
+ * {3}{B}{B}
+ * Legendary Creature — God
+ * P/T 4/5
+ * Menace
+ * Whenever an opponent sacrifices a nontoken permanent or discards a permanent card, you may put that card from a graveyard onto the battlefield under your control.
+ *
+ * {3}{B}
+ * Legendary Artifact
+ * {T}: Target player loses 3 life unless they sacrifice a nonland permanent or discard a card.
+ * {3}{B}: Untap Tergrid’s Lantern.
+ *
+ * @author jimga150
+ */
+public class TergridsLanternTest extends CardTestPlayerBase {
+
+    private static final String tergrid = "Tergrid, God of Fright // Tergrid's Lantern";
+    private static final String tergridFirstSide = "Tergrid, God of Fright";
+    private static final String tergridSecondSide = "Tergrid's Lantern";
+
+    @Test
+    public void testLoseLife() {
+        addCard(Zone.BATTLEFIELD, playerA, "Swamp", 4);
+        addCard(Zone.BATTLEFIELD, playerB, "Memnite", 1);
+        addCard(Zone.HAND, playerA, tergrid);
+        addCard(Zone.HAND, playerB, "Memnarch");
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, tergridSecondSide, true);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Target player");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "No"); // Sac or discard to avoid life loss?
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+
+        assertLife(playerB, currentGame.getStartingLife() - 3);
+        assertPermanentCount(playerB, "Memnite", 1);
+        assertHandCount(playerB, "Memnarch", 1);
+    }
+
+    @Test
+    public void testSacCreature() {
+        addCard(Zone.BATTLEFIELD, playerA, "Swamp", 4);
+        addCard(Zone.BATTLEFIELD, playerB, "Memnite", 1);
+        addCard(Zone.HAND, playerA, tergrid);
+        addCard(Zone.HAND, playerB, "Memnarch");
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, tergridSecondSide, true);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Target player");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "Yes"); // Sac or discard to avoid life loss?
+        setChoice(playerB, "Yes"); // Yes - Sacrifice, No - Discard
+        setChoice(playerB, "Memnite"); // To sacrifice
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+
+        assertLife(playerB, currentGame.getStartingLife());
+        assertPermanentCount(playerB, "Memnite", 0);
+        assertGraveyardCount(playerB, "Memnite", 1);
+        assertHandCount(playerB, "Memnarch", 1);
+    }
+
+    @Test
+    public void testDiscard() {
+        addCard(Zone.BATTLEFIELD, playerA, "Swamp", 4);
+        addCard(Zone.BATTLEFIELD, playerB, "Memnite", 1);
+        addCard(Zone.HAND, playerA, tergrid);
+        addCard(Zone.HAND, playerB, "Memnarch");
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, tergridSecondSide, true);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "{T}: Target player");
+        addTarget(playerA, playerB);
+        setChoice(playerB, "Yes"); // Sac or discard to avoid life loss?
+        setChoice(playerB, "No"); // Yes - Sacrifice, No - Discard
+        setChoice(playerB, "Memnarch"); // To discard
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+
+        assertLife(playerB, currentGame.getStartingLife());
+        assertPermanentCount(playerB, "Memnite", 1);
+        assertGraveyardCount(playerB, "Memnarch", 1);
+        assertHandCount(playerB, "Memnarch", 0);
+    }
+
+}

Mage/src/main/java/mage/abilities/costs/common/SacrificeAllCost.java

@@ -1,14 +1,13 @@
 package mage.abilities.costs.common;
 
 import mage.abilities.Ability;
-import mage.abilities.ActivatedAbilityImpl;
 import mage.abilities.costs.Cost;
 import mage.abilities.costs.CostImpl;
 import mage.abilities.costs.SacrificeCost;
-import mage.constants.AbilityType;
 import mage.filter.FilterPermanent;
 import mage.game.Game;
 import mage.game.permanent.Permanent;
+import mage.players.Player;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -46,19 +45,15 @@ public class SacrificeAllCost extends CostImpl implements SacrificeCost {
 
     @Override
     public boolean canPay(Ability ability, Ability source, UUID controllerId, Game game) {
-        UUID activator = controllerId;
+        Player controller = game.getPlayer(controllerId);
-        if (ability.getAbilityType().isActivatedAbility() || ability.getAbilityType() == AbilityType.SPECIAL_ACTION) {
+        if (controller == null){
-            if (((ActivatedAbilityImpl) ability).getActivatorId() != null) {
+            return false;
-                activator = ((ActivatedAbilityImpl) ability).getActivatorId();
-            }  // else, Activator not filled?
         }
-
         for (Permanent permanent : game.getBattlefield().getAllActivePermanents(filter, controllerId, game)) {
-            if (!game.getPlayer(activator).canPaySacrificeCost(permanent, source, controllerId, game)) {
+            if (!controller.canPaySacrificeCost(permanent, source, controllerId, game)) {
                 return false;
             }
         }
-
         return true;
     }
 

Mage/src/main/java/mage/abilities/costs/common/SacrificeTargetCost.java

@@ -1,15 +1,14 @@
 package mage.abilities.costs.common;
 
 import mage.abilities.Ability;
-import mage.abilities.ActivatedAbilityImpl;
 import mage.abilities.costs.Cost;
 import mage.abilities.costs.CostImpl;
 import mage.abilities.costs.SacrificeCost;
-import mage.constants.AbilityType;
 import mage.constants.Outcome;
 import mage.filter.FilterPermanent;
 import mage.game.Game;
 import mage.game.permanent.Permanent;
+import mage.players.Player;
 import mage.target.TargetPermanent;
 import mage.target.common.TargetSacrifice;
 import mage.util.CardUtil;
@@ -58,12 +57,8 @@ public class SacrificeTargetCost extends CostImpl implements SacrificeCost {
 
     @Override
     public boolean pay(Ability ability, Game game, Ability source, UUID controllerId, boolean noMana, Cost costToPay) {
-        UUID activator = controllerId;
+        // can be cancelled by user
-        if (ability.getAbilityType().isActivatedAbility() || ability.getAbilityType() == AbilityType.SPECIAL_ACTION) {
+        if (this.getTargets().choose(Outcome.Sacrifice, controllerId, source.getSourceId(), source, game)) {
-            activator = ((ActivatedAbilityImpl) ability).getActivatorId();
-        }
-        // can be cancel by user
-        if (this.getTargets().choose(Outcome.Sacrifice, activator, source.getSourceId(), source, game)) {
             for (UUID targetId : this.getTargets().get(0).getTargets()) {
                 Permanent permanent = game.getPermanent(targetId);
                 if (permanent == null) {
@@ -88,17 +83,14 @@ public class SacrificeTargetCost extends CostImpl implements SacrificeCost {
 
     @Override
     public boolean canPay(Ability ability, Ability source, UUID controllerId, Game game) {
-        UUID activator = controllerId;
+        Player controller = game.getPlayer(controllerId);
-        if (ability.getAbilityType().isActivatedAbility() || ability.getAbilityType() == AbilityType.SPECIAL_ACTION) {
+        if (controller == null){
-            if (((ActivatedAbilityImpl) ability).getActivatorId() != null) {
+            return false;
-                activator = ((ActivatedAbilityImpl) ability).getActivatorId();
-            }  // else, Activator not filled?
         }
-
         int validTargets = 0;
         int neededTargets = this.getTargets().get(0).getNumberOfTargets();
         for (Permanent permanent : game.getBattlefield().getActivePermanents(((TargetPermanent) this.getTargets().get(0)).getFilter(), controllerId, source, game)) {
-            if (game.getPlayer(activator).canPaySacrificeCost(permanent, source, controllerId, game)) {
+            if (controller.canPaySacrificeCost(permanent, source, controllerId, game)) {
                 validTargets++;
                 if (validTargets >= neededTargets) {
                     return true;
@@ -106,10 +98,7 @@ public class SacrificeTargetCost extends CostImpl implements SacrificeCost {
             }
         }
         // solves issue #8097, if a sacrifice cost is optional and you don't have valid targets, then the cost can be paid
-        if (validTargets == 0 && this.getTargets().get(0).getMinNumberOfTargets() == 0) {
+        return validTargets == 0 && this.getTargets().get(0).getMinNumberOfTargets() == 0;
-            return true;
-        }
-        return false;
     }
 
     @Override