From 6508421568b79777c1a2429ed4febcdf52853020 Mon Sep 17 00:00:00 2001 From: Idain Date: Sat, 23 Dec 2023 21:00:44 -0400 Subject: [PATCH 1/6] Fix doc about Magikarp length calculation and bugfix doc --- docs/bugs_and_glitches.md | 1 + engine/events/magikarp.asm | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/bugs_and_glitches.md b/docs/bugs_and_glitches.md index 77152efb5c0..c1bdc37fba3 100644 --- a/docs/bugs_and_glitches.md +++ b/docs/bugs_and_glitches.md @@ -2416,6 +2416,7 @@ CopyPokemonName_Buffer1_Buffer3: cp d ret c - ret nc ++ ret nz ld a, c cp e ret diff --git a/engine/events/magikarp.asm b/engine/events/magikarp.asm index 09d75967e5e..763c59a67f0 100644 --- a/engine/events/magikarp.asm +++ b/engine/events/magikarp.asm @@ -110,7 +110,7 @@ CalcMagikarpLength: ; This function is poorly commented. -; In short, it generates a value between 190 and 1786 using +; In short, it generates a value between 190 and 1625 using ; a Magikarp's DVs and its trainer ID. This value is further ; filtered in LoadEnemyMon to make longer Magikarp even rarer. From 08358d82c64aebbe68ac0875b50f012afd5c207f Mon Sep 17 00:00:00 2001 From: Idain Date: Sat, 23 Dec 2023 22:35:18 -0400 Subject: [PATCH 2/6] Fix comment about maximum possible value --- engine/events/magikarp.asm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/engine/events/magikarp.asm b/engine/events/magikarp.asm index 763c59a67f0..ce46857853b 100644 --- a/engine/events/magikarp.asm +++ b/engine/events/magikarp.asm @@ -110,12 +110,13 @@ CalcMagikarpLength: ; This function is poorly commented. -; In short, it generates a value between 190 and 1625 using +; 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. +; The index is determined by the dv xored with the player's trainer ID. ; bc = rrc(dv[0]) ++ rrc(dv[1]) ^ rrc(id) From 497eb8761c1f30a04d382852c9163d82536c4a89 Mon Sep 17 00:00:00 2001 From: Idain Date: Sat, 23 Dec 2023 23:09:33 -0400 Subject: [PATCH 3/6] Update bugfix diff and comments in the code --- data/events/magikarp_lengths.asm | 2 +- docs/bugs_and_glitches.md | 64 ++++++++++++++++++++++++++++++-- engine/events/magikarp.asm | 2 +- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/data/events/magikarp_lengths.asm b/data/events/magikarp_lengths.asm index 1a1040d6c68..b01e5e6c6f7 100644 --- a/data/events/magikarp_lengths.asm +++ b/data/events/magikarp_lengths.asm @@ -17,4 +17,4 @@ MagikarpLengths: dwb 64710, 20 dwb 65210, 5 dwb 65410, 2 - dwb 65510, 1 ; not used + dwb 65510, 1 ; not used unless the bug is fixed diff --git a/docs/bugs_and_glitches.md b/docs/bugs_and_glitches.md index c1bdc37fba3..e0ed62f1f15 100644 --- a/docs/bugs_and_glitches.md +++ b/docs/bugs_and_glitches.md @@ -2407,14 +2407,70 @@ CopyPokemonName_Buffer1_Buffer3: ### Magikarp lengths can be miscalculated -**Fix:** Edit `CalcMagikarpLength.BCLessThanDE` in [engine/events/magikarp.asm](https://github.com/pret/pokecrystal/blob/master/engine/events/magikarp.asm): - -```diff +**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 + + ... + .BCLessThanDE: -; BUG: Magikarp lengths can be miscalculated (see docs/bugs_and_glitches.md) ld a, b cp d - ret c +- ret c - ret nc + ret nz ld a, c diff --git a/engine/events/magikarp.asm b/engine/events/magikarp.asm index ce46857853b..1e2ff74c56e 100644 --- a/engine/events/magikarp.asm +++ b/engine/events/magikarp.asm @@ -116,7 +116,7 @@ CalcMagikarpLength: ; 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. +; The index is determined by the DV xored with the player's trainer ID. ; bc = rrc(dv[0]) ++ rrc(dv[1]) ^ rrc(id) From 12e5c90b43bdce800f7147921d0dea22109b7392 Mon Sep 17 00:00:00 2001 From: Idain Date: Mon, 25 Dec 2023 02:34:01 -0400 Subject: [PATCH 4/6] 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 b01e5e6c6f7..3a6b9e1ea21 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 e0ed62f1f15..d3c9e3329c2 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 1e2ff74c56e..3bff73f1c3d 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) From 6f430d40d3ed48fdaf382b3132f721377f856547 Mon Sep 17 00:00:00 2001 From: Idain Date: Mon, 1 Jan 2024 23:39:20 -0400 Subject: [PATCH 5/6] Shorten bug explanation --- docs/bugs_and_glitches.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/bugs_and_glitches.md b/docs/bugs_and_glitches.md index d3c9e3329c2..25fcc14fdd7 100644 --- a/docs/bugs_and_glitches.md +++ b/docs/bugs_and_glitches.md @@ -2407,9 +2407,7 @@ CopyPokemonName_Buffer1_Buffer3: ### Magikarp lengths can be miscalculated -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). +`CalcMagikarpLength.BCLessThanDE` only compares the high bytes of `bc` and `de`. This causes the first entry of the `MagikarpLengths` table to go unused, and `bc` values between 65,280 and 65,509 to use the wrong formula. As a side effect, the highest possible length is capped at 1,625mm (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): From 8a4d9ab297a3fbebdb037a59db0e4e0cac022fe9 Mon Sep 17 00:00:00 2001 From: Idain Date: Mon, 1 Jan 2024 23:41:22 -0400 Subject: [PATCH 6/6] Delete comment with possible X, Y and Z values. The MagikarpLengths table already has the X and Y values, and the code shows that the value of Z starts at 2 and increments per iteration. --- engine/events/magikarp.asm | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/engine/events/magikarp.asm b/engine/events/magikarp.asm index 3bff73f1c3d..d5d2c5ecb54 100644 --- a/engine/events/magikarp.asm +++ b/engine/events/magikarp.asm @@ -120,23 +120,6 @@ CalcMagikarpLength: ; if bc ≥ 65510: [wMagikarpLength] = bc - 65510 + 1600 ; else: [wMagikarpLength] = z * 100 + (bc - x) / y -; 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) ; id