-
Notifications
You must be signed in to change notification settings - Fork 760
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
[OTC] Implement Felix Five-Boots #12074
Conversation
[[Felix Five-Boots]] |
Felix Five-Boots - (Gatherer) (Scryfall) (EDHREC)
|
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! |
|
||
GameEvent sourceEvent = numberOfTriggersEvent.getSourceEvent(); | ||
if (sourceEvent == null | ||
|| sourceEvent.getType() != GameEvent.EventType.DAMAGED_PLAYER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Mage.Tests/src/test/java/org/mage/test/cards/triggers/damage/FelixFiveBootsTest.java
Show resolved
Hide resolved
Here is a version working with batches (I also set all tests to strict and fixed the choices): I introduced a method to check a single Then for batch events, look at all events inside, and any 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 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: |
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 |
That would cause a bunch of weird bugs where Felix would double triggers it shouldn't affect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
yep, can merge (assuming travis still passes) and fix the bugged edge case separately as part of larger rework (still WIP) |
No description provided.