Remove duplicate code for X costs (#12551)

* Replace "([a-zA-Z]+).getManaCostsToPay().getX()" with CardUtil.getSourceCostsTag(game, $1, "X", 0)
Fix Disrupting Shoal

* Change final card .getX() calls

* Condense all ManacostVariableValue enum values into "instance"

* Further removal of getX, Display X symbol for non-mana X cards

* Fix test

* Fully remove ManaCosts.getX

* Replace all different X dynamic values with GetXValue

* Remove individual cards checking getAmount for X values (leaving cost reduction that does not use X)

* Add null check for game object inside getSourceCostsTagsMap

* fix build errors

* fix Vicious Betrayal

* text fix

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

@@ -272,7 +272,7 @@ public abstract class AbilityImpl implements Ability {
         // For Flashback ability can be set X before, so the X costs have to be restored for the flashbacked ability
         if (noMana) {
             if (!this.getManaCostsToPay().getVariableCosts().isEmpty()) {
-                int xValue = this.getManaCostsToPay().getX();
+                int xValue = CardUtil.getSourceCostsTag(game, this, "X", 0);
                 this.clearManaCostsToPay();
                 VariableManaCost xCosts = new VariableManaCost(VariableCostType.ADDITIONAL);
                 // no x events - rules from Unbound Flourishing:
@@ -438,7 +438,7 @@ public abstract class AbilityImpl implements Ability {
             game.informPlayers(announceString);
         }
         if (variableManaCost != null) {
-            int xValue = getManaCostsToPay().getX();
+            int xValue = CardUtil.getSourceCostsTag(game, this, "X", 0);
             game.informPlayers(controller.getLogName() + " announces a value of " + xValue + " for " + variableManaCost.getText()
                     + CardUtil.getSourceLogName(game, this));
         }

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

@@ -3,8 +3,6 @@ package mage.abilities;
 import mage.ApprovingObject;
 import mage.MageIdentifier;
 import mage.MageObject;
-import mage.abilities.costs.Cost;
-import mage.abilities.costs.VariableCost;
 import mage.abilities.costs.mana.ManaCost;
 import mage.abilities.costs.mana.VariableManaCost;
 import mage.abilities.keyword.FlashAbility;
@@ -251,7 +249,6 @@ public class SpellAbility extends ActivatedAbilityImpl {
 
     public int getConvertedXManaCost(Card card) {
         int xMultiplier = 0;
-        int amount = 0;
         if (card == null) {
             return 0;
         }
@@ -265,18 +262,11 @@ public class SpellAbility extends ActivatedAbilityImpl {
         }
 
         // mana cost final X value
-        boolean hasNonManaXCost = false;
-        for (Cost cost : getCosts()) {
-            if (cost instanceof VariableCost) {
-                hasNonManaXCost = true;
-                amount = ((VariableCost) cost).getAmount();
-                break;
-            }
-        }
-
-        if (!hasNonManaXCost) {
-            amount = getManaCostsToPay().getX();
+        Map<String, Object> tagMap = this.getCostsTagMap();
+        if (tagMap == null) {
+            return 0;
         }
+        int amount = (int) tagMap.getOrDefault("X", 0);
         return amount * xMultiplier;
     }
 

Mage/src/main/java/mage/abilities/common/TurnFaceUpAbility.java

@@ -13,6 +13,7 @@ import mage.counters.CounterType;
 import mage.game.Game;
 import mage.game.permanent.Permanent;
 import mage.players.Player;
+import mage.util.CardUtil;
 
 /**
  * @author LevelX2
@@ -74,7 +75,7 @@ class TurnFaceUpEffect extends OneShotEffect {
                     if (megamorph) {
                         sourcePermanent.addCounters(CounterType.P1P1.createInstance(), source.getControllerId(), source, game);
                     }
-                    game.getState().setValue(source.getSourceId().toString() + "TurnFaceUpX", source.getManaCostsToPay().getX());
+                    game.getState().setValue(source.getSourceId().toString() + "TurnFaceUpX", CardUtil.getSourceCostsTag(game, source, "X", 0));
                     return true;
                 }
             }

Mage/src/main/java/mage/abilities/costs/mana/ManaCosts.java

@@ -19,10 +19,12 @@ public interface ManaCosts<T extends ManaCost> extends List<T>, ManaCost {
 
     List<VariableCost> getVariableCosts();
 
+    /**
+     *
+     * @return if the mana cost contains an X - note that this is specifically for *mana* X costs
+     */
     boolean containsX();
 
-    int getX();
-
     /**
      * @param xValue final X value -- announced X * xMultiplier, where xMultiplier can be changed by replace events like Unbound Flourishing)
      * @param xPay   real number of pay amount (x * xMultiplier * xInstances, where xInstances is number of {X} in pay like 1, 2, 3)

Mage/src/main/java/mage/abilities/costs/mana/ManaCostsImpl.java

@@ -267,16 +267,6 @@ public class ManaCostsImpl<T extends ManaCost> extends ArrayList<T> implements M
         return !getVariableCosts().isEmpty();
     }
 
-    @Override
-    public int getX() {
-        int amount = 0;
-        List<VariableCost> variableCosts = getVariableCosts();
-        if (!variableCosts.isEmpty()) {
-            amount = variableCosts.get(0).getAmount();
-        }
-        return amount;
-    }
-
     @Override
     public void setX(int xValue, int xPay) {
         List<VariableCost> variableCosts = getVariableCosts();
@@ -430,8 +420,12 @@ public class ManaCostsImpl<T extends ManaCost> extends ArrayList<T> implements M
 
                     // TODO: checks Word of Command with Unbound Flourishing's X multiplier
                     // TODO: checks Word of Command with {X}{X} cards
-                    int xValue = referenceCosts.getX();
-                    this.setX(xValue, xValue);
+                    int amount = 0;
+                    List<VariableCost> variableCosts = getVariableCosts();
+                    if (!variableCosts.isEmpty()) {
+                        amount = variableCosts.get(0).getAmount();
+                    }
+                    this.setX(amount, amount);
 
                     player.getManaPool().restoreMana(pool.getPoolBookmark());
                     game.bookmarkState();

Mage/src/main/java/mage/abilities/dynamicvalue/common/ExileFromHandCostCardConvertedMana.java

@@ -8,10 +8,11 @@ import mage.abilities.dynamicvalue.DynamicValue;
 import mage.abilities.effects.Effect;
 import mage.cards.Card;
 import mage.game.Game;
+import mage.util.CardUtil;
 
 /**
  * Calculates the converted mana costs of a card that was exiled from hand as
- * cost. If no card was exiled the getManaCostsToPay().getX() will be used as
+ * cost. If no card was exiled CardUtil.getSourceCostsTag(game, the, "X", 0) will be used as
  * value.
  *
  * @author LevelX2
@@ -30,7 +31,7 @@ public enum ExileFromHandCostCardConvertedMana implements DynamicValue {
                 return xValue;
             }
         }
-        return sourceAbility.getManaCostsToPay().getX();
+        return CardUtil.getSourceCostsTag(game, sourceAbility, "X", 0);
     }
 
     @Override

Mage/src/main/java/mage/abilities/dynamicvalue/common/GetKickerXValue.java

@@ -1,39 +0,0 @@
-package mage.abilities.dynamicvalue.common;
-
-import mage.abilities.Ability;
-import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.effects.Effect;
-import mage.game.Game;
-import mage.util.CardUtil;
-
-
-/**
- * Kicker {X}
- *
- * @author JayDi85
- */
-public enum GetKickerXValue implements DynamicValue {
-    instance;
-
-    @Override
-    public int calculate(Game game, Ability sourceAbility, Effect effect) {
-        // Currently identical logic to the Manacost X value
-        // which should be fine since you can only have one X at a time
-        return CardUtil.getSourceCostsTag(game, sourceAbility, "X", 0);
-    }
-
-    @Override
-    public GetKickerXValue copy() {
-        return GetKickerXValue.instance;
-    }
-
-    @Override
-    public String toString() {
-        return "X";
-    }
-
-    @Override
-    public String getMessage() {
-        return "";
-    }
-}

Mage/src/main/java/mage/abilities/dynamicvalue/common/GetXLoyaltyValue.java

@@ -1,34 +0,0 @@
-package mage.abilities.dynamicvalue.common;
-
-import mage.abilities.Ability;
-import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.effects.Effect;
-import mage.game.Game;
-import mage.util.CardUtil;
-
-/**
- * @author TheElk801
- */
-public enum GetXLoyaltyValue implements DynamicValue {
-    instance;
-
-    @Override
-    public int calculate(Game game, Ability sourceAbility, Effect effect) {
-        return CardUtil.getSourceCostsTag(game, sourceAbility, "X", 0);
-    }
-
-    @Override
-    public GetXLoyaltyValue copy() {
-        return instance;
-    }
-
-    @Override
-    public String toString() {
-        return "X";
-    }
-
-    @Override
-    public String getMessage() {
-        return "";
-    }
-}

Mage/src/main/java/mage/abilities/dynamicvalue/common/GetXValue.java

@@ -6,9 +6,6 @@ import mage.abilities.effects.Effect;
 import mage.game.Game;
 import mage.util.CardUtil;
 
-/**
- * @author BetaSteward_at_googlemail.com
- */
 public enum GetXValue implements DynamicValue {
     instance;
 

Mage/src/main/java/mage/abilities/dynamicvalue/common/ManacostVariableValue.java

@@ -1,36 +0,0 @@
-package mage.abilities.dynamicvalue.common;
-
-import mage.abilities.Ability;
-import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.effects.Effect;
-import mage.game.Game;
-import mage.util.CardUtil;
-
-public enum ManacostVariableValue implements DynamicValue {
-    //TODO: all three of these variants plus GetXValue, GetKickerXValue, and GetXLoyaltyValue use the same logic
-    // and should be consolidated into a single instance
-    REGULAR, // if you need X on cast/activate (in stack) - reset each turn
-    ETB, // if you need X after ETB (in battlefield) - reset each turn
-    END_GAME; // if you need X until end game - keep data forever
-
-
-    @Override
-    public int calculate(Game game, Ability sourceAbility, Effect effect) {
-        return CardUtil.getSourceCostsTag(game, sourceAbility, "X", 0);
-    }
-
-    @Override
-    public ManacostVariableValue copy() {
-        return this;
-    }
-
-    @Override
-    public String toString() {
-        return "X";
-    }
-
-    @Override
-    public String getMessage() {
-        return "";
-    }
-}

Mage/src/main/java/mage/abilities/dynamicvalue/common/RemovedCountersForCostValue.java

@@ -1,40 +0,0 @@
-package mage.abilities.dynamicvalue.common;
-
-import mage.abilities.Ability;
-import mage.abilities.costs.Cost;
-import mage.abilities.costs.common.RemoveVariableCountersSourceCost;
-import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.effects.Effect;
-import mage.game.Game;
-
-/**
- * @author LevelX2
- */
-public enum RemovedCountersForCostValue implements DynamicValue {
-    instance;
-
-    @Override
-    public int calculate(Game game, Ability sourceAbility, Effect effect) {
-        for (Cost cost : sourceAbility.getCosts()) {
-            if (cost instanceof RemoveVariableCountersSourceCost) {
-                return ((RemoveVariableCountersSourceCost) cost).getAmount();
-            }
-        }
-        return 0;
-    }
-
-    @Override
-    public String getMessage() {
-        return "";
-    }
-
-    @Override
-    public RemovedCountersForCostValue copy() {
-        return RemovedCountersForCostValue.instance;
-    }
-
-    @Override
-    public String toString() {
-        return "X";
-    }
-}

Mage/src/main/java/mage/abilities/effects/common/search/SearchLibraryGraveyardWithLessMVPutIntoPlay.java

@@ -2,10 +2,10 @@ package mage.abilities.effects.common.search;
 
 import mage.MageObject;
 import mage.abilities.Ability;
-import mage.constants.ComparisonType;
 import mage.abilities.effects.OneShotEffect;
 import mage.cards.Card;
 import mage.cards.CardsImpl;
+import mage.constants.ComparisonType;
 import mage.constants.Outcome;
 import mage.constants.Zone;
 import mage.filter.FilterCard;
@@ -14,6 +14,7 @@ import mage.game.Game;
 import mage.players.Player;
 import mage.target.TargetCard;
 import mage.target.common.TargetCardInLibrary;
+import mage.util.CardUtil;
 
 /**
  * @author antoni-g
@@ -50,7 +51,7 @@ public class SearchLibraryGraveyardWithLessMVPutIntoPlay extends OneShotEffect {
         if (controller != null && sourceObject != null) {
             // create x cost filter
             FilterCard advancedFilter = filter.copy(); // never change static objects so copy the object here before
-            advancedFilter.add(new ManaValuePredicate(ComparisonType.FEWER_THAN, source.getManaCostsToPay().getX() + 1));
+            advancedFilter.add(new ManaValuePredicate(ComparisonType.FEWER_THAN, CardUtil.getSourceCostsTag(game, source, "X", 0) + 1));
 
             if (controller.chooseUse(outcome, "Search your library for a " + filter.getMessage() + " with mana value X or less" + '?', source, game)) {
                 TargetCardInLibrary target = new TargetCardInLibrary(advancedFilter);

Mage/src/main/java/mage/abilities/effects/common/search/SearchLibraryWithLessCMCPutInPlayEffect.java

@@ -2,9 +2,9 @@
 package mage.abilities.effects.common.search;
 
 import mage.abilities.Ability;
-import mage.constants.ComparisonType;
 import mage.abilities.effects.OneShotEffect;
 import mage.cards.Card;
+import mage.constants.ComparisonType;
 import mage.constants.Outcome;
 import mage.constants.Zone;
 import mage.filter.FilterCard;
@@ -41,7 +41,7 @@ public class SearchLibraryWithLessCMCPutInPlayEffect extends OneShotEffect {
         Player controller = game.getPlayer(source.getControllerId());
         if (controller != null) {
             FilterCard advancedFilter = filter.copy(); // never change static objects so copy the object here before
-            advancedFilter.add(new ManaValuePredicate(ComparisonType.FEWER_THAN, source.getManaCostsToPay().getX() + 1));
+            advancedFilter.add(new ManaValuePredicate(ComparisonType.FEWER_THAN, CardUtil.getSourceCostsTag(game, source, "X", 0) + 1));
             TargetCardInLibrary target = new TargetCardInLibrary(advancedFilter);
             if (controller.searchLibrary(target, source, game)) {
                 if (!target.getTargets().isEmpty()) {

Mage/src/main/java/mage/abilities/keyword/MonstrosityAbility.java

@@ -4,7 +4,6 @@ import mage.abilities.Ability;
 import mage.abilities.ActivatedAbilityImpl;
 import mage.abilities.costs.CostAdjuster;
 import mage.abilities.costs.mana.ManaCostsImpl;
-import mage.abilities.effects.Effect;
 import mage.abilities.effects.OneShotEffect;
 import mage.abilities.hint.common.MonstrousHint;
 import mage.constants.Outcome;
@@ -108,7 +107,7 @@ class BecomeMonstrousSourceEffect extends OneShotEffect {
         int monstrosityValue = ((MonstrosityAbility) source).getMonstrosityValue();
         // handle monstrosity = X
         if (monstrosityValue == Integer.MAX_VALUE) {
-            monstrosityValue = source.getManaCostsToPay().getX();
+            monstrosityValue = CardUtil.getSourceCostsTag(game, source, "X", 0);
         }
         permanent.addCounters(
                 CounterType.P1P1.createInstance(monstrosityValue),

Mage/src/main/java/mage/abilities/keyword/RavenousAbility.java

@@ -5,7 +5,7 @@ import mage.abilities.common.EntersBattlefieldAbility;
 import mage.abilities.common.EntersBattlefieldTriggeredAbility;
 import mage.abilities.condition.Condition;
 import mage.abilities.decorator.ConditionalInterveningIfTriggeredAbility;
-import mage.abilities.dynamicvalue.common.ManacostVariableValue;
+import mage.abilities.dynamicvalue.common.GetXValue;
 import mage.abilities.effects.common.DrawCardSourceControllerEffect;
 import mage.abilities.effects.common.EntersBattlefieldWithXCountersEffect;
 import mage.counters.CounterType;
@@ -48,6 +48,6 @@ enum RavenousAbilityCondition implements Condition {
 
     @Override
     public boolean apply(Game game, Ability source) {
-        return ManacostVariableValue.ETB.calculate(game, source, null) >= 5;
+        return GetXValue.instance.calculate(game, source, null) >= 5;
     }
 }

Mage/src/main/java/mage/abilities/keyword/RenownAbility.java

@@ -85,7 +85,7 @@ class BecomesRenownedSourceEffect extends OneShotEffect {
             int renownValue = ((RenownAbility) source).getRenownValue();
             // handle renown = X
             if (renownValue == Integer.MAX_VALUE) {
-                renownValue = source.getManaCostsToPay().getX();
+                renownValue = CardUtil.getSourceCostsTag(game, source, "X", 0);
             }
             new AddCountersSourceEffect(CounterType.P1P1.createInstance(renownValue), true).apply(game, source);
             permanent.setRenowned(true);

Mage/src/main/java/mage/abilities/keyword/SuspendAbility.java

@@ -269,7 +269,7 @@ class SuspendExileEffect extends OneShotEffect {
         if (controller.moveCardToExileWithInfo(card, exileId, "Suspended cards of "
                 + controller.getName(), source, game, Zone.HAND, true)) {
             if (suspend == Integer.MAX_VALUE) {
-                suspend = source.getManaCostsToPay().getX();
+                suspend = CardUtil.getSourceCostsTag(game, source, "X", 0);
             }
             card.addCounters(CounterType.TIME.createInstance(suspend), source.getControllerId(), source, game);
             game.informPlayers(controller.getLogName()

Mage/src/main/java/mage/game/command/emblems/MomirEmblem.java

@@ -20,6 +20,7 @@ import mage.game.Game;
 import mage.game.command.Emblem;
 import mage.game.permanent.token.Token;
 import mage.game.permanent.token.custom.CreatureToken;
+import mage.util.CardUtil;
 import mage.util.RandomUtil;
 import mage.util.functions.CopyTokenFunction;
 
@@ -70,7 +71,7 @@ class MomirEffect extends OneShotEffect {
 
     @Override
     public boolean apply(Game game, Ability source) {
-        int value = source.getManaCostsToPay().getX();
+        int value = CardUtil.getSourceCostsTag(game, source, "X", 0);
         if (game.isSimulation()) {
             // Create dummy token to prevent multiple DB find cards what causes H2 java.lang.IllegalStateException if AI cancels calculation because of time out
             Token token = new CreatureToken(value, value + 1);

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

@@ -1268,7 +1268,7 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
         if (this.isPlaneswalker(game)) {
             int loyalty;
             if (this.getStartingLoyalty() == -2) {
-                loyalty = source.getManaCostsToPay().getX();
+                loyalty = CardUtil.getSourceCostsTag(game, source, "X", 0);
             } else {
                 loyalty = this.getStartingLoyalty();
             }
@@ -1285,7 +1285,7 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
         if (this.isBattle(game)) {
             int defense;
             if (this.getStartingDefense() == -2) {
-                defense = source.getManaCostsToPay().getX();
+                defense = CardUtil.getSourceCostsTag(game, source, "X", 0);
             } else {
                 defense = this.getStartingDefense();
             }

Mage/src/main/java/mage/target/targetadjustment/PowerTargetAdjuster.java

@@ -3,7 +3,7 @@ package mage.target.targetadjustment;
 import mage.MageObject;
 import mage.abilities.Ability;
 import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.dynamicvalue.common.ManacostVariableValue;
+import mage.abilities.dynamicvalue.common.GetXValue;
 import mage.constants.ComparisonType;
 import mage.filter.Filter;
 import mage.filter.predicate.mageobject.PowerPredicate;
@@ -29,7 +29,7 @@ public class PowerTargetAdjuster extends GenericTargetAdjuster {
     }
 
     public PowerTargetAdjuster(ComparisonType comparison) {
-        this(ManacostVariableValue.REGULAR, comparison);
+        this(GetXValue.instance, comparison);
     }
 
 

Mage/src/main/java/mage/target/targetadjustment/ToughnessTargetAdjuster.java

@@ -3,7 +3,7 @@ package mage.target.targetadjustment;
 import mage.MageObject;
 import mage.abilities.Ability;
 import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.dynamicvalue.common.ManacostVariableValue;
+import mage.abilities.dynamicvalue.common.GetXValue;
 import mage.constants.ComparisonType;
 import mage.filter.Filter;
 import mage.filter.predicate.mageobject.ToughnessPredicate;
@@ -29,7 +29,7 @@ public class ToughnessTargetAdjuster extends GenericTargetAdjuster {
     }
 
     public ToughnessTargetAdjuster(ComparisonType comparison) {
-        this(ManacostVariableValue.REGULAR, comparison);
+        this(GetXValue.instance, comparison);
     }
 
     @Override

Mage/src/main/java/mage/target/targetadjustment/XManaValueTargetAdjuster.java

@@ -1,6 +1,6 @@
 package mage.target.targetadjustment;
 
-import mage.abilities.dynamicvalue.common.ManacostVariableValue;
+import mage.abilities.dynamicvalue.common.GetXValue;
 import mage.constants.ComparisonType;
 
 /**
@@ -9,10 +9,10 @@ import mage.constants.ComparisonType;
 public class XManaValueTargetAdjuster extends ManaValueTargetAdjuster {
 
     public XManaValueTargetAdjuster() {
-        super(ManacostVariableValue.REGULAR, ComparisonType.EQUAL_TO);
+        super(GetXValue.instance, ComparisonType.EQUAL_TO);
     }
 
     public XManaValueTargetAdjuster(ComparisonType comparison) {
-        super(ManacostVariableValue.REGULAR, comparison);
+        super(GetXValue.instance, comparison);
     }
 }

Mage/src/main/java/mage/target/targetadjustment/XTargetsCountAdjuster.java

@@ -1,6 +1,6 @@
 package mage.target.targetadjustment;
 
-import mage.abilities.dynamicvalue.common.ManacostVariableValue;
+import mage.abilities.dynamicvalue.common.GetXValue;
 
 /**
  * @author notgreat
@@ -8,6 +8,6 @@ import mage.abilities.dynamicvalue.common.ManacostVariableValue;
 public class XTargetsCountAdjuster extends TargetsCountAdjuster {
 
     public XTargetsCountAdjuster() {
-        super(ManacostVariableValue.REGULAR);
+        super(GetXValue.instance);
     }
 }

Mage/src/main/java/mage/util/CardUtil.java

@@ -1737,6 +1737,9 @@ public final class CardUtil {
      */
     public static Map<String, Object> getSourceCostsTagsMap(Game game, Ability source) {
         Map<String, Object> costTags;
+        if (game == null) {
+            return null;
+        }
 
         // from spell ability - direct access
         costTags = source.getCostsTagMap();

Mage/src/main/java/mage/util/ManaUtil.java

@@ -10,7 +10,7 @@ import mage.abilities.costs.VariableCostType;
 import mage.abilities.costs.common.TapSourceCost;
 import mage.abilities.costs.mana.*;
 import mage.abilities.dynamicvalue.DynamicValue;
-import mage.abilities.dynamicvalue.common.ManacostVariableValue;
+import mage.abilities.dynamicvalue.common.GetXValue;
 import mage.abilities.effects.Effect;
 import mage.abilities.mana.*;
 import mage.cards.AdventureCard;
@@ -689,7 +689,7 @@ public final class ManaUtil {
 
     public static ManaCost createManaCost(DynamicValue genericManaCount, Game game, Ability sourceAbility, Effect effect) {
         int costValue = genericManaCount.calculate(game, sourceAbility, effect);
-        if (genericManaCount instanceof ManacostVariableValue) {
+        if (genericManaCount instanceof GetXValue) {
             // variable (X must be final value after all events and effects)
             return createManaCost(costValue, true);
         } else {