WIP - Rework/continuous effects apply logic #13689

Jmlundeen · 2025-05-28T03:57:16Z

This is a rework for continuous effects to use queryAffectedObjects and applyToObjects instead of apply. This is part 1 of 2 to rework continuous effects layering and dependencies.

* apply effect is now simpler * when applying a layer, dependency checks are now dynamic * applied effects are also tracked during apply to have the engine handle rule 613.6 instead of cards

* Added method to get if an effect is character defining * Added method to check if sublayer matches to help simplify applying effects * Added filter variables for cards, permanents and stackObjects to help determine objects * Added queryAffectedObjects to get the effects potential set of objects * Added calculateResult which should return an expected value to help determine dependency. (e.g. Necrotic Ooze returns the number of abilities it will gain)

…n apply effects

…ayer test

…layer test

JayDi85

Few important notes and possible problems of the new logic

JayDi85 · 2025-05-28T04:16:58Z

Mage/src/main/java/mage/abilities/effects/ContinuousEffects.java

+        DefaultDirectedGraph<ContinuousEffect, DefaultEdge> dependencyGraph = new DefaultDirectedGraph<>(DefaultEdge.class);
+        Game gameSim;
+        try {
+            gameSim = game.copy();


Run game sim with game cycle before apply each game’s effect — that’s very bad and super slow. Can be allowed for debug mode only, not for each game cycle.

If you apply only single effect to find a real result then it can be ok, but if you apply all other effects in game sim — that’s exponentially slow.

Yeah, this is purely for simulating what happens to the affected objects from applying one effect at a time. This gets called too frequently to try and do a full application. Memoization for dependency order would also be good to prevent recalculating the same set of effects.

For current code I'm used GameView/GameState compare to find out changed objects and characteristics after each effect apply. It was able to construct full layers with all objects and effects in apply order and with all change history (per layer, per effect, per object). Just as proof of work for potential test mode's GUI tool for layers debugging/research.

That would be useful to have. Instead of copying the game, would bookmarking and doing a gamestate compare with the original game be ok? I was copying the game to be safe, but having a copy of the gamestate before doing changes to restore would probably be fine? I noticed the simulation games don't do anything for restore state.

Yes, it's ok for your tasks here (just copy a game state to compare). When you copy game object then you are create a fully independent "game" and can change data inside it without worry about changes in parent game. If you want to simulate some actions then copy it as game sim -- it's will create independent game object without human interactions in choice dialogs, so you can apply effects and other things to "look ahead".

But do not use "bookmark" code for such tasks -- it's a part of rollback feature and save copy of game state for potential restore on user's request, game error or cancel action. All bookmarked game states store inside current game and copying with it. More bookmarks -- more saved game states and more memory used by a game.

For example:

you have effects list to apply/check;

copy real game as game sim;

copy and cycle effects and call apply with copied game param;

check copied game for changed data;

real game will be safe;

JayDi85 · 2025-05-28T04:21:54Z

Mage/src/main/java/mage/abilities/effects/ContinuousEffect.java

+
+    FilterStackObject getAffectedStackObjectFilter();
+
+    FilterCard getAffectedCardFilter();


It doesn’t help to find really affected objects - xmage effects uses “apply” and custom code logic inside it, not filters only. There are 600 effects to rework to support it.

JayDi85 · 2025-05-28T07:14:03Z

For info: it’s a new forge’s approach (only few months in development) and they do not finish it yet (with performance issues, with mtg rules compatibility). Also their’s approach requires to rework all cards with layer effects too. See main and related issues from Card-Forge/forge#5642 and implementation from Card-Forge/forge#6869 — it’s a good example of required amount of work to support it in xmage too.

JayDi85 · 2025-05-28T08:43:01Z

I recommend to split it to 2 big tasks/pr:

Rework continuous effects to use affected objects logic (split single apply(layer) to objects getAffectedObjects(layer) and applyToObjects(layer, objects)) -- it can be used with current layers and code base, and it will be useful anyway, e.g. for layers debugging or some GUI or card hints tools;
Rework layers logic to use dependency calculation with game sims and graph search instead predefined dependencies;

Jmlundeen · 2025-05-28T17:13:33Z

I can see splitting to getAffectedObjects and applyToObjects working and could revert dependency changes to turn this branch/PR into reworking to affected objects logic first. This split approach will help later with dependency comparing "results" of an effect as well.

jeffwadsworth · 2025-05-29T13:21:45Z

Just a prudent test. The following cards are played in this order: Opalescence, Humility, Opalescence. They should be 4/4, 4/4/, 1/1.

…players can be affected

Jmlundeen · 2025-06-03T23:23:55Z

Gotcha, I misunderstood what you were saying. You wanted the queryObjects and applyToObjects to be inside of the apply methods? I don't think these are necessary for applies since those aren't related to layers, at least I don't see any layered effects that use applies. Those could be changed though. I'm pushing some changes now that should align with what you were asking for.

Changed applyToObjects to be void
Changed queryAffectedObjects to return a Map
Added affectedObjectMap <UUID, MageItem> to ContinuousEffectsImpl
Refactored a few sample effects

…for effects

… when copying

Jmlundeen · 2025-06-07T13:50:26Z

@JayDi85 is this current implementation more appropriate?

JayDi85 · 2025-06-07T20:07:58Z

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

-        Permanent licid = source.getSourcePermanentIfItStillExists(game);
-        if (licid != null) {
+    public Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game) {
+        if (!affectedObjectMap.isEmpty()) {


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.

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?

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).

JayDi85 · 2025-06-07T20:13:13Z

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

+        if (spell != null && spell.getCard().getSecondCardFace() != null) {
+            affectedObjectMap.put(spell.getId(), spell);
+        }
+        return affectedObjectMap;


Good example of weird code. Original idea -- queryAffectedObjects must return new lists all the time and do not store it inside effect. Now it can return one list, but store another one.

It's very strange. Maybe you need it for multiple effects call and collect full affected objects list from all effect's calls. I don't know. If true then it must has comments and some research -- how to find and store fully affected list.

JayDi85 · 2025-06-07T20:14:25Z

Mage/src/main/java/mage/abilities/effects/ContinuousEffectImpl.java

        this.characterDefining = effect.characterDefining;
        this.nextTurnNumber = effect.nextTurnNumber;
        this.effectStartingStepNum = effect.effectStartingStepNum;
+        this.affectedObjectMap.putAll(effect.affectedObjectMap);


muse use deep copy

JayDi85 · 2025-06-07T20:15:14Z

Mage/src/main/java/mage/abilities/effects/ContinuousEffect.java


+    void applyToObjects(Layer layer, SubLayer sublayer, Ability source, Game game, Map<UUID, MageItem> objects);
+
+    Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game);


New methods must have some docs on usage/override

JayDi85 · 2025-06-07T20:21:46Z

Mage/src/main/java/mage/abilities/effects/ContinuousEffects.java

        List<ContinuousEffect> layerEffects = new ArrayList<>();
        for (ContinuousEffect effect : layeredEffects) {
+            if (timestampGroupName.equals("main")) {
+                effect.clearAffectedObjectMap();


well, then I don't understand -- why you need to store full affected objects list instead create it by single call (I don't see any usages of it outside of queryAffectedObjects + applyToObjects cycle

* queryAffectedObjects returns boolean * add docs for applyToObjects and queryAffectedObjects * add default apply(game, source) function, new query logic doesn't need it

Jmlundeen · 2025-06-08T17:27:31Z

@JayDi85 OK, I did some more tweaking. The only thing I'm not completely sold on is discard() location for the single layer effects. Since those don't really need to implement the apply(game, source) with this iteration, I have discard() in the applyToObjects. Another alternative was overriding apply to check for discard, but that seemed worse.

JayDi85 · 2025-06-08T17:39:31Z

It's ok to call effects's discard at any moment -- it's just mark effect as outdated, so effect will be removed on next clean up (e.g. on next game cycle). Must be used by effects with non standard duration or if it looks up for some linked object and that's object doesn't exists anymore, so effect is useless.

* refactored ContinuousEffects starting with "C" and their children * created BecomesXXConstructSourceEffect common effect

# Conflicts: # Mage.Sets/src/mage/cards/a/AettirAndPriwen.java # Mage.Sets/src/mage/cards/b/BlueDragon.java # Mage.Sets/src/mage/cards/k/KondasBanner.java # Mage.Sets/src/mage/cards/t/ThranWeaponry.java # Mage.Sets/src/mage/cards/t/TideShaper.java

* Refactor "D" continuous effects

JayDi85 · 2025-07-09T19:30:34Z

@Jmlundeen do not modify all cards. Take only few and make it all workable and testable (all tests must work fine too). E.g. game engine must support both code styles (old with apply and new with query). It's will be impossible to review your changes to the cards soon (there are already 200+ files in PR). It's important to keep tests workable while refactor too (if it fail then something broken and must be research before continue).

Jmlundeen · 2025-07-09T20:36:44Z

Both styles are currently supported and I'm making sure to run the tests as well. Most of these changes are small as well. I'll just hold off on pushing more until I get the greenlight.

ssk97 · 2025-07-09T22:03:51Z

If I understand correctly, this change is to fix the Blood Moon problem of entering permanents not having effects applied until after they hit the battlefield, correct?

Will this also fix the problem of casting transformed spells? See DisturbTest.test_SpellAttributesIndirectCostModifications.

Jmlundeen · 2025-07-09T22:19:35Z

This is just to rework the logic for how continuous effects are applied by splitting the logic into gathering objects and applying to a list of objects. This will provide setup for adding dynamic dependencies and cleaning up the layers logic after.

The "Blood Moon" issue isn't being addressed here.

I'm not sure about the transformed spells, but I don't think so, since this is only touching how continuous effects are applied.

ssk97 · 2025-07-09T22:38:50Z

If you're doing a major rework of continuous effects like this, it'd probably be a good idea to consider those problems and, if not fix them immediately, to make it much easier to do so in the future. Both the Blood Moon problem and the Transformed Spells problems are about continuous effects not applying to objects early enough: Blood Moon being permanents entering the battlefield, Transformed Spells being spells entering the stack. I think that'd involve having some way to apply all continuous effects that should apply to a given object that hasn't yet entered the relevant zone.

JayDi85 · 2025-07-09T23:01:23Z

@ssk97 from another discussions: original task - replace manual effects dependency to auto-dependency due game sim. It’s big rework and can stuck with many problems. So whole work is to split code by multiple PRs:

prepare game engine to use query and objects logic (after rework it will be able to find really affected objects per effect on each layer). It’s not fix any bugs;
migrate effects from “apply” to “query” logic (~600) cards. It’s not fix any bugs;
implement auto-dependency logic. It’s can fix many combo related bugs but not etb/bloodmoon problem;

ssk97 · 2025-07-09T23:09:11Z

My point though is that with some minor changes, it should be possible for this change to also fix those two problems. I haven't studied it closely so I might be missing something, but if it had some query function that checked a single object as well as the current queryAffectedObjects, it'd be possible to fix the issues by checking and applying the relevant effects to the object before it's placed in the relevant zone.

Jmlundeen · 2025-07-10T22:20:36Z

I don't quite understand what 'it' refers to that would have a query function? But those can, and probably should be, solved separately. This won't change the timings of effects getting applied, like with permanents entering. If the battlefield class was reworked to include permanents entering, and effects were applied once a permanent enters that zone, is one option.

Jmlundeen added 10 commits May 27, 2025 21:34

ContinuousEffects: Update timestamping

b7e1f3a

ContinuousEffects: Refactor apply and add dependency checking logic

083bb47

* apply effect is now simpler * when applying a layer, dependency checks are now dynamic * applied effects are also tracked during apply to have the engine handle rule 613.6 instead of cards

GameImpl: add null check on playerList when trying to restore state i…

9dd2656

…n apply effects

MageObjectReference: Add method to return generic MageObject

1f89cb2

Dependency: Add jgrapht to determine continuous effect dependency cycles

c4508e7

Update March of the Machines, Humility, Mycosynth Lattice and their l…

7bd2100

…ayer test

Update Blood Moon, (Urborg) AddBasicLandTypeAllLandsEffect and their …

8a35b7a

…layer test

Update Necrotic Ooze and yixlid Jailer

5411166

Move TODO for calculateResult to method

a564176

github-actions bot added cards engine tests dev labels May 28, 2025

JayDi85 reviewed May 28, 2025

View reviewed changes

Jmlundeen added 5 commits May 29, 2025 09:06

Revert dependency changes

a565c93

Merge branch 'master' into rework/continuous-effects-layers

48b9a6b

ContinuousEffects: Add queryAffectedObjects and applyToObjects methods

7cc245c

refactor common effects

f679b87

refactor more common effects

c5baa4a

Jmlundeen changed the title ~~WIP - Rework/continuous effects layers~~ WIP - Rework/continuous effects apply logic May 31, 2025

Jmlundeen added 5 commits May 31, 2025 15:56

refactor more common effects

0d7246a

refactor more common effects

0e6661c

refactor more common effects

36fd6d2

refactor more common effects

02d1d27

ContinuousEffect: Update query and apply to use List<MageItem> since …

a8b0c8d

…players can be affected

Jmlundeen added 3 commits June 3, 2025 17:59

Revert incorrect changes

3d30999

Update continuous effects with applyToObjects and queryAffectedObjects

38c0a08

Refactor some continuous effects

b6112e7

Jmlundeen added 5 commits June 4, 2025 08:13

Add test for EnduringGlimmerTriggeredAbility

99673c7

fix affectedObjectsMap checking for single layer effects

59012a8

fix ConditionalContinuousEffect to correctly clear affectedObjectMap …

26aa703

…for effects

Refactor some more effects

19f22f1

Fix affectedObjectMap wrongly assigning instead of adding keys/values…

1487559

… when copying

JayDi85 reviewed Jun 7, 2025

View reviewed changes

Jmlundeen added 2 commits June 8, 2025 12:01

Merge branch 'refs/heads/master' into rework/continuous-effects-layers

6851dc1

Readjust query+applyTo logic

09af4b6

* queryAffectedObjects returns boolean * add docs for applyToObjects and queryAffectedObjects * add default apply(game, source) function, new query logic doesn't need it

Jmlundeen added 6 commits June 9, 2025 12:06

Refactor more effects

bc2db46

Refactor more effects

3eb9d24

A little more refactoring

102c18d

I "C" more refactors ahead

0c7df6f

* refactored ContinuousEffects starting with "C" and their children * created BecomesXXConstructSourceEffect common effect

"D"on't stop refactoring

64401be

* Refactor "D" continuous effects

Jmlundeen marked this pull request as ready for review July 10, 2025 22:20


		FilterStackObject getAffectedStackObjectFilter();

		FilterCard getAffectedCardFilter();


		void applyToObjects(Layer layer, SubLayer sublayer, Ability source, Game game, Map<UUID, MageItem> objects);

		Map<UUID, MageItem> queryAffectedObjects(Layer layer, Ability source, Game game);

WIP - Rework/continuous effects apply logic #13689

Are you sure you want to change the base?

WIP - Rework/continuous effects apply logic #13689

Uh oh!

Conversation

Jmlundeen commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JayDi85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayDi85 May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayDi85 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayDi85 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayDi85 May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayDi85 commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JayDi85 commented May 28, 2025

Uh oh!

Jmlundeen commented May 28, 2025

Uh oh!

jeffwadsworth commented May 29, 2025

Uh oh!

Jmlundeen commented Jun 3, 2025

Uh oh!

Jmlundeen commented Jun 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jmlundeen commented Jun 8, 2025

Uh oh!

JayDi85 commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JayDi85 commented Jul 9, 2025

Uh oh!

Jmlundeen commented Jul 9, 2025

Uh oh!

ssk97 commented Jul 9, 2025

Uh oh!

Jmlundeen commented Jul 9, 2025

Uh oh!

ssk97 commented Jul 9, 2025

Uh oh!

JayDi85 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jmlundeen commented May 28, 2025 •

edited

Loading

JayDi85 May 28, 2025 •

edited

Loading

JayDi85 May 29, 2025 •

edited

Loading

JayDi85 May 29, 2025 •

edited

Loading

JayDi85 May 28, 2025 •

edited

Loading

JayDi85 commented May 28, 2025 •

edited

Loading

JayDi85 commented Jun 8, 2025 •

edited

Loading

JayDi85 commented Jul 9, 2025 •

edited

Loading