Various bugs in PhantomPreventionEffect

[xenohedron]

[[Phantom Wurm]] as example
Affected 7 cards: https://scryfall.com/search?q=phantom%20o%3A%22prevent%20that%22

If Phantom Wurm is dealt damage by multiple sources at once (most likely because it was blocked by multiple creatures in combat), all of that damage is prevented and you remove only one counter from it.

Reported bug on discord for Phantom Nishoba: if dealt damage by multiple sources during the first strike combat damage step, a counter is removed for each source.

Effect has a hack to remove only a single counter during combat damage step, but it doesn't consider the first strike combat damage step. The hack is also problematic in that subsequent non-simultaneous damage within the same combat damage step would (presumably) be caught by the same code and not remove a counter.

Storing info within the class is bad style anyway.

Needs reworked, probably using a watcher, but not clear exactly how to reset it - probably on every process action?

Must have unit tests for various cases:

• Simultaneous damage in combat (one counter removed)
• Simultaneous first strike damage (one counter removed)
• Double strike (one counter for each instance)
• Combat damage, and then noncombat damage (e.g. lightning bolt) still within combat damage step (second counter removed)
• Simultanous noncombat damage (e.g. Band Together) (one counter removed)

[github-actions]

Phantom Wurm - (Gatherer) (Scryfall) (EDHREC)

{4}{G}{G}
Creature — Wurm Spirit
2/0
Phantom Wurm enters the battlefield with four +1/+1 counters on it.
If damage would be dealt to Phantom Wurm, prevent that damage. Remove a +1/+1 counter from Phantom Wurm.

[jimga150]

New to this object--would it not be better to instead check the event.getType() against a list of all batch damage events? right now this kind of tracking is necessary because preventDamage just checks to see if the event is instanceof DamageEvent which double-matches damage in any case.

[xenohedron]

Nope, DamageEvent is for replacement effects, different from DamagedEvent for watchers/triggers. Batching isn't applicable here.

[Susucre]

I think we could have an event whenever GameState::procesAction is called and reset the watcher then?

Really not sure.

[JayDi85]

Needs reworked, probably using a watcher, but not clear exactly how to reset it - probably on every process action?

Just use two events - one to apply (on damage) and one to reset (on damaged). It’s a standard code style to reset some values in many places.

[Susucre]

Issue is that replacement effect prevents all the damage. So we may not see the proper damaged event when we'd like to reset. And grouping/batching multiple pre-replacement effects seems wrong, but that's just my gut feeling.

I guess I'll need to see it in action a lot of testing to be convinced.

[JayDi85]

Phantom cards has three problems (see tests list in starting post):

1. Confirmed. Miss damage step for first strike (original bug report). How to fix: add conditional with miss combat step in check code;
2. Need to confirm. Multiple damages in same step like bolts after combat damage or double strike. How to fix: depends on size of the problem. Possible solution: refactor code to use watcher with reset values by events;
3. Confirmed. Migrate code to watcher, not class fields (must be done anyway: with or without bug fixes);

[JayDi85]

There are only one batch event with all damages. So watcher can be reset after each (e.g. on next damage it will take new counter). Queue example:

• damage event - filter event to source - if has not status then remove counter, add status and prevent;
• damage event - filter event to source - if has status then prevent;
• damage event - filter event to source - if has status then prevent;
• batch damaged event - any - reset status (next damage event will take counter again);

[Susucre]

Yes, but when all damage is prevented, there is no batch event of all damage. At least I don't think so. Will check later.