-
Notifications
You must be signed in to change notification settings - Fork 16
Add UUIDs to the Tokens File #340
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?
Add UUIDs to the Tokens File #340
Conversation
tokens.xml
Outdated
| <set picURL="https://cards.scryfall.io/large/front/c/7/c76a6163-6fd9-4cdf-9b14-07f75c2f0fa1.jpg?1720338560">ACR</set> | ||
| <set picURL="https://cards.scryfall.io/large/front/2/4/241b3b6d-a25f-4a43-b5d6-1d1079e7e498.jpg?1705432997">MKM</set> | ||
| <set picURL="https://cards.scryfall.io/large/front/c/7/c76a6163-6fd9-4cdf-9b14-07f75c2f0fa1.jpg?1720338560" uuid="786e800f-ce0f-50d5-b942-c99bc8430f0d">ACR</set> | ||
| <set picURL="https://cards.scryfall.io/large/front/2/4/241b3b6d-a25f-4a43-b5d6-1d1079e7e498.jpg?1705432997" uuid="f5b1634e-a14b-51d6-a679-356f7f0ac60f">MKM</set> |
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.
this output looks incorrect, why is it not similar to the picurl?
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.
Seemingly because MTGJSON and Scryfall use different UUIDs. The script tries to get the 'uuid' field from the token entry on MTGJSON first and then, if that doesn't work, grabs the UUID from the Scryfall URL, if available. So I guess the UUID and the URL will only match sometimes. I am worried about uniqueness here, but I haven't found any issues yet.
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.
As an alternative to this, I could add all the UUIDs by pulling them from the Scryfall URLs and then update the script I use to add new token sets to add a uuid property when it pulls info from Scryfall. But maybe we don't want to rely that much on Scryfall over MTGJSON.
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.
cockatrice does not use mtgjson's uuids only scryfall
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.
That's what I thought. I can change it to use the Scryfall URL in all cases then.
|
This PR now adds a |
|
I added sequential UUIDs (0, 1, 2, 3, etc) to token entries with art but no Scryfall URL since as I understand it, it doesn't super matter what the value is as long as it's unique. 11 tokens still don't have the uuid property, but they also don't have any art at all so I assume they don't actually need it. Have not seen any problems while testing so far. |
|
I'd prefer the made up uuids to be a bit more complex, in my opinion the client should be able to work without the uuid and send a different token like set num over the protocol, this'd make things much easier. |
|
Alright, I can make the made up UUIDs longer and put some dashes in or something. I just don't want to make them the same format as Scryfall's because I don't want to have to check the whole file for uniqueness. I can definitely add the set number to the tokens file to support something like that, but I think changing the client protocol is beyond the scope of my current ability. |
|
this seems fine now, they don't necessarily need to have numbers and dashes, it's just an arbitrary string though |
|
Is there anything else you would like me to do with this PR or is it ready to go? for the made up ones, I suppose I could hash the name of the token instead of doing them sequentially if you wanted a less arbitrary identifier. Also, I have a version of the tokens file with a |
|
The made up UUIDs are now a sha256 hash of the token entry name (including spaces). Each should now be both deterministic and unique. This, of course, only works for one UUID per token entry, but there should never be a case where we need more than one made up UUID per entry. |
|
what's the benefit of hashing at all if the uuid could have been the input of the hash just the same? |
|
I was under the assumption that the UUIDs should be a number and you seemed unhappy with arbitrarily or sequentially assigned strings of numbers. I figured that hashing the name would give a UUID that was both non-arbitrary and a number. You also suggested hashing to create UUIDs, though you suggested hashing the props rather than the name. I was also under the impression that it didn't actually matter what these made up UUIDs were and that I could just put anything, but that doesn't seem to be the case. Please just tell me exactly what you want the made up UUIDs to be and I will make them that. I would really just like to get support for printing selections for tokens in the xml. |
Progress towards #306.Fixes #306.As discussed in the Discord, I have updated BruebachL's script to fallback to using the UUID found in the Scryfall URL if a match can't be found in MTGJSON, which means the vast majority of tokens will get unique UUIDs with this PR. This still leaves 78 tokens without UUIDs, because they either have no printing at all, a printing not on Scryfall, or custom token art.
Interestingly, these tokens actually seem to work fine in my testing, even the ones that have unique printings in other sets (see, for instance, the Black Cat token from APC). I am not sure how that would interact with custom tokens though, so maybe they still need unique UUIDs despite this. I am happy to adjust this PR to give them UUIDs or make a subsequent one, as needed.
I have attached the modified version of the script I used to generate the UUIDs. Thank you, BruebachL!