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

Remove duplicate code for X costs #12551

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

ssk97
Copy link
Contributor

@ssk97 ssk97 commented Jul 4, 2024

Based on #12059 but expanded in scope to remove all duplicate ways of getting X values, combining them all into GetXValue. Notably this does not remove the ways of accessing variable costs that do not involve X, in particular for cost reduction effects.

TODO before un-drafting:

  1. Extensively test Word of Command with X spells (preferably adding some automated tests for it)
  2. Test Glacian Powerstone Engineer and Nahiri's Sacrifice (their code was slightly unusual)

@xenohedron
Copy link
Contributor

Looking good.

@JayDi85
Copy link
Member

JayDi85 commented Jul 4, 2024

Word of Command don't use X -- why you need to test it? Make sure all works with double X effects from [[Unbound Flourishing]]

Copy link

github-actions bot commented Jul 4, 2024

Unbound Flourishing - (Gatherer) (Scryfall) (EDHREC)

{2}{G}
Enchantment
Whenever you cast a permanent spell with a mana cost that contains {X}, double the value of X.
Whenever you cast an instant or sorcery spell or activate an ability, if that spell's mana cost or that ability's activation cost contains {X}, copy that spell or ability. You may choose new targets for the copy.

@ssk97
Copy link
Contributor Author

ssk97 commented Jul 4, 2024

Word of Command don't use X -- why you need to test it?

ManaCostsImpl.handleForcedToPayOnlyForCurrentPayment is used only for Word of Command, and has some code to handle variable mana costs that I don't fully understand the surrounding context of.

Make sure all works with double X effects from [[Unbound Flourishing]]

Unbound Flourishing has an extensive test suite, I'm confident that it works. However, I just noticed that the permanent X value doubling ability is implemented as a replacement effect instead of as a triggered ability like the card text demands. I'll make a new PR to fix that once I'm done here, should be pretty easy once this PR is merged and everything uses the costs tags system. (I don't want to add functional changes to cards in this PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants