FDN New Combat Rules #13279

ssk97 · 2025-02-01T10:36:55Z

#13031, Additional task from #12538

This is still in-progress. I'm pretty sure I broke Banding and maybe something involving multi-block creatures, will fix and add related tests before un-drafting.

This also does not currently attempt to fix #11567, might try to fit that in too.

JayDi85

Banding is rare used feature, low tested and old.

JayDi85 · 2025-02-01T10:52:16Z

Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java

@@ -2974,19 +2974,26 @@ public List<Integer> getMultiAmountWithIndividualConstraints(Outcome outcome, Li
                                                                 int totalMin, int totalMax, MultiAmountType type, Game game) {
        assertAliasSupportInChoices(false);

+        boolean skippable = (totalMin == totalMax &&


Smells bad. If game require to make a choice from a user then test code also must require it.

For a majority of tests, any value works fine. In the vast majority of those that don't, the default (dealing lethal damage to each creature in order of blocks declared) is what is wanted. I consider this comparable to manually tapping lands for mana.

Are you talking about tests from deleted choices only? Or it was made for another tests too (to simplify new choice commands setup)?

BTW tapped for mana support manual mode (by pool or by manual mana choose). It's important to keep testable code, so dev can test different order and check result

Are you talking about tests from deleted choices only? Or it was made for another tests too (to simplify new choice commands setup)?

Both, yes. Admittedly, there weren't that many tests that would've needed to be changed. One example of a test that would need changing is test_RingEffect_3_and_4_BlockSacrificeAndLifeLose, where the old combat rules require assigning all damage to the first blocker.

getMultiAmountWithIndividualConstraints already support default choice by setChoice(playerA, TestPlayer.CHOICE_SKIP)

The point is to avoid needing any setChoice at all. And yes, it does still support manual choice, as seen in WallOfEssenceTest.

Though looking closer I also have it manually being done in one of the AttackBlockRestrictionsTest where it doesn't need to be, I'll remove that (assuming we allow the defaulting as I currently do)

And yes, it does still support manual choice, as seen in WallOfEssenceTest

Nope, that's important logic from test's strict mode -- if game require to choose then dev must setup that choice (if it miss then wrong choice command error will be visible). In outdated non-strict mode AI will choose instead dev (before strict mode introduced there were ~20% of false positive tests).

Current PR's code without auto-skip line already support it:

in strict mode: dev must set setChoiceAmount or use setChoice(playerA, TestPlayer.CHOICE_SKIP);

in non strict mode: AI must make a choice (it's outdated logic and keeps for backward compatibility);

Ok then, I'll remove the skipping code and modify the tests that need it.

JayDi85 · 2025-02-01T10:53:53Z

Mage/src/main/java/mage/constants/MultiAmountType.java

@@ -12,6 +12,7 @@ public enum MultiAmountType {

    MANA("Add mana", "Distribute mana among colors"),
    DAMAGE("Assign damage", "Assign damage among targets"),
+    DAMAGE_TRAMPLE("Assign damage", "Assign damage among targets, excess damage dealt to defender"),


DAMAGE_WITH_EXCESS is better. Same excess damage naming in combat group too. It’s not trample only.

The code that calls this explicitly checks for the Trample ability. If the attacker does not have Trample, the full combat damage must be dealt to the blockers.

JayDi85 · 2025-02-01T11:16:36Z

Mage/src/main/java/mage/game/combat/CombatGroup.java

+                List<Integer> amounts;
+                if (hasTrample(attacker)){
+                    amounts = player.getMultiAmountWithIndividualConstraints(Outcome.Damage, damageDivision, damage-remainingDamage, damage, MultiAmountType.DAMAGE_TRAMPLE, game);
+                    int trampleDamage = damage-(amounts.stream().mapToInt(x -> x).sum());


There are possible infinite or big values like Integer.MAX_VALUE. If it’s a direct values from permanents/players then I recommend to use overflow protection methods like overflowInc and overflowDec from CardUtil and min/max protection to make sure it’s positive only values.

Big values should be fine, I don't think there's any possible overflow issues.

The only direct value is the original damage amount. The code for defaultDamage uses min to ensure remainingDamage never go below zero. Unless the MultiAmount constraints are somehow broken, the sum of the amounts will always be less than or equal to the damage to be dealt and as such no overflow protection should be necessary. I can add it in anyway just in case if you'd like?

However, it might be wrong if the attacker has negative power. I'll check that.

ssk97 · 2025-02-04T09:53:50Z

It could be merged as-is, but I think I want to

make it so that the "deals damage" window explicitly calls out which creature is dealing damage
look at allowing the user to choose the order of the creatures' damage choices (the cause of the test failure was the arbitrary ordering of creatures dealing combat damage)

edit: oh huh it's not arbitrary, it's seemingly random. Ugh, definitely going to need to do something about 2). Can't believe the problem hadn't come up before.

JayDi85 · 2025-02-07T19:06:46Z

Current choose dialogs already support message with permanent name and card hint popup (use attacker.getLogName or like that)
For current failed test — it’s ai test. It must pass strict check with enabled ai, look at another dialogs to use same logic.

JayDi85 · 2025-02-07T19:17:15Z

About random order.

If you talking about choice dialog result then you can modify it and control choices order, e.g. prepare and sort permanents list by name/id. Fixes example: a7480ae

If you talking about mtg rules and option to choose blocking order then it must be researched. Maybe I can modify choose dialog to support the order of elements (not sure about it). Maybe it’s better to keep default order and take it from declare blockers step.

ssk97 added 2 commits January 31, 2025 18:59

Remove all combat ordering code

5f2097a

Use MultiAmount division for damage

cc7e9c9

github-actions bot added server cards engine tests client labels Feb 1, 2025

JayDi85 reviewed Feb 1, 2025

View reviewed changes

ssk97 added 3 commits February 1, 2025 14:12

Remove damage selection division skipping in tests

55a3d29

Fix Banding, Multi-block, add tests

674f1a8

Fix test

a18aff8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FDN New Combat Rules #13279

FDN New Combat Rules #13279

ssk97 commented Feb 1, 2025 •

edited

Loading

JayDi85 left a comment

JayDi85 Feb 1, 2025

ssk97 Feb 1, 2025

JayDi85 Feb 1, 2025

JayDi85 Feb 1, 2025

ssk97 Feb 1, 2025

JayDi85 Feb 1, 2025

ssk97 Feb 1, 2025 •

edited

Loading

JayDi85 Feb 1, 2025 •

edited

Loading

ssk97 Feb 1, 2025

JayDi85 Feb 1, 2025

ssk97 Feb 1, 2025

JayDi85 Feb 1, 2025

ssk97 Feb 1, 2025

ssk97 commented Feb 4, 2025 •

edited

Loading

JayDi85 commented Feb 7, 2025

JayDi85 commented Feb 7, 2025

FDN New Combat Rules #13279

Are you sure you want to change the base?

FDN New Combat Rules #13279

Conversation

ssk97 commented Feb 1, 2025 • edited Loading

JayDi85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssk97 Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

JayDi85 Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssk97 commented Feb 4, 2025 • edited Loading

JayDi85 commented Feb 7, 2025

JayDi85 commented Feb 7, 2025

ssk97 commented Feb 1, 2025 •

edited

Loading

ssk97 Feb 1, 2025 •

edited

Loading

JayDi85 Feb 1, 2025 •

edited

Loading

ssk97 commented Feb 4, 2025 •

edited

Loading