-
Notifications
You must be signed in to change notification settings - Fork 782
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
[BLC] Implement Ms. Bumbleflower & Zinnia, Valley's Voice #12655
base: master
Are you sure you want to change the base?
Conversation
Bumbleflower works perfectly. Zinnia, was having issues connecting the cast and enters triggers, but adding something extra (That shouldn't trigger unless the cost was paid) seems to work for now. Left the remainders so others may fiddle with it and see if a better fix can be coded.
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.
if anyone else has ideas/opinions on this one, please share
also, consider splitting the PR so the simpler card can be merged without getting held up by the complicated one
|
||
// Whenever you cast a spell, target opponent draws a card. Put a +1/+1 counter on target creature. It gains | ||
// flying until end of turn. If this is the second time this ability has resolved this turn, you draw two cards. | ||
Effect draw = new DrawCardTargetEffect(1).setText(" target opponent draws a 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.
you should not need to set effect text except for the "it gains flying". Even the "you draw two cards" can be handled by a boolean param in the constructor for that effect
this.addAbility( | ||
new SimpleStaticAbility(new GainAbilityControlledSpellsEffect(new OffspringAbility("{2}"), cfilter) | ||
.setText("Creature spells you cast have offspring {2}."))); | ||
this.addAbility(new SimpleStaticAbility(new GainAbilityControlledEffect( |
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.
I don't like this workaround. Aside from it being a hack, I'm concerned that it might not comply with
702.175b. If a spell has multiple instances of offspring, each is paid separately and triggers based on the payments made for it, not any other instances of offspring.
I will not accept this unless unit tests are added to demonstrate functionality in each case:
- Cast a spell and decline to pay offspring, no token
- Cast a spell and pay offspring, get token
- Cast a spell that already has offspring (some cost other than {2}), each of the four possibilities for paying offspring costs or not, correct number of tokens in each case
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.
I've tested the first two dozens of times, and it seems to work perfectly.
Just messed around with the last one, and sadly its not working right...
REALLY hope this wont end up as an "unable to be implemented" card!
} | ||
} | ||
|
||
class ZinniaValleysVoiceGainOffspringEffect extends ContinuousEffectImpl { |
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.
if not used, don't include it
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.
Its so other devs can work with it to see if a better fix can be found. As it is, the card does work as intended in almost all cases.
*/ | ||
public final class ZinniaValleysVoice extends CardImpl { | ||
|
||
static final FilterCreaturePermanent bfilter = new FilterCreaturePermanent("other creatures with base power 1"); |
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.
style: these fields should be private
import java.util.Objects; | ||
import java.util.Set; | ||
|
||
import static mage.abilities.dynamicvalue.common.StaticValue.*; |
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.
Don't use static imports
Bumbleflower works perfectly.
Zinnia, was having issues connecting the cast and enters triggers, but adding something extra (That shouldn't trigger unless the cost was paid) seems to work for now.
Left the remainders so others may fiddle with it and see if a better fix can be coded.