WIP - Rework/continuous effects apply logic

[Jmlundeen]

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.

[JayDi85]

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 https://github.com/Card-Forge/forge/issues/5642 and implementation from https://github.com/Card-Forge/forge/pull/6869 — it’s a good example of required amount of work to support it in xmage too.

[JayDi85]

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

1. 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;
2. Rework layers logic to use dependency calculation with game sims and graph search instead predefined dependencies;

[Jmlundeen]

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]

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

[JayDi85]

You are fully rework all methods in all cards without any workable version. That’s wrong refactor logic. You must able to build, run and check tests at any time. So start to make compatible code, make sure it’s workable and then replace all other cards. There are 600 effects — it’s impossible to make such big PRs and review it.

As example. There are two apply methods: simple without layer param (e.g. already filtered - for single layer effects) and with layer param (effect must check layer by itself — for multi layer effects). You must take parent class like ContEffectImp, create default and empty methods like queryObjects+applyToObjects, modify original “applies” and “apply” methods like “if applyToObjects is not empty - return or else use old logic below” (both for applies and apply).

So your code will be compatible with current effects lifecycle, but you can easy to replace existing “apply” method by queryObjects+applyToObjects in single classes. Make it abstract (or like that) later - so new devs don’t forget to override it in new card/effect.

[Jmlundeen]

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

[Jmlundeen]

@JayDi85 is this current implementation more appropriate?

[Jmlundeen]

@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]

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.

[JayDi85]

@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]

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]

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]

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]

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]

@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]

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]

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.