Skip to content

Extend original ID concept to apply to effects as well as abilities #14854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
matoro wants to merge 2 commits into
base: master
Choose a base branch
from
+78 −6
Open

Extend original ID concept to apply to effects as well as abilities #14854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5794905
Extend original ID concept to apply to effects as well as abilities
matoro May 3, 2026
83c8563
Add additional test for multiple instances from a clone
matoro May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion Mage.Tests/src/test/java/org/mage/test/cards/copy/CloneTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,5 +286,38 @@ public void testCloningFaceDownCreature() {
assertPermanentCount(playerA, EmptyNames.FACE_DOWN_CREATURE.getTestCommand(), 2);
assertPowerToughness(playerA, EmptyNames.FACE_DOWN_CREATURE.getTestCommand(), 2, 2, Filter.ComparisonScope.All);
}


@Test
public void testSivitri() {
addCard(Zone.BATTLEFIELD, playerA, "Oath of Teferi");
addCard(Zone.BATTLEFIELD, playerA, "Sivitri, Dragon Master");
addCard(Zone.HAND, playerA, "Spark Double");
addCard(Zone.BATTLEFIELD, playerA, "Island", 4);
addCard(Zone.BATTLEFIELD, playerB, "Balduvian Bears");
addCard(Zone.HAND, playerA, "Fateful Absence");
addCard(Zone.BATTLEFIELD, playerA, "Plains", 4);

castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Spark Double");
setChoice(playerA, true);
setChoice(playerA, "Sivitri, Dragon Master");
waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Fateful Absence");
addTarget(playerA, "Sivitri, Dragon Master[no copy]");
waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "+1: Until");
waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "+1: Until");

attack(2, playerB, "Balduvian Bears", playerA);
setChoice(playerB, "Sivitri");
setChoice(playerB, true);
setChoice(playerB, true);

setStrictChooseMode(true);
setStopAt(2, PhaseStep.POSTCOMBAT_MAIN);
execute();

assertLife(playerA, 18);
assertLife(playerB, 16);
}
}
29 changes: 29 additions & 0 deletions Mage.Tests/src/test/java/org/mage/test/cards/replacement/PropagandaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,33 @@ public void test_Propaganda2() {
assertTappedCount("Plains", true, 4);
}

// https://github.com/magefree/mage/issues/10372
// https://github.com/magefree/mage/issues/14754
@Test
public void testClone() {
addCard(Zone.BATTLEFIELD, playerB, "Archangel of Tithes");
addCard(Zone.HAND, playerA, "Clone");
addCard(Zone.BATTLEFIELD, playerA, "Island", 4);
addCard(Zone.BATTLEFIELD, playerB, "Balduvian Bears");
addCard(Zone.BATTLEFIELD, playerB, "Wastes");
addCard(Zone.HAND, playerA, "Fell");
addCard(Zone.BATTLEFIELD, playerA, "Swamp", 4);

castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Clone");
setChoice(playerA, true);
setChoice(playerA, "Archangel of Tithes");
castSpell(1, PhaseStep.POSTCOMBAT_MAIN, playerA, "Fell");
addTarget(playerA, "Archangel of Tithes[no copy]");

attack(2, playerB, "Balduvian Bears");
setChoice(playerB, true);

setStrictChooseMode(true);
setStopAt(2, PhaseStep.POSTCOMBAT_MAIN);
execute();

assertLife(playerA, 18);
assertTappedCount("Balduvian Bears", true, 1);
}

}
10 changes: 5 additions & 5 deletions Mage/src/main/java/mage/abilities/effects/ContinuousEffects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,8 @@ public boolean replaceEvent(GameEvent event, Game game) {
// Remove all consumed effects (ability dependant)
for (Iterator<ReplacementEffect> it1 = rEffects.keySet().iterator(); it1.hasNext(); ) {
ReplacementEffect entry = it1.next();
if (consumed.containsKey(entry.getId()) /*&& !(entry instanceof CommanderReplacementEffect) */) { // 903.9.
Set<UUID> consumedAbilitiesIds = consumed.get(entry.getId());
if (consumed.containsKey(entry.getOriginalId()) /*&& !(entry instanceof CommanderReplacementEffect) */) { // 903.9.
Set<UUID> consumedAbilitiesIds = consumed.get(entry.getOriginalId());
if (rEffects.get(entry) == null || consumedAbilitiesIds.size() == rEffects.get(entry).size()) {
it1.remove();
} else {
Expand Down Expand Up @@ -944,8 +944,8 @@ public boolean replaceEvent(GameEvent event, Game game) {

// add the applied effect to the consumed effects
if (rEffect != null) {
if (consumed.containsKey(rEffect.getId())) {
Set<UUID> set = consumed.get(rEffect.getId());
if (consumed.containsKey(rEffect.getOriginalId())) {
Copy link
Copy Markdown
Member

@JayDi85 JayDi85 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bad solution. Instead research and fix wrong ApplyEffects usage or effects logic -- it's try to skip it. OriginalId concept for linked abilities, not for game engine.

Whole permanent logic with apply effects uses idempotency -- you must able to call it multiple times and catch same game state result. Not different.

Copy link
Copy Markdown
Member

@JayDi85 JayDi85 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also require review of original issue #10372 and unfinished changes with copy abilities ids in #11077. It can depends on each other.

Copy link
Copy Markdown
Contributor Author

@matoro matoro May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the tapping of lands to pay for the tax that triggers the apply, see:

INFO  2026-05-22 01:41:02,088 [LOG][GAME] T2.DA: PlayerB using choice: Yes (by setChoice, ability: Archangel of Tithes [ed0]) =>[main] PrintGameLogsDataCollector.writeLog
INFO  2026-05-22 01:41:02,091 applying effects                                                                           =>[main] GameState.applyEffects
java.lang.Exception
        at mage.game.GameState.applyEffects(GameState.java:669)
        at mage.game.GameImpl.applyEffects(GameImpl.java:1975)
        at mage.abilities.AbilityImpl.activate(AbilityImpl.java:276)
        at mage.abilities.ActivatedAbilityImpl.activate(ActivatedAbilityImpl.java:227)
        at mage.abilities.Ability.activate(Ability.java:297)
        at mage.players.PlayerImpl.playManaAbility(PlayerImpl.java:1525)
        at mage.players.PlayerImpl.activateAbility(PlayerImpl.java:1669)
        at mage.player.ai.ComputerPlayer.playManaHandling(ComputerPlayer.java:578)
        at mage.player.ai.ComputerPlayer.playMana(ComputerPlayer.java:393)
        at org.mage.test.player.TestPlayer.playMana(TestPlayer.java:4415)
        at mage.abilities.costs.mana.ManaCostsImpl.pay(ManaCostsImpl.java:146)
        at mage.abilities.costs.mana.ManaCostsImpl.payOrRollback(ManaCostsImpl.java:173)
        at mage.abilities.effects.PayCostToAttackBlockEffectImpl.handleManaCosts(PayCostToAttackBlockEffectImpl.java:115)
        at mage.abilities.effects.PayCostToAttackBlockEffectImpl.replaceEvent(PayCostToAttackBlockEffectImpl.java:93)
        at mage.abilities.effects.ContinuousEffects.replaceEvent(ContinuousEffects.java:936)
        at mage.game.GameState.replaceEvent(GameState.java:1054)
        at mage.game.GameState.replaceEvent(GameState.java:1047)
        at mage.game.GameImpl.replaceEvent(GameImpl.java:3511)
        at mage.game.combat.Combat.declareAttacker(Combat.java:1468)
        at mage.players.PlayerImpl.declareAttacker(PlayerImpl.java:2863)
        at org.mage.test.player.TestPlayer.selectAttackers(TestPlayer.java:1978)
        at mage.game.combat.Combat.selectAttackers(Combat.java:271)
        at mage.game.turn.DeclareAttackersStep.beginStep(DeclareAttackersStep.java:36)
        at mage.game.turn.Phase.prePriority(Phase.java:189)
        at mage.game.turn.Phase.playStep(Phase.java:203)
        at mage.game.turn.Phase.play(Phase.java:91)
        at mage.game.turn.Turn.play(Turn.java:132)
        at mage.game.GameImpl.playTurn(GameImpl.java:1180)
        at mage.game.GameImpl.play(GameImpl.java:1087)
        at mage.game.GameImpl.start(GameImpl.java:1063)
        at org.mage.test.serverside.base.impl.CardTestPlayerAPIImpl.execute(CardTestPlayerAPIImpl.java:324)
        at org.mage.test.cards.copy.CloneTest.testCloneAttackTax(CloneTest.java:309)

so how do we go about this? tapping the lands is an ability being activated, so I do imagine this needs to be here to trigger abilities e.g. Manabarbs.

Set<UUID> set = consumed.get(rEffect.getOriginalId());
if (rAbility != null) {
set.add(rAbility.getId());

Expand All @@ -955,7 +955,7 @@ public boolean replaceEvent(GameEvent event, Game game) {
if (rAbility != null) { // in case of AuraReplacementEffect or PlaneswalkerReplacementEffect there is no Ability
set.add(rAbility.getId());
}
consumed.put(rEffect.getId(), set);
consumed.put(rEffect.getOriginalId(), set);
}
}
// Must be called here for some effects to be able to work correctly
Expand Down
2 changes: 2 additions & 0 deletions Mage/src/main/java/mage/abilities/effects/Effect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ public interface Effect extends Serializable, Copyable<Effect> {

UUID getId();

UUID getOriginalId();

void newId();

/**
Expand Down
8 changes: 8 additions & 0 deletions Mage/src/main/java/mage/abilities/effects/EffectImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
public abstract class EffectImpl implements Effect {

protected UUID id;
protected UUID originalId;
protected Outcome outcome;
protected EffectType effectType;

Expand All @@ -30,13 +31,15 @@ public abstract class EffectImpl implements Effect {

public EffectImpl(Outcome outcome) {
this.id = UUID.randomUUID();
this.originalId = this.id;
this.outcome = outcome;

initNewTargetPointer();
}

protected EffectImpl(final EffectImpl effect) {
this.id = effect.id;
this.originalId = effect.id;
this.outcome = effect.outcome;
this.staticText = effect.staticText;
this.effectType = effect.effectType;
Expand All @@ -61,6 +64,11 @@ public UUID getId() {
return id;
}

@Override
public UUID getOriginalId() {
return originalId;
}

@Override
public String getText(Mode mode) {
return staticText;
Expand Down
Loading