pret / pokered

Disassembly of Pokémon Red/Blue
4.01k stars 983 forks source link

Rename unused items #424

Closed SatoMew closed 11 months ago

SatoMew commented 1 year ago

This PR also fixes some typos and inconsistencies, and cleans up a few comments.

dannye commented 1 year ago

I'm still not sold on the value of renaming SURFBOARD to SURF_ITEM.

All item constants are derived from their text name, except for the 2 items with the placeholder name of "?????", so for these two there is no objectively "right" name -- it's up to us. And I like Surfboard.

If we conceptualize the item as a physical object that the player could reasonably obtain and hold in their pack, surfboard makes sense. What would it mean to hold a "surf item" in your bag?

Functionally, the relationship between the surf move and the surf item is exactly the same as the relationship between the dig move and the escape rope item, with the only difference being that escape rope is a properly legitimate item with a real name and obtainable in-game. I just don't think the name of the item needs to be "abstracted" to the point of not referring to a real object.

Getting less important and more subjective, I think it's nice that the surfboard comes immediately after the bicycle in the items list. The two form a pair of the two objects in the game that enable the player to enhance their movement around the overworld. I think bicycle/surfboard are a better yin/yang than bicycle/surf item.

Overall, it's not the biggest deal either way, but I don't really want to make the change if I'm not truly compelled.

SatoMew commented 1 year ago

If we conceptualize the item as a physical object that the player could reasonably obtain and hold in their pack, surfboard makes sense. What would it mean to hold a "surf item" in your bag?

We actually know the item's true name, but I believe we can't use it here. It's definitely not a surfboard though.

dannye commented 1 year ago

It's definitely not a surfboard though.

Going off of only the rom, what we have, objectively, is an item named "?????" which when used allows the player to surf on water without the help of a pokemon. What does this sound like? Sounds like a surfboard to me.

dannye commented 1 year ago

This patch feels like a mixed bag of changes for change's sake, which I've never been too keen on. Some of the changes are good, but most seem unnecessary.

Rangi42 commented 11 months ago

I would actually take the opposite tack of this PR: leave SURFBOARD alone, and take the pokecrystal approach of renaming UNUSED_ITEM to ITEM_2C and PP_UP_2 to ITEM_32.

(It's mainly the PP_UP_2 name that bothers me: it's potentially confusing since PP_UP exists, and the item doesn't even have the ItemUsePPUp effect. It just shares the name, and for that we could just clarify with a comment: li "PP UP" ; ITEM_32. But then that means there are two "unused" items, and I would prefer to avoid the ordinal UNUSED_ITEM_2, so the Gen 2 approach unambiguously identifies it no matter how many unused items there are.)

dannye commented 11 months ago

I would actually take the opposite tack of this PR: leave SURFBOARD alone, and take the pokecrystal approach of renaming UNUSED_ITEM to ITEM_2C and PP_UP_2 to ITEM_32.

I'm on board with that. It's also worth noting that constants/map_constants.asm uses the convention of UNUSED_MAP_0B, UNUSED_MAP_69, etc, which I like. So we could also go with UNUSED_ITEM_2C and UNUSED_ITEM_32. I'm fine either way.

Rangi42 commented 11 months ago

UNUSED_ITEM_XX sounds good.

vulcandth commented 11 months ago

UNUSED_ITEM_XX sounds good.

I mean.. i'm fine with it.. but its redundant with the ; unused comment... which is what people will probably grep/search for when looking for unused things to delete.

Rangi42 commented 11 months ago

Sure, UNUSED_ or not.