pret / pokecrystal

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

Address issue #1078 (Inaccurate function names) #1105

Closed xCrystal closed 5 months ago

xCrystal commented 6 months ago

The most generic label renaming changes are in the first two commits (first commit related to my first issue comment, second commit related to my more recent issue comment), but some more specific changes are each in their own separate commit for visibility.

I'll point out just the changes that can't be derived from the comments in issue #1078:

mid-kid commented 6 months ago

Thanks a lot for the pull request! And sorry, I just noticed it now, it's been a busy few weeks for me. Don't be afraid to ping us on the discord if we take too long. Clear and concise commits that can be reviewed individually, it's gonna be fun to review.

First comment off the bat: I'd rather not modify hardware_constants.asm in such a repo-wide manner unless it's to realign with https://github.com/gbdev/hardware.inc/blob/master/hardware.inc (which would be great to switch to in the future). If a constant is used for two separate purposes, I'd rather have two constants than one (hardware.inc does this, too).

xCrystal commented 6 months ago

First comment off the bat: I'd rather not modify hardware_constants.asm in such a repo-wide manner unless it's to realign with https://github.com/gbdev/hardware.inc/blob/master/hardware.inc (which would be great to switch to in the future). If a constant is used for two separate purposes, I'd rather have two constants than one (hardware.inc does this, too).

Didn't even think of hardware.inc actually. Now I'm surprised that I'm only finding OAM attr constants and not BG Map attr constants there, but most likely there is a sensible reason for it that I'm not aware of.

I can get rid of the commit addressing this for now, I just found it weird that OAM_* flags where being used in the context of BG Map tiles as well.

mid-kid commented 6 months ago

No, it's a good commit, but I will request you consider splitting it into a new PR if you feel like it. That way, we can discuss the situation regarding the lack of BG map attribute constants with the hardware.inc upstream. I figure it's mostly an oversight - most homebrew relies on tools to generate BG maps, instead of specifying attributes in the source like we do.

xCrystal commented 6 months ago

No, it's a good commit, but I will request you consider splitting it into a new PR if you feel like it. That way, we can discuss the situation regarding the lack of BG map attribute constants with the hardware.inc upstream. I figure it's mostly an oversight - most homebrew relies on tools to generate BG maps, instead of specifying attributes in the source like we do.

Sounds good. Moved that commit elsewhere and applied all other suggestions in 17d3a1a. Thanks!