Skip to content
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

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

Open
3 of 7 tasks
Alex-Vasile opened this issue Sep 23, 2022 · 4 comments
Open
3 of 7 tasks
Labels
refactoring Developers topics about code and programming tracking tasks Todo list for big tasks

Comments

@Alex-Vasile
Copy link
Contributor

Alex-Vasile commented Sep 23, 2022

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}.
  • 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 [NCC] Implementation remainder of cards #9328 (comment)
  • 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 Alex-Vasile added refactoring Developers topics about code and programming tracking tasks Todo list for big tasks labels Sep 23, 2022
@jeffwadsworth
Copy link
Contributor

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

@github-actions
Copy link

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.

@awjackson
Copy link
Contributor

awjackson commented Sep 23, 2022

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.

@github-actions
Copy link

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.)
🔄
{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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Developers topics about code and programming tracking tasks Todo list for big tasks
Projects
None yet
Development

No branches or pull requests

3 participants