Skip to content

Refactor learnset checking - #9951

Merged
Zarel merged 3 commits into
masterfrom
movepool
Dec 9, 2023
Merged

Refactor learnset checking#9951
Zarel merged 3 commits into
masterfrom
movepool

Conversation

@Zarel

@Zarel Zarel commented Dec 8, 2023

Copy link
Copy Markdown
Member

After seeing fifty different ways we use getLearnset, most of which are just "haphazardly assemble a movepool", I decided to write getFullLearnset and getMovePool, which centralizes the implementations and prevents weird bugs like 9713dc6 which we spent two years trying to figure out.

Zarel added 2 commits December 7, 2023 20:41
After seeing fifty different ways we use `getLearnset`, most of which
are just "haphazardly assemble a movepool", I decided to write
`getFullLearnset` and `getMovePool`, which centralizes the
implementations and prevents weird bugs like 9713dc6 which
we spent two years trying to figure out.
@pyuk-bot

pyuk-bot commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

It looks like there’s a quirk with Mirror Herb egg moves that’s missing from this: Ursaluna-Bloodmoon and Greninja-Bond can inherit egg moves for Teddiursa and Froakie, respectively, despite not actually evolving from them.

@Karthik99999

Copy link
Copy Markdown
Member

It looks like there’s a quirk with Mirror Herb egg moves that’s missing from this: Ursaluna-Bloodmoon and Greninja-Bond can inherit egg moves for Teddiursa and Froakie, respectively, despite not actually evolving from them.

This is already handled by putting the egg moves on Ursaluna and Greninja's forme respective movesets (see #9888).

@pyuk-bot

pyuk-bot commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

Oh yeah I forgot it got done that way, my bad.

// Since we assume we have no target mons at first
// then the valid moveset we can search is the set of all moves.
const validMoves = new Set(Object.keys(mod.data.Moves));
const validMoves = new Set(Object.keys(mod.data.Moves)) as Set<ID>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const validMoves = new Set(Object.keys(mod.data.Moves)) as Set<ID>;
const validMoves = new Set<ID>(Object.keys(mod.data.Moves));

wouldn't this work? or would it cause an error since the keys for mod.data.Moves are technically strings?

@Zarel Zarel Dec 9, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would cause an error because they're technically strings.

@Zarel

Zarel commented Dec 9, 2023

Copy link
Copy Markdown
Member Author

I was gonna leave this up for review a while longer, but we just caught another crash that makes sense to be fixed here, so I suppose it's time to rush this out.

@Zarel
Zarel merged commit eb60769 into master Dec 9, 2023
MathyFurret pushed a commit to MathyFurret/pokemon-showdown that referenced this pull request May 21, 2024
After seeing fifty different ways we use `getLearnset`, most of which
are just "haphazardly assemble a movepool", I decided to write
`getFullLearnset` and `getMovePool`, which centralizes the
implementations and prevents weird bugs like 9713dc6 which
we spent two years trying to figure out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants