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

Fix documentation about Magikarp length calculation and bugfix about "Magikarp lengths can be miscalculated" #1100

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Idain
Copy link
Contributor

@Idain Idain commented Dec 24, 2023

This pull request tackles two documentation topics about Magikarp's length:

  1. The maximum length of a Magikarp when judged by the Guru Fisher in Lake of Rage.
  2. A badly documented fix in "Magikarp lengths can be miscalculated".

1. Wrong comment about maximum Magikarp's length in CalcMagikarpLength

First of all, let's talk about CalcMagikarpLength. This function first calculates the length in millimeters by using the Magikarp's DVs and the Trainer ID, and then convert it to feet and inches. The maximum length a Magikarp can have (in millimeters) is 1625mm (bugged version)/1755mm (fixed version), but the comment inside the function says the maximum is 1786mm (the bug makes it so that only b is used when calculating the length, instead of bc). To check this, I've run various calculations, both accounting for the bug in CalcMagikarpLength.BCLessThanDE and with the fixed version.

To prove this, let's assume bc has the maximum possible value ($ffff). According to CalcMagikarpLength, if bc ≥ $ff00: [wMagikarpLength] = c + 1370, whose code is located here, so let's start calculating:

; If bc = $ffff, then c = $ff = 255
length = c + 1370
length = 255 + 1370
length = 1625

Q: "According to the comment inside CalcMagikarpLength, the formula is different if bc < $ff00, so maybe the result could be 1786mm?"

A: I also accounted for this, and no, the max possible result is either 1590mm (with the bug) or 1755mm (fixed version). If you fix the function, the last threshold in the MagikarpLength table is used, which is 65510. Let's make two calculations, one for each version (the formula is z * 100 + (bc - x) / y):

Bugged version

; The highest threshold is x = 65410, where y = 2 and z = 14. We'll use bc = $ff00 - $1 = 65279

length = z * 100 + (bc - x) / y
length = 14 * 100 + (65279 - 65410) / 2
length = 1400 + (65405) / 2  ; The result is stored in bc and becomes -131 = 65405
length = 1400 + 190  ; The result is $7FBE = 32702, but the code only takes the low byte, which is $BE = 190
length = 1590

Fixed version

; With the code fixed, the highest threshold is x = 65510, where y = 1 and z = 15. We'll use bc = 65509

length = z * 100 + (bc - x) / y
length = 15 * 100 + (65509 - 65510) / 1
length = 1500 + (65535) / 1  ; The result is stored in bc and becomes -1 = 65535
length = 1500 + 255  ; The code only takes the low byte of the result, which is $FF = 255
length = 1755

2. Wrong fix in "Magikarp lengths can be miscalculated"

The bugfix doesn't account for cases where b > d, so instead of returning, it tries to compare the low bytes bc and de as if the high bytes were equal, which will give the wrong result. A simple fix is to ret nz to discard all cases where b ≠ d.

engine/events/magikarp.asm Outdated Show resolved Hide resolved
@Rangi42
Copy link
Member

Rangi42 commented Dec 24, 2023

Nicely explained, thank you!

@Idain
Copy link
Contributor Author

Idain commented Dec 24, 2023

@Rangi42, done. Let me know if there's something else to address.

@mid-kid
Copy link
Member

mid-kid commented Dec 24, 2023

While we're at it, I'd add a ; BUG: marker to this, replacing some redundant bits of explanation if possible.
https://github.com/pret/pokecrystal/wiki/Code-cleanup#refer-to-docsbugs_and_glitchesmd-instead-of-explaining-a-bug-inline

@Idain
Copy link
Contributor Author

Idain commented Dec 25, 2023

@mid-kid, how is it now?

docs/bugs_and_glitches.md Outdated Show resolved Hide resolved
engine/events/magikarp.asm Outdated Show resolved Hide resolved
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.
Copy link
Contributor

@SnorlaxMonster SnorlaxMonster left a comment

Choose a reason for hiding this comment

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

CalcMagikarpLength is called both when the Fisherman is evaluating the length of your Magikarp (from CheckMagikarpLength), and when encountering a Magikarp in the wild (from LoadEnemyMon). However, the comments in various parts assume it is being called from a specific one of those two places. This is important because when the Fisherman evaluates a Magikarp, he uses the Magikarp's Original Trainer's ID number. But when LoadEnemyMon is calculating the length of a wild Magikarp, it uses the player's Trainer ID (since the wild Magikarp doesn't have a Trainer ID yet).

I was actually planning to submit a PR to address these issues, but noticed there was already a PR adjusting the comments on the same function, so figured it would make more sense to amend this one.

Comment on lines 108 to 109
; de: wEnemyMonDVs
; bc: wPlayerID
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; de: wEnemyMonDVs
; bc: wPlayerID
; de: Magikarp's DVs (MON_DVS / wEnemyMonDVs )
; bc: Trainer ID (MON_ID / wPlayerID)

Those are only the inputs when called from LoadEnemyMon in engine/battle/core.asm. When called from CheckMagikarpLength, the inputs are MON_DVS and MON_ID. (This distinction is important, as MON_ID is the Original Trainer's ID, not the player's.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll address your suggestions when I get my laptop back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be MON_OT_ID now.

Comment on lines +111 to +113
; 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; 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.
; It generates a length (in mm) between 190 and 1755 using a Magikarp's DVs
; and its Trainer ID.

The comment about being further filtered in LoadEnemyMon assumes this function is only called from LoadEnemyMon (ignoring the case when it is called from CheckMagikarpLength). IMO, what a calling function does with the return value is out of the scope of what belongs in comments on this function.

Comment on lines +116 to +117
; The index is determined by the DV xored with the player's trainer ID
; and then stored in bc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; The index is determined by the DV xored with the player's trainer ID
; and then stored in bc.
; The index is determined by the DV xored with the Magikarp's Trainer ID
; and then stored in bc.

The player's Trainer ID is only used when called from LoadEnemyMon. When this function is called from CheckMagikarpLength, it uses Magikarp's Original Trainer's Trainer ID, not the player's. Even though the Magikarp technically doesn't have a Trainer ID when it is generated in the wild, I think it's clear enough what is being referred to if this is just generalized as "the Magikarp's Trainer ID".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it can feel misleading if we don't also clarify it uses the Player's ID when fighting against a wild Magikarp.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's completely reasonable. Perhaps something like this would be better

Suggested change
; The index is determined by the DV xored with the player's trainer ID
; and then stored in bc.
; The index is determined by the DV xored with a Trainer ID
; (the player's for wild Magikarp; the OT's when checking owned Magikarp)
; then stored in bc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds better.

; filtered in LoadEnemyMon to make longer Magikarp even rarer.
; 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; The value is generated from a lookup table.
; The length is generated from a lookup table.

Comment on lines 108 to 109
; de: wEnemyMonDVs
; bc: wPlayerID
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be MON_OT_ID now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants