Consistent unused content references, and predef and special label names #459
Conversation
|
I'm done with this PR |
engine/card_flip.asm
Outdated
| Unknown_e00ed: ; e00ed (38:40ed) | ||
| ; Graphics for an unused Game Corner | ||
| ; game were meant to be here. | ||
| Ret_e00ed: ; e00ed (38:40ed) |
There was a problem hiding this comment.
How about Stubbed_DummyGameAskPlay?
There was a problem hiding this comment.
Also, the name of Unknown_e00ed could reflect that it was some 4-tile image for the dummy game. (Most likely a cursor, given that SPRITE_ANIM_INDEX_DUMMY_GAME uses 4 tiles at $00 just like other cursors.)
There was a problem hiding this comment.
I left empty functions with generic names, and only used meaningful names when there is actual code (even if unreferenced or dummied out after a ret) to back it up. If this one's so evident though, I guess it could be changed to something to reflect what it was supposed to be there, but I feel it's enough if reflected in a comment.
maps/CopycatsHouse2F.asm
Outdated
| @@ -70,7 +70,7 @@ Copycat: | |||
| .Default_Merge_3a: | |||
| faceplayer | |||
| variablesprite SPRITE_COPYCAT, SPRITE_LASS | |||
| special MapCallbackSprites_LoadUsedSpritesGFX | |||
| special Special_MapCallbackSprites_LoadUsedSpritesGFX | |||
There was a problem hiding this comment.
Can this have a shorter name, like Special_LoadUsedSpritesGFX?
There was a problem hiding this comment.
Sure thing, will do it tomorrow along with anything else that might come up. Assuming #456 gets merged first, I'll also try to do the merge myself
engine/battle/ai/move.asm
Outdated
| @@ -92,7 +92,7 @@ AIChooseMove: ; 440ce | |||
|
|
|||
| push bc | |||
| ld d, BANK(TrainerClassAttributes) | |||
| predef FlagPredef | |||
| predef Predef_Flag | |||
There was a problem hiding this comment.
Predef_FlagAction would be more clear, and matches FlagAction and EventFlagAction.
engine/routines/flagpredef.asm
Outdated
| @@ -1,5 +1,5 @@ | |||
| Predef_FlagAction: ; 4d7c1 | |||
| ; Perform action b on flag c in flag array hl. | |||
| Predef_SmallFarFlagAction: ; 4d7c1 | |||
There was a problem hiding this comment.
This is not always far, only when d = 1.
There was a problem hiding this comment.
I know... it's kinda like the FarCopyBytes kinda functions. You can use them to copy a byte from the current bank. I know, there's always a bankswitch in those regardless, but you are still forced to set d to the bank argument here as well (even if it's just setting it to 0 to avoid bankswitch).
I mean, I don't really care. I can rename it again if wanted.
There was a problem hiding this comment.
Eh, it's fine. Come to think of it, FlagAction does not support bank switching, so "SmallFar" accurately describes this one's differences.
I would, however, either rename the file to predef_smallfarflagaction.asm (ugly) or find a better place for it than engine/routines/lowercaselabelname.asm (which was never meant to be permanent anyway).
There was a problem hiding this comment.
Yes, the file should be renamed (along with many others), but I don't think it's a good idea to do more in this PR until the current open ones are merged. I know it's just a single file that won't cause any conflict, but it would probably become renamed/moved again when that stuff is dealt with in another PR anyway.
|
The conflict with #456 is very minor: my copy of data/sprite_anims/framesets.asm subsumes your changes; and there's a one-line manual merge needed in engine/item_effects.asm. |
|
Good to hear, feel free to merge this PR into your repo, as I'm not going to push any more commits to it. As far as I know, #386 is obsolete, #446 and #448 aren't settled yet, #457 is not a problem, and #452 isn't finished yet. If #460 is finished too, it could be merged as well to get a head start fixing potential conflicts. |
…olve PR pret#459) # Conflicts: # data/items/descriptions.asm # data/sprite_anims/framesets.asm # engine/crystal_colors.asm # engine/events/kurt.asm # engine/events/special.asm # engine/events/std_scripts.asm # engine/events_3.asm # engine/item_effects.asm # engine/namingscreen.asm # engine/scripting.asm # engine/stats_screen.asm # engine/trade_animation.asm # home/audio.asm # main.asm # maps/BattleTower1F.asm # maps/BattleTowerBattleRoom.asm # maps/BurnedTowerB1F.asm # maps/ElmsLab.asm # maps/GoldenrodDeptStore5F.asm # maps/GoldenrodUnderground.asm # maps/HallOfFame.asm # maps/MahoganyTown.asm # maps/ManiasHouse.asm # maps/MobileBattleRoom.asm # maps/MobileTradeRoomMobile.asm # maps/RadioTower2F.asm # maps/Route35NationalParkGate.asm # maps/Route36NationalParkGate.asm # maps/Route39Farmhouse.asm # tilesets/palette_maps.asm
Ready to merge. However, it's going to conflict somewhere with #456, so feel free to merge that one first and then I could look at merging and resolving conflicts. There shouldn't be too many though.
Addresses issue #453. Gets rid of the
mobilestring, consolidates unused content withUnreferenced_orStubbed_label prefixes, or with an; unusedcomment. These are mostly consistent and should be easy to locate with a simple search, even though words likeunusedanddummyare still scattered in many labels and comments of unused content, particularly for constants, text and scripts.As for
predefandspecials, I addedPredef_andSpecial_and a prefix in function labels (for consistency with most current specials), using also the wordDummyorUnusedwhen appropriate. Unecessary bank 0 specials were not renamed and instead commented with; bank 0.Also took out leftover unused data from main.asm and changed some
UnknownText_xxxthat are unreferenced toUnusedText_xxx.