Fix Omega Ruby/Alpha Sapphire sprite key in generation-vi sprites - #1689
santichausis wants to merge 4 commits into
Conversation
POKEMON_SPRITE_CONFIG used the sprites repo's folder name
("omegaruby-alphasapphire") as the dict key for generation-vi ORAS
sprites, instead of the actual version-group name
("omega-ruby-alpha-sapphire", per version-group/16). Since this dict
is resolved verbatim into the sprites JSON stored on Pokemon and
PokemonForm, both pokemon/<id>/sprites/gen-vi/ and
pokemon-form/<id>/sprites/gen-vi/ returned the wrong key.
Only the dict key changes; the file path strings inside it keep
referencing the real (non-hyphenated) sprites repo folder.
|
do the files still resolve currently from the sprites repo? |
|
Yes — the file paths are unchanged, only the dict key is renamed. I checked the sprites repo directly ( |
| ) | ||
|
|
||
|
|
||
| class PokemonSpriteConfigTestCase(TestCase): |
There was a problem hiding this comment.
a standalone test case just for a single key seems weird, perhaps you can check our existing tests for sprite models? and the key is there, a asset not in is not required in the future it would be confusing for people why its there in first place
we could have a key and path assertion per group
There was a problem hiding this comment.
Reworked it. No existing tests exercise POKEMON_SPRITE_CONFIG (the sprite tests in tests.py build the sprites JSON by hand), so I kept it next to the other non-DB checks in test_models.py. It now does the key-and-path assertion per group you suggested: for each generation-vi group, the expected key, and that its paths live under the matching sprites-repo folder. The path part matters here because the repo folder really is omegaruby-alphasapphire (…/omegaruby-alphasapphire/1.png → 200, …/omega-ruby-alpha-sapphire/1.png → 404), so the test also catches someone "fixing" the paths to match the key. I dropped the assertNotIn.
I didn't extend it to every generation, because keys don't map 1:1 to version groups everywhere. For example, generation-ii has separate gold/silver keys for the single gold-silver version group (their sprites differ), so a blanket rule would need an exception list.
|
hi left a review on a sidenote currently there is an open issue #1687 which should be fixed with an def _pokemon_form_sprite_lookup(info: list[str]) -> Callable[[str, str], str | None]:
form_identifier = info[2]
pokemon_id = int(info[3])
is_default = info[5] == "1"
file_names = []
if form_identifier:
file_names.append(f"{pokemon_id}-{form_identifier}")
if is_default or not form_identifier:
file_names.append(str(pokemon_id))
def lookup(path: str, extension: str) -> str | None:
for file_name in file_names:
sprite = file_path_or_none(f"{path}{file_name}.{extension}")
if sprite:
return sprite
return None
return lookupshould be a small change, the above code should fix it can you make a quick edit and spotcheck the forms please thanks |
Replace the single-key check with one that asserts, per generation-vi group, the expected key and that its paths live under the sprites repo folder. This also guards against renaming the paths to match the key, which would break resolution: the sprites repo folder really is omegaruby-alphasapphire.
_pokemon_form_sprite_lookup only looked for {pokemon_id}-{form}.png,
but a default form's sprites are usually stored under the bare pokemon
id. unown-a's front sprite is 201.png (201-a.png doesn't exist), so
most of its form sprites came out null (PokeAPI#1687).
Try the form-specific file first and fall back to the bare id for
default forms. Non-default forms keep resolving only to their own
files.
|
Done, applied your change to Spot-checked all 618 forms that have a form identifier against a mirror of the sprites repo tree, comparing old vs new lookup:
One note: the |
| "omegaruby-alphasapphire": { | ||
| "omega-ruby-alpha-sapphire": { | ||
| "front_default": ( | ||
| "pokemon/versions/generation-vi/omegaruby-alphasapphire/", |
There was a problem hiding this comment.
Hey @FallenDeity what do you think about changing the naming also on the sprites repo? I know we would break some applications but for the sake of consistency maybe it's a good move?
There was a problem hiding this comment.
yeah having it consistent would be nice I can make a separate change for this later this weekend I'll need to expose the new sprites I added as well will do it in one go then
I need to make a few folder changes/additions for gen 8 as well so it would fit in nicely with that
There was a problem hiding this comment.
OK, I set a banner on the homepage. I set a go-live date to next week the 7th. If you don't have time we can postpone with no issues.
@santichausis can you modify this PR to take into account this change? Either now or after @FallenDeity will update the sprites repo.
On the 7th then we can merge the sprites PR and then this PR.
There was a problem hiding this comment.
Sure. I'll wait for the sprites repo PR so I can check the new paths against it, then update the gen-vi entries in POKEMON_SPRITE_CONFIG and the test that pins the folder name, well before the 7th. @FallenDeity since you'll also be exposing new sprites and changing gen 8 folders, let me know if you'd rather handle the config side yourself in one go, to avoid conflicting edits in build.py.
There was a problem hiding this comment.
Done. I switched the gen-vi ORAS paths to pokemon/versions/generation-vi/omega-ruby-alpha-sapphire/ and updated the test, so it now requires every gen-vi group's key and sprites folder to match. Heads-up: the sprites repo still has the old omegaruby-alphasapphire folder, so this needs to land after the sprites rename. If merged before it, all ORAS sprites would resolve to null.
|
changes look good thanks :) ps: naramsim lmk how you feel about my suggestion if so we can merge this and do the folder refactor in another pr |
Which one? I'm a bit lost :) @santichausis maybe you can update the code even before Fallen will merge his PR over at the sprites repo. Anyways we already know the path that we will use, it's gonna be |
The sprites repo is renaming pokemon/versions/generation-vi/ omegaruby-alphasapphire to omega-ruby-alpha-sapphire so the folder matches the version-group name used as the key. Update the eight gen-vi ORAS paths accordingly and have the test require every gen-vi group's key and folder to match. This must land after the sprites repo rename; before it, ORAS sprites would resolve to null.
Fixes #1684, fixes #1687.
Warning
Merge only after the sprites repo renames
pokemon/versions/generation-vi/omegaruby-alphasapphiretoomega-ruby-alpha-sapphire(planned for 2026-10-07). Merged before that, all ORAS sprites would resolve tonull.Problem
POKEMON_SPRITE_CONFIG["versions"]["generation-vi"]used"omegaruby-alphasapphire"as the dict key for Omega Ruby/Alpha Sapphire sprites — that's the sprites repo's folder name, not the actual version-group name (omega-ruby-alpha-sapphire, perversion-group/16/data/v2/csv/version_groups.csv)._resolve_sprite_configcopies this dict's keys verbatim into the sprites JSON stored onPokemon/PokemonFormat build time, so bothpokemon/<id>/sprites/gen-vi/andpokemon-form/<id>/sprites/gen-vi/returned"omegaruby-alphasapphire"instead of"omega-ruby-alpha-sapphire".Fix
Renamed the dict key to
omega-ruby-alpha-sapphire, and pointed its eight paths atpokemon/versions/generation-vi/omega-ruby-alpha-sapphire/..., matching the upcoming rename of that folder in the sprites repo so key and folder agree.Note: this renames a key in the sprites JSON, so clients reading
versions.generation-vi["omegaruby-alphasapphire"]will need to switch to"omega-ruby-alpha-sapphire"(and the sprite URLs themselves change with the folder rename).Default form sprites (#1687)
_pokemon_form_sprite_lookuponly looked for{pokemon_id}-{form}.png, but a default form's sprites are usually stored under the bare pokemon id: unown-a's front sprite is201.png(201-a.pngis a 404), so most of its form sprites came out null. It now tries the form-specific file first and falls back to the bare id for default forms; non-default forms keep resolving only to their own files.Spot-checked all 618 forms with a form identifier against a mirror of the sprites repo tree, old vs new lookup: the 382 default forms gain 10,061 previously-null sprites in total, nothing that already had a value is lost or changed, and the 236 non-default forms are untouched.
Test plan
PokemonSpriteConfigTestCase.test_generation_vi_sprite_group_keys_and_pathsinpokemon_v2/test_models.py: asserts the expected generation-vi groups and that every group's paths live in a folder named like its key. Fails if the key is reverted or if any path is left on the old folder.test_form_sprite_lookup_falls_back_to_pokemon_id_for_default_forms: a default form falls back to the bare id, a form-specific sprite still wins, and non-default forms don't fall back. Fails on the old lookup and if the fallback is applied to every form.manage.py test pokemon_v2passes (65 tests);ruff check,ruff format --checkandtypass.