search

pret / pokecrystal

Disassembly of Pokémon Crystal
https://pret.github.io/pokecrystal/
2.07k stars 778 forks source link

Inaccurate function names #1078

Closed xCrystal closed 5 months ago

xCrystal commented 1 year ago

Obviously this is not black and white and renaming things will break tutorials and cause headaches to people comparing old commits, so I'm just opening an issue for now.

Anyway, for now I have these suggestions. Please, spare me the need to link to every single of these functions...

CanUseSweetScent -> CanEncounterWildMonInThisTile While borrowed by Sweet Scent, this is part of the RandomEncounter logic.

FadeInPalettes -> FadeInPalettesFromWhite FadeOutPalettes -> FadeOutPalettesToWhite FadeInQuickly -> FadeInPalettesFromBlack FadeBlackQuickly -> FadeOutPalettesToBlack These four are all equivalent counterparts, and are all consecutive Specials. They work at the same speed. The only difference is that, while all four are used in scripts, white fading is also used by mapsetup scripts.

PlaceHLTextAtBC -> PrintHLTextAtBC PrintPartyMenuText -> PlacePartyMenuText Stick to Place for text without delay, and Print for text with letter delay. PlaceHLTextAtBC is just the latter part of PrintText (a generic PrintTextboxText). Speaking of this, NO_TEXT_DELAY_F from wTextboxFlags should actually be TEXT_DELAY_F (see PrintLetterDelay)

SetPalettes -> SetDefaultPaletteShades This sets BG and OB palettes to the standard 3,2,1,0. The current function name suggests to me more that the actual palette colors are being set.

HDMATransfer_Wait123Scanlines -> HDMATransfer_EndBeforeScanline124 HDMATransfer_Wait127Scanlines -> HDMATransfer_EndBeforeScanline128 Rather than waiting N scanlines, it waits the current frame to ensure the function completes before a specific scanline. Haven't digged up the timing requirements, it must be related to what the callers need. _Note: this includes the wrapper functions that end with _toBGMap._

ReloadMapPart -> HDMATransferTilemapAndAttrmap_OverworldEffect OpenAndCloseMenu_HDMATransferTilemapAndAttrmap -> HDMATransferTilemapAndAttrmap_OpenAndCloseMenu These only differ on that the former uses HDMATransfer_Wait127Scanlines and the latter uses HDMATransfer_Wait123Scanlines. They are used in different situations, with ReloadMapPart being mainly used after a changeblock or a field move effect. _Note: In addition, _OpenAndCloseMenu_HDMATransferTilemapAndAttrmap has the leading underscore flipped (used in the external function rather than the internal)_

LoadMapPart -> LoadScreenTilemap: From the metatile-based 24x20 map in wSurroundingTiles, load the corresponding 20x18 tiles to wTilemap. Later, BackupBGMap* from ScrollMap* copies new row/column from wTilemap to wBGMapBuffer. _ScrollBGMapPalettes populates wBGMapPalBuffer based on the tiles at wBGMapBuffer. These are read during vblank by UpdateBGMapBuffer. SwapTextboxPalettes -> LoadScreenAttrmapPals: Load wAttrmap palette numbers based on the tileset palettes of the current map. Called only by LoadScreenTilemapAndAttrmapPals. OverworldTextModeSwitch -> LoadScreenTilemapAndAttrmapPals: LoadScreenTilemap + LoadScreenAttrmapPals. Often used to reload screen after closing a text box, but also called when first loading the map. _Note: These could be LoadMap* or LoadMapScreen* too. I used Screen to denote that it's not the big 256x256 BG map what is targeted._

Script reloadmappart -> ???: OverworldTextModeSwitch + GetMovementPermissions + ReloadMapPart + UpdateSprites. Often used after a block change or field move, which can affect collisions. Ironically, the name I would use for this script is refreshscreen, which is another existing script. refreshscreen just calls RefreshScreen, which does the same anchoring part as OpenText besides drawing the textbox. reloadmappart does not reanchor. On the other hand, it refreshes movement permissions and refreshes sprites.

wVramState flags could use constants. ; bit 7: SCRIPTED_MOVEMENT_STATE_F ; bit 6: TEXT_STATE_F ; bit 1: LAST_12_SPRITE_OAM_STRUCTS_RESERVED_F ; bit 0: SPRITE_UPDATES_DISABLED_F

vulcandth commented 1 year ago

Please provide a link to every single one of the routines and labels mentioned in this issue. lol. No, we don't need links we can look them up.

Thanks for taking the time to write this up! We will begin looking through all of these instances. We may ask for you to open a PR with these later after we look this over if you have the time.

mid-kid commented 1 year ago

Thanks a lot for this, a lot of these names have irked me as well, but I always fail to make some time to go through them. Here's my suggestions:

FadeInPalettes -> FadeInFromWhite FadeOutPalettes -> FadeOutToWhite FadeInQuickly -> FadeInFromBlack FadeBlackQuickly -> FadeOutToBlack

PlaceHLTextAtBC is a terrible name, even if called "Print". Anything that explicitly mentions the registers is awkward to write and read, as both PrintText and PlaceHLTextAtBC take hl as the text parameter. In an ideal world I'd do: PrintText -> PrintTextbox PlaceHLTextAtBC -> PrintText This world is less than ideal and the transition will be confusing, however, so maybe PlaceHLTextAtBC can simply become PrintTextAt.

As for SetDefaultPaletteShades, I think we can simply reuse the DMG's register name, like we do in many other places, and say either "InitializeBGP", "ResetBGP" or "SetDefaultBGP" (in order of personal preference)

HDMATransfer_Wait123Scanlines -> HDMATransfer_WaitForScanline124 HDMATransfer_Wait127Scanlines -> HDMATransfer_WaitForScanline128

ReloadMapPart -> HDMATransferTilemapAndAttrmap_Overworld OpenAndCloseMenu_HDMATransferTilemapAndAttrmap -> HDMATransferTilemapAndAttrmap_Menu

I don't like "LoadMapPart" or "LoadScreenTilemap", neither really convey what is happening, which is copying map tiles while scrolling the overworld. This applies to the whole set of "LoadScreen" names.

reloadmappart -> reloadmap refreshscreen -> reanchormap

xCrystal commented 1 year ago

Thanks for the feedback! I agree with the suggestions so far.

I came up with these suggestions basically as a side effect of doing unrelated stuff, but in the long run it might be interesting to consider consolidating what all the buzzwords mean if we were to do a large review of names, labels and comments: load, copy, write, transfer, get, print, place, update, reload, refresh, etc. ; tilemap, screen, map, background, object, sprite, etc ; when to be more or less literal/specific naming things after hardware terminology; and so on.

I'm likely exaggerating, honestly. Obviously, I'm not saying that the state of the repo is bad at all, it's impressive how it's come so far. In fact, I don't think there's that much else that could be done at this point, is it? :)

I worked on poketecg in the past mainly, and I tried to be specifically meticulous with how I named and documented everything I disassembled which made me go really slow and got burned out after a while barely getting of of bank0! But realistically, the most relevant stuff is dissecting data structures, scripts, gfx, etc. Luckily other people followed excellently after me :)

mid-kid commented 12 months ago

No, you shouldn't feel bad, this is exactly what I wish most people using this repository would do. It's way easier to notice things are wrong when you actually try and work with the code.

Unlike projects developed by a small set of people, it was way harder to coordinate properly when this repository was being made. We've been trying to clean it up for a while, and have a small guide to achieve this, but this work is always easier said than done, and we'll probably have a lot more revisions before it's anywhere near "good".

Anyway, I appreciate the enthusiasm in wanting to set everything right, but personally I'm fine with incremental fixes like this if you notice them. I'd suggest making changes locally, and once you've collected a decent amount of them, whipping up a pull request. If you have any suggestions with regards to the naming conventions and whatnot, feel free to open separate issues.

xCrystal commented 8 months ago

I will hopefully go over this in the actual repo, but for now I have a few more.

GAME_TIMER_PAUSED_F -> GAME_TIMER_COUNTING_F: it has the opposite logic

The following are mainly semantics with sorrounding functions: CheckTrainerBattle_GetPlayerEvent -> CheckTrainerEvent ReadWarps -> ReadWarpEvents wCurMapWarpCount -> wCurMapWarpEventCount wCurMapWarpsPointer -> wCurMapWarpEventsPointer LoadScriptBDE -> LoadMemScript

wScriptFlags2 could use a better name. To put it simply, wScriptFlags2 is to wMapEventStatus what the Interrupt Enable (IE) register is to the Interrupt Master Enable flag (IME).

In RockSmashScript: disappear -2 -> disappear LAST_TALKED

In hardware_constants.asm, the definition of OAM attribute flags and BG Map attribute flags looks messy. I don't think the latter should be derived from the former like that, but rather have their own definition. And because OAM attr flags are defined as bit numbers but BG Map attr flags are defined as values, there are instances in the code where the wrong one is used because the code specifically needs the flag or the value.

In TryTileCollisionEvent: The ld a, $ff in .done is ld a, PLAYEREVENT_MAPSCRIPT

Some usages of tile/collision/permission names seem incorrect: