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

Unable to download card art (1.96 on Windows 10) #1655

Open
aknight2015 opened this issue Nov 17, 2020 · 10 comments
Open

Unable to download card art (1.96 on Windows 10) #1655

aknight2015 opened this issue Nov 17, 2020 · 10 comments

Comments

@aknight2015
Copy link

I'm trying to download the missing card art and all I'm getting is a "file not found" error for every card.

@JakkinStewart1140
Copy link

Same problem here, but macOS 11.01 (Bg Sur)

@Zapeth
Copy link
Contributor

Zapeth commented Apr 1, 2021

I don't know if this is the case (just stumbled upon this game recently), but it looks to me like the endpoints for the images need to be reworked? The failed requests point to https://scryfall.com which has a documentation for the REST API to retrieve card information (including images) -> https://scryfall.com/docs/api/cards

For example, the request for 0_1_black_Cleric_creature_token.txt resolves to https://c1.scryfall.com/file/scryfall-cards/large/en/tdom/4.jpg which returns a 404, but the (or a) proper method of getting the image would be to call https://api.scryfall.com/cards/tdom/4/en?format=image&version=large instead (though large version is default anyway)

Though unfortunately this doesn't seem applicable to every card, for example I haven't even been able to find 0_0_black_Germ_creature_token.txt with a normal text search on https://scryfall.com/

edit: The proper link for 0_0_black_Germ_creature_token seems to be https://api.scryfall.com/cards/tmm2/7/en?format=image

@Zapeth
Copy link
Contributor

Zapeth commented Apr 2, 2021

I wrote script to check any bad card image links and fix them if possible. Luckily only a certain type of link was affected in the cards from the scripts_missing folder, so all of them could be fixed automatically.

As for the cards in the scripts folder I was able to fix about half of them, the rest needs to be fixed manually (and I don't really feel like looking up ~135 cards right now), listed in the attached scripts_todo.txt.

I could do a PR for this, but I'd like to hear feedback from the maintainers first. In the meantime you could fix the links yourself by running this bash script from the Magarena folder where the card folders are, using the attached scripts.txt and scripts_missing.txt as first argument (should work on Linux, unsure about macOS):

#/bin/bash
if [ -z "$1" ]; then
    echo "specify input file"
    exit 1
fi

while read -r target; do
    read -r data
    echo "$target -> $data"
    sed -i "/^image=/c\\$data" $target
done < $1

However, I wonder if it wouldn't be better to just rework all card data, considering https://scryfall.com/docs/api/cards provides most if not all the required data (via the format=json parameter, the images could still be fetched separately) and most image links resolve to scryfall.com already (a lot of cards have a link to https://magiccards.info, but they get redirected to https://scryfall.com)

@melvinzhang
Copy link
Contributor

melvinzhang commented Apr 5, 2021

Thanks for looking into the broken links. A PR for this will be great!

We use https://github.com/magarena/magarena-scripts-builder to generate the scripts from https://mtgjson.com/ so the image link can be fixed there.

@Zapeth
Copy link
Contributor

Zapeth commented Apr 6, 2021

Thanks for the pointer, though after looking through the files I have some more remarks:

  1. is the images-from-scryfall script still used for anything? Its wget link seems to be broken, but could be replaced with https://api.scryfall.com/bulk-data/default-cards?format=file to work again

  2. I'm guessing the image links are constructed here? However since those are still valid links (ultimately just redirected to scryfall.com) it seems like there are no changes necessary and the scripts just need to be regenerated from the scripts-builder code?

  3. I think the ideal solution would be for the client to generate the image links when requesting the card images, using the rest api from scryfall.com. Its undefined how long those links will remain valid too, but I'm guessing it will be longer than any direct link. This would also address switch to scryfall border_crop for card images #1190 by just adding the version=border_crop parameter to the request.
    I dont know if the required data for this (card set id and number in the set) is available in the data from https://mtgjson.com, but it could be generated from https://scryfall.com using a slightly adapted version of the images-from-scryfall script.

Unfortunately I don't have an environment set up for Java development/testing so I won't be able to do a PR in that regard, but I could fix/update the images-from-scryfall script if necessary.

@melvinzhang
Copy link
Contributor

  1. If I recall, images-from-scryfall script is used to generate CardImages.txt which is used as an override to the default image generated by scripts-builder. In a sense, it would be better to fix scripts-builder and only use CardImages.txt for the exceptional cases

  2. Yes, that's the place.

  3. Hmm, but that would require hardcoding the scryfall handling in the client itself. Part of the use case of the card file is to allow anyone to add a new card, for example a custom card, so we would still need to handle a direct URL. For now, I think it is better to keep the image as direct link to the image so that it would handle custom cards as well.

@Zapeth
Copy link
Contributor

Zapeth commented Apr 12, 2021

Hmm, but that would require hardcoding the scryfall handling in the client itself. Part of the use case of the card file is to allow anyone to add a new card, for example a custom card, so we would still need to handle a direct URL.

It could be implemented in a way where the image link is used if one is specified for the card (and otherwise a scryfall link is constructed internally). But considering all card script files would need to be recreated to include the necessary information for a scryfall link anyway (card set name and number in set, or scryfall id) its probably not worth the effort right now.

As for the current bad links, I don't know how they were created, but I don't see anything inherently wrong in scripts-builder Java code, so I'll just do a direct PR on the scripts at https://github.com/magarena/magarena/tree/master/release/Magarena if thats okay with you.

melvinzhang pushed a commit that referenced this issue Apr 13, 2021
All card image links from `scripts_missing` should be valid again

There are still about 135 invalid image links in `scripts`, those probably
need to be fixed manually (see
#1655 (comment))
@bilbo2
Copy link
Contributor

bilbo2 commented Apr 13, 2021

I remember the links were changed in the past perhaps few times as original URLs stopped working.
I assume we may need to update them in the future if scryfall changes ...

One solution might be to construct the scryfall link for a card automatically (perhaps have an algorithm or some database aside?), but be able to override the image in the card file itself for those few cases where the default algorithm does not work as expected.

@ghost
Copy link

ghost commented Apr 2, 2022

Can this fix be released in a new release please?

@PnYMaT
Copy link

PnYMaT commented Apr 23, 2023

+1

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

No branches or pull requests

6 participants