Clean up bugs commented in the source code (#912)

Co-authored-by: Rangi <remy.oukaour+rangi42@gmail.com>
2025-04-09 05:44:44 -07:00 · 2022-07-09 16:12:02 -05:00
parent bd27c0d8dc
commit ef9b9bb437
49 changed files with 267 additions and 324 deletions
--- a/engine/battle/ai/items.asm
+++ b/engine/battle/ai/items.asm
@@ -552,6 +552,7 @@ EnemyUsedMaxPotion:
 	jr FullRestoreContinue

 EnemyUsedFullRestore:
+; BUG: AI use of Full Heal does not cure confusion status (see docs/bugs_and_glitches.md)
 	call AI_HealStatus
 	ld a, FULL_RESTORE
 	ld [wCurEnemyItem], a
@@ -559,6 +560,7 @@ EnemyUsedFullRestore:
 	res SUBSTATUS_CONFUSED, [hl]
 	xor a
 	ld [wEnemyConfuseCount], a
+	; fallthrough

 FullRestoreContinue:
 	ld de, wCurHPAnimOldHP
@@ -725,6 +727,7 @@ EnemyUsedFullHealRed: ; unreferenced
 	jp PrintText_UsedItemOn_AND_AIUpdateHUD

 AI_HealStatus:
+; BUG: AI use of Full Heal or Full Restore does not cure Nightmare status (see docs/bugs_and_glitches.md)
 	ld a, [wCurOTMon]
 	ld hl, wOTPartyMon1Status
 	ld bc, PARTYMON_STRUCT_LENGTH
@@ -732,14 +735,6 @@ AI_HealStatus:
 	xor a
 	ld [hl], a
 	ld [wEnemyMonStatus], a
-	; Bug: this should reset SUBSTATUS_NIGHTMARE
-	; Uncomment the 2 lines below to fix
-	; ld hl, wEnemySubStatus1
-	; res SUBSTATUS_NIGHTMARE, [hl]
-	; Bug: this should reset SUBSTATUS_CONFUSED
-	; Uncomment the 2 lines below to fix
-	; ld hl, wEnemySubStatus3
-	; res SUBSTATUS_CONFUSED, [hl]
 	ld hl, wEnemySubStatus5
 	res SUBSTATUS_TOXIC, [hl]
 	ret
--- a/engine/battle/ai/redundant.asm
+++ b/engine/battle/ai/redundant.asm
@@ -176,6 +176,7 @@ AI_Redundant:
 	ret

 .FutureSight:
+; BUG: AI does not discourage Future Sight when it's already been used (see docs/bugs_and_glitches.md)
 	ld a, [wEnemyScreens]
 	bit 5, a
 	ret
--- a/engine/battle/ai/scoring.asm
+++ b/engine/battle/ai/scoring.asm
@@ -1665,9 +1665,10 @@ AI_Smart_Thief:
 	ret

 AI_Smart_Conversion2:
+; BUG: "Smart" AI discourages Conversion2 after the first turn (see docs/bugs_and_glitches.md)
 	ld a, [wLastPlayerMove]
 	and a
-	jr nz, .discourage ; should be jr z
+	jr nz, .discourage

 	push hl
 	dec a
@@ -1743,8 +1744,8 @@ AI_Smart_MeanLook:
 	pop hl
 	jp z, AIDiscourageMove

-; 80% chance to greatly encourage this move if the enemy is badly poisoned (buggy).
-; Should check wPlayerSubStatus5 instead.
+; 80% chance to greatly encourage this move if the enemy is badly poisoned.
+; BUG: "Smart" AI encourages Mean Look if its own Pokémon is badly poisoned (see docs/bugs_and_glitches.md)
 	ld a, [wEnemySubStatus5]
 	bit SUBSTATUS_TOXIC, a
 	jr nz, .encourage
--- a/engine/battle/anim_hp_bar.asm
+++ b/engine/battle/anim_hp_bar.asm
@@ -179,16 +179,12 @@ LongAnim_UpdateVariables:
 	ld c, a
 	ld a, [hli]
 	ld b, a
-	; This routine is buggy. The result from ComputeHPBarPixels is stored
-	; in e. However, the pop de opcode deletes this result before it is even
-	; used. The game then proceeds as though it never deleted that output.
-	; To fix, uncomment the line below.
+; BUG: HP bar animation is slow for high HP (see docs/bugs_and_glitches.md)
 	call ComputeHPBarPixels
-	; ld a, e
 	pop bc
 	pop de
 	pop hl
-	ld a, e ; Comment or delete this line to fix the above bug.
+	ld a, e
 	ld hl, wCurHPBarPixels
 	cp [hl]
 	jr z, .loop
@@ -373,17 +369,14 @@ ShortHPBar_CalcPixelFrame:
 	call AddNTimes

 	ld b, 0
-; This routine is buggy. If [wCurHPAnimMaxHP] * [wCurHPBarPixels] is
-; divisible by HP_BAR_LENGTH_PX, the loop runs one extra time.
-; To fix, uncomment the line below.
 .loop
+; BUG: HP bar animation off-by-one error for low HP (see docs/bugs_and_glitches.md)
 	ld a, l
 	sub HP_BAR_LENGTH_PX
 	ld l, a
 	ld a, h
 	sbc $0
 	ld h, a
-	; jr z, .done
 	jr c, .done
 	inc b
 	jr .loop
--- a/engine/battle/battle_transition.asm
+++ b/engine/battle/battle_transition.asm
@@ -214,8 +214,7 @@ DEF TRANS_NO_CAVE_F  EQU 1 ; bit set in TRANS_NO_CAVE and TRANS_NO_CAVE_STRONGER
 StartTrainerBattle_DetermineWhichAnimation:
 ; The screen flashes a different number of times depending on the level of
 ; your lead Pokemon relative to the opponent's.
-; BUG: wBattleMonLevel and wEnemyMonLevel are not set at this point, so whatever
-; values happen to be there will determine the animation.
+; BUG: Battle transitions fail to account for enemy's level (see docs/bugs_and_glitches.md)
 	ld de, 0
 	ld a, [wBattleMonLevel]
 	add 3
--- a/engine/battle/core.asm
+++ b/engine/battle/core.asm
@@ -295,6 +295,7 @@ HandleBetweenTurnEffects:
 	jp HandleEncore

 CheckFaint_PlayerThenEnemy:
+; BUG: Perish Song and Spikes can leave a Pokemon with 0 HP and not faint (see docs/bugs_and_glitches.md)
 	call HasPlayerFainted
 	jr nz, .PlayerNotFainted
 	call HandlePlayerMonFaint
@@ -319,6 +320,7 @@ CheckFaint_PlayerThenEnemy:
 	ret

 CheckFaint_EnemyThenPlayer:
+; BUG: Perish Song and Spikes can leave a Pokemon with 0 HP and not faint (see docs/bugs_and_glitches.md)
 	call HasEnemyFainted
 	jr nz, .EnemyNotFainted
 	call HandleEnemyMonFaint
@@ -387,6 +389,7 @@ HandleBerserkGene:
 	call GetPartyLocation
 	xor a
 	ld [hl], a
+; BUG: Berserk Gene's confusion lasts for 256 turns or the previous Pokémon's confusion count (see docs/bugs_and_glitches.md)
 	ld a, BATTLE_VARS_SUBSTATUS3
 	call GetBattleVarAddr
 	push af
@@ -4186,6 +4189,7 @@ PursuitSwitch:
 	or [hl]
 	jr nz, .done

+; BUG: A Pokémon that fainted from Pursuit will have its old status condition when revived (see docs/bugs_and_glitches.md)
 	ld a, $f0
 	ld [wCryTracks], a
 	ld a, [wBattleMonSpecies]
@@ -5759,8 +5763,8 @@ CheckPlayerHasUsableMoves:
 	jr .loop

 .done
-	; Bug: this will result in a move with PP Up confusing the game.
-	and a ; should be "and PP_MASK"
+; BUG: A Disabled but PP Up–enhanced move may not trigger Struggle (see docs/bugs_and_glitches.md)
+	and a
 	ret nz

 .force_struggle
@@ -6159,13 +6163,14 @@ LoadEnemyMon:
 	jr nz, .Happiness

 ; Get Magikarp's length
+; BUG: Magikarp length limits have a unit conversion error (see docs/bugs_and_glitches.md)
 	ld de, wEnemyMonDVs
 	ld bc, wPlayerID
 	callfar CalcMagikarpLength

 ; No reason to keep going if length > 1536 mm (i.e. if HIGH(length) > 6 feet)
 	ld a, [wMagikarpLength]
-	cp HIGH(1536) ; should be "cp 5", since 1536 mm = 5'0", but HIGH(1536) = 6
+	cp HIGH(1536)
 	jr nz, .CheckMagikarpArea

 ; 5% chance of skipping both size checks
@@ -6174,7 +6179,7 @@ LoadEnemyMon:
 	jr c, .CheckMagikarpArea
 ; Try again if length >= 1616 mm (i.e. if LOW(length) >= 4 inches)
 	ld a, [wMagikarpLength + 1]
-	cp LOW(1616) ; should be "cp 4", since 1616 mm = 5'4", but LOW(1616) = 80
+	cp LOW(1616)
 	jr nc, .GenerateDVs

 ; 20% chance of skipping this check
@@ -6183,23 +6188,11 @@ LoadEnemyMon:
 	jr c, .CheckMagikarpArea
 ; Try again if length >= 1600 mm (i.e. if LOW(length) >= 3 inches)
 	ld a, [wMagikarpLength + 1]
-	cp LOW(1600) ; should be "cp 3", since 1600 mm = 5'3", but LOW(1600) = 64
+	cp LOW(1600)
 	jr nc, .GenerateDVs

 .CheckMagikarpArea:
-; The "jr z" checks are supposed to be "jr nz".
-
-; Instead, all maps in GROUP_LAKE_OF_RAGE (Mahogany area)
-; and Routes 20 and 44 are treated as Lake of Rage.
-
-; This also means Lake of Rage Magikarp can be smaller than ones
-; caught elsewhere rather than the other way around.
-
-; Intended behavior enforces a minimum size at Lake of Rage.
-; The real behavior prevents a minimum size in the Lake of Rage area.
-
-; Moreover, due to the check not being translated to feet+inches, all Magikarp
-; smaller than 4'0" may be caught by the filter, a lot more than intended.
+; BUG: Magikarp in Lake of Rage are shorter, not longer (see docs/bugs_and_glitches.md)
 	ld a, [wMapGroup]
 	cp GROUP_LAKE_OF_RAGE
 	jr z, .Happiness
@@ -6212,7 +6205,7 @@ LoadEnemyMon:
 	jr c, .Happiness
 ; Try again if length < 1024 mm (i.e. if HIGH(length) < 3 feet)
 	ld a, [wMagikarpLength]
-	cp HIGH(1024) ; should be "cp 3", since 1024 mm = 3'4", but HIGH(1024) = 4
+	cp HIGH(1024)
 	jr c, .GenerateDVs ; try again

 ; Finally done with DVs
@@ -6805,6 +6798,7 @@ BadgeStatBoosts:
 	ld hl, wBattleMonAttack
 	ld c, 4
 .CheckBadge:
+; BUG: Glacier Badge may not boost Special Defense depending on the value of Special Attack (see docs/bugs_and_glitches.md)
 	ld a, b
 	srl b
 	call c, BoostStat
@@ -6814,9 +6808,6 @@ BadgeStatBoosts:
 	srl b
 	dec c
 	jr nz, .CheckBadge
-; Check GlacierBadge again for Special Defense.
-; This check is buggy because it assumes that a is set by the "ld a, b" in the above loop,
-; but it can actually be overwritten by the call to BoostStat.
 	srl a
 	call c, BoostStat
 	ret
@@ -7637,6 +7628,7 @@ SendOutMonText:
 	ld hl, GoMonText
 	jr z, .skip_to_textbox

+; BUG: Switching out or switching against a Pokémon with max HP below 4 freezes the game (see docs/bugs_and_glitches.md)
 	; compute enemy health remaining as a percentage
 	xor a
 	ldh [hMultiplicand + 0], a
--- a/engine/battle/effect_commands.asm
+++ b/engine/battle/effect_commands.asm
@@ -1401,13 +1401,9 @@ BattleCheckTypeMatchup:
 	and a
 	jr z, CheckTypeMatchup
 	ld hl, wBattleMonType1
+ 	; fallthrough
 CheckTypeMatchup:
-; There is an incorrect assumption about this function made in the AI related code: when
-; the AI calls CheckTypeMatchup (not BattleCheckTypeMatchup), it assumes that placing the
-; offensive type in a will make this function do the right thing. Since a is overwritten,
-; this assumption is incorrect. A simple fix would be to load the move type for the
-; current move into a in BattleCheckTypeMatchup, before falling through, which is
-; consistent with how the rest of the code assumes this code works like.
+; BUG: AI makes a false assumption about CheckTypeMatchup (see docs/bugs_and_glitches.md)
 	push hl
 	push de
 	push bc
@@ -1865,9 +1861,7 @@ BattleCommand_EffectChance:
 	jr z, .got_move_chance
 	ld hl, wEnemyMoveStruct + MOVE_CHANCE
 .got_move_chance
-
-	; BUG: 1/256 chance to fail even for a 100% effect chance,
-	; since carry is not set if BattleRandom == [hl] == 255
+; BUG: Moves with a 100% secondary effect chance will not trigger it in 1/256 uses (see docs/bugs_and_glitches.md)
 	call BattleRandom
 	cp [hl]
 	pop hl
@@ -2089,6 +2083,7 @@ BattleCommand_FailureText:
 	inc hl
 	ld a, [hl]

+; BUG: Beat Up may fail to raise Substitute (see docs/bugs_and_glitches.md)
 	cp EFFECT_MULTI_HIT
 	jr z, .multihit
 	cp EFFECT_DOUBLE_HIT
@@ -2510,6 +2505,7 @@ DittoMetalPowder:
 	pop bc
 	ret nz

+; BUG: Metal Powder can increase damage taken with boosted (Special) Defense (see docs/bugs_and_glitches.md)
 	ld a, c
 	srl a
 	add c
@@ -2646,6 +2642,7 @@ TruncateHL_BC:
 	inc l

 .finish
+; BUG: Reflect and Light Screen can make (Special) Defense wrap around above 1024 (see docs/bugs_and_glitches.md)
 	ld a, [wLinkMode]
 	cp LINK_COLOSSEUM
 	jr z, .done
@@ -2772,6 +2769,7 @@ SpeciesItemBoost:
 	ret nz

 ; Double the stat
+; BUG: Thick Club and Light Ball can make (Special) Attack wrap around above 1024 (see docs/bugs_and_glitches.md)
 	sla l
 	rl h
 	ret
@@ -2901,8 +2899,7 @@ HitSelfInConfusion:

 BattleCommand_DamageCalc:
 ; Return a damage value for move power d, player level e, enemy defense c and player attack b.
-
-; Return 1 if successful, else 0.
+; BUG: Confusion damage is affected by type-boosting items and Explosion/Self-Destruct doubling (see docs/bugs_and_glitches.md)

 	ld a, BATTLE_VARS_MOVE_EFFECT
 	call GetBattleVar
@@ -5263,6 +5260,7 @@ BattleCommand_EndLoop:
 	jr .double_hit

 .only_one_beatup
+; BUG: Beat Up works incorrectly with only one Pokémon in the party (see docs/bugs_and_glitches.md)
 	ld a, BATTLE_VARS_SUBSTATUS3
 	call GetBattleVarAddr
 	res SUBSTATUS_IN_LOOP, [hl]
@@ -6546,7 +6544,7 @@ INCLUDE "engine/battle/move_effects/future_sight.asm"
 INCLUDE "engine/battle/move_effects/thunder.asm"

 CheckHiddenOpponent:
-; BUG: This routine is completely redundant and introduces a bug, since BattleCommand_CheckHit does these checks properly.
+; BUG: Lock-On and Mind Reader don't always bypass Fly and Dig (see docs/bugs_and_glitches.md)
 	ld a, BATTLE_VARS_SUBSTATUS3_OPP
 	call GetBattleVar
 	and 1 << SUBSTATUS_FLYING | 1 << SUBSTATUS_UNDERGROUND
--- a/engine/battle/move_effects/beat_up.asm
+++ b/engine/battle/move_effects/beat_up.asm
@@ -24,6 +24,7 @@ BattleCommand_BeatUp:
 	ld [wCurBeatUpPartyMon], a

 .got_mon
+; BUG: Beat Up can desynchronize link battles (see docs/bugs_and_glitches.md)
 	ld a, [wCurBeatUpPartyMon]
 	ld hl, wPartyMonNicknames
 	call GetNickname
@@ -35,8 +36,6 @@ BattleCommand_BeatUp:
 	ld a, [wCurBeatUpPartyMon]
 	ld c, a
 	ld a, [wCurBattleMon]
-	; BUG: this can desynchronize link battles
-	; Change "cp [hl]" to "cp c" to fix
 	cp [hl]
 	ld hl, wBattleMonStatus
 	jr z, .active_mon
@@ -196,6 +195,7 @@ BattleCommand_BeatUp:
 	jp SkipToBattleCommand

 BattleCommand_BeatUpFailText:
+; BUG: Beat Up may trigger King's Rock even if it failed (see docs/bugs_and_glitches.md)
 	ld a, [wBeatUpHitAtLeastOnce]
 	and a
 	ret nz
--- a/engine/battle/move_effects/belly_drum.asm
+++ b/engine/battle/move_effects/belly_drum.asm
@@ -1,7 +1,5 @@
 BattleCommand_BellyDrum:
-; This command is buggy because it raises the user's attack
-; before checking that it has enough HP to use the move.
-; Swap the order of these two blocks to fix.
+; BUG: Belly Drum sharply boosts Attack even with under 50% HP (see docs/bugs_and_glitches.md)
 	call BattleCommand_AttackUp2
 	ld a, [wAttackMissed]
 	and a
--- a/engine/battle/move_effects/counter.asm
+++ b/engine/battle/move_effects/counter.asm
@@ -34,7 +34,7 @@ BattleCommand_Counter:
 	cp SPECIAL
 	ret nc

-	; BUG: Move should fail with all non-damaging battle actions
+; BUG: Counter and Mirror Coat still work if the opponent uses an item (see docs/bugs_and_glitches.md)
 	ld hl, wCurDamage
 	ld a, [hli]
 	or [hl]
--- a/engine/battle/move_effects/frustration.asm
+++ b/engine/battle/move_effects/frustration.asm
@@ -1,4 +1,5 @@
 BattleCommand_FrustrationPower:
+; BUG: Return and Frustration deal no damage when the user's happiness is low or high, respectively (see docs/bugs_and_glitches.md)
 	push bc
 	ld hl, wBattleMonHappiness
 	ldh a, [hBattleTurn]
--- a/engine/battle/move_effects/mirror_coat.asm
+++ b/engine/battle/move_effects/mirror_coat.asm
@@ -35,7 +35,7 @@ BattleCommand_MirrorCoat:
 	cp SPECIAL
 	ret c

-	; BUG: Move should fail with all non-damaging battle actions
+; BUG: Counter and Mirror Coat still work if the opponent uses an item (see docs/bugs_and_glitches.md)
 	ld hl, wCurDamage
 	ld a, [hli]
 	or [hl]
--- a/engine/battle/move_effects/present.asm
+++ b/engine/battle/move_effects/present.asm
@@ -1,4 +1,5 @@
 BattleCommand_Present:
+; BUG: Present damage is incorrect in link battles (see docs/bugs_and_glitches.md)
 	ld a, [wLinkMode]
 	cp LINK_COLOSSEUM
 	jr z, .colosseum_skippush
--- a/engine/battle/move_effects/return.asm
+++ b/engine/battle/move_effects/return.asm
@@ -1,4 +1,5 @@
 BattleCommand_HappinessPower:
+; BUG: Return and Frustration deal no damage when the user's happiness is low or high, respectively (see docs/bugs_and_glitches.md)
 	push bc
 	ld hl, wBattleMonHappiness
 	ldh a, [hBattleTurn]
--- a/engine/battle/move_effects/sketch.asm
+++ b/engine/battle/move_effects/sketch.asm
@@ -12,6 +12,7 @@ BattleCommand_Sketch:
 	call CheckSubstituteOpp
 	jp nz, .fail
 ; If the opponent is transformed, fail.
+; BUG: A Transformed Pokémon can use Sketch and learn otherwise unobtainable moves (see docs/bugs_and_glitches.md)
 	ld a, BATTLE_VARS_SUBSTATUS5_OPP
 	call GetBattleVarAddr
 	bit SUBSTATUS_TRANSFORMED, [hl]
--- a/engine/battle/move_effects/teleport.asm
+++ b/engine/battle/move_effects/teleport.asm
@@ -66,14 +66,13 @@ BattleCommand_Teleport:
 	inc c
 	; Generate a number less than c
 .loop_enemy
+; BUG: Wild Pokémon can always Teleport regardless of level difference (see docs/bugs_and_glitches.md)
 	call BattleRandom
 	cp c
 	jr nc, .loop_enemy
 	; b = player level / 4
 	srl b
 	srl b
-	; This should be "jr c, .failed"
-	; As written, it makes enemy use of Teleport always succeed if able
 	cp b
 	jr nc, .run_away

--- a/engine/battle/start_battle.asm
+++ b/engine/battle/start_battle.asm
@@ -93,7 +93,7 @@ PlayBattleMusic:
 	cp RED
 	jr z, .done

-	; They should have included EXECUTIVEM, EXECUTIVEF, and SCIENTIST too...
+; BUG: Team Rocket battle music is not used for Executives or Scientists (see docs/bugs_and_glitches.md)
 	ld de, MUSIC_ROCKET_BATTLE
 	cp GRUNTM
 	jr z, .done