Inaccurate function names about pokecrystal HOT 5 CLOSED

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.

from pokecrystal.

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

from pokecrystal.

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

from pokecrystal.

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.

from pokecrystal.

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:

• GetTileCollision -> GetTilePermission: you are getting the permission byte of a given tile number
  • ; Get the collision type of tile a. -> ; Get the permission of tile a.
• TileCollisionTable -> CollisionPermissionTable: you are mapping collisions to permissions, not tiles to collisions.
• OBJECT_TILE -> OBJECT_TILE_COLLISION, and in object_struct MACRO: \1Tile -> \1TileCollision
• GetCoordTile -> GetCoordTileCollision: you are getting the collision byte given x,y coords
• wPlayerTile -> wPlayerTileCollision: it stores a collision value, not a tile number
• CheckPitTile, CheckIceTile, etc. are ok because in reality you are deriving the type of tile from a grouping of COLL_ values

from pokecrystal.