smogon / pokemon-showdown-client

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

psim.us client does not update info (move descriptions/stats/etc) for mods not on main server #758

Closed KendallPark closed 7 years ago

KendallPark commented 8 years ago

As it stands right now, the psim.us client used by individuals depends on githooks to generate search indexes and the teambuilder table based off the main PS repo. If you make any modifications to moves/pokedexes/items/etc hosted on your own server, it will not show up on the prim.us client. This forces server owners that want to use their own custom mods to configure their own client.

It's a non-trivial headache but probably only affects a tiny subset of PS devs.

One idea thrown around was to move teambuilder-table logic to the server and have a "use at your own risk" option to serve this table to the client. This would also require the client to get smarter about getting the override tables and using them. On the downside, this is not a feasible solution for PS main because of bandwidth.

Another option is for the client to generate the tables and search index on the fly. The client needs access to tools.js, and the entire data/ and mods/ directories. I can't think of a performant way to get these to the client side of things without adding something like S3 into the mix. You could create static endpoints for all of those resources on the server but there's no way that will scale.

Currently I have a half-baked solution on my own branch of the server: https://github.com/Zarel/Pokemon-Showdown/compare/master...KendallPark:server-teambuilder-table?expand=1

Honestly having a separate, dedicated pokedex/movedex/item data API seems better for the non-logic pieces of this (and then users could spin up their own). Clients wouldn't have to load all-info-ever-from-all-time to play PS but could simply query for what they needed in what generation, etc. And it would make search less hacky. But that's a massive rewrite (and who has that kind of time?) ;) So like Rails + postgres server that periodically pulls the master PS branch and updates a database with pertinent data. Then the client interfaces with that instead for all the frontend-y necessities.

Zarel commented 8 years ago

The way I'd design this is to mostly keep the current design, but allow the server to override the client by sending JSON blobs to replace (or merge, mod-style) the client's data tables with.

The main issue is malicious data: I'm not sure we do sufficient escaping/sanitization of Tools data because we generally expect it to be trustworthy; it's possible a malicious server could send data.

So, two issues:

  1. Make sure all access to BattlePokedex, BattleItems, etc is properly sanitized/escaped; the best way may just to strip < from every string in them when applying JSON blobs from the server; as far as I know we don't need that character for anything.
  2. Support sending JSON blobs from the server to replace the server's JSON blobs.
  3. (optional! bonus!) Maybe some sort of automated detection of whether or not the data files have changed on the server, to determine whether or not to send said JSON blobs.
KendallPark commented 8 years ago

Yeah, that would work. Then the client runs its own build-indexes over the newly replaced data?

Build-indexes needs to be majorly refactored as it's currently a super hard-coded god method and isn't very flexible. It could probably be teased apart into two or three more modular functions. You could even make it lazy so it only builds tables for what it needs (like when you enter a room).

(optional! bonus!) Maybe some sort of automated detection of whether or not the data files have changed on the server, to determine whether or not to send said JSON blobs.

Sounds like a job for hashes!!! IDK whether this is a bad smell or not, but theoretically the server could take a peek into the git internals and compare SHAs for blobs and directories.

Zarel commented 8 years ago

Yeah, that would work. Then the client runs its own build-indexes over the newly replaced data?

More like the server runs its build-indexes equivalent before sending data to the client. That way minimizes bandwidth, after all.

Build-indexes needs to be majorly refactored as it's currently a super hard-coded god method and isn't very flexible.

:(

It's as hardcoded as it needs to be, gosh.

Sounds like a job for hashes!!! IDK whether this is a bad smell or not, but theoretically the server could take a peek into the git internals and compare SHAs for blobs and directories.

Yes, I'd use hashes. Taking a peek into Git internals is a bad idea because some people just download ZIPs or whatever. Maybe a file with a hash of every data file, which gets updated...

Or maybe just support it for mods, by comparing mod data with main data (build-indexes already does this). Honestly, I think the server might be able to get away with just sending teambuilder table data to the client; it doesn't need anything else, right?

KendallPark commented 8 years ago

:( It's as hardcoded as it needs to be, gosh.

Sorry, didn't mean to be critical. The way it's programmed makes sense for the needs of the main server right now. It doesn't take into account the addition of new mods because that's not a feature main PS needs. If you iterate through [5, 4, 3, 2, 1] it won't hit any of the custom mods. (Like it doesn't hit stadium for example.)

I took at stab at refactoring it right here. https://github.com/KendallPark/micromon/blob/bdba95a9f498ba667ffbcbec96ce812c55bc6f8a/teambuilder-tables.js#L313-L394

It's not optimized for node though. And it's using an internal of Tool.js that doesn't seem to be intentionally exposed so not completely ideal stylistically.

some people just download ZIPs or whatever

Oh right that's a thing.

Honestly, I think the server might be able to get away with just sending teambuilder table data to the client; it doesn't need anything else, right?

So that was my thought. Like a use-at-your-own-risk option that other servers can flip on. The only other thing is search. I haven't really dug into how search works yet because the stuff I'm working on is all random team-based stuff.

KendallPark commented 8 years ago
let canonModsByGen = {};
let customModsByGen = {};
for (const modName of Object.keys(Tools.moddedTools)) {
  const mod = Tools.moddedTools[modName];
  if (!customModsByGen['gen'+mod.gen]) customModsByGen['gen'+mod.gen] = {};
  if (modName === 'gen'+mod.gen) {
    canonModsByGen[modName] = mod.includeData().data;
  } else {
    customModsByGen['gen'+mod.gen][modName] = mod.includeData().data;
  }
}

let genNextData;
for (const canonGenName of Object.keys(canonModsByGen).sort().reverse()) {
  const genData = canonModsByGen[canonGenName];
  // compare with younger gen for overrides
  if(genNextData) {
    BattleTeambuilderTable[canonGenName] = getOverrides(genData, genNextData, BattleTeambuilderTable[canonGenName]);
  }
  // compare non-canon mods with their inherited generation and get additional overrides
  for (const modName of Object.keys(customModsByGen[canonGenName])) {
    BattleTeambuilderTable[modName] = getOverrides(customModsByGen[canonGenName][modName], genData, BattleTeambuilderTable[canonGenName]);
  }
  genNextData = genData;
}

What this does is iterate through what I call the "canon mods" so like gen1, gen2, and then for each "canon mod" it iterates through non-canon mods that are technically of that generation as well and merges that generation's overrides with any additional ones the mod provides. Like stadium is gen 1 and should have all the move descriptions of gen 1 plus any that it added on it's own. Then it add those mods to the BattleTeambuildertable for a 1-to-1 correspondence between directories in the mods folder and mods in the builder table.

Zarel commented 7 years ago

This is a clone of Zarel/Pokemon-Showdown#2557.

Since this would require both client and server changes, I'm consolidating in that issue.