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

[Idain]

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.

[Rangi42]

Nicely explained, thank you!

[Idain]

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

[mid-kid]

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]

@mid-kid, how is it now?

[SnorlaxMonster]

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.

[SnorlaxMonster]

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.)

[Idain]

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

[vulcandth]

This would be MON_OT_ID now.

[SnorlaxMonster]

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.

[SnorlaxMonster]

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".

[Idain]

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.

[SnorlaxMonster]

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.

[Idain]

It sounds better.

[SnorlaxMonster]

Suggested change

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

[vulcandth]

This would be MON_OT_ID now.