pkmn / ps

Modular Pokémon Showdown
MIT License
107 stars 15 forks source link

@pkmn/dex: G-Max Move Handling Changed Unexpectedly #17

Closed te1 closed 1 year ago

te1 commented 1 year ago

Describe the bug:

G-Max moves in showdown are classified as isNonstandard: "Gigantamax" (see https://github.com/smogon/pokemon-showdown/blob/master/data/moves.ts#L6756) which was also the case in @pkmn/dex as of version 0.6.4.

In 0.7.1 they are now isNonstandard: "Future" even in gen 8.

Example:

expect(Dex.forGen(8).moves.get("G-Max Befuddle").isNonstandard).toBe("Gigantamax");

// Expected: "Gigantamax"
// Received: "Future"

console.log(Dex.forGen(8).moves.get("G-Max Befuddle"));

Expected behavior:

It should still be "Gigantamax".

Additional context:

I think the issue is caused by this line: https://github.com/pkmn/ps/blob/main/dex/index.ts#L593 It sets the value to 'Future' because move.gen is 9 which is greater than 8. The line seems correct but gen shouldn't be 9 here.

In https://github.com/pkmn/ps/blob/main/dex/index.ts#L528 the gen for moves is set based on their number. Seems fine but for some reason all G-Max moves in showdown have number 1000 and now this breaks the logic.

Thanks for updating this for gen 9 btw!

scheibo commented 1 year ago

Hi @te1 - thanks for filing this issue!

I'm not sure this is technically a "bug" from @pkmn's POV per se, because it happens to be how PS defines these things upstream:

const ps = require('./vendor/pokemon-showdown');
const {Dex} = require('./dex');

console.log(ps.Dex.forGen(8).moves.get("G-Max Befuddle").isNonstandard);
console.log(Dex.forGen(8).moves.get("G-Max Befuddle").isNonstandard);

These both print Future.

I 100% agree this is nonintuitive and I would probably call this an upstream bug (ie. a bug/PR you should open against the main PS repository so that it will then get imported here) but I think the issue here is that these should both now print Past instead of Future. PS's isNonstandard field is unfortunately scalar and can only take on a single value, and usually Past is what PS fills in for past gen stuff like this, which is frustrating because it results in a loss of information (this has proved to be a problem with National Dex in the past - see the comment above the first bit of code here - Kris recently solved this by introducing a natDexTier instead).

I certainly think you can make the argument that the "Gigantamax" moves should retain isNonstandard: Gigantamax given that Gigantamax already implies gen 8 implies Past- I think thats a reasonable line to take, though I think it needs to be done upstream for consistency reasons.

tl;dr: please try to submit the equivalent of #18 upstream (https://github.com/smogon/pokemon-showdown/blob/master/sim/dex-moves.ts#L568) and once imported I will happily merge #18 (thanks for opening the PR!)

te1 commented 1 year ago

I will PR upstream. I wasn't aware the problem also exists there and understand things should be kept in sync. This will fix the gen jumping to 9.

Re Past: I'm not sure how PS handles that. In gen 9 past seems reasonable for gmax moves (although it would be preferred to still know they are gmax moves). But in gen 8 it shouldn't be past. As far as I can see they have a gen 8 mod that includes overrides for the max moves (to set isNonstandard: null), but not the gmax moves. I will ask in the PR to see what the reason for that is.

te1 commented 1 year ago

https://github.com/smogon/pokemon-showdown/pull/9027

scheibo commented 1 year ago

Reopened until I import PS and release this! The release wont happen until later today I think because i'd like to also include @livid-washed's latest changes to Gen 1 which are still pending but I will hopefully have someone merge today and also an optimization im adding to how JSON is bundled into the various packages. But if those dont get done I'll still release with just this fix so youre not blocked EDIT: I'm blocked on https://github.com/smogon/pokemon-showdown/commit/27346ee4ace8a43c1863f8d317451fce251b5875 introducing bugs. So ill need to wait for that to be fixed as well

te1 commented 1 year ago

Thanks and no need to rush. I'm currently using my forked version so no worries.

scheibo commented 1 year ago

Thanks to both @KrisXV for fixing things with https://github.com/smogon/pokemon-showdown/commit/f50cc6aba34c2c5638142375890fc5ec82bee433 & https://github.com/smogon/pokemon-showdown/commit/9a412789e5b14131f70c1c1e89a20be24d12986d and @AnnikaCodes for merging @livid-washed's work. All this has now been released in 0.7.3. Thanks again @te1 for filing this issue and writing PRs against two projects to get it fixed!