-
Notifications
You must be signed in to change notification settings - Fork 859
Refactor a couple of cards to use CreatureToken over custom private Tokens #14315
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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
| game.addEffect(new BecomesCreatureSourceEffect( | ||
| new AnsweredPrayersToken(), CardType.ENCHANTMENT, Duration.EndOfTurn | ||
| new Angel33Token(), CardType.ENCHANTMENT, Duration.EndOfTurn |
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.
In this case, there is an existing token class that represents what this permanent becomes. If we're happy to piggy back onto those classes where the effect isn't wholly custom, I can keep this in mind.
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.
@theelk801 any opinion on this?
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 did get curious at how this works in game, and with a locally built client it seems fine? I had the Answered Prayers with the correct attributes, and the art was still visually Answered Prayers.
It's very possible I'm overlooking some nuance here to why we wouldn't want to pass a fully fledged Token in as the blueprint to BecomesCreatureSourceEffect, but from my cursory look - maybe it's okay? 🤷
|
For info:
|
|
Wizards can print new token with new sets, so old private token can be replaced by new public token as well. I don’t have any details on wizards logic for token printing. So the main point for this — look at official token image. |
|
100%. It was rattling about the back of my head, but I had wondered about something to audit the use of custom tokens (either private ones currently, or ones declared via Public tokens represent items where Wizards have printed actual token items. They have certainly "gone back" and printed missing tokens with the release of new sets and commander decks in the past and gotten better about printing cards with no supporting token - but it's not wholly consistent either. In the case of cards becoming a token, be it public token or Mirroring this off of tabletop play, it's valid I guess to use newly printed tokens to represent an item made by an older card where a token didn't originally exist. But yeah, as a guiding set of principles:
|
|
Moreover, Example: permanent.setExpansionSetCode(token.getExpansionSetCode());
permanent.setUsesVariousArt(token.getUsesVariousArt());
permanent.setCardNumber(token.getCardNumber());
permanent.setImageFileName(token.getImageFileName());
permanent.setImageNumber(token.getImageNumber());Maybe it will work fine but I'm not sure -- need research and test. |
Testing the waters a bit...
In other efforts to address some TODOs around Token implementations I saw there's a comment encouraging that we move towards using CreatureToken;
mage/Mage.Verify/src/test/java/mage/verify/VerifyCardDataTest.java
Line 1467 in 57ad574
That coupled with @theelk801 (rightly!) calling me out on PR comments to align with this, suggests to be that there's a long-standing desire to just get away from custom Private Tokens and it's just a matter of time/effort to do that...
So, I've had a stab at adjusting two instances below to remove their dependency on custom classes. If this PR is approved, or more guidance can be given, I can align with that and have a go at cleaning up other instances retroactively that exist in the codebase 🙏 Happy to break the PRs out into a handful at a time to make reviewing easier to digest too 😄