magefree / mage

Magic Another Game Engine
http://xmage.today
MIT License
1.86k stars 762 forks source link

Combat causes too many life loss triggers #10805

Open AaronMcGarry opened 1 year ago

AaronMcGarry commented 1 year ago

Exquisite Blood and similar cards should only trigger once if a player is damaged by multiple creatures in the same combat, right now it triggers once for each creature.

Susucre commented 1 year ago

[[Exquisite Blood]]

github-actions[bot] commented 1 year ago

Exquisite Blood - (Gatherer) (Scryfall) (EDHREC)

{4}{B} Enchantment Whenever an opponent loses life, you gain that much life.

Susucre commented 1 year ago

Indeed, the triggers should use a batched Event for those life loss. Not sure we have one at the ready.

xenohedron commented 1 year ago

Oof, that could be a substantial rework to implement. Might have to change logic in CombatGroup. Maybe related to #10773 .

Looking at 510.2 I don't see a straightforward rules reference describing how this works though... can anyone point to a ruling?

AaronMcGarry commented 1 year ago

I found a few places that say that it works that way: https://www.reddit.com/r/magicTCG/comments/91e7f5/exquisite_blood_rules_question/ https://www.mtgsalvation.com/forums/magic-fundamentals/magic-rulings/magic-rulings-archives/540011-exquisite-blood-archangel-of-thune

AaronMcGarry commented 1 year ago

Just ran a test with [[Rakdos Charm]] where the opponent controlled two creatures and it triggered multiple times. Pretty sure this should have the same behavior, but I couldn't find a specific ruling online.

github-actions[bot] commented 1 year ago

Rakdos Charm - (Gatherer) (Scryfall) (EDHREC)

{B}{R} Instant Choose one — • Exile target player's graveyard. • Destroy target artifact. • Each creature deals 1 damage to its controller.

AaronMcGarry commented 1 year ago

It looks like rakdos charm should also trigger just once https://www.mtgsalvation.com/forums/magic-fundamentals/magic-rulings/829907-rakdos-charm-damage

Susucre commented 1 year ago

Ok there is no official ruling for that it seems.

Relevant rulings on damage:

Combat damage is done simulatenously.

510.2. Second, all combat damage that's been assigned is dealt simultaneously. [...]

Damage is done to player in the form of life loss, should keep the simultaenous part?

120.3a. Damage dealt to a player by a source without infect causes that player to lose that much life.

Quite irrelevant here, but life loss is taken into accont when damage is processed.

120.4c. Third, damage that's been dealt is processed into its results, as modified by replacement effects that interact with those results (such as life loss or counters).

I did not find anything relevant to describe if that particular trigger event is batching or not, so it is not clear to me if there is batching or not.

AaronMcGarry commented 1 year ago

It looks like Arena handles it as one trigger. https://youtu.be/hg7CS40n0fw?t=994

xenohedron commented 1 year ago

Also see #8725

JayDi85 commented 10 months ago

Exquisite Blood and similar cards should only trigger once if a player is damaged by multiple creatures in the same combat

Yes, in combat phase must be only one event with life lose (after all damage assigned and applied), see rule 510.1e about combat damage assignment:

510.1e Once a player has assigned combat damage from each attacking or blocking creature he or she controls, the total damage assignment (not solely the damage assignment of any individual attacking or blocking creature) is checked to see if it complies with the above rules. If it doesn’t, the combat damage assignment is illegal; the game returns to the moment before that player began to assign combat damage. (See rule 717, “Handling Illegal Actions”).

But in other phases must be multiple lose life events (e.g. from Rakdos Charm and multi creatures).

So Exquisite Blood's code is fine, it must catch life lose event, not batches

skiwkr commented 7 months ago

I believe that this is also an issue with implementing [[Ob Nixilis, Captive Kingpin]] (#10020) as it also needs to trigger off the life loss event, however the LOST_LIFE event fires for every player that loses life, and Ob Nixilis should only trigger once regardless of how many players lose one life.

Taking damage has DAMAGED_BATCH_FOR_PLAYERS to ensure triggers only happen once, so i would think the same batch event should be made for life loss, however after giving it my best effort I think I'll leave it as an opportunity for someone who understands batch events better than I do.

Also, I believe that [[Enduring Angel]] (#8725) would also benefit from this event as it currently uses LOSE_LIFE, which either fires as often as LOST_LIFE or never fires at all based on my interpretation of the loseLife method in PlayerImpl.java. [[Bloodletter Of Aclazotz]] also uses this same LOSE_LIFE event, however it does need to fire the event for each player so it can do the replacement effect.

github-actions[bot] commented 7 months ago

Ob Nixilis, Captive Kingpin - (Gatherer) (Scryfall) (EDHREC)

{2}{B}{R} Legendary Creature — Demon 4/3 Flying, trample Whenever one or more opponents each lose exactly 1 life, put a +1/+1 counter on Ob Nixilis, Captive Kingpin. Exile the top card of your library. Until your next end step, you may play that card.

Enduring Angel // Angelic Enforcer - (Gatherer) (Scryfall) (EDHREC)

{2}{W}{W}{W} Creature — Angel 3/3 Flying, double strike You have hexproof. If your life total would be reduced to 0 or less, instead transform Enduring Angel and your life total becomes 3. Then if Enduring Angel didn't transform this way, you lose the game. :arrows_counterclockwise: Creature — Angel / Flying You have hexproof. Angelic Enforcer's power and toughness are each equal to your life total. Whenever Angelic Enforcer attacks, double your life total.

Bloodletter of Aclazotz - (Gatherer) (Scryfall) (EDHREC)

{1}{B}{B}{B} Creature — Vampire Demon 2/4 Flying If an opponent would lose life during your turn, they lose twice that much life instead. (Damage causes loss of life.)

xenohedron commented 6 months ago

See rework from #11974 , perhaps can be used to fix this

JayDi85 commented 5 months ago

@Susucre is it still actual after latest batched damage changes?

Susucre commented 5 months ago

Uhm I'll check that tomorrow or the day after. There should be a batch event now for life loss, but not sure it is used everywhere.

Susucre commented 5 months ago

Ok quick check shows it is not using the batch event: image

I'll try to find them all later this week. Could you give a look at #12173 ? I'd like to not add other batch refactor before some version of that gets merged.

Susucre commented 5 months ago

Ok, I found why gaining life is different from losing life with very similar wording:

119.9. Some triggered abilities are written, "Whenever [a player] gains life, . . . ." Such abilities are treated as though they are written, "Whenever a source causes [a player] to gain life, . . . ." If a player gains 0 life, no life gain event has occurred, and these abilities won't trigger.

So "Whenever you gain life" triggers should not be batched, but "Whenever you lose life" should, as there is no similar rule for losing life.

I think this should be all of them:

https://scryfall.com/search?q=o%3A%2Fwhen%5B%5E%2C%5D%2Alose%5B%5E%2C%5D%2Alife%2F+game%3Apaper&order=name&as=grid&unique=cards