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

Multi-sided cards #746

Merged
merged 16 commits into from
May 20, 2023
Merged

Multi-sided cards #746

merged 16 commits into from
May 20, 2023

Conversation

distributive
Copy link
Contributor

@distributive distributive commented Jan 12, 2023

This is a WIP attempt to formalise double-faced cards (e.g. Hoshiko) and cards with functional variants (e.g. Jinteki Biotech).

My approach has been to give cards and printings an optional "sides" field (working name) that defines a series of objects representing a subset of fields of their parent. This allows the sides to only redefine properties that are different, while leaving the rest implicitly the same:

{
  {
    "title": "Card Name",
    "text": "Flip this card to see side 2.",
    "sides": [
      {
        "text": "Flip this card to see side 1."
      }
    ]
}

The downside of this is it's a bit asymmetric and can't always accurately represent what the flip side/other variants represent.

For instance, each copy of Matryoshka has its own flavour text, but none of them (printing order aside) takes precedence over the others, even though this implementation requires that. Also, while flip cards are trivial to understand (one side is always the initial side) Jinteki Biotech is actually three double-sided cards, represented here as 4 objects (the parent card and its three sides).

Applying this system to the Terminal Directive Campaign cards has similar issues, and rewriting the validation tests is going to be a real hassle.

An alternative system might be to represent the sides as separate cards/printings and have them reference each other in some way (this is effectively how it was implemented for TDC). This makes them easier to search for and avoid some of the above issues, but we'd need a way to coalesce cards/printings that represent the same card.

@NoahTheDuke
Copy link
Collaborator

NoahTheDuke commented Jan 12, 2023

Scryfall splits multiface cards into different "faces" that are referenced in a card_faces array on the base card object, and has a "layout" attribute that arranges the different faces. If a given card is "normal", then instead of card_faces, the single face info is merged into the base object.

I don't think magic has cards like Biotech where the same base card has multiple different flipped sides, but I don't think that fundamentally invalidates the model.

Think that would work for nrdb?

@NoahTheDuke
Copy link
Collaborator

Examples from Scryfall/Magic the Gathering (make sure to click through to "Copy-pasteable JSON"):

Here's the full list: Scryfall layouts

@distributive
Copy link
Contributor Author

Magic is weird.

@distributive
Copy link
Contributor Author

distributive commented Jan 12, 2023

That's handy though. It looks like they do it broadly like I've done. Does Magic have an equivalent to Jinteki Biotech or a TDC card like Machilocation (A and B)

Edit: I suppose the split cards are like Machilocation.
Edit 2: I completely missed your first comment mb

@NoahTheDuke
Copy link
Collaborator

No worries. Glad I could help.

@NoahTheDuke
Copy link
Collaborator

Something to be explicit about, I think Scryfall's faces are not just properties that are different from the base card. I think they're intended to be "full" information that is merged into the base card object.

From my reading of the json, the double face cards don't have a base "text" property, each face has that. I think the first face entry in the array is considered the "upright" or "front", depending on the style, but that is less meaningful with the split cards (which we don't have to worry about).

I don't want to saddle you with a bunch of work lol, so please ignore any and all advice I give if you think it's too much work.

That said, I think it might be smart to determine a set of card data that can or could change depending on face, maybe treating each like a printing? And then put the full info into the face entries.

Having individual card entries for each side would complicate our data model, but would allow us to accurately handle biotech and matrioshka and sync (which I think has the same name on both sides).

lol This is a classic case of edge case poisoning. So annoying.

@NoahTheDuke
Copy link
Collaborator

Your layouts have nice names. 👍

@distributive
Copy link
Contributor Author

The MtG system makes this so much easier to work with. I think it lets us deal with all the flip cards, the campaign cards, and Biotech trivially.

Matryoshka on the other hand...

@distributive
Copy link
Contributor Author

The problem with Matryoshka is as far as the card is concerned it's a regular card, but the printing itself has multiple versions.

I don't want the card object to know it has differing flavour text, but shoehorning the layout system onto printings feels weird to me.

v2/layout.json Outdated
Comment on lines 8 to 10
"id": "flip",
"description": "A card with a front and back side.",
"name": "Flip"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.g. Hoshiko

v2/layout.json Outdated
Comment on lines 13 to 15
"id": "copy",
"description": "A card that is made up of multiple distinct cards with the same name.",
"name": "Copy"
Copy link
Contributor Author

@distributive distributive Feb 12, 2023

Choose a reason for hiding this comment

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

e.g. Machilocation and other campaign cards, and possible Matryoshka

v2/layout.json Outdated
Comment on lines 18 to 20
"id": "facade",
"description": "A card that is made up of multiple cards with the same front face, but distinct flip sides.",
"name": "Facade"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only Biotech

v2/layout.json Outdated
Comment on lines 23 to 25
"id": "progression",
"description": "A card with iterations on its values.",
"name": "Progression"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.g. Evidence Collection and other campaign cards with the sticker mechanic

Copy link
Collaborator

@plural plural left a comment

Choose a reason for hiding this comment

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

First, thank you for tackling this!

A few thoughts.

  • TD Campaign cards: I'm not sure we should care about them. They aren't now and don't plan on being legal in any formats, they are mostly here as a historical record. Should we do anything to them at all? I'd give some thought to not doing anything with them.
  • V1 cards. I would also consider doing the minimal change here to keep existing NRDB useful and treating V1/V2 differences for just these cards in the test files. I am certainly not against changing V1, but not losing info in the current system is probably useful. That being said, this is a mild take. :)

I have one open design question for you, but this is overall great!

schema/v2/cards_schema.json Outdated Show resolved Hide resolved
test/validate_v2.ts Outdated Show resolved Hide resolved
"quantity": 6
"quantity": 6,
"layout_id": "copy",
"copy_quantity": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about dropping this field out here, but keeping "quantity" in the sides? You can still sum the quantities up for the test, but move all instances of the field into sides where they vary, rather than treating one differently.

wdyt?

We are going to need to do some formatting shenanigans for these cards either way.

Copy link
Contributor Author

@distributive distributive Feb 22, 2023

Choose a reason for hiding this comment

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

Technically the base printing is also a copy. I know it's sort of weird to have 6 equal cards have one arbitrarily denoted to dominant version, but see my comment below.

expect(c[v1Field], `${v2Field} mismatch for ${code}`).to.equal(printingsById.get(code)[v2Field]);
});
}

function checkTdc(v1: any, code: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have this function defined twice, but with different signatures. Which do you need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're in separate blocks:

  • describe('Card Sets v1/v2', () => {
  • describe('Printings v1/v2 equality', () => {

v2/cards/bmi_buffer.json Show resolved Hide resolved
v2/cards/jinteki_biotech_life_imagined.json Show resolved Hide resolved
@NoahTheDuke
Copy link
Collaborator

Idk how much weight my voice has, but I think y'all should support the campaign cards. They don't require much to track and they're interesting and mildly relevant. I'm a maximalist in this respect.

@distributive
Copy link
Contributor Author

I'd prefer to keep the campaign cards in as well, at least in part because the work for it is now done and "we might as well". One of the layout types (progression) becomes unused without the set.

I also think despite NSG not having anything to do with the cards, we can't assume nobody wants that data.

@distributive
Copy link
Contributor Author

distributive commented Feb 22, 2023

I think the only changed I made to v1 were in the flavor text and Biotech, which I just changed to make the validation convenient.

If you want I can change the standard for flavour back to "\nFlip side:\n" (although I'd still like to make the ft on Matryoshka a bit more aesthetic).

Biotech we can just ignore if we really want, but I made the change since there's no way to validate the existing text without hardcoding something.

Copy link
Collaborator

@plural plural left a comment

Choose a reason for hiding this comment

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

there is also still one warning for 'code' being an unused parameter as well.

test/validate_v2.ts Outdated Show resolved Hide resolved
@distributive
Copy link
Contributor Author

As per our last meeting the plan is to accept the schema for this will be a bit janky and put an issue up to eventually revisit this. If someone can have another spool through this and confirm it looks good I'll merge it.

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

3 participants