mage icon indicating copy to clipboard operation
mage copied to clipboard

Refactor: Several refactors that should be done found during implementing cards

Open Alex-Vasile opened this issue 1 year ago • 4 comments

Leftover TODOs from #9328:

  • [ ] Several very similar MoveCounterFromTargetToTargetEffect should be refactored to a common class
  • [ ] Change cards to make use of new SpellCastControllerTriggeredAbility's fromZone (e.g. Kami of Celebration, Sunbird's Invocation)
  • [ ] Make use of new non-token creature static filter
  • [ ] Change Ob Nixilis, the Adversary to use standard Casualty ability rather than custom implementation
  • [ ] Refactor TargetOfOpponentsSpellOrAbilityTriggeredAbility and BecomesTargetControlledPermanentTriggeredAbility into a single class for "Whenever {a} becomes the target of a {b}" that takes filters for both {a} and {b}.
  • [x] ~~Refactor MageObject.getColor(game) so that it handles the intricacies of MDFC and split cards internally rather than needing to be wrapped externally.~~ Clean up and double check by https://github.com/magefree/mage/pull/9328#issuecomment-1255699247
  • [ ] Refactor Replicate cost show the final cost of the spell at once when paying. Currently the replicate cost is only shown after the regular cost has been paid.

Alex-Vasile avatar Sep 23 '22 02:09 Alex-Vasile

[[Kami of Celebration]] [[Sunbird's Invocation]] [[Ob Nixilis, the Adversary]]

jeffwadsworth avatar Sep 23 '22 14:09 jeffwadsworth

Kami of Celebration - (Gatherer) (Scryfall) (EDHREC)

{4}{R} Creature — Spirit 3/3 Whenever a modified creature you control attacks, exile the top card of your library. You may play that card this turn. (Equipment, Auras you control, and counters are modifications.) Whenever you cast a spell from exile, put a +1/+1 counter on target creature you control.

Sunbird's Invocation - (Gatherer) (Scryfall) (EDHREC)

{5}{R} Enchantment Whenever you cast a spell from your hand, reveal the top X cards of your library, where X is that spell's mana value. You may cast a spell with mana value X or less from among cards revealed this way without paying its mana cost. Put the rest on the bottom of your library in a random order.

Ob Nixilis, the Adversary - (Gatherer) (Scryfall) (EDHREC)

{1}{B}{R} Legendary Planeswalker — Nixilis 3 Casualty X. The copy isn't legendary and has starting loyalty X. (As you cast this spell, you may sacrifice a creature with power X. When you do, copy this spell. The copy becomes a token.) +1: Each opponent loses 2 life unless they discard a card. If you control a Demon or Devil, you gain 2 life. −2: Create a 1/1 red Devil creature token with "When this creature dies, it deals 1 damage to any target." −7: Target player draws seven cards and loses 7 life.

github-actions[bot] avatar Sep 23 '22 14:09 github-actions[bot]

Regarding the apparently unnecessary split/MDFC code in MulticoloredPredicate, I came up with a hypothesis as to why it exists. The rules for split cards were greatly simplified in 2017, with the release of Amonkhet. Before Amonkhet, split cards that weren't on the stack actually had three sets of characteristics at once: those of the left side, of the right side, and the union of the two sides. Which characteristics you used depended on what kind of comparison you were doing. For example, if you revealed [[Wear // Tear]] to [[Counterbalance]] it would counter a 1-mana spell or a 2-mana spell but not a 3-mana spell, whereas if you revealed it to Dark Confidant you would lose 3 life. In the current rules, split cards not on the stack always and only have the characteristics of the union of both sides.

The code in question might be a leftover from when xmage implemented (or attempted to implement) the complex pre-Amonkhet rules. Under the modern rules, there's no reason a SplitCardHalf that isn't on the stack should ever be getting passed to a filter, but under the pre-Amonkhet rules there was because the characteristics of the separate halves mattered.

EDIT: I just checked the Amonkhet rules update diff and the rule quoted in the comment in MulticoloredPredicate.java is a rule that was removed in the update. It was condensed along with a whole bunch of other rules into "In every zone except the stack, the characteristics of a split card are those of its two halves combined. This is a change from previous rules." So I think my hypothesis for why that code exists is almost confirmed.

awjackson avatar Sep 23 '22 14:09 awjackson

Wear // Tear - (Gatherer) (Scryfall) (EDHREC)

{1}{R} Instant Destroy target artifact. Fuse (You may cast one or both halves of this card from your hand.) :arrows_counterclockwise: {W} Instant Destroy target enchantment. Fuse (You may cast one or both halves of this card from your hand.)

Counterbalance - (Gatherer) (Scryfall) (EDHREC)

{U}{U} Enchantment Whenever an opponent casts a spell, you may reveal the top card of your library. If you do, counter that spell if it has the same mana value as the revealed card.

github-actions[bot] avatar Sep 23 '22 14:09 github-actions[bot]