-
Notifications
You must be signed in to change notification settings - Fork 778
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
[ACR] Implement Ezio Auditore da Firenze #13067
Conversation
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.
Mostly minor comments that should be quick to address.
|
||
static { | ||
filter.add(SubType.ASSASSIN.getPredicate()); | ||
filter.add(Predicates.not(new AbilityPredicate(FreerunningAbility.class))); // So there are not redundant copies being added to each card |
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.
Ezio's granted Freerunning is the only {B}{B} Freerunning, so you still need to add it to all Assassins because Brotherhood Ambushers and Achilles Davenport have more expensive or differently-restrictive costs than {B}{B}. Having redundant copies of things like this are fine.
//Phage this.addAbility(new DealsCombatDamageToAPlayerTriggeredAbility(new LoseGameTargetPlayerEffect(), false, true)); | ||
|
||
//this.addAbility(new ConditionalTriggeredAbility(new DealsCombatDamageToAPlayerTriggeredAbility(new EzioAuditoreDaFirenzeEffect(), true, true)), new LifeLossOtherFromCombatWatcher()); | ||
//this.addAbility(new ConditionalTriggeredAbility(new DealsCombatDamageToAPlayerTriggeredAbility(new DoIfAnyNumberCostPaid(new EzioAuditoreDaFirenzeEffect(),new ManaCostsImpl<>("{W}{U}{B}{R}{G}")), true, true),condition, "pay {W}{U}{B}{R}{G} if that player has 10 or less life. When you do, that player loses the game.")); | ||
//Ability ability = new DealsCombatDamageToAPlayerTriggeredAbility(new DoIfCostPaid(new EzioAuditoreDaFirenzeEffect(), new ManaCostsImpl<>("{W}{U}{B}{R}{G}")), true, true) |
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.
Clean these commented-out attempts up.
this.toughness = new MageInt(2); | ||
|
||
// Menace | ||
this.addAbility(new MenaceAbility(), makeWatcher()); |
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.
- MenaceAbility(false) because Ezio doesn't have the reminder text of Menace.
- Menace doesn't need the watcher, don't add it here (add it to the ability that uses it)
new DoIfCostPaid( | ||
new LoseGameTargetPlayerEffect(),new ManaCostsImpl<>("{W}{U}{B}{R}{G}") | ||
), false, true | ||
),EzioAuditoreDaFirenzeWatcher::checkPermanent, "Whenever {this} deals combat damage to a player, you may pay {W}{U}{B}{R}{G} if that player has 10 or less life. When you do, that player loses the game." |
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.
Does this need to be a Watcher, or is there something more static that could determine if the targeted player is below 10 life? Is it that the DealsCombatDamageToAPlayerTriggeredAbility doesn't set the player target until after ConditionalTriggeredAbility checks the condition? To be clear, I'm not objecting to this, just wondering if the complication of a Watcher is necessary.
Also, as long as the watcher is necessary, add new EzioAuditoreDaFirenzeWatcher()
after the close of ConditionalTriggeredAbility
in this addAbility
.
public static Watcher makeWatcher(){ | ||
return new EzioAuditoreDaFirenzeWatcher(); | ||
} |
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.
Other cards use this function so that when ~2-3 cards need the same watcher but it's not frequent enough to be a Common watcher, you can use makeWatcher
to share it. Since nothing else uses this watcher, you can just add it to the ability directly and remove this makeWatcher
until something else needs it.
Thank you for the feedback @Grath! I implemented the changes you proposed. |
Swap from Watcher to a Condition
I didn't know how to guide you into doing it, but I pulled down the code and swapped it over to a Condition instead of a Watcher, and tested to make sure it works right with the Condition. |
Thank you! It was this line |
this.addAbility(new SimpleStaticAbility(Zone.BATTLEFIELD, new GainAbilityControlledSpellsEffect(new FreerunningAbility("{B}{B}"), filter))); | ||
// Whenever Ezio deals combat damage to a player, you may pay {W}{U}{B}{R}{G} if that player has 10 or less life. When you do, that player loses the game. | ||
this.addAbility( | ||
new ConditionalTriggeredAbility( |
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.
This isn't quite right. The condition needs to be checked on resolution, not on trigger. And it's a reflexive trigger.
You want a DoWhenCostPaid inside a ConditionalOneShotEffect.
https://scryfall.com/card/acr/25/ezio-auditore-da-firenze