Skip to content

Fix bulk card image downloader to use Scryfall CDN - #10928

Closed
phughk wants to merge 21 commits into
Card-Forge:masterfrom
phughk:fix-10413-card-image-downloader
Closed

Fix bulk card image downloader to use Scryfall CDN#10928
phughk wants to merge 21 commits into
Card-Forge:masterfrom
phughk:fix-10413-card-image-downloader

Conversation

@phughk

@phughk phughk commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Card image downloader: CDN cache + real rate limiting

Summary

Image downloads went through Scryfall's rate-limited API one card at a time.
A single 429 froze all downloads for a flat 5 minutes.
This adds a local cache of Scryfall's CDN image URLs, built via bulk data or per-set search, so most downloads skip the API and hit the unthrottled CDN directly.
Rate limiting itself is also fixed: correct per-endpoint pacing, and honoring Retry-After instead of a blanket cooldown.

Key changes

  • CdnUuidCache — local per-set cache: card/lang/face → CDN image UUID. Read-only on the gameplay path; a miss queues a background sync instead of blocking.
  • ScryfallBulkDataSync — one-shot download of Scryfall's full catalog (~350MB) to populate the cache for every set at once.
  • ScryfallSetSync — per-set fallback via /cards/search, for when only a few sets are missing.
  • ScryfallRateLimiter — replaces the old ad-hoc pacing/cooldown code; matches Scryfall's real per-endpoint limits and respects Retry-After.
  • ImageFetcher, GuiDownloadFilteredCardImages, GuiDownloadService — prefer CDN URLs when cached, warm the cache before a bulk run, wait out cooldowns instead of skipping.
  • DialogDownloadCardImages (desktop) / CardImageBrowserScreen (mobile) — new "sync bulk data" and "clear CDN cache" buttons.
  • FView, Forge, SplashScreen — first-run prompt offering the bulk sync.
  • New tests (CdnUuidCacheTest, ScryfallSetSyncTest), new gson dependency, new translation strings.

Component interaction

flowchart LR
    subgraph UI
        Dialog[DialogDownloadCardImages /<br/>CardImageBrowserScreen]
    end

    subgraph "Gameplay image path"
        IF[ImageFetcher<br/>Swing/LibGDX impls]
    end

    subgraph "Bulk download path"
        GDF[GuiDownloadFilteredCardImages]
        GDS[GuiDownloadService]
    end

    subgraph "Sync sources"
        SetSync[ScryfallSetSync<br/>per-set /cards/search]
        BulkSync[ScryfallBulkDataSync<br/>full catalog file]
    end

    Cache[(CdnUuidCache<br/>per-set .json.gz)]
    RL{{ScryfallRateLimiter}}

    Dialog -->|Download button| GDF
    Dialog -->|Sync Bulk Data button| BulkSync
    Dialog -->|Clear CDN Cache button| Cache

    IF -->|getCdnUrlIfCached — read-only| Cache
    GDF -->|isSetCached / getCdnUrlIfCached| Cache
    GDF -->|warm missing sets| SetSync
    GDF --> GDS

    Cache -->|miss: queue background sync| SetSync
    SetSync -->|mergeSetEntriesWithFaces| Cache
    BulkSync -->|mergeSetEntriesWithFaces| Cache

    SetSync -.->|acquire / 429| RL
    BulkSync -.->|acquire — listing call only| RL
    GDS -.->|acquire / 429 / cooldown wait| RL
    IF -.->|acquire / 429 / shouldSkip| RL
Loading

Bulk sync sequence

sequenceDiagram
    participant User
    participant Dialog as DialogDownloadCardImages
    participant Bulk as ScryfallBulkDataSync
    participant RL as ScryfallRateLimiter
    participant API as api.scryfall.com
    participant Files as *.scryfall.io (bulk file host)
    participant Cache as CdnUuidCache

    User->>Dialog: Click "Sync Bulk Data"
    Dialog->>User: Confirm ~350MB download?
    User-->>Dialog: Confirm
    Dialog->>Bulk: sync(progressListener, cancelled)

    Bulk->>RL: acquire()
    Bulk->>API: GET /bulk-data
    API-->>Bulk: listing JSON
    Bulk->>Bulk: find "default_cards" download_uri

    Bulk->>Files: GET default_cards file (gzip, unthrottled)
    loop stream + parse each JSONL line
        Files-->>Bulk: next card record
        Bulk->>Bulk: group by set → cn → lang → [front, back]
        Bulk-->>Dialog: onProgress(bytes/%, cards seen)
        Dialog-->>User: update progress bar
    end

    loop for each set collected
        Bulk->>Cache: mergeSetEntriesWithFaces(setCode, entries)
        Cache->>Cache: write {set}.json.gz, drop stale in-memory entry
    end

    Bulk-->>Dialog: return setCount
    Dialog->>Dialog: re-enable buttons, refresh total/downloaded/missing stats
    Dialog-->>User: "N sets synced"
Loading

Reviewer notes

  • Bulk sync is ~350MB — confirm the size is prominent in the prompt.
  • Cooldowns now block-and-wait instead of skip — confirm cancel actually interrupts the wait.
  • Leftover debug System.out.printlns in Forge.java / SplashScreen.java.
  • First-run prompt fires from three places (FView, Forge.afterDbLoaded, SplashScreen) — check for double-prompting.
  • gson version pinned explicitly in forge-gui/pom.xml — check for conflicts elsewhere in the build.
  • Sync/clear-cache UI logic is duplicated between desktop and mobile, not shared.

Introduces CdnUuidCache which lazily loads res/cdn_uuid/{setCode}/{cn}.json
files and resolves CDN URLs (cards.scryfall.io, no rate limit) for card image
fetching. ImageFetcher and GuiDownloadFilteredCardImages prefer CDN URLs when
the asset files are present, falling back to the Scryfall API and then the
cardforge server as before.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@phughk
phughk force-pushed the fix-10413-card-image-downloader branch from e4dd5ad to 6430183 Compare June 9, 2026 22:34
Hugh Kaznowski and others added 3 commits June 10, 2026 00:05
- Add package-private `cdnBaseDirOverride` and `clearCacheForTesting()` so
  tests can supply a temp directory without triggering ForgeConstants/GUI init
- Switch ensureSetLoaded to use the override when set (production path unchanged)
- Log DEBUG with absolute path when a set directory is not found, making
  path-resolution failures visible in runtime logs
- Add CdnUuidCacheTest covering happy path, language fallback, DFC front/back,
  same-UUID DFC, missing set (MISSING_SET sentinel caching), missing collector
  number, null inputs, and set-code case normalisation (16 tests total, all pass)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both SwingImageFetcher and LibGDXImageFetcher only applied the
.full → .fullborder path transform for api.scryfall.com URLs.
CDN URLs (cards.scryfall.io) were saved as .full.jpg, which the
game's image display code doesn't find — causing repeated
re-download attempts.

Add URL_SCRYFALL_CDN constant to ForgeConstants and use it in
both fetchers so CDN downloads are treated identically to API
downloads for file-path purposes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CdnUuidCache now loads per-set JSON from {cacheDir}/cdn_uuid/{set}.json.
On first lookup for a set the file is fetched from forge-extras (no rate
limit) and written to the local cache for all subsequent resolutions.
Returns null on any failure — callers fall back to the Scryfall API as before.

ForgeConstants: replace CDN_UUID_DIR (res/) with CACHE_CDN_UUID_DIR (cache/)
and FORGE_EXTRAS_CDN_UUID_URL.

Tests use localCacheDirOverride + remoteBaseUrlOverride (file:// in tests)
to verify the full local-hit and remote-fetch paths without network access.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed

@phughk

phughk commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Will tackle tonight or tomorrow night

Hugh Kaznowski added 2 commits July 28, 2026 08:34
- Request Accept-Encoding: gzip when fetching a set's UUID JSON from
  forge-extras; raw.githubusercontent.com honors it (~48% smaller on
  the wire for this UUID-heavy JSON), decompressed transparently.
- Store the local disk cache gzip-compressed too (.json.gz), saving
  space for whichever sets a user actually triggers a lookup for.

The files committed to forge-extras itself stay plain, sorted text —
compressing those would turn every regeneration into a full binary
rewrite instead of the minimal diff the CLI now produces.

Verified against the live forge-extras branch: server returns
Content-Encoding: gzip (110,709 bytes) and decompresses back to the
full 214,541-byte file. All 17 CdnUuidCacheTest cases still pass.
Comment thread forge-gui/src/main/java/forge/gui/download/ScryfallBulkData.java Outdated
Hugh Kaznowski added 2 commits July 29, 2026 11:33
…est API

Addresses the Discord discussion on forge-owned vs. user-owned CDN data:
this adds a user-triggered path that doesn't depend on forge-extras at
all, alongside (not instead of) the existing hosted data.

Scryfall's /cards/manifest endpoint returns just id/set/collector_number
/lang per card -- everything this lookup needs -- in ~15k-entry pages,
without the ~100MB-2.5GB cost of a full bulk-data export. English
coverage is currently ~8 pages (~25-30MB). It doesn't expose the actual
CDN image URL, so a double-faced card's back face is assumed to share
the front's id; verified against live Scryfall data that this holds for
the overwhelming majority of DFCs (the CDN URL only differs by a
front/back path segment), and the existing CDN-miss fallback already
covers the rare exceptions.

ScryfallManifestSync paginates the manifest (respecting its documented
10/minute rate limit), and supports incremental resync: entries are
requested newest-image-update-first, with a persisted per-language
watermark so a repeat sync typically stops after one page instead of
re-walking the whole catalog.

CdnUuidCache gains mergeSetEntries() (fills gaps without ever
overwriting existing entries -- forge-extras/CLI data can carry a
double-faced card's real distinct front/back UUIDs, which a manifest-
derived guess must not clobber) and a public clearCache(), covering the
simpler "let users purge it from settings" alternative to per-file
expiry that came up in the same discussion.

Both actions are wired into the mobile card-image-download screen,
which is where this whole feature already lives (desktop never exposed
"Download Card Images" -- only mobile/CardImageBrowserScreen does).

10 new tests in ScryfallManifestSyncTest (embedded HTTP server, no live
network dependency) cover pagination, the incremental watermark, the
never-overwrite merge policy, and cache clearing; all 27 CDN-related
tests pass together.
Addresses tool4ever's review comment on PR Card-Forge#10928: a single-method
class wrapping a URL format string didn't warrant its own file.
CdnUuidCache.getCdnUrl() was the only production caller, so cdnUrl()
moves there as a public static method; test call sites updated to
match, and ScryfallBulkDataTest's formula assertion is folded into
CdnUuidCacheTest instead of being dropped.
Hugh Kaznowski added 5 commits August 11, 2026 18:33
CdnUuidCache no longer fetches per-set JSON from the forge-extras repo.
On a cache miss it now asks the new ScryfallSetSync to build that set's
mapping on the spot from Scryfall's card search API (scoped to that one
set, all languages), the client-side equivalent of what the old
forge-extras/CLI generator did against a full bulk-data export. Unlike
ScryfallManifestSync (which only ever sees a card's own id), the search
API exposes each face's own image URL, so a double-faced card's rare
genuinely-distinct back-face UUID is captured precisely via the new
CdnUuidCache.mergeSetEntriesWithFaces, instead of assumed to match the
front.

This replaces the forge-scryfall-uuid-map CLI and forge-extras cdn_uuid
data entirely -- both PRs are being closed in favor of this.
…age-downloader

# Conflicts:
#	forge-gui-desktop/src/main/java/forge/util/SwingImageFetcher.java
Hugh Kaznowski added 3 commits August 17, 2026 00:17
…age-downloader

# Conflicts:
#	forge-gui/res/languages/ko-KR.properties
No behavior change: condenses multi-paragraph javadoc down to one or two
lines per method/class, drops inline comments that just restated an
adjacent assert message or code branch, and removes a duplicated
CDN/API/cardforge priority list that appeared in both a class and method
javadoc.
@phughk

phughk commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

For context

  • the other prs are closed (forge-extras mirror, cli tool)
  • local only cdn uuid resolution
  • resolution comes from api instead of bulk exports
  • cdn resolution is non-blocking
  • pr message has been re-written to cover details

Hugh Kaznowski and others added 2 commits August 17, 2026 22:56
Global ScryfallRateLimiter replaces scattered per-class throttling, paces
per Scryfall's documented per-endpoint limits (500ms for search/named,
100ms otherwise), and backs off using the response's Retry-After header
instead of a flat 5-minute cooldown. cards.scryfall.io CDN URLs continue
to bypass the limiter entirely.

CdnUuidCache's implicit auto-sync-on-miss no longer fires from gameplay's
lazy image fetch or the bulk downloader's per-card lookup -- both are now
read-only against the local cache (getCdnUrlIfCached), so a missing image
during normal play never silently kicks off a full-set Scryfall search in
the background. The bulk downloader's explicit warm-up loop is now the
only thing that triggers a sync, waits out an active cooldown instead of
skipping through it, and skips any set already cached locally.

Add ScryfallBulkDataSync: resolves CDN links for every set in one pass
from Scryfall's bulk data export instead of one paginated /cards/search
call per set, wired up as a new button in both the desktop and mobile
card image downloader screens.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@phughk

phughk commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #11709

@phughk phughk closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants