Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address issue #1078 (Inaccurate function names) #1105

Merged
merged 7 commits into from
Jan 29, 2024

Conversation

xCrystal
Copy link
Member

@xCrystal xCrystal commented Jan 7, 2024

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:

  • Fade in/out function names use the suggestions from mid-kid.
  • PlaceHLTextAtBC has been renamed to PrintTextboxTextAt (5be9856)
  • SetPalettes has been renamed to SetDefaultBGPAndOBP.
  • HDMATransfer_ and HDMATransferTilemapAndAttrmap_ function names use the suggestions from mid-kid.
  • LoadOverworldTilemap, LoadOverworldAttrmapPals, and LoadOverworldTilemapAndAttrmapPals for the respective functions (changing Screen to Overworld from the initial proposal in the first issue comment)
  • wVramState renamed to wStateFlags in addition to adding flag constants (f72f078)
  • refreshscreen -> reanchormap, and reloadmappart -> refreshmap (7f3b879)
  • wScriptFlags2 has been renamed to wEnabledPlayerEvents (because it only apples to events within PlayerEvents), and the names of the functions that set/reset/check bits from it have been consolidated.
  • For the attribute flag changes in 0266f64, the renaming is just in hardware_constants.asm. The other changes are just applying the new names.

@xCrystal xCrystal changed the title Address #1078 Address issue #1078 (Inaccurate function names) Jan 7, 2024
@mid-kid
Copy link
Member

mid-kid commented Jan 18, 2024

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

Copy link
Member

@mid-kid mid-kid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First commit comments

constants/map_data_constants.asm Show resolved Hide resolved
constants/wram_constants.asm Show resolved Hide resolved
engine/gfx/color.asm Show resolved Hide resolved
engine/gfx/dma_transfer.asm Show resolved Hide resolved
@@ -1175,7 +1173,7 @@ WildBattleScript:
reloadmapafterbattle
end

CanUseSweetScent::
CanEncounterWildMonInThisTile::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just a tile check, so CanEncounterWildMon is good enough.

_LoadMapPart::
_LoadOverworldTilemap::
; From the metatile-based 24x20 map in wSurroundingTiles,
; load the corresponding 20x18 tiles to wTilemap.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

; This function is only used for the initial loading of the map,
; incremental loads while moving happen through <I don't remember the name of the func rn>

I'd like to clarify here.

@@ -1,4 +1,5 @@
_SwapTextboxPalettes::
_LoadOverworldAttrmapPals::
; Load wAttrmap palette numbers based on the tileset palettes of current map.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as _LoadOverworldTilemap

@xCrystal
Copy link
Member Author

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
Copy link
Member

mid-kid commented Jan 26, 2024

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 xCrystal force-pushed the renaming/innacurate-function-names branch from 0266f64 to 17d3a1a Compare January 28, 2024 20:19
@xCrystal
Copy link
Member Author

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!

@mid-kid mid-kid closed this Jan 29, 2024
@mid-kid mid-kid reopened this Jan 29, 2024
@mid-kid mid-kid merged commit 17d3a1a into pret:master Jan 29, 2024
2 checks passed
vulcandth added a commit to vulcandth/polishedcrystal that referenced this pull request Apr 14, 2024
Rangi42 pushed a commit to Rangi42/polishedcrystal that referenced this pull request Apr 14, 2024
ShadowOne333 added a commit to ShadowOne333/polishedcrystal that referenced this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants