Conversation
|
Hi @DJ-Meyers if you have some spare time could you review this PR? It's about encounters and you recently merged a similar PR. |
|
Hi @jemarq04, now that I fully trust you on anything related to encounters, I might take too much advantage of your kindness. If you could help us here again with this PR, we would appreciate it. Let me know if I'm pinging you too often! |
No worries, I enjoy helping with the project! I'll take a look. |
| 43,5,Apparition rare dans l'eau avec un parfum ou un combo capture | ||
| 43,9,Rare spawn in water with lure or catch combo | ||
| 43,5,Apparition rare dans l'eau avec un parfum ou un combo capture No newline at end of file | ||
| 44,9,"Walking in grass, flowers, terrain, or in a cave" |
There was a problem hiding this comment.
I wonder if we can come up with something more descriptive here that make this unique to horde encounters, as this is a bit general. Maybe something like "Horde encounter within grass, flowers, terrain, or in a cave".
With that said, what is meant by "terrain" here?
There was a problem hiding this comment.
How about "Encountering 5 Pokemon within grass, flowers, on terrain, or within a cave"?
Terrain was a catch-all for cases like the Sandshrew in Sand (ORAS). I don't recall if you can encounter hordes in the swamp tiles in X and Y, but for now, we take out the mention of terrain.
There was a problem hiding this comment.
I'll confirm that you can't get hordes in swamps in X/Y. I was doing rng manips there specifically to avoid hordes.
jemarq04
left a comment
jemarq04
left a comment
There was a problem hiding this comment.
I've left a couple comments to review above, once those are settled I'll do a more in-depth check of the encounters themselves with a helper script I used to do this previously.
Updated Encounter Method Id to match Horde Encounter Id
|
Small issue is this pr conflicts with mine about the event data. We're both currently using encounter method 44. Is this something that can be fixed on merge, or should I update pr 1417 to accommodate for this? And was this something I should've looked at in advance and taken into account when I made the PR initially? |
This usually gets handled when dealing with merge conflicts I believe. In this case, it would be easier to refactor this encounter method to the one after yours and update accordingly. Maybe a maintainer can comment, but from what I've seen it usually just gets handled when things get final approval and need to be updated pre-merge |
@jemarq04 It looks like the issue extends to encounters and encounter_slots as well. |
Ah, you're right I overlooked that. I need some time to look over this PR, so you can always update things to work alongside @notblisy's PR in the meantime. Sorry about the inconvenience on that, I think it's the unfortunate consequence of both of you submitting encounter PRs at the same time by coincidence |
|
Fair point, it doesn’t matter who modifies their PR as long as you can both agree. Both options are fine, I’ll be taking a look at the encounter data in this PR tonight |
|
Sure I don’t mind. I’ll do it later tonight |
I have moved up & added horde to the encounter methods as position 44, and then moved all of my encounters and encounter_slots to be after the last encounters in pr PokeAPI#1393 so these two can be easily merged one after another. Their last encounter slot is 1336 and their last encounter is 70679, so I start with 1337 and 70680 respectively.
|
You can see the changes I made here. |
| 70450,26,361,1252,41,6,6 | ||
| 70451,26,361,1253,74,6,6 | ||
| 70452,26,361,1254,296,6,6 | ||
| 70453,26,361,1255,296,6,6 |
There was a problem hiding this comment.
There seems to be duplicate entries for Makuhita in Omega Ruby and Alpha Sapphire - one with encounter slot chance set to 6 and one with it set to 35. Maybe a duplicate entry?
There was a problem hiding this comment.
Makuhita has two different hordes that can form, 35% with a 4-1 Makuhita-Geodude split and a 5% for all Makuhita.
This might be something we add later as a new table (horde_encounter_split?) to handle pulling in this split encounter.
There was a problem hiding this comment.
Ah, ok I see - thanks! These "duplicates" are perfectly fine then of course.
| 70219,24,753,1183,610,8,8 | ||
| 70220,23,738,1184,439,11,11 | ||
| 70221,23,738,1185,524,11,11 | ||
| 70222,23,738,1186,524,11,11 |
There was a problem hiding this comment.
Duplicate entries for Roggenrola here (and in all other location areas for reflection-cave)
There was a problem hiding this comment.
Same issue as Makuhita. Higher encounter rate is a 4-1 split and lower is 5-of-a-kind.
data/v2/csv/encounters.csv
Outdated
| 70538,25,385,1281,41,17,17 | ||
| 70539,25,385,1282,363,17,17 | ||
| 70540,25,386,1283,41,17,17 | ||
| 70541,25,386,1284,361,17,17 |
There was a problem hiding this comment.
I see that Snorunt is listed here, but Snorunt only appears in Low Tide B3F. In fact, it looks like this location area is not available in the API yet. Maybe you can add this in?
There was a problem hiding this comment.
I have a question about this one: The games handle Shoal Cave differently for encounters, but we are currently using only two floors to handle the encounters in ORAS?
If okay, can I update this to have "" and "bf1" of the current Shoal Cave adjusted to "Main Cave" and "Ice Room", while adding in new location area data to handle ORAS as "High Tide Cave", "Low Tide Cave", "Low Tide Ice Room"?
There was a problem hiding this comment.
I think that things are a bit less defined with "main room" and "ice room" - maybe we could follow the naming convention from Serebii? https://www.serebii.net/pokearth/hoenn/shoalcave.shtml
data/v2/csv/encounters.csv
Outdated
| 70287,24,746,1193,613,20,20 | ||
| 70288,23,754,1194,74,23,23 | ||
| 70289,24,754,1194,74,23,23 | ||
| 70290,23,754,1195,246,24,24 |
There was a problem hiding this comment.
Looks like this entry for Larvitar has it listed for Pokemon X, but actually it should be Pokemon Y. Maybe the version IDs got swapped between this one and Aron, which is listed as Y but needs X.
(This error is also found in other location areas for terminus-cave.)
There was a problem hiding this comment.
Ah, you're right. I'll update Aron and Larvitar in all 4 locations of Terminus Cave
| 70397,24,725,1230,451,16,16 | ||
| 70398,23,726,1231,198,18,18 | ||
| 70399,23,726,1232,590,18,18 | ||
| 70400,23,726,1233,707,19,19 |
There was a problem hiding this comment.
Looks like Klefki is listed in hordes Route 12 for Pokemon X but not Y, but Serebii has it listed in both versions.
Edit: Actually, it looks like there is a duplicate entry for Klefki in Pokemon Y for Route 16 - probably easier just to swap the location area ID for that one.
There was a problem hiding this comment.
Looks like that was actually meant to be for Route 15 in Pokemon Y.
I'm not sure where you're seeing Route 12. Both Serebii and Bulbapedia have it listed for only 15 and 16.
| 70410,23,729,1234,74,23,23 | ||
| 70411,23,729,1235,631,23,23 | ||
| 70412,23,729,1236,632,23,23 | ||
| 70413,23,729,1237,632,23,23 |
There was a problem hiding this comment.
Duplicate entry for Durant in both Pokemon X and Pokemon Y in this location
There was a problem hiding this comment.
Same issue as Makuhita. Higher encounter rate is a 4-1 split and lower is 5-of-a-kind.
| 70423,24,730,1240,207,24,24 | ||
| 70424,23,731,1241,185,25,25 | ||
| 70425,23,731,1242,590,25,25 | ||
| 70426,23,731,1243,709,25,25 |
There was a problem hiding this comment.
Duplicate entry for Trevenant in both Pokemon X and Y for this location
There was a problem hiding this comment.
Same issue as Makuhita. Higher encounter rate is a 4-1 split and lower is 5-of-a-kind.
data/v2/csv/encounters.csv
Outdated
| 70608,25,396,1304,276,3,3 | ||
| 70609,26,396,1302,263,2,2 | ||
| 70610,26,396,1303,278,2,2 | ||
| 70611,26,396,1304,276,3,3 |
There was a problem hiding this comment.
It actually looks like there are different horde encounters in Route 104 to the North and South of Petalburg Woods, but we don't have location areas for those. Can you add those in and add copy over the needed encounter information for it? Taillow is found to the north and Wingull is found to the south.
There was a problem hiding this comment.
I'll add in the locations and associate them to these encounters correctly. I'll also need to add an extra set for the Zigzagoon encounters to handle the extra location.
| 70616,26,402,1307,311,6,6 | ||
| 70617,25,402,1308,312,6,6 | ||
| 70618,25,402,1309,312,6,6 | ||
| 70619,26,402,1309,312,6,6 |
There was a problem hiding this comment.
Looks like one duplicate entry for each of Plusle and Minun
There was a problem hiding this comment.
Same issue as Makuhita. Higher encounter rate is a 4-1 split and lower is 5-of-a-kind.
data/v2/csv/encounters.csv
Outdated
| 70634,26,406,1315,270,9,9 | ||
| 70635,26,406,1316,273,9,9 | ||
| 70636,26,406,1317,333,9,9 | ||
| 70637,26,406,1318,333,9,9 |
There was a problem hiding this comment.
A couple issues here - there are duplicate entries for Swablu for both Omega Ruby and Alpha Sapphire and Lotad and Seedot are listed for both versions, but Seedot is exclusive to Omega Ruby and Lotad is to Alpha Sapphire
There was a problem hiding this comment.
The Swablu issue is the same as Makuhita.
I removed the wrong version Seedot and Lotad Entries as well as the wrong Swablu Entry.
Swablu has an interesting ratio split where Bulbapedia states that it shows up in a 35% 4-1 split in AS only as well as its 65% in both games. Although with the other AS slot already being 35% and a missing 5% for ORAS, I think Bulbapedia may be wrong on this. Unfortunately Serebii doesn't have encounter rate percentages, so I'm almost thinking this might be a 5% 4-1 Swablu Horde in both OR and AS.
We'll need to ask around to see if this is the case...
jemarq04
left a comment
jemarq04
left a comment
There was a problem hiding this comment.
I've finished my look-through of the encounter information and found a few duplicate entries and some evidence that we're missing some needed location areas. Otherwise, the information looked good!
(I hope all my comments didn't ping too much.. I'll have to fiddle with Github to see if I can submit them all at once.)
This change is speculative, but makes sense when check against other encounter percentages
@jemarq04 I've made my changes based on your comments. Let me know if there's anything else we need to adjust. @notblisy I've updated location_area (now ends at ID 1208) and encounters (now ends at ID 70683). |
|
I'll hold on any changes until this commit is merged in case any more updates have to happen! |
data/v2/csv/location_areas.csv
Outdated
| 1204,445,0,high tide cave | ||
| 1205,445,0,low tide cave | ||
| 1206,445,0,low tide ice room | ||
| 1207,452,0,north | ||
| 1208,452,0,south |
There was a problem hiding this comment.
As I mentioned in the comment, maybe we could follow something a bit more structured like in Serebii: https://www.serebii.net/pokearth/hoenn/shoalcave.shtml. I see that Bulbapedia only has two rooms, but that doesn't seem like it describes the location well enough.
Also, regarding north/south for Route 104, maybe we can have these something like north-oras and south-oras, as differences in encounter tables only happen in these games and not in R/S/E.
Also, I believe we can't have spaces here (or at the very least it's not convention) so any separations between words can be done with "-".
There was a problem hiding this comment.
So for now, I'll add them in as high-tide-cave, low-tide-cave, ..cave-bf1, ..cave-bf2, and ..cave-bf3, as the the first two rooms match in encounters (technically low tide rooms 1-2 and bf1 match, but we can keep them separate).
Updated location names for clarity and consistency.
data/v2/csv/location_areas.csv
Outdated
| 1204,445,0,high tide cave | ||
| 1205,445,0,low tide cave | ||
| 1206,445,0,low tide ice room | ||
| 1207,452,0,north | ||
| 1208,452,0,south | ||
| 1204,445,0,high-tide-cave | ||
| 1205,445,0,low-tide-cave | ||
| 1206,445,0,low-tide-cave-bf1 | ||
| 1207,445,0,low-tide-cave-bf2 | ||
| 1208,445,0,low-tide-cave-bf3 |
There was a problem hiding this comment.
Can you clarify the difference between the original b1f location area and low-tide-cave-b1f?
Also, is the originally unnamed location area referring to what Serebii lists as High Tide Cave 1?
There was a problem hiding this comment.
I guess other than names on Serebii, just the encounters. You think I should rename the original ones and remove the two I added?
There was a problem hiding this comment.
That would likely be best, as right now there are two b1f areas, but b1f can only be encountered in Low Tide so it should only appear once. We should definitely have things as clear as we can.
Side note, I just noticed they say "bf1" and "bf2" instead of "b1f" and "b2f"
There was a problem hiding this comment.
Okay, I'll updated my location areas. I will alter one of the ones I added for the first floor as low-tide-oras since there are encounter % differences from high-tide to low-tide in ORAS (5% drop of Sealeo and 5% addition of Snorunt). This is no difference from high to low present in RSE.
jemarq04
left a comment
jemarq04
left a comment
There was a problem hiding this comment.
This is looking great so far - thank you for bearing with me with the suggestions. These last couple comments just have to do with the name used for the location areas, but everything else is looking perfect!
data/v2/csv/location_areas.csv
Outdated
| 1208,445,0,low-tide-cave-bf3 | ||
| 1209,452,0,north-oras | ||
| 1210,452,0,south-oras | ||
| 1204,445,0,low-tide-oras |
There was a problem hiding this comment.
Since this location area can be re-used for R/S/E, maybe just low-tide is sufficient here.
data/v2/csv/location_areas.csv
Outdated
| @@ -340,7 +340,7 @@ id,location_id,game_index,identifier | |||
| 383,444,41,b1f | |||
| 384,444,42,b2f | |||
| 385,445,43, | |||
There was a problem hiding this comment.
Is this location area that existed before equivalent to the High Tide room's encounters? If so, maybe we could add a name to this like high-tide to match the low tide one.
There was a problem hiding this comment.
In R/S/E, this room was the same both in High Tide and Low Tide, probably why only one location area was used initially. We can rename it to high-tide and, like you suggested, the new one to low-tide.
We'll need to eventually come back and reassign the new low-tide location area to new copies of the current R/S/E encounters. It's starting to get out of the scope of this commit's intention, so we can add it as a new issue. Maybe as a "RSE/ORAS Differences Commit"
There was a problem hiding this comment.
Yes, I was thinking of the same thing. I think the only big things would be updating Route 104 and Shoal Cave with encounters, right?
jemarq04
left a comment
jemarq04
left a comment
There was a problem hiding this comment.
This is all looking great! Like you mentioned, I agree that the relevant encounters should be copied into the new location areas to keep things accurate. You're welcome to do this if you'd like, but if not mention in the thread so it can be worked on soon.
Thanks for the contribution!
|
Great! Thanks you both the the time taken! Feel free to open an issue or another PR for the low tide high tide matter. @jemarq04 would you like to be invited to be part of the PokeAPI org? |
|
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. |
Yes, I would love to! If there are any guidelines/expectations let me know so I can keep things consistent with the org. Is there any sort of communication (discord/slack) I should join? |
|
The updater script has finished its job and has now opened a Pull Request towards PokeAPI/api-data with the updated data. |
|
@jemarq04 actually we currently don't have any messaging platform. We used to have Slack but then I found myself as the sole maintainer and now we don't have anything anymore. I'll send you an invitation. There are no guidelines, the invitation is a token of our appreciation for your job, the only thing we can give you, unfotunately :) . I'll see how things evolve with Paul and if he's really interested in putting some efforts into PokeAPI and then we might need to use a messaging platform. |
6 participants
Adding missing Horde Encounter information for XY and ORAS. The information was copied from Bulbapedia's page about the mechanic. Also noticed some minor differences (level max for lombre, and an area labeled "unknown" that can be set as "outside" to match what Bulbapedia refers to it as.)
Some considerations for the future are the differences in encounter between RSE and ORAS are not correctly documented (see Shoal Cave), and some Horde Encounters having a 4-1 split in certain encounters (not sure how to accomplish this with current data structure).
(also sorry for not merging latest master into my branch last time. 😣)