Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Learnset updates for SV DLC1 #920

Merged
merged 13 commits into from Oct 10, 2023
Merged

Learnset updates for SV DLC1 #920

merged 13 commits into from Oct 10, 2023

Conversation

penelopeysm
Copy link
Contributor

Hello!

This PR updates SV learnsets for all Pokemon which returned in the first part of the DLC (The Teal Mask). It also adds egg moves for Cacnea and Luvdisc, which GF somehow forgot to add in the base game, but quietly re-added in the DLC. Data were obtained by scraping Serebii.

Data for the new species (Dipplin, Poltchageist family, and the legendaries) aren't yet included, since they don't exist in the database yet.

The same script as in #915 can be used to test the data (replace "brilliant-diamond-and-shining-pearl" with "scarlet-violet").

@Naramsim
Copy link
Member

Hi @giginet , could you be so kind to review this PR and guide the merge process? There's another ongoing PR and data may overlap

@giginet
Copy link
Contributor

giginet commented Sep 22, 2023

Sure. I'll check this change in a human-readable way on this weekend. Please moment.

Copy link
Member

@Naramsim Naramsim left a comment

Choose a reason for hiding this comment

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

I'm gonna approve without reviewing all the data. I don't have any time. We'll see in the issues if there's a change needed.

@giginet
Copy link
Contributor

giginet commented Oct 3, 2023

I'll check this in a few days. Please moment.

@giginet
Copy link
Contributor

giginet commented Oct 3, 2023

I think all version_group_id should be 26. It means Teal Mask.
https://github.com/PokeAPI/pokeapi/blob/master/data/v2/csv/version_groups.csv#L27

Could you update it to 26 instead of 25?

@penelopeysm
Copy link
Contributor Author

Hi, @giginet!

Yes, I could. My impression though is that 25 (i.e. scarlet-violet) would be more consistent with the existing data - for example, returning Pokemon in SwSh DLC are also listed as 20 (sword-shield), such as https://pokeapi.co/api/v2/pokemon/clauncher.

Let me know if you would still prefer to change it to 26, and I'll gladly update the PR!

{
    "moves": [
        {
            "move": {
                "name": "vice-grip",
                "url": "https://pokeapi.co/api/v2/move/11/"
            },
            "version_group_details": [
                {
                    "level_learned_at": 5,
                    "move_learn_method": {
                        "name": "level-up",
                        "url": "https://pokeapi.co/api/v2/move-learn-method/1/"
                    },
                    "version_group": {
                        "name": "sword-shield",
                        "url": "https://pokeapi.co/api/v2/version-group/20/"
                    }
                }
            ]
        }
    ]
}

@giginet
Copy link
Contributor

giginet commented Oct 3, 2023

Thank you for this great contribution.
I checked your data further. I noticed some points.

1. All egg moves seem to be missing

All egg moves are missing.

For example, Furret learns "Tidy Up" since DLC as an egg move. But all egg move tables are not reflected.

https://www.serebii.net/pokedex-sv/furret/

Screenshot 2023-10-03 at 23 29 52

2. All moves of new Pokémon are missing

(e.g. Orgerpon, etc...)

This issue may be fixed with running the script again after merging #921

3. Moves of existing pokemon from new TMs are missing

This data lacks new moves of existing Pokémon from new TMs.

For example, Dragonite learns new moves from new TMs. But these are missing.
(e.g. Focus Punch, Weather Ball, Scale Shot...)

https://serebii.net/pokedex-sv/dragonite/

@giginet
Copy link
Contributor

giginet commented Oct 3, 2023

Yes, I could. My impression though is that 25 (i.e. scarlet-violet) would be more consistent with the existing data

I agree with your opinion. It's difficult to track diff between original table and DLC one.

@giginet
Copy link
Contributor

giginet commented Oct 3, 2023

If you can share the scraper. I can improve your script to collect data.

@penelopeysm
Copy link
Contributor Author

Thanks! Very good points and sorry for the delay, just wanted to give a quick update.

For (1), I noticed egg moves for evolved forms are consistently omitted in pokeapi (this is true at least for USUM and SwSh, and I followed the same thing when adding BDSP learnsets in #915), so when scraping I explicitly avoided putting those in. If you would like to change this then I would suggest that this be retroactively applied to the old games, probably as part of a different PR.

For (2) and (3) I'm more than happy to work on it, but I'm a little busy at the moment. I will try to do it on Sunday 🙂

@giginet giginet mentioned this pull request Oct 7, 2023
@giginet
Copy link
Contributor

giginet commented Oct 7, 2023

@penelopeysm Thank you! I'll wait for your availability. 😄

For (1)

I added a moves table for Scarlet/Violet. (#806)
In the data, all egg data of evolved forms are included. So, in my understanding, since Gen9, It's better to add a move data.

Strangely, some evolved Pokemon's egg moves differ from pre-evolved ones. So I think it's better to add egg moves of evolved pokemon.

If it's difficult by your script. I think you needn't add the data at this time. I'll fix this later.

What do you think about it?

For (2)

I noticed some data of new pokemon is not completed. So I opened #939 to add missing data.
You may need #939. It works fine if the PR is merged.

For (3)

If you can, fetch all data of existing pokemon again and ignore duplicated data. It will work fine.

@penelopeysm
Copy link
Contributor Author

Okay, the above commits should address (1) and (2) 🙂

I'll do some work on (3) some time soon, it shouldn't be too hard as there are only a limited number of TMs and I can filter to only add those in. I'll also add in Indeedee-F now getting Trick Room via TM.

Re. egg moves for evolved forms: on my end, I don't see any of these. For example, Fletchling has EMs in SV, but not Fletchinder or Talonflame. Is it possible that at some point in time between your PR and now they got removed?

In principle, I totally agree that EMs should be present for evolutions — this is also consistent with the fact that they can be learnt via Mirror Herb (SV) / daycare transfer (in SwSh).

@giginet
Copy link
Contributor

giginet commented Oct 8, 2023

@penelopeysm
I confirmed to address (1) and (2). Thank you for your quick fix! Looks good to me.
As you said, I can confirm (3) has not been addressed yet.

I'll review this after addressing (3) again. Feel free to tell me you want help.

Re. egg moves for evolved forms: on my end, I don't see any of these. For example, Fletchling has EMs in SV, but not Fletchinder or Talonflame. Is it possible that at some point in time between your PR and now they got removed?

Sorry, this may be my misunderstanding. Never mind if you didn't find such a case.

@giginet
Copy link
Contributor

giginet commented Oct 8, 2023

This PR can be merged after addressing (3).

@penelopeysm
Copy link
Contributor Author

Okay, @giginet I think it should be ready. :)

Copy link
Contributor

@giginet giginet left a comment

Choose a reason for hiding this comment

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

I checked in some situations; your data works fine.

Thank you very much for your effort.

@giginet
Copy link
Contributor

giginet commented Oct 10, 2023

@Naramsim It's ready to merge

@penelopeysm
Copy link
Contributor Author

penelopeysm commented Oct 10, 2023 via email

@Naramsim Naramsim merged commit 07c8b5c into PokeAPI:master Oct 10, 2023
2 checks passed
@pokeapi-machine-user
Copy link

A PokeAPI/api-data refresh has started. In 45 minutes the staging branch of PokeAPI/api-data will be pushed with the new generated data.

The staging branch will be deployed in our staging environment and you will be able to review the entire API.

A Pull Request (master<-staging) will be also created at PokeAPI/api-data and assigned to the PokeAPI Core team to be reviewed. If approved and merged new data will soon be available worldwide at pokeapi.co.

@pokeapi-machine-user
Copy link

The updater script has finished its job and has now opened a Pull Request towards PokeAPI/api-data with the updated data.

You can see the Pull Request deployed at our staging environment when CircleCI deploy will be finished (check the started time of the last build).

@giginet
Copy link
Contributor

giginet commented Nov 11, 2023

I found an issue: tables are missing some Pokémon.
#956 (comment)

@penelopeysm What do you know something about that?

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.

None yet

4 participants