smogon / pokemon-showdown-client

The client for Pokémon Showdown
http://pokemonshowdown.com
GNU Affero General Public License v3.0
552 stars 770 forks source link

Add support for alternative past gen sprites #1369

Open scheibo opened 4 years ago

scheibo commented 4 years ago

Context: https://www.smogon.com/forums/threads/dppt-sprites.3653313/

Contributors are working on providing the 96x96 transparent PNG resources for past gens, but we need to provide a way to allow for users to select their preference (see NetBattle screenshot below)

NetBattle

A new preference, spritegfx, will be added which will be populated with the keys from SPRITE_SOURCES, with the value in that mapping being the value that becomes persisted in local storage. This select box contains information that will replace or alter the following preferences

Which sprites to use will depend on the gen of the battle taking place, the sprite preference and the Pokemon's generation (to be able to determine availability).

function getSpritePreference(template: Template, gen: number) {
    const pref = Dex.prefs('spritegfx');
    if (pref && SPRITE_GENS[pref]) {
        if (SPRITE_GENS[pref] >= template.gen) return {dir: pref, gen: SPRITE_GENS[pref]};
        gen = 5; // fallback
    }
    const ani = !pref || !pref.endsWith('ani');
    gen = Math.max(gen, Math.min(template.gen, 5));
    return {dir: SPRITE_GEN_DEFAULTS[ani ? 'animated' : 'static'][gen], gen};
}

Whether we actually support animating old gen sprites (only on switch, by toggling back and forth between frames 1 and 2) is out of scope for this change (NetBattle just allowed selecting whether you prefer the static frame 1 or frame 2, but Zarel seems to think that would complicate the UI, though its already a long select box...).

One open question is directory naming - currently the folders are named 'rby'...'xy' instead of 'gen1'...'gen6' - a first step here may be to rename the existing resources to allow for the naming convention used by the trainer sprites (eg. 'gen1' for RBY and gen1rb for RB).

Zarel commented 4 years ago

Yes, I agree with renaming them to gen1-gen5. Models should be prefixless, I think.

scheibo commented 4 years ago

I have written @pkmn/img which contains all of the logic required for displaying Pokémon Showdown's sprite and icon resources and handles alternative gens and signficantly more of the sprite edge cases than the functions in battle-dex.ts. This is useful as its own package for use in other projects (we will be using it in the damage calc and the new stats UI im writing), and is written to be flexible and not depend on PS's data layer. This is the logic intended to implement this bug (outside of the UI changes and in-battle offsets, which are PS-specific and would be implemented after), however, you have several options:

  1. add a dependency on @pkmn/img, either via NPM or via directly depending on its bundled output (which is deliberately made to be compatible with PS flags etc)
  2. allow me to move the entire img/ tree out of https://github.com/pkmn/ps and into https://github.com/smogon/pokemon-showdown-client and publish the package from here as @pokemon-showdown/img instead of @pkmn/img
  3. copy the functionality out of the package yourself into PS if you wish to improve your existing sprite support and add support for alternative old gens

Option 2 is by far my preference - this logic makes sense to belong in this tree, with its build step replacing build-minidexes instead of building on top of its output. Please review the package - it is already more comprehensive than PS's logic (though I'm still working through yet more edge cases) and significantly better tested. I am open to discussion of changes to interfaces or APIs provided they do not compromise what this package offers in its current state.

If you wish to go with either option 1 or 2, we would:

No APIs would need to change within the PS client, the one main change would be a single data file for sprites instead of 2. Technically this is a very small initial load regression (~34KB total instead of ~20KB for BattlePokemonSprites and ~8KB for the tables, all numbers compressed), though you come out ahead in total size if BW sprites are also used (~16KB compressed, so 44KB total if BW sprites are loaded). However, the package allows for very fine grained loading and supports adapting to your own data library if necessary, so its entirely possible to accept a 0KB initial load regression and maintain the exact same loading semantics if you wanted (I would not really endorse this, the Pokemon's IDs are the heaviest part of the file and putting them all together means you can condense them). Furthermore, because its store as JSON you'll probably come out ahead anyway given engines optimization of JSON parsing.

Zarel commented 4 years ago

You hate it when I review your code. Just push the tree into client, I won't mind.

scheibo commented 4 years ago

I do not ‘hate it when you review my code’. I think code review is important and would like to make sure that you’re on board with the approach and design if this is to be housed under your project and used in the client. 🙂

Zarel commented 4 years ago

You say "you're right but I don't care about this anymore" and close your PRs even when I bring up legitimate points. I don't want to get yelled at for trying to understand what your code does. I don't want to get yelled at for pointing out legitimate problems. You're not the only one who gets burnt out by stuff like that. I don't want to review your code when you do things like that.

Push it yourself; you're a collaborator. I don't mind.

scheibo commented 4 years ago

The disagreement lies on what you characterize as legitimate problems. I would like to see what you consider legitimate problems with the current code as it would provide critical insight into whether we're on the same page or whether there are irreconcilable differences.

Zarel commented 4 years ago

You often cut me off before I even bring up any problems and am just trying to understand what your code is trying to do. It's especially frustrating because it's often a PR I think would be beneficial to the codebase, if I could only understand it enough to be able to maintain it.

I don't want to deal with that anymore. Reviewing your code is not a good experience for me. Just push it. Option 2 is fine by me.

scheibo commented 4 years ago

If thats the case, I'm not comfortable moving the code under this repo.