-
Notifications
You must be signed in to change notification settings - Fork 748
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
new feature: Emblem Cards #10498
new feature: Emblem Cards #10498
Conversation
Allows match/tournament creator to specify cards to give each player emblem versions of (or just the starting player for symmetric effects). Technical details: - new UI for specifying emblem cards (.dck files) - available for all match/tournament types - new class `EmblemOfCard` - new method `copyWithZone` on `AbilityImpl` (used to make abilities work from command zone) - new fields on `GameOptions` and `MatchOptions` for emblem cards - emblems are granted after mulligans, before first turn (technically after Planechase starting plane creation)
@artemiswkearney are you willing to implement the new custom dialog as described in #10551 as part of this enhancement? Or do you think a different strategy could be better? |
If you're asking for it to be part of this PR, I think it's out of scope.
If you're asking for help implementing it as a separate PR, and asking me
specifically because this PR is evidence I can make UI changes... quite
possibly, I'll look into it next time I have a day or so to spend on XMage
coding.
…On Thu, Jul 6, 2023, 9:17 PM xenohedron ***@***.***> wrote:
@artemiswkearney <https://github.com/artemiswkearney> are you willing to
implement the new custom dialog as described in #10551
<#10551> as part of this
enhancement? Or do you think a different strategy could be better?
—
Reply to this email directly, view it on GitHub
<#10498 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEWNVVALLGQPNA2YDOQ33XTXO5WUFANCNFSM6AAAAAAZMRUXJ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Alright. @JayDi85 do you think this PR is good to merge as is? |
Omniscience - (Gatherer) (Scryfall) (EDHREC)
Griselbrand - (Gatherer) (Scryfall) (EDHREC)
Paradox Engine - (Gatherer) (Scryfall) (EDHREC)
Yurlok of Scorch Thrash - (Gatherer) (Scryfall) (EDHREC)
Doubling Season - (Gatherer) (Scryfall) (EDHREC)
Key to the Archive - (Gatherer) (Scryfall) (EDHREC)
Animar, Soul of Elements - (Gatherer) (Scryfall) (EDHREC)
Wizard Class - (Gatherer) (Scryfall) (EDHREC)
Mogg Fanatic - (Gatherer) (Scryfall) (EDHREC)
Bonesplitter - (Gatherer) (Scryfall) (EDHREC)
Experiment Kraj - (Gatherer) (Scryfall) (EDHREC)
Emblem of the Warmind - (Gatherer) (Scryfall) (EDHREC)
Essence of the Wild - (Gatherer) (Scryfall) (EDHREC)
Gond Gate - (Gatherer) (Scryfall) (EDHREC)
|
What's the purpose of that feature (emblem cards)? Are there plans to use it in other features or game modes? It's looks like Momir game mode but with any possible cards in command zone. |
It's designed to be open-ended. The inspiration was the events Arena runs with custom emblems (including Omniscience draft), but it can also be used to enable mana burn ([[Yurlok of Scorch Thrash]] emblem) for historically accurate drafts of older sets. I have no idea what else the XDHS community will come up with using it, but I bet it'll be cool. |
Yurlok of Scorch Thrash - (Gatherer) (Scryfall) (EDHREC)
|
So I know that yurlok has been brought up before as an example of adding mana burn, but some of these Arena events have had activatable emblems before. Does it not make sense to keep the activated abilities of these emblems as they are? There is precedent for activatable "emblems" in the system with the planes. |
Not sure I understand the question? Activated abilities of emblem cards
work fine, as long as they don't cost {T} or require doing something with
the permanent the ability is on (since it's an emblem rather than a
permanent).
…On Sat, Jul 8, 2023, 3:17 PM Alexander Novotny ***@***.***> wrote:
So I know that yurlok has been brought up before as an example of adding
mana burn, but some of these Arena events have had activatable emblems
before. Does it not make sense to keep the activated abilities of these
emblems as they are?
—
Reply to this email directly, view it on GitHub
<#10498 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEWNVVBURQ5YTITTHWKZ5H3XPG54LANCNFSM6AAAAAAZMRUXJ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Oh I see, the {T} cost makes sense. |
Updated to use the new Custom Options dialog (which also means no more duplicated code for emblem cards across table/tournament dialogs!) |
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.
It's can be useful feature and can be approved after some fixes and improves:
- User's notification about that feature;
- More stable implementation (if possible), see comment about commander and source object;
- Needs some unit tests for that feature. Test framework doesn't support direct emblems usage like
addPlane
, so it must be added -- I'll add it later in the master.
0043ee6
to
090c662
Compare
Any other issues, or is this ready for merge? (I'd like to do some other UI work like removing the broken "Seats" option in tournament creation and adding mnemonics to various parts of the UI, but merge conflicts in forms are hell so I'm holding off for now.) |
(I'd be happy to try writing some unit tests, if you can point me to documentation about how things are tested, or at least some existing integration tests for other table/tournament options) |
I'd like this to get merged soon. There are other UI improvements to be made and they kinda have to be done sequentially to avoid merge conflicts in the form files. It seems that all the requested improvements have been made aside from unit tests for the new functionality, and without a test framework it's unclear to me what the expectation is there. |
Any objections to xenohedron merging 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.
Test framework updated in a3f787b to provide addEmblem
method for unit tests- should allow you to make a unit test that adds EmblemOfCard
to confirm functionality in a test game.
Okay, good thing you had me test - turns out |
Anyway, added some tests for all the emblem cards mentioned in the original post that should do things. Let me know if you want anything else tested. (I don't really want tests saying "this should do nothing relevant", especially for cards like Gond Gate that might someday work properly as emblems, but I'm happy to test that emblems of certain cards don't break specific other things, whether in the same test or globally as with the previous bug.) |
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 believe all feedback is addressed now that the unit tests are present. I will merge this next week unless any further issues can be found in the meantime.
Allows match/tournament creator to specify cards to give each player emblem versions of (or just the starting player for symmetric effects).
Technical details:
EmblemOfCard
copyWithZone
onAbilityImpl
(used to make abilities work from command zone)GameOptions
andMatchOptions
for emblem cardsEC
indicator in table info, with explanation in the tooltip alongside all the other explanationsI've tested this with the most perverse cards I could think of, and it never crashed the server and usually did something reasonable. (Since the emblems aren't permanents, anything that retrieves the permanent for its source and null checks it will usually do nothing.)
List of cards tested during development: