[WOC] Implement Tegwyll's Scouring #12891
Conversation
| } | ||
| } | ||
|
|
||
| class TegwyllsScouringCastAsThoughtItHadFlashAbility extends SpellAbility { |
There was a problem hiding this comment.
Look at [[Asinine Antics]] implementation and PayMoreToCastAsThoughtItHadFlashAbility — I recommend to modify standard class to support tap cost (as additional costs param in constructor) instead copy-paste whole code.
There was a problem hiding this comment.
Look at [[Asinine Antics]] implementation and
PayMoreToCastAsThoughtItHadFlashAbility— I recommend to modify standard class to support tap cost (as additional costs param in constructor) instead copy-paste whole code.
Oh, I did so at first, but then decided that such kind of cost's only used by one card, so ability code should be located in the card file.
I've returned my previous changes to PayMoreToCastAsThoughtItHadFlashAbility and card's implementation.
There's one question about card reprints. Is it okay that they're failing tests at the moment? It seems reprints were added correctly.
|
If set has multiple reprints then all card versions must use |
| } | ||
|
|
||
| public PayMoreToCastAsThoughtItHadFlashAbility(Card card, Cost costsToAdd) { | ||
| super(card.getSpellAbility().getManaCosts().copy(), card.getName(), Zone.HAND, SpellAbilityType.BASE_ALTERNATE); |
There was a problem hiding this comment.
why did you remove the "as though it had flash" from the second argument to the super constructor?
There was a problem hiding this comment.
There was a small issue. When "as thought it had flash" string is used both in super constructor and in getRule() method, you can see tautology in spell ability choice dialog like:
"You may cast Tegwyll's Scouring as thought it had flash: You may cast Tegwyll's Scouring as thought it had flash as though it had flash if you pay <...>"
So I can't remove this words from the getRule() and it was the easiest way to fix. But now it needs to scroll through a long line of text to recognize ability as flash... So I'm doubtful about it.
| CardUtil.increaseCost(this, costsToAdd); | ||
| } | ||
|
|
||
| public PayMoreToCastAsThoughtItHadFlashAbility(Card card, Costs<Cost> costsToAdd) { |
There was a problem hiding this comment.
so you added two new constructors, because the cost increase can't be handled in the general case?
manacosts need CardUtil.increaseCost while other costs need this.addCost
is it really necessary to have separate constructors, or can you just use a single general constructor and check instanceof ManaCosts to determine how to do it?
There was a problem hiding this comment.
No, it's really not necessary. I was only afraid of uncheked cast warning (Cost to ManaCosts<ManaCost>), but after your review I examined hierarchy and noticed that there's no problem. I've changed it.
It looks much clearer now! Thank you!
| this.costsToAdd = costsToAdd; | ||
| this.timing = TimingRule.INSTANT; | ||
| this.setRuleAtTheTop(true); |
There was a problem hiding this comment.
why did you remove setRuleAtTheTop? always true for this ability right?
There was a problem hiding this comment.
That's how it should be, but setRuleAtTheTop does nothing in case of this ability. Its rule text is always at the bottom in generated rules despite of card image text. I didn't research the cause and relied on previous experience, considering it right.
Added Tegwyll's Scouring and missing skins from Wilds of Eldraine commander.
Closes #10815 tracking set as this is the last card
Closes #11129 previous implementation attempt