Skip to content

WIP - Rework/continuous effects apply logic #13689

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
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

WIP - Rework/continuous effects apply logic #13689

Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b7e1f3a
ContinuousEffects: Update timestamping
Jmlundeen May 24, 2025
083bb47
ContinuousEffects: Refactor apply and add dependency checking logic
Jmlundeen May 28, 2025
e3d147e
ContinuousEffect: Add methods for dynamic dependency checks
Jmlundeen May 28, 2025
9dd2656
GameImpl: add null check on playerList when trying to restore state i…
Jmlundeen May 28, 2025
1f89cb2
MageObjectReference: Add method to return generic MageObject
Jmlundeen May 28, 2025
c4508e7
Dependency: Add jgrapht to determine continuous effect dependency cycles
Jmlundeen May 28, 2025
7bd2100
Update March of the Machines, Humility, Mycosynth Lattice and their l…
Jmlundeen May 28, 2025
8a35b7a
Update Blood Moon, (Urborg) AddBasicLandTypeAllLandsEffect and their …
Jmlundeen May 28, 2025
5411166
Update Necrotic Ooze and yixlid Jailer
Jmlundeen May 28, 2025
a564176
Move TODO for calculateResult to method
Jmlundeen May 28, 2025
a565c93
Revert dependency changes
Jmlundeen May 29, 2025
48b9a6b
Merge branch 'master' into rework/continuous-effects-layers
Jmlundeen May 29, 2025
7cc245c
ContinuousEffects: Add queryAffectedObjects and applyToObjects methods
Jmlundeen May 29, 2025
f679b87
refactor common effects
Jmlundeen May 29, 2025
c5baa4a
refactor more common effects
Jmlundeen May 31, 2025
0d7246a
refactor more common effects
Jmlundeen May 31, 2025
0e6661c
refactor more common effects
Jmlundeen Jun 1, 2025
36fd6d2
refactor more common effects
Jmlundeen Jun 1, 2025
02d1d27
refactor more common effects
Jmlundeen Jun 1, 2025
a8b0c8d
ContinuousEffect: Update query and apply to use List<MageItem> since …
Jmlundeen Jun 1, 2025
efd717b
refactor more common effects
Jmlundeen Jun 2, 2025
bb37304
refactor more common effects
Jmlundeen Jun 2, 2025
34b0e5f
Merge branch 'master' into rework/continuous-effects-layers
Jmlundeen Jun 2, 2025
3d30999
Revert incorrect changes
Jmlundeen Jun 3, 2025
38c0a08
Update continuous effects with applyToObjects and queryAffectedObjects
Jmlundeen Jun 3, 2025
b6112e7
Refactor some continuous effects
Jmlundeen Jun 3, 2025
99673c7
Add test for EnduringGlimmerTriggeredAbility
Jmlundeen Jun 4, 2025
59012a8
fix affectedObjectsMap checking for single layer effects
Jmlundeen Jun 4, 2025
26aa703
fix ConditionalContinuousEffect to correctly clear affectedObjectMap …
Jmlundeen Jun 5, 2025
19f22f1
Refactor some more effects
Jmlundeen Jun 5, 2025
1487559
Fix affectedObjectMap wrongly assigning instead of adding keys/values…
Jmlundeen Jun 6, 2025
6851dc1
Merge branch 'refs/heads/master' into rework/continuous-effects-layers
Jmlundeen Jun 8, 2025
09af4b6
Readjust query+applyTo logic
Jmlundeen Jun 8, 2025
bc2db46
Refactor more effects
Jmlundeen Jun 9, 2025
3eb9d24
Refactor more effects
Jmlundeen Jun 11, 2025
102c18d
A little more refactoring
Jmlundeen Jun 13, 2025
0c7df6f
I "C" more refactors ahead
Jmlundeen Jul 8, 2025
e64a10b
Merge branch 'master' into rework/continuous-effects-layers
Jmlundeen Jul 9, 2025
64401be
"D"on't stop refactoring
Jmlundeen Jul 9, 2025
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
43 changes: 43 additions & 0 deletions ...Tests/src/test/java/org/mage/test/cards/triggers/EnduringGlimmerTriggeredAbilityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package org.mage.test.cards.triggers;

import mage.abilities.keyword.HasteAbility;
import mage.constants.CardType;
import mage.constants.PhaseStep;
import mage.constants.Zone;
import org.junit.Test;
import org.mage.test.serverside.base.CardTestPlayerBase;

public class EnduringGlimmerTriggeredAbilityTest extends CardTestPlayerBase {
// Enchantment Creature - Dog Glimmer - 3/3
// Whenever another creature you control enters, it gets +2/+0 and gains haste until end of turn.
// When Enduring Courage dies, if it was a creature, return it to the battlefield under its owner's control. It's an enchantment.
static String courage = "Enduring Courage";
// Deal 3 damage to any target
static String bolt = "Lightning Bolt";
// Creature - Bear - 2/2
static String cub = "Bear Cub";

@Test
public void testEnduringCourage() {
setStrictChooseMode(true);

addCard(Zone.BATTLEFIELD, playerA, courage);
addCard(Zone.HAND, playerA, bolt);
addCard(Zone.HAND, playerA, cub);
addCard(Zone.BATTLEFIELD, playerA, "Taiga", 3);

castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, cub);
waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);

checkAbility("Bear Cub has haste", 1, PhaseStep.PRECOMBAT_MAIN, playerA, cub, HasteAbility.class, true);
checkPT("Bear Cub has +2/+0", 1, PhaseStep.PRECOMBAT_MAIN, playerA, cub, 2 + 2, 2);

castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, bolt, courage);
waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);

checkType("Courage is just an enchantment",1, PhaseStep.PRECOMBAT_MAIN, playerA, courage, CardType.CREATURE, false);

setStopAt(1, PhaseStep.END_TURN);
execute();
}
}
74 changes: 46 additions & 28 deletions Mage/src/main/java/mage/abilities/common/AnimateDeadTriggeredAbility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package mage.abilities.common;

import mage.MageItem;
import mage.MageObjectReference;
import mage.abilities.Ability;
import mage.abilities.DelayedTriggeredAbility;
Expand All @@ -21,11 +22,7 @@
import mage.util.CardUtil;
import mage.watchers.Watcher;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.UUID;
import java.util.*;

/**
* @author LevelX2, awjackson
Expand Down Expand Up @@ -103,33 +100,54 @@ public void init(Ability source, Game game) {
}

@Override
public boolean apply(Layer layer, SubLayer sublayer, Ability source, Game game) {
Permanent permanent = affectedObjectList.get(0).getPermanent(game);
if (permanent == null) {
discard();
return true;
public Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game) {
if (!affectedObjectMap.isEmpty()) {
return affectedObjectMap;
}
switch (layer) {
case TypeChangingEffects_4:
if (becomesAura) {
permanent.addSubType(game, SubType.AURA);
}
break;
case AbilityAddingRemovingEffects_6:
if (!becomesAura) {
List<Ability> toRemove = new ArrayList<>();
for (Ability ability : permanent.getAbilities(game)) {
if (ability instanceof EnchantAbility &&
ability.getRule().equals("Enchant creature card in a graveyard")) {
toRemove.add(ability);
for (MageObjectReference mor : affectedObjectList) {
Permanent permanent = mor.getPermanent(game);
if (permanent != null) {
affectedObjectMap.put(permanent.getId(), permanent);
}
}
return affectedObjectMap;
}

@Override
public void applyToObjects(Layer layer, SubLayer sublayer, Ability source, Game game, Map<UUID, MageItem> objects) {
for (MageItem object : objects.values()) {
Permanent permanent = (Permanent) object;
switch (layer) {
case TypeChangingEffects_4:
if (becomesAura) {
permanent.addSubType(game, SubType.AURA);
}
break;
case AbilityAddingRemovingEffects_6:
if (!becomesAura) {
List<Ability> toRemove = new ArrayList<>();
for (Ability ability : permanent.getAbilities(game)) {
if (ability instanceof EnchantAbility &&
ability.getRule().equals("Enchant creature card in a graveyard")) {
toRemove.add(ability);
}
}
permanent.removeAbilities(toRemove, source.getSourceId(), game);
}
permanent.removeAbilities(toRemove, source.getSourceId(), game);
}
permanent.addAbility(newAbility, source.getSourceId(), game);
permanent.getSpellAbility().getTargets().clear();
permanent.getSpellAbility().getTargets().add(newTarget);
permanent.addAbility(newAbility, source.getSourceId(), game);
permanent.getSpellAbility().getTargets().clear();
permanent.getSpellAbility().getTargets().add(newTarget);
}
}
}

@Override
public boolean apply(Layer layer, SubLayer sublayer, Ability source, Game game) {
if (queryAffectedObjects(layer, source, game).isEmpty()) {
discard();
return true;
}
applyToObjects(layer, sublayer, source, game, affectedObjectMap);
return true;
}

Expand Down
34 changes: 29 additions & 5 deletions Mage/src/main/java/mage/abilities/common/EnduringGlimmerTriggeredAbility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package mage.abilities.common;

import mage.MageItem;
import mage.MageObjectReference;
import mage.abilities.Ability;
import mage.abilities.effects.ContinuousEffectImpl;
Expand All @@ -11,6 +12,9 @@
import mage.players.Player;
import mage.target.targetpointer.FixedTarget;

import java.util.Map;
import java.util.UUID;

/**
* @author TheElk801, PurpleCrowbar
*/
Expand Down Expand Up @@ -80,16 +84,36 @@ public EnduringGlimmerTypeEffect copy() {
return new EnduringGlimmerTypeEffect(this);
}


@Override
public boolean apply(Game game, Ability source) {
public Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game) {
if (!affectedObjectMap.isEmpty()) {
return affectedObjectMap;
}
Permanent permanent = game.getPermanent(getTargetPointer().getFirst(game, source));
if (permanent == null) {
if (permanent != null) {
affectedObjectMap.put(permanent.getId(), permanent);
}
return affectedObjectMap;
}

@Override
public void applyToObjects(Layer layer, SubLayer sublayer, Ability source, Game game, Map<UUID, MageItem> objects) {
for (MageItem object : objects.values()) {
Permanent permanent = (Permanent) object;
permanent.retainAllEnchantmentSubTypes(game);
permanent.removeAllCardTypes(game);
permanent.addCardType(game, CardType.ENCHANTMENT);
}
}

@Override
public boolean apply(Game game, Ability source) {
if (queryAffectedObjects(layer, source, game).isEmpty()) {
discard();
return false;
}
permanent.retainAllEnchantmentSubTypes(game);
permanent.removeAllCardTypes(game);
permanent.addCardType(game, CardType.ENCHANTMENT);
applyToObjects(layer, sublayer, source, game, affectedObjectMap);
return true;
}
}
70 changes: 45 additions & 25 deletions Mage/src/main/java/mage/abilities/common/GiveManaAbilityAndCastSourceAbility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package mage.abilities.common;

import mage.MageItem;
import mage.MageObjectReference;
import mage.abilities.Ability;
import mage.abilities.ActivatedAbilityImpl;
Expand Down Expand Up @@ -100,40 +101,59 @@ public GainManaAbilitiesWhileExiledEffect copy() {
return new GainManaAbilitiesWhileExiledEffect(this);
}

@Override
public Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game) {
if (!affectedObjectMap.isEmpty()) {
return affectedObjectMap;
}
Permanent permanent = game.getPermanent(getTargetPointer().getFirst(game, source));
if (permanent != null) {
affectedObjectMap.put(permanent.getId(), permanent);
}
return affectedObjectMap;
}

@Override
public void applyToObjects(Layer layer, SubLayer sublayer, Ability source, Game game, Map<UUID, MageItem> objects) {
for (MageItem object : objects.values()) {
Permanent permanent = (Permanent) object;
for (char c : colors.toCharArray()) {
Ability ability;
switch (c) {
case 'W':
ability = new WhiteManaAbility();
break;
case 'U':
ability = new BlueManaAbility();
break;
case 'B':
ability = new BlackManaAbility();
break;
case 'R':
ability = new RedManaAbility();
break;
case 'G':
ability = new GreenManaAbility();
break;
default:
continue;
}
permanent.addAbility(ability, source.getSourceId(), game);
}
}
}

@Override
public boolean apply(Game game, Ability source) {
if (WasCastFromExileWatcher.check((MageObjectReference) getValue("exiledHandCardRef"), game)) {
discard();
return false;
}
Permanent permanent = game.getPermanent(getTargetPointer().getFirst(game, source));
if (permanent == null) {
if (queryAffectedObjects(layer, source, game).isEmpty()) {
discard();
return false;
}
for (char c : colors.toCharArray()) {
Ability ability;
switch (c) {
case 'W':
ability = new WhiteManaAbility();
break;
case 'U':
ability = new BlueManaAbility();
break;
case 'B':
ability = new BlackManaAbility();
break;
case 'R':
ability = new RedManaAbility();
break;
case 'G':
ability = new GreenManaAbility();
break;
default:
continue;
}
permanent.addAbility(ability, source.getSourceId(), game);
}
applyToObjects(layer, sublayer, source, game, affectedObjectMap);
return true;
}
}
Expand Down
27 changes: 24 additions & 3 deletions Mage/src/main/java/mage/abilities/common/LicidAbility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package mage.abilities.common;

import mage.MageItem;
import mage.abilities.Ability;
import mage.abilities.ActivatedAbilityImpl;
import mage.abilities.SpecialAction;
Expand All @@ -21,6 +22,7 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.UUID;

/**
Expand Down Expand Up @@ -102,9 +104,21 @@ public boolean apply(Game game, Ability source) {
}

@Override
public boolean apply(Layer layer, SubLayer sublayer, Ability source, Game game) {
Permanent licid = source.getSourcePermanentIfItStillExists(game);
if (licid != null) {
public Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game) {
if (!affectedObjectMap.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

If you call queryAffectedObjects then it must build affectedObjectMap every time from scratch. If you catch performance issue or duplication bugs -- then it's the problem of caller code, not queryAffectedObjects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, I think I will remove the map from ContinuousEffectImpl. I don't think it will be used outside of apply and dependency checking later. I think in that case it could be changed back to a list of objects. Unless you think it would still benefit from being a map?

Copy link
Member

Choose a reason for hiding this comment

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

Unless you think it would still benefit from being a map?

If you don't need it in existing effects then do not add -- use simple List and cycle code.

P.S. I recommend to view improved approach -- create objects list outside and pass it to query method. So real effects must simply fill it without create/clean code in each effect.

Possible code -- I don't build it, just an idea -- make empty apply method (2 params) in ContinuousEffectImpl and insert query code in main apply method (4 params). So It must keep workable all old effects, but allow to migrate to new query logic for new/modified effects (you must remove apply method from effect and replace it by query and applyToObject methods).
shot_250608_102438
shot_250608_102445

return affectedObjectMap;
}
Permanent permanent = source.getSourcePermanentIfItStillExists(game);
if (permanent != null) {
affectedObjectMap.put(permanent.getId(), permanent);
}
return affectedObjectMap;
}

@Override
public void applyToObjects(Layer layer, SubLayer sublayer, Ability source, Game game, Map<UUID, MageItem> objects) {
for (MageItem object : objects.values()) {
Permanent licid = (Permanent) object;
switch (layer) {
case TypeChangingEffects_4:
licid.removeAllCardTypes(game);
Expand All @@ -131,6 +145,13 @@ public boolean apply(Layer layer, SubLayer sublayer, Ability source, Game game)
licid.getSpellAbility().getTargets().clear();
licid.getSpellAbility().getTargets().add(target);
}
}
}

@Override
public boolean apply(Layer layer, SubLayer sublayer, Ability source, Game game) {
if (!queryAffectedObjects(layer, source, game).isEmpty()) {
applyToObjects(layer, sublayer, source, game, affectedObjectMap);
return true;
}
discard();
Expand Down
Loading