Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work outline - Fixing enrage and similar triggered abilities #11992

Closed
jimga150 opened this issue Mar 24, 2024 · 2 comments
Closed

Work outline - Fixing enrage and similar triggered abilities #11992

jimga150 opened this issue Mar 24, 2024 · 2 comments
Assignees

Comments

@jimga150
Copy link
Contributor

Started in #11961 and continuing work done in #11841, addressing part of issue mentioned in #10805.

My plan is to take any abilities that trigger "when <permanent/player> is dealt damage", such as enrage abilities, and fix anything with the following criterion:

  1. checkEventType is looking for DAMAGED_PERMANENT or DAMAGED_PLAYER
  2. The ability should trigger only once for simultaneous damage from multiple sources

Ironically, most "Enrage" cards are fine (such as Apex Altisaur), since they use DealtDamageToSourceTriggeredAbility, though that could stand to switch to using the new DAMAGED_BATCH_FOR_ONE_PERMANENT event (this is not necessary though it seems since the filtering used in checkTrigger looks like it takes care of it)

Also, cards like Acidic Dagger, which use DAMAGED_PERMANENT but should trigger on each specific instance of damage meeting its criterion, need not be changed.

However, cards like Aegar, the Freezing Flame (can you tell I'm sorting alphabetically) need to be addressed. All of these cards would get a change similar to what i did in #11961, except the DAMAGED_BATCH_FOR_ONE_PERMANENT and DAMAGED_BATCH_FOR_ONE_PLAYER events would get a new getTargetId override in order to make the code cleaner and since everyone would normally need to say:

CardUtil.getEventTargets(event)
                .stream()
                .findFirst()
                .orElse(null)

Currently in master, 93 cards, 1 token, 4 watchers, and 17 abilities use the DAMAGED_PERMANENT event.
206 cards, 2 tokens, 8 watchers, 1 Plane, and 11 abilities use the DAMAGED_PLAYER event.

Each of these cards will need to be reviewed to see if its using those properly or if they should switch to the single-target (or potentially multi-target) batches.

Additionally, changes made as a result of #11985 would need to hit anything hit by this change if its done after i do this change, so right now I'm going to wait until that issue is resolved before starting this.

Comments welcome @JayDi85 @xenohedron

@xenohedron
Copy link
Contributor

Thanks for the writeup. Sounds good to me. Let me know if you have any related feedback on #11995 - probably can get that merged this week.

@jimga150
Copy link
Contributor Author

Having a bit of a conundrum with Fall of Cair Andros.

  1. Is it possible to have combat and noncombat damage dealt in the same instant? (i'm assuming yes since there are no assumptions made about damage that is lumped into the batch for one permanent)
  2. If that does occur, how should it be determined which part(s) of the damage were in excess of the permanent's toughness/loyalty/defense?

Seems like the same deal for Toralf, God of Fury.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants