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

Conjured tokens xml attribute #4646

Merged
merged 11 commits into from
Sep 1, 2022
Merged

Conversation

CajunAvenger
Copy link
Contributor

@CajunAvenger CajunAvenger commented Jun 26, 2022

Related Ticket(s)

Short roundup of the initial problem

Tokens are able to be marked "don't destroy when it leaves the battlefield" but only from the token window. These permanent tokens couldn't be made through related scripts and weren't user friendly to make, especially in multiples, which is more notable with new Conjure cards in canon.

What will change with this Pull Request?

  • The "persistent" attribute can be added to card relations in the xml. Tokens made from this relation won't be destroyed when they leave the battlefield.
    Oracle importer can create these relations by spellbook, an array of card names in the cards json. This isn't currently part of mtgjson but is by Scryfall (and can be on custom jsons). Removed until mtgjson support exists.

Screenshots

example AllPrintings that just has J21 Sarkhan with spellbook added, and Shivan Dragon which is used below
AllPrintingsTest.zip
Related scripts
menu
tokens created this way are permanent
tokens

Other note

It'd be possible to have oracle check rules text for conjures like it does for melds, but wotc has not been consistent with how those are worded so you'd need a messy regex like /conjure ([^ ] cards? named|a) (.*) (card )?[io]nto/ to grab it and hope they don't keep doing more iterations, and it still doesn't do spellbooks.

@ebbit1q
Copy link
Member

ebbit1q commented Jun 27, 2022

👍 seems good.

I don't think the xml tag needs to be named conjure though, what it really does is make a non destroyable card, this might be useful for other mechanics as well, in the original ticket you've suggested permanent or maybe something like indestructable or moveable.

this does introduce the issue of being unable to remove these cards of course (moving them to the sideboard is a workaround) that's out of scope here though.

Co-authored-by: Zach H <zahalpern+github@gmail.com>
@CajunAvenger
Copy link
Contributor Author

I don't think the xml tag needs to be named conjure though

ye, we can rename as you like.

this does introduce the issue of being unable to remove these cards of course

Not being able to destroy a token made this way already exists, as this is just adding the 'destroy when this leaves the battlefield' checkbox functionality from the token dialogue window to the xml. For most iterations of this we've seen so far this is fine, conjures are meant to stick around for the rest of the game and they get cleared out when you close a game back to the sideboard screen. It wouldn't handle the "can only exist on the stack" idea from #4376, true.

(Looking at createCard, it seems like we could add something there by updating cmd.set_zone("table"); but i've not gone deep into the code guts yet i would not be surprised if it's more complicated.)

@ebbit1q
Copy link
Member

ebbit1q commented Jun 27, 2022

the location commands are handled is in the common server player, this is where the server/local game logic is and the getDestroyOnZoneChange property is interpreted.

@@ -377,7 +377,13 @@ int OracleImporter::importCardsFromSet(const CardSetPtr &currentSet,
}
name = faceName;
}

// support for array of cards this conjures
if (card.contains("spellbook")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to find the "spellbook" property on any cards, does this attribute correlate to a feature that isn't available yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea as said in the first post this is just opening the support so jsons can create those relations, it isn't currently on mtgjson. As it is something that Scryfall supports, it seems likely that data can get added though.

Can more specifically poke @ZeldaZach on if that had made sense for him or if we should start a ticket over there to hash out what the field looks like/if it gets added at all. If it doesn't, this still has a use case for custom jsons to create these relations.

spellbook

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked through scryfall's api as well, next to the spellbook: prompt there doesn't seem to be a property of the card that contains the cards in the spellbook

https://api.scryfall.com/cards/search?q=bigspender

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use an api call to get the spellbook this way: https://api.scryfall.com/cards/search?&q=spellbook%3Abigspender

@CajunAvenger
Copy link
Contributor Author

at ebbit's suggestions, renamed "conjured" to persistent, and removed the oracle changes. The oracle changes will be pr'd again in future when/if mtgjson gets access to a spellbook property.

@ebbit1q
Copy link
Member

ebbit1q commented Jul 17, 2022

this part is basically unchanged from the v3 parser, it could simply be added to the v3 parser as well without issue, I know for example mse's export code still generates v3 xmls

@ebbit1q
Copy link
Member

ebbit1q commented Jul 17, 2022

future note: after this is merged https://github.com/Cockatrice/Cockatrice/wiki/Custom-Cards-&-Sets needs to be updated

Copy link
Member

@ebbit1q ebbit1q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I've been absent because of personal reasons, this pr is simple and does exactly what it needs to do, note that the changes are only present in the v4 xml format and the wiki needs to be updated

@ebbit1q ebbit1q merged commit 40c88fe into Cockatrice:master Sep 1, 2022
@tooomm
Copy link
Member

tooomm commented Sep 1, 2022

after this is merged Cockatrice/Cockatrice/wiki/Custom-Cards-&-Sets needs to be updated

The wiki is not yet updated.
@CajunAvenger Do you want to take care of that?

@ebbit1q
Copy link
Member

ebbit1q commented Sep 1, 2022

thanks for bringing that up! I've updated the wiki

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

Successfully merging this pull request may close these issues.

None yet

4 participants