[OTC] Implement Felix Five-Boots

[PurpleCrowbar]

No description provided.

[PurpleCrowbar]

[[Felix Five-Boots]]

[github-actions]

Felix Five-Boots - (Gatherer) (Scryfall) (EDHREC)

{2}{B}{G}{U}
Legendary Creature — Ooze Rogue
5/4
Menace, ward {2}
If a creature you control dealing combat damage to a player causes a triggered ability of a permanent you control to trigger, that ability triggers an additional time.

[Susucre]

Since this is to my knowledge the first card looking specifically at damage to player triggers, it could use a couple unit tests.

Aside from that (and the LKI comment, correct me if I'm wrong on that), seems good!

[JayDi85]

That’s wrong. There are other event types like batches.

See WaytaTrainerProdigy with same ability:

[JayDi85]

Must use all player related events:

[PurpleCrowbar]

@JayDi85 Are the changes made in 9d32bb0 what you had in mind?

[JayDi85]

Yes, but it must process batches as BatchEvent. That's code will fail:

[PurpleCrowbar]

Alright, my most recent commit doesn't fix the issue of the isCombatDamage check, because I'm not sure how to go about doing that. I do think it handles batch events in the sense that it goes through each creature involved in the batch until it reaches a valid one (tests pass and works fine in-game, so presumably fine?). I don't fully understand the necessity of using batches, as this card, unlike Umezawa's Jitte, has a pretty cut and dry condition ("deal combat damage to a player", as opposed to "deal combat damage", which raises multiple events). As a result, I can't think of how to write a test to ensure that this is actually working as intended, hence my lack of understanding as to its necessity. I also don't, as mentioned, know how to check that the damage dealt is combat damage.

[Susucre]

Here is a version working with batches (I also set all tests to strict and fixed the choices):
master...Susucre:mage:felix-five-boots

I introduced a method to check a single DamagedEvent:

Then for batch events, look at all events inside, and any DamagedEvent is checked, if matching the condition, then the whole batch event matches the condition.

There is no longer need to filter on the batch event type. To be more abstract like that should be better if we ever add more types of batch damage events. Any Batch event containing at least one DamagedEvent matching the check will be affected by Felix's effect.

I didn't want to push directly on your work branch, but if that's good enough, you can cherry-pick the last commit of my branch above on top of your PR.

Edit: on second thought, there might still be a bugged behavior in my code:
If batch event contains events A B C and D;
If A & B match Felix condition, but the trigger ignores those and triggers due to C & D being there, Felix will falsely affect the trigger.
We might need some bigger change somewhere for triggers on batch events to expose exactly which subevent are relevant for them.

[PurpleCrowbar]

Edit: on second thought, there might still be a bugged behavior in my code:
If batch event contains events A B C and D;
If A & B match Felix condition, but the trigger ignores those and triggers due to C & D being there, Felix will falsely affect the trigger.
We might need some bigger change somewhere for triggers on batch events to expose exactly which subevent are relevant for them.

Your current implementation still passes the batch event test you wrote at least, which I checked doesn't pass on my original implementation without batch handling, so definitely an improvement. I'd personally be happy to cherrypick that commit and get this merged unless you're unhappy with the possibility of broken edge-cases. I don't like the odds of me being able to figure out how to fix the problem you described and add it to this PR given my inexperience with batch events in general. This currently seems to work in the vast majority of cases

[Susucre]

That would cause a bunch of weird bugs where Felix would double triggers it shouldn't affect.
Haven't yet thought how to handle those situations gracefully.
Ideally triggers on batch events should describe which part of the batch made them trigger.

[Susucre]

Alright, I just can't come up with a case where the logic would fail with the current card pool (notably because you can't have different controller for creatures dealing combat damage to players in the same event)

So this might become an issue with batching in the future, just not here.

Unless someone else has a strong opinion on the subject, I think this one is good to merge.

[Susucre]

Oh nevermind, just thought about this:

I have an idea of a refactor of batched triggers to solve this exposing which part of the batch event made it trigger.

found a bugged case

[JayDi85]

Failed test must be ignored in PR before merge - that’s use case is part of batch events improve from #12095. All other must be fine.