Miscellaneous cleanup (January 2025) #486
Conversation
| .invalidDexNumber | ||
| ld a, RHYDON ; $1 | ||
| ; This is the so-called "Rhydon trap". |
There was a problem hiding this comment.
Please explain what "Rhydon trap" means if the comment must use that nickname (I've never heard of it).
There was a problem hiding this comment.
It's the well-known and historical term used in the glitching community 😃
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the links! ; <https://glitchcity.wiki/wiki/Rhydon_trap> would be sufficient for now; MissingNo is infamous enough to have its own Wikipedia article and even it has an explanation in data/trainers/parties.asm, so something in Pokemon's glitching subcommunity should definitely be explained. (Eventually fully in the bugs+glitches doc.)
There was a problem hiding this comment.
Does this look good?
.invalidDexNumber
; This is the so-called "Rhydon trap".
+ ; More info: https://glitchcity.wiki/wiki/Rhydon_trap
ld a, RHYDON
ld [wCurPartySpecies], a
ret
There was a problem hiding this comment.
In general, I would expect "BUG:" comments to match the pokecrystal convention: single-line headings for bugs documented elsewhere. IMO it's premature to add them here; there are no docs/ for Gen 1 yet. Likewise for rephrasing "glitch" to "bug"; as long as it's easily greppable when searching for bugs to put in a centralized document, it's fine.
(It's more helpful for "BUG:"s not to exist yet, so when they eventually do, people will know they've been added in the doc, and they can keep searching for more "bug"s to add.)
On the other hand, adding comments with the words "bug" or "glitch" for finding later is helpful now. 👍
| ld hl, wPlayerName | ||
| ld de, wLinkEnemyTrainerName | ||
| ld de, wLinkEnemyTrainerName ; same as wGrassRate |
There was a problem hiding this comment.
If this were a necessary condition, assert wLinkEnemyTrainerName == wGrassRate would be appropriate. Since it's not, a brief explanation of why the coincidence is meaningful (it leads to the Missingno glitch) would still be helpful, if it's going to be called out here at all.
There was a problem hiding this comment.
In that case, would it be fine to omit the comment altogether given the underlying issue is elsewhere?
There was a problem hiding this comment.
As usual, I don't really see why the explanations that are already written in the comments need to be deleted in the first place (old lines 2020-2026).
There was a problem hiding this comment.
There were comments referring to the bug in the two files below so how about this?
engine/battle/core.asm:
+ ; Since wLinkEnemyTrainerName corresponds to wGrassRate in this case,
+ ; this routine is affected by the bug in TryDoWildEncounter.next.
ld a, [wBattleType]
dec a ; BATTLE_TYPE_OLD_MAN
jp nz, .handleBattleMenuInput
ld hl, wPlayerName
- ld de, wLinkEnemyTrainerName ; same as wGrassRate
+ ld de, wLinkEnemyTrainerName
ld bc, NAME_LENGTH
call CopyData
ld hl, .oldManName
ld de, wPlayerName
ld bc, NAME_LENGTH
call CopyDataengine/items/item_effects.asm:
.oldManBattle
+ ; Since wLinkEnemyTrainerName corresponds to wGrassRate in this case,
+ ; this routine is affected by the bug in TryDoWildEncounter.next.
- ld hl, wLinkEnemyTrainerName ; same as wGrassRate
+ ld hl, wLinkEnemyTrainerName
ld de, wPlayerName
ld bc, NAME_LENGTH
call CopyData
jp .capturedNPC trades are also affected, but I'm not sure of where to add the comment for those (InGameTrade_PrepareTradeData)?
| @@ -2043,7 +2034,7 @@ DisplayBattleMenu:: | |||
| ld c, 50 | |||
| call DelayFrames | |||
| ld [hl], "▷" | |||
| ld a, $2 ; select the "ITEM" menu | |||
| ld a, 2 ; ITEM | |||
There was a problem hiding this comment.
If you're not going to introduce FIGHT/ITEM/PKMN/RUN constants (and I'm not suggesting to, it's not this PR's goal) then IMO no need to change this comment.
There was a problem hiding this comment.
@SatoMew I noticed you marked this thread as "resolved". I would prefer if you didn't do that until the changes in question are pushed and settled.
Resolving the thread too early buries the things that we need to re-verify.
There was a problem hiding this comment.
I'll leave the threads open then, thanks!
| @@ -28,8 +27,7 @@ DoubleSelectedStats: | |||
| ld [hli], a | |||
| ret | |||
|
|
|||
| ; does nothing since no stats are ever selected (barring glitches) | |||
| HalveSelectedStats: | |||
| HalveSelectedStats: ; unused | |||
There was a problem hiding this comment.
ItemUseMedicine may call predef DoubleOrHalveSelectedStats, which just does callfar DoubleSelectedStats and jpfar HalveSelectedStats. So this is used. If no possible item/move/stat/etc combo ever happens to halve a stat, that's worth noting as a comment, but it's not the ; unused convention.
There was a problem hiding this comment.
Indeed, they have no effect in practice. The file where these routines are located also already calls them unused so should its name be changed as well? They are not unreferenced like you mentioned, but they are otherwise unused.
| @@ -29,25 +29,23 @@ ENDC | |||
| IF DEF(_DEBUG) | |||
| db PIKACHU, 5 | |||
| ENDC | |||
| db -1 ; end | |||
| db -1 | |||
There was a problem hiding this comment.
; end comments are appropriate for 0/-1 list terminators.
There was a problem hiding this comment.
Is it something we should preserve even in future comment cleanups then?
There was a problem hiding this comment.
Yes.. That's what Rangi means by "appropriate".
There was a problem hiding this comment.
I have restored the ; end comments.
| ld a, ~(1 << BIT_EARTHBADGE) | ||
| ld [wObtainedBadges], a | ||
|
|
||
| call SetDebugNewGameParty | ||
|
|
||
| ; Exeggutor gets four HM moves. | ||
| ; EXEGGUTOR |
There was a problem hiding this comment.
Personally, I'd remove the comments altogether, but it might not be immediately obvious which Pokémon index wPartyMon1Moves is referring to, especially if you're not checking DebugNewGameParty to confirm.
There was a problem hiding this comment.
In this case, I think repeating the move names in the comments (thunderbolt, fly, etc) is a tad verbose, so I don't mind some of this simplification.
But some of the comments here (and debug_menu.asm) that explain the purpose in plain English is good. Like, "Don't mess around with obedience." and "Fly anywhere." etc. Again, I don't understand how deleting comments is "better".
There was a problem hiding this comment.
Alright, but what about the Pokémon name comments? Can those go?
+ ; Guaranteed obedience.
ld a, 1 << BIT_EARTHBADGE
ld [wObtainedBadges], a+ ; Allow flying to all cities and towns.
+ dec a ; -1
ld [wTownVisitedFlag], a
ld [wTownVisitedFlag + 1], a- ; EXEGGUTOR
ld hl, wPartyMon1Moves
...There was a problem hiding this comment.
what about the Pokémon name comments? Can those go?
I don't see why they should. Our goal here is not "ultra slim source code" but that feels like the direction you're always trying to pull.
Personally, I wouldn't shorten these comments at all. I said earlier that I don't "mind" slight simplification by removing the repeated move names, but I wouldn't make that change myself either.
If you want to ask yourself "Can those go?", the way to answer that question is to ask yourself: Is the code easier or harder to understand without the comments?
In this case, it's harder to understand without the comments. So they are useful.
There was a problem hiding this comment.
If you want to ask yourself "Can those go?", the way to answer that question is to ask yourself: Is the code easier or harder to understand without the comments?
In this case, it's harder to understand without the comments. So they are useful.
For me, it's indifferent since I would understand what the code is doing without the comments.
There was a problem hiding this comment.
Okay then let me elaborate...
If one were to stare just at the routine PrepareNewGameDebug: without all of its comments, how fully could they actually understand it?
Without the comments, they would wonder things like; Wait, who gets Fly? Who gets Surf?
Without the comments, the code only tells you the party slot number.
The comments paint a fuller picture. So they are useful.
| .ok | ||
| ld a, [wPartyCount] | ||
| dec a | ||
| ld [wWhichPokemon], a | ||
| ld a, $1 | ||
| ld a, 1 |
There was a problem hiding this comment.
When 0/1 are used as a Boolean:
| ld a, 1 | |
| ld a, TRUE |
There was a problem hiding this comment.
I wasn't sure about the approach here, thanks!
|
|
||
| ; "SPECTRE" (HAUNTER) | ||
| cp "S" | ||
| cp "S" ; "SPECTRE" (HAUNTER) |
There was a problem hiding this comment.
I was going to say "A single heading comment would be more appropriate than repeating the same one twice" and then noticed that's how it already used to be. :P
There was a problem hiding this comment.
Like this? 😆
+ ; The letters SP refer to "SPECTRE", HAUNTER's earlier name.
- cp "S" ; "SPECTRE" (HAUNTER)
+ cp "S"
ret nz
ld a, [wInGameTradeReceiveMonName + 1]
- cp "P" ; "SPECTRE" (HAUNTER)
+ cp "P"
ret nzThere was a problem hiding this comment.
Sure! Although "Haunter" not "HAUNTER" -- this is a sentence in English, not a reference to our HAUNTER constant for its index number.
|
|
||
| PrepareNewGameDebug: ; dummy except in _DEBUG | ||
| IF DEF(_DEBUG) | ||
| xor a ; PLAYER_PARTY_DATA | ||
| ld [wMonDataLocation], a | ||
|
|
||
| ; Fly anywhere. | ||
| dec a ; $ff (all bits) | ||
| dec a ; NUM_CITY_MAPS |
There was a problem hiding this comment.
No it's not. xor followed by dec a sets a to -1. NUM_CITY_MAPS is 12.
There was a problem hiding this comment.
I think I put that as a reference to the definition of wTownVisitedFlag itself and forgot to adjust the comment. Thank you for spotting it! ❤️
- dec a ; NUM_CITY_MAPS
+ dec a ; -1
ld [wTownVisitedFlag], a
ld [wTownVisitedFlag + 1], aThere was a problem hiding this comment.
- dec a ; NUM_CITY_MAPS + dec a ; -1 ld [wTownVisitedFlag], a ld [wTownVisitedFlag + 1], a
Since wTownVisitedFlag holds flags, commenting a negative number with ; -1 isn't helpful.
The original comment ; $ff (all bits) was good.
There was a problem hiding this comment.
Reverted:
- dec a ; -1
+ dec a ; $ff (all bits)
ld [wTownVisitedFlag], a
ld [wTownVisitedFlag + 1], a
Comments with |
| ; should, it borrows from the high byte of the current frequency instead. | ||
| ; This means that the result will be 0x200 greater than it should be if the | ||
| ; low byte of the current frequency is greater than the low byte of the | ||
| ; target frequency. |
There was a problem hiding this comment.
Pretty minor, but I kind of like when "paragraph" comments are un-indented.
There was a problem hiding this comment.
I indented them in advance since they will become far shorter in the future (once #258 is resolved).
There was a problem hiding this comment.
But they're not short yet, so it's not appropriate to indent them now.
There was a problem hiding this comment.
Alright, I'll revert the indentation changes.
| @@ -2187,8 +2189,7 @@ AnimCopyRowRight: | |||
| jr nz, AnimCopyRowRight | |||
| ret | |||
|
|
|||
| ; get the sound of the move id in b | |||
| GetMoveSoundB: | |||
| GetMoveSoundB: ; unused | |||
There was a problem hiding this comment.
Technically, this is not unused. It's called by some other unused code. I'm splitting hairs though, and I don't have a strong opinion about whether this routine should be labelled "unused" or not. Not sure what other similar examples might be in the code to use as reference.
Also, I don't see the point in deleting the input/output comment. Comments that summarize the input/output (especially which registers) are nice. This comment honestly isn't perfect and could be improved, but I wouldn't delete it. (Honestly, the more important routine below, GetMoveSound, is completely lacking an input/output comment but I think it deserves one too of course.)
There was a problem hiding this comment.
What would you like to have as comments for both routines?
Also, I'd rename PlayMoveSoundB to PlayIntroMoveSound and GetMoveSoundB to GetIntroMoveSound since the latter is only called by the former, so it being specific to the Opening Movie would imo make their purpose clearer.
There was a problem hiding this comment.
; only called by the unused routine PlayMoveSoundB is a terse complete English sentence saying the thing you want to indicate in a comment. There's no convention for such comments because nobody has written a reliable recursive-usage-tracking analyzer yet (just some throwaway Python scripts).
There was a problem hiding this comment.
Also, it would be GetIntroMoveSound: ; unreferenced; not that ; unused comment below the definition line (much harder to grep with).
| ld hl, wPlayerName | ||
| ld de, wLinkEnemyTrainerName | ||
| ld de, wLinkEnemyTrainerName ; same as wGrassRate |
There was a problem hiding this comment.
As usual, I don't really see why the explanations that are already written in the comments need to be deleted in the first place (old lines 2020-2026).
| @@ -2043,7 +2034,7 @@ DisplayBattleMenu:: | |||
| ld c, 50 | |||
| call DelayFrames | |||
| ld [hl], "▷" | |||
| ld a, $2 ; select the "ITEM" menu | |||
| ld a, 2 ; ITEM | |||
There was a problem hiding this comment.
@SatoMew I noticed you marked this thread as "resolved". I would prefer if you didn't do that until the changes in question are pushed and settled.
Resolving the thread too early buries the things that we need to re-verify.
| @@ -6385,8 +6374,7 @@ LoadPlayerBackPic: | |||
| hlcoord 1, 5 | |||
| predef_jump CopyUncompressedPicToTilemap | |||
|
|
|||
| ; does nothing since no stats are ever selected (barring glitches) | |||
| DoubleOrHalveSelectedStats: | |||
| DoubleOrHalveSelectedStats: ; unused | |||
There was a problem hiding this comment.
This is not unused. (It's called in ItemUseMedicine: from item_effects.asm)
There was a problem hiding this comment.
That means it's not unreferenced, which is different from merely being unused.
There was a problem hiding this comment.
This code is executed by Full Heal. It is absolutely not unused.
There was a problem hiding this comment.
Is the original comment wrong then?
| .CanEncounter | ||
| ; compare encounter chance with a random number to determine if there will be an encounter |
There was a problem hiding this comment.
I'm inclined to say that these one-line comments that announce which step of the procedure we're on are also good.
Again, I'm not claiming the comments are perfect or amazing, but I don't see what's being achieved by deleting such "procedure outline" comments.
|
|
||
| ; This was fixed in Yellow. | ||
|
|
||
| CheckEvolveTradeMonName: ; unused |
There was a problem hiding this comment.
It is, the routine was designed for a context that no longer exists so the conditions it is looking for are never met. What you mean is that it is not unreferenced, which is correct.
There was a problem hiding this comment.
You are wrong. Having no affect is not the same as being unused. This routine is absolutely executed under ordinary circumstances.
There was a problem hiding this comment.
"Unused" does not imply "never executed". If you want to be technical, then this is dead code, but not unreachable code.
There was a problem hiding this comment.
"Unused" does not imply "never executed"
Yes it does.
If you want to be technical, then this is dead code, but not unreachable code.
You are absolutely right. That's exactly what the original comments were there to communicate. But you deleted all the comments in favor of a much more brief and unhelpful comment.
There was a problem hiding this comment.
The comments I added as replacements are perfectly accurate.
There was a problem hiding this comment.
I didn't say your new comments contain inaccuracies. I said they are more brief and less helpful.
| pop af | ||
| jr c, .next ; skip the delay if the user interrupted the animation | ||
| jr c, .next |
There was a problem hiding this comment.
This is one of the best examples of a comment that shouldn't be deleted.
When we have gone on sprees of deleting comments, it's because we went overboard in the past with truly redundant comments, like:
call LoadThing ; load the thing
But this comment about user interruption is actual insight about the meaning of the carry flag at this point that would not be obvious from staring at this code if it weren't for the comment.
There was a problem hiding this comment.
Reverted:
- jr c, .next
+ jr c, .next ; skip the delay if the user interrupted the animation| @@ -462,5 +463,4 @@ FightIntroFrontMon3: | |||
| ENDC | |||
|
|
|||
| FightIntroFrontMonEnd: | |||
|
|
|||
There was a problem hiding this comment.
This blank line represents separation between two things which are not strictly connected.
| ld a, [hli] | ||
| ld [wGrassRate], a | ||
| ld [wGrassRate], a ; same as [wLinkEnemyTrainerName] |
There was a problem hiding this comment.
Because of the Cinnabar/Old Man glitch, I understand making some cross references between wGrassRate and wLinkEnemyTrainerName in some spots, but this straightforward routine doesn't feel like the right place. (This code isn't where any of the problems stem from.)
If there were already all-caps " You might even want to split this into two PRs, for bug comments and for miscellaneous observations. |
I think I'll replace |
For example, a lot of comments identifying bugs were better standardized as a step further towards #258.