Refactor: clean filter logic #13713

Susucre · 2025-06-01T16:41:55Z

This pass the unit tests.

I'm not that happy with the 2 overloads for add, and addExtra in addition, but that seemed like the easiest way to get through. Without that, the compiler was kinda lost:

Potential follow-ups:

move arround the Filter in filter/common to more specific folders for-permanent for-player for-stackobject, etc... Also move FilterOpponent, FilterPermanentThisOrAnother that are extending one of the type-specific FilterImpl, currently they are just in filter folder.
Not sure what to do of FilterMana, but it does not feel like it deserve to be called a Filter.
There is some weirdness of FilterSpell, that can be used on a card before it is cast. I'm not sure how to cleanup that properly going forward.
make use of 4-argument match in more places now that it is defined for all Filters. 2-argument match should be limited to a very specific usage.
I dislike the use of FilterObject, and think we can at term remove it in favor of specific Filters. You should know the general nature of the object you are filtering, or ignore implementation and use Filter if more generic.
FilterCard might want to exclude Permanent from checkObjectClass?

JayDi85

It's fine except few potential problems, see below

Mage/src/main/java/mage/filter/FilterImpl.java

JayDi85 · 2025-06-01T17:13:18Z

Mage/src/main/java/mage/filter/common/FilterPermanentOrSuspendedCard.java

-        if (o instanceof Permanent) {
-            return permanentFilter.match((Permanent) o, playerId, source, game);
-        } else if (o instanceof Card) {
-            return cardFilter.match((Card) o, playerId, source, game);


Looks buggy. New code do not filter object and apply it to all filters. Permanent is Card. So instead strict check in old code -- new multiple filters will check it for both. (it's ok for default MultiFilters logic, but bad for more strict use cases like that).

Must re-check MultiFilters replace and override that method to be more strict

The check is actually done with the checkObjectClass call in FilterImpl::match.

So for the three current usage, FilterPermanent has a (object instanceof Permanent) check, FilterPlayer has a (object instanceof Player) check, and FilterCard has a (object instanceof Card) check.

For future-proofing, the intent is "do any of the filter match the object?"

oh right I get the Permanent is Card issue. I'll think how to solve.

Uhm maybe in the future, we would want to have FilterCard exclude Permanents?
That would need proper investigation if that breaks card functionality.

We would want to have FilterCard exclude Permanents

It's can be good, but it can break current effects. I'm sure some shared effects can use card filters to apply to any objects in hand, stack and battlefield, e.g. adding additional types -- it must be checked manually:

For info: permanent extends card -- it's xmage only feature to share big amount of code between cards and permanents. It's not same by mtg rules. So strict permanent/card filters can be good.

Mage/src/main/java/mage/filter/MultiFilterImpl.java

xenohedron

I like the general direction.

I thought of another idea though for after this- what if we refactored Predicate interface to always be four argument match itself? Then we'd have only one type of predicate, no need for the ObjectSourcePlayer wrapper. Could be much cleaner in many ways.

Mage/src/main/java/mage/filter/Filter.java

Mage/src/main/java/mage/filter/FilterImpl.java

xenohedron · 2025-06-07T23:19:44Z

Mage/src/main/java/mage/filter/FilterObject.java

+import mage.filter.predicate.Predicates;

 /**
+ * // TODO: migrate all FilterObject to more specific ones, then remove this class?


I tend to agree. I think right now it's only used for filters that implement a protection ability? Could the new FilterSource be used for that instead? Not sure

From a quick look, yes protection ability is mostly what is remaining. There are a few kind of protection that FilterSource will not match (protection from players/opponents). But maybe we just need to have a protection ability that can be made out of a FilterSource or a FilterPlayer?

And there are also a few odd uses like in SpliceAbility and SnowManaCost. Nothing that couldn't be refactored down the line.

Oof, ProtectionAbility is a mess of hacks checking instanceof the generic Filter and blindly groping to figure out how it should work. That's a whole separate thing to tackle and rework to a FilterSource for everything.

Under the rules, it should still be quite possible to implement as a FilterSource, with a new type of predicate that matches objects controlled by that player, with no need for a FilterPlayer at all.

A permanent or player with protection from a specific player has protection from each object that player controls and protection from each object that player owns not controlled by another player, regardless of that object's characteristic values.

For info. From xmage side — protection logic implemented not only by “canTarget” (it’s a final destination for a check), but by “Ability source” param too. E.g. if it not target then send null instead source param. But not all choose dialogs use it. So it can be wrongly enabled/disabled in some choice dialogs.

Also some SBA do not have source info and send null in filters/dialogs. So it’s not about replace all x2 by x4 params call, but to find and fix/improve depended places too. x4 params usage must be a final goal, but it’s impossible without break/rework some hidden logic. Just try to use x4 as much as possible.

One of the possible solution — introduce global/fake ability to such places. So it will call x4 params all the time, but source info will lead to non existing/system object.

For info. From xmage side — protection logic implemented not only by “canTarget” (it’s a final destination for a check), but by “Ability source” param too. E.g. if it not target then send null instead source param. But not all choose dialogs use it. So it can be wrongly enabled/disabled in some choice dialogs.

Well, after some research -- many filters require source info and do not have null protection (search bad code by input.getSource().). All that filters will fail/raise errors with AI games. So it's better to fill source info as much as possible (e.g. call 4th params methods). I'll remove null source logic from choose dialogs (example: choose and chooseTarget) and keep nullable source for SBA actions only (when it's impossible to find a real source).

So target's notTarget field is the one and the final source of the "targetted" and "not targetted" info. Target::canTarget methods must check protections by itself or call super (it's already use that logic).

xenohedron · 2025-06-07T23:43:46Z

Mage/src/main/java/mage/filter/FilterPermanent.java


    @Override
-    public boolean match(Permanent permanent, UUID playerId, Ability source, Game game) {
-        if (!this.match(permanent, game) || !permanent.isPhasedIn()) {


is that check for phasedIn ever relevant?

In my opinion, this is a concern for the Target class to iterate on the non-phased objects.

Be careful with PhasedIn - they are poorly supported in xmage code and tests so it may be one of the few defenses here that everything rests on)))

Battlefield::getAllActivePermanents is the usual way to exclude phased out permanents.

Battlefield::getAllPermanents is the one that is not checking, and there is a bug on that every so often (but Phasing is not played that much, so it is not all that concerning.)

Just for safety, I'm adding back the check, but I think we could do without it.

I'm going to tag #12121 for tracking.

I think this is the wrong place for phased in check in the engine. Unfortunately, after investigating, it's best to leave it in place for now.

Usages that already handle phased in check and don't need this check:

Battlefield:: getActivePermanents, count, and similar

TargetPermanent:: canChoose, possibleTargets

Usages that could be relying on this:

TargetPermanent:: canTarget (though that shouldn't result in a bug in any case)

Any effect that calls getPermanent() and then checks it against a filter

Only the 4-argument match had it before this rework, so no need to add it to 2-argument match. Alternatively, is it cleaner to check in checkObjectClass instead of match?

xenohedron · 2025-06-07T23:58:44Z

Mage/src/main/java/mage/filter/FilterCard.java

+
+    @Override
+    public boolean checkObjectClass(Object object) {
+        // TODO: investigate if we can/should exclude Permanent here.


I vaguely recall that FilterCard is sometimes used for Spells. It's possible that it's (mis)used somewhere for Permanents as well, but I'm not really sure how to investigate

xenohedron · 2025-06-08T00:05:59Z

Mage/src/main/java/mage/filter/FilterSpell.java

-        return super.match(stackObject, playerId, source, game);
+    public boolean checkObjectClass(Object object) {
+        return object instanceof Spell
+                || object instanceof Card; // TODO: investigate. Is sometimes used for checking a spell's characteristic before cast


I think it's just normal usage to check characteristics, as you often need to check before the Spell is actually created. also Spell implements Card interface. Not sure if there's really an action here

Ideally, I would like to FilterSpell to only work on Spells (for stuff like 'counter target creature spell' or 'Spells you control can't be countered'), and would introduce another class for effects that affect spells and cards about to be cast (like 'Nonartifact spells you cast have improvise' or 'You may cast a spell with mana value 4 or less from your hand without paying its mana cost.').

Maybe that is too much refactor work for too little gain. I find it a little weird to use a FilterSpell with a TargetCard though.

I could get behind that - distinction between a Spell on the stack, and something that could be cast. I'd want to see a detailed proposal for how it would work though. In any case, not in scope here

Mage/src/main/java/mage/filter/MultiFilterImpl.java

Mage/src/main/java/mage/filter/Filter.java

xenohedron · 2025-06-08T04:17:01Z

Mage/src/main/java/mage/filter/FilterImpl.java

        return predicates;
    }
+
+    public List<ObjectSourcePlayerPredicate<E>> getExtraPredicates() {


remember to add @Override where appropriate

xenohedron · 2025-06-08T06:23:58Z

(sorry, tried to address a merge conflict and screwed it up, should be fixed properly now)

Susucre · 2025-06-08T14:22:47Z

I thought of another idea though for after this- what if we refactored Predicate interface to always be four argument match itself? Then we'd have only one type of predicate, no need for the ObjectSourcePlayer wrapper. Could be much cleaner in many ways.

I do agree. We still have to carefully refactor a bunch of code to remove the use of the 2-argument match all over the codebase.

Susucre · 2025-06-08T15:47:23Z

So I'm not entirely sure the 2-argument match in FilterPermanent added with last commit is a good idea.
It caused some tests to fail, that I fixed in that commit with a few checkObjectClass before calls for the 2-argument match.

This is in the territory of hard to set right overloads with abstract types, so I'm not saying it can not be improved, but for now I do not see how to do it nicely. I'd like to have filters have a failsafe overload for any object, that does class checkObjectClass before attempting to match, but it is hard to set up.

Examples of tests that failed with ManaPool sourceFilters before adding checkObjectClass:

- fixed game errors with source related filters (related to #13713); - fixed game freeze in hand's cards selection (related to #13290); - fixed game freeze in target amount selection with X=0 (related to #13290);

Susucre added 2 commits June 1, 2025 17:17

initial implementation

6e85b2e

more refactor, add MultiFilterImpl

19b8a80

github-actions bot added cards engine labels Jun 1, 2025

JayDi85 approved these changes Jun 1, 2025

View reviewed changes

JayDi85 reviewed Jun 1, 2025

View reviewed changes

Mage/src/main/java/mage/filter/MultiFilterImpl.java Outdated Show resolved Hide resolved

apply review

d1f190a

xenohedron reviewed Jun 8, 2025

View reviewed changes

xenohedron force-pushed the refactor/clean-filter-logic branch from edbd847 to d1f190a Compare June 8, 2025 06:22

Merge branch 'master' into refactor/clean-filter-logic

836caec

apply more review changes

141d78a

Refactor: clean filter logic #13713

Are you sure you want to change the base?

Refactor: clean filter logic #13713

Uh oh!

Conversation

Susucre commented Jun 1, 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!

Uh oh!

JayDi85 Jun 1, 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!

Susucre Jun 1, 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!

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!

Uh oh!

xenohedron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

JayDi85 Jun 14, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xenohedron commented Jun 8, 2025

Uh oh!

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

Uh oh!

Susucre commented Jun 1, 2025 •

edited

Loading

JayDi85 Jun 1, 2025 •

edited

Loading

Susucre Jun 1, 2025 •

edited

Loading

JayDi85 Jun 14, 2025 •

edited

Loading

Susucre commented Jun 8, 2025 •

edited

Loading

Susucre commented Jun 8, 2025 •

edited

Loading