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

Spec for card image download priority order #2479

Closed
Daenyth opened this Issue Mar 14, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@Daenyth
Copy link
Member

Daenyth commented Mar 14, 2017

Currently we have a loosely-defined image url download priority order with fallback.

The goal of this ticket is to come up with a flexible, understandable design that helps users get the images they want.

At a high level, we currently have:
if card has muid:

  • try main url for every enabled set containing that card in priority order
    If card does not have muid:
  • use fallback

I propose instead:

  • Allow users to make a list of download urls to attempt, with a priority order.

When a card image is requested:

  • For each enabled set in order, try all download url templates in order, skipping url templates which ask for information that the card doesn't have
    • eg if the template asks for muid, and card has none, skip that url.

In pseudocode:

QList<UrlTemplate> urls = get_urls_for_card(cardInfo, enabledSetConfiguration);
for (UrlTemplate url, urls):
    Url url = UrlTemplate.apply(cardInfo)
    response = download(url)
    if response.ok:
        save(response.image)
    else:
        continue

This ticket supersedes #1984 and #2346

@andrewzwicky

This comment has been minimized.

Copy link
Contributor

andrewzwicky commented Aug 5, 2018

FYI I'm going to take a look at this (so multiple people aren't working on the same thing). The first thing I'd like to do is just take the 2 existing URLs and fall back as expected: On 404 for the first one, (instead of muid of card), going by set in priority order.

The list of multiple URLs by priority could come in a later PR. No idea how long I expect it to take.

@Daenyth

This comment has been minimized.

Copy link
Member Author

Daenyth commented Aug 5, 2018

@andrewzwicky come grab us in gitter if you get stuck! This is exciting :)

@andrewzwicky

This comment has been minimized.

Copy link
Contributor

andrewzwicky commented Aug 10, 2018

I have something tentatively working, here's a log in action.

Trying to load picture for card: "Angelic Benediction"  from set: "DD3_DVD"
Picture not found, trying to download
Trying to download picture for card: "Angelic Benediction"  from set: "DD3_DVD" from url: "http://magiccards.info/scans/en/dd3_dvd/19.jpg"
following redirect: "Angelic Benediction" Url: "https://magiccards.info/scans/en/dd3_dvd/19.jpg"
Download failed: "Error transferring https://magiccards.info/scans/en/dd3_dvd/19.jpg - server replied: Not Found"
Possible picture at  "https://magiccards.info/scans/en/dd3_dvd/19.jpg"  could not be loaded
Trying to load picture for card: "Angelic Benediction"  from set: "DD3_DVD"
Picture not found, trying to download
Trying to download picture for card: "Angelic Benediction"  from set: "DD3_DVD" from url: "http://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=394008&type=deck"
Possible picture at  "http://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=394008&type=deck"  could not be loaded
Trying to load picture for card: "Angelic Benediction"  from set: "M13"
Picture not found, trying to download
Trying to download picture for card: "Angelic Benediction"  from set: "M13" from url: "http://magiccards.info/scans/en/m13/4.jpg"
following redirect: "Angelic Benediction" Url: "https://magiccards.info/scans/en/m13/4.jpg"
Image successfully downloaded from  "https://magiccards.info/scans/en/m13/4.jpg"

This is using:
http://magiccards.info/scans/en/!setcode_lower!/!setnumber!.jpg as my primary URL and http://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=!cardid!&type=deck as my secondary URL, (with =deck on the end, so it will intentionally fail).

For the set that doesn't match, the download is attempted, but then it moves the secondary URL, then to the next set. Code needs to be cleaned up and tested more thoroughly, but it's looking good so far.

@Daenyth

This comment has been minimized.

Copy link
Member Author

Daenyth commented Aug 10, 2018

@andrewzwicky awesome! Do you want to open a WIP PR for any early feedback?

@andrewzwicky

This comment has been minimized.

Copy link
Contributor

andrewzwicky commented Aug 10, 2018

@Daenyth Yeah that's a good idea, I'll do that.

@tooomm tooomm reopened this Aug 11, 2018

ZeldaZach added a commit that referenced this issue Dec 20, 2018

Use the loader defined #2479 slightly better
Signed-off-by: Zach Halpern <ZaHalpern+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.