From 12e5c90b43bdce800f7147921d0dea22109b7392 Mon Sep 17 00:00:00 2001 From: Idain Date: Mon, 25 Dec 2023 02:34:01 -0400 Subject: [PATCH] Move bug's explanation to bugs_and_glitches.md --- data/events/magikarp_lengths.asm | 6 +-- docs/bugs_and_glitches.md | 66 ++++---------------------------- engine/events/magikarp.asm | 48 +++++++++++------------ 3 files changed, 32 insertions(+), 88 deletions(-) diff --git a/data/events/magikarp_lengths.asm b/data/events/magikarp_lengths.asm index b01e5e6c6f..3a6b9e1ea2 100644 --- a/data/events/magikarp_lengths.asm +++ b/data/events/magikarp_lengths.asm @@ -2,9 +2,7 @@ MagikarpLengths: ; [wMagikarpLength] = z * 100 + (bc - x) / y ; First argument is the bc threshold as well as x. ; Second argument is y. -; In reality, due to the bug at .BCLessThanDE, -; the threshold is determined by only register b. - dwb 110, 1 ; not used unless the bug is fixed + dwb 110, 1 dwb 310, 2 dwb 710, 4 dwb 2710, 20 @@ -17,4 +15,4 @@ MagikarpLengths: dwb 64710, 20 dwb 65210, 5 dwb 65410, 2 - dwb 65510, 1 ; not used unless the bug is fixed + dwb 65510, 1 diff --git a/docs/bugs_and_glitches.md b/docs/bugs_and_glitches.md index e0ed62f1f1..d3c9e3329c 100644 --- a/docs/bugs_and_glitches.md +++ b/docs/bugs_and_glitches.md @@ -2407,65 +2407,13 @@ CopyPokemonName_Buffer1_Buffer3: ### Magikarp lengths can be miscalculated -**Fix:** Edit `CalcMagikarpLength` in [engine/events/magikarp.asm](https://github.com/pret/pokecrystal/blob/master/engine/events/magikarp.asm): - -```diff - CalcMagikarpLength: - ; Return Magikarp's length (in feet and inches) at wMagikarpLengthMm (big endian). - ; - ; input: - ; de: wEnemyMonDVs - ; bc: wPlayerID - - ; This function is poorly commented. - - ; In short, it generates a value between 190 and 1755 using - ; a Magikarp's DVs and its trainer ID. This value is further - ; filtered in LoadEnemyMon to make longer Magikarp even rarer. --; Due to the bug at .BCLessThanDE, the max possible value is 1625. - - ; The value is generated from a lookup table. - ; The index is determined by the DV xored with the player's trainer ID. - - ; bc = rrc(dv[0]) ++ rrc(dv[1]) ^ rrc(id) - - ; if bc < 10: [wMagikarpLength] = c + 190 --; if bc ≥ $ff00: [wMagikarpLength] = c + 1370 -+; if bc ≥ 65510: [wMagikarpLength] = bc - 65510 + 1600 - ; else: [wMagikarpLength] = z * 100 + (bc - x) / y - --; X, Y, and Z depend on the value of b as follows: -+; X, Y, and Z depend on the value of bc as follows: - --; if b = 0: x = 310, y = 2, z = 3 --; if b = 1: x = 710, y = 4, z = 4 --; if b = 2-9: x = 2710, y = 20, z = 5 --; if b = 10-29: x = 7710, y = 50, z = 6 --; if b = 30-68: x = 17710, y = 100, z = 7 --; if b = 69-126: x = 32710, y = 150, z = 8 --; if b = 127-185: x = 47710, y = 150, z = 9 --; if b = 186-224: x = 57710, y = 100, z = 10 --; if b = 225-243: x = 62710, y = 50, z = 11 --; if b = 244-251: x = 64710, y = 20, z = 12 --; if b = 252-253: x = 65210, y = 5, z = 13 --; if b = 254: x = 65410, y = 2, z = 14 -+; if bc = 10-109: x = 110, y = 1, z = 2 -+; if bc = 110-309: x = 310, y = 2, z = 3 -+; if bc = 310-709: x = 710, y = 4, z = 4 -+; if bc = 710-2709: x = 2710, y = 20, z = 5 -+; if bc = 2710-7709: x = 7710, y = 50, z = 6 -+; if bc = 7710-17709: x = 17710, y = 100, z = 7 -+; if bc = 17710-32709: x = 32710, y = 150, z = 8 -+; if bc = 32710-47709: x = 47710, y = 150, z = 9 -+; if bc = 47710-57709: x = 57710, y = 100, z = 10 -+; if bc = 57710-62709: x = 62710, y = 50, z = 11 -+; if bc = 62710-64709: x = 64710, y = 20, z = 12 -+; if bc = 64710-65209: x = 65210, y = 5, z = 13 -+; if bc = 65210-65409: x = 65410, y = 2, z = 14 -+; if bc = 65410-65509: x = 65510, y = 1, z = 15 - - ... - +The function `CalcMagikarpLength` is supposed to determine a Magikarp's length by using the Trainer's ID and the Magikarp's DVs, `xor`ing the values, store the result in `bc`, use it as an index to select an entry from the `MagikarpLengths` table and calculate the Magikarp's length, but it doesn't work as intended. The local function `.BCLessThanDE` is supposed to compare the index (stored in `bc`) with a threshold (stored in `de`) from the table to select the correct entry, but it only compares the high bytes of both inputs, which means that the wrong entry might be selected. + +This bug also causes the first entry of the `MagikarpLengths` table to go unused and `bc` values between 65280 and 65509 to use the wrong formula. As a side effect, the highest possible length is capped at 1625mm (before being converted to feet and inches). + +**Fix:** Edit `CalcMagikarpLength.BCLessThanDE` in [engine/events/magikarp.asm](https://github.com/pret/pokecrystal/blob/master/engine/events/magikarp.asm): + +```diff .BCLessThanDE: -; BUG: Magikarp lengths can be miscalculated (see docs/bugs_and_glitches.md) ld a, b diff --git a/engine/events/magikarp.asm b/engine/events/magikarp.asm index 1e2ff74c56..3bff73f1c3 100644 --- a/engine/events/magikarp.asm +++ b/engine/events/magikarp.asm @@ -108,36 +108,34 @@ CalcMagikarpLength: ; de: wEnemyMonDVs ; bc: wPlayerID -; This function is poorly commented. - -; In short, it generates a value between 190 and 1755 using -; a Magikarp's DVs and its trainer ID. This value is further -; filtered in LoadEnemyMon to make longer Magikarp even rarer. -; Due to the bug at .BCLessThanDE, the max possible value is 1625. +; It generates a value between 190 and 1755 using a Magikarp's DVs +; and its trainer ID. This value is further filtered in LoadEnemyMon +; to make longer Magikarp even rarer. ; The value is generated from a lookup table. -; The index is determined by the DV xored with the player's trainer ID. - -; bc = rrc(dv[0]) ++ rrc(dv[1]) ^ rrc(id) +; The index is determined by the DV xored with the player's trainer ID +; and then stored in bc. -; if bc < 10: [wMagikarpLength] = c + 190 -; if bc ≥ $ff00: [wMagikarpLength] = c + 1370 +; if bc < 10: [wMagikarpLength] = bc + 190 +; if bc ≥ 65510: [wMagikarpLength] = bc - 65510 + 1600 ; else: [wMagikarpLength] = z * 100 + (bc - x) / y -; X, Y, and Z depend on the value of b as follows: - -; if b = 0: x = 310, y = 2, z = 3 -; if b = 1: x = 710, y = 4, z = 4 -; if b = 2-9: x = 2710, y = 20, z = 5 -; if b = 10-29: x = 7710, y = 50, z = 6 -; if b = 30-68: x = 17710, y = 100, z = 7 -; if b = 69-126: x = 32710, y = 150, z = 8 -; if b = 127-185: x = 47710, y = 150, z = 9 -; if b = 186-224: x = 57710, y = 100, z = 10 -; if b = 225-243: x = 62710, y = 50, z = 11 -; if b = 244-251: x = 64710, y = 20, z = 12 -; if b = 252-253: x = 65210, y = 5, z = 13 -; if b = 254: x = 65410, y = 2, z = 14 +; X, Y, and Z depend on the value of bc as follows: + +; if bc = 10-109: x = 110, y = 1, z = 2 +; if bc = 110-309: x = 310, y = 2, z = 3 +; if bc = 310-709: x = 710, y = 4, z = 4 +; if bc = 710-2709: x = 2710, y = 20, z = 5 +; if bc = 2710-7709: x = 7710, y = 50, z = 6 +; if bc = 7710-17709: x = 17710, y = 100, z = 7 +; if bc = 17710-32709: x = 32710, y = 150, z = 8 +; if bc = 32710-47709: x = 47710, y = 150, z = 9 +; if bc = 47710-57709: x = 57710, y = 100, z = 10 +; if bc = 57710-62709: x = 62710, y = 50, z = 11 +; if bc = 62710-64709: x = 64710, y = 20, z = 12 +; if bc = 64710-65209: x = 65210, y = 5, z = 13 +; if bc = 65210-65409: x = 65410, y = 2, z = 14 +; if bc = 65410-65509: x = 65510, y = 1, z = 15 ; bc = rrc(dv[0]) ++ rrc(dv[1]) ^ rrc(id)