Rename unused items #424

SatoMew · 2023-07-21T23:20:13Z

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

dannye · 2023-07-23T18:33:51Z

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.

constants/item_constants.asm

dannye · 2023-07-23T18:36:50Z

home/item.asm

-; [wWhichPokemon] = index of item within inventory
-; [wItemQuantity] = quantity to toss
-; OUTPUT:
-; clears carry flag if the item is tossed, sets carry flag if not


Why delete these descriptions? Explaining input and output is nice actually.

These comments are misplaced since they refer to UseItem_, TossItem_, and IsKeyItem_, respectively. See also #302.

Comments which describe the input/output of a routine are not at all the type of redundant comments referred to by #302.

That being said, these description comments are also already repeated on TossItem_, IsKeyItem_, so removing them from here could be fine, but the description on UseItem is not repeated on UseItem_.

That being said, it's kind of nice for the description to be on the entrypoint in home/ rather than the implementation in engine/, since the entrypoints are kind of like the "api" of home; convenient to access and convenient to read.

I really like the way this approach is used in poketcg for example:
https://github.com/pret/poketcg/blob/43b7b92/src/home/coin_toss.asm#L1-L13

The comments are not "misplaced" as you say; they are absolutely true in reference to the routines where they are placed.

Comments which describe the input/output of a routine are not at all the type of redundant comments referred to by #302.

Do current pokegold and pokecrystal feature similar comments? If not, should they? It'd be nice to have a standardized approach across the different projects...

That being said, these description comments are also already repeated on TossItem_, IsKeyItem_, so removing them from here could be fine, but the description on UseItem is not repeated on UseItem_.

That being said, it's kind of nice for the description to be on the entrypoint in home/ rather than the implementation in engine/, since the entrypoints are kind of like the "api" of home; convenient to access and convenient to read.

Would you like to add the comment in UseItem_ or to restore the deleted comments and remove the duplicates from the implementation routines?

I could be persuaded both ways. I see pros and cons to both.

Taking the coin toss example from poketcg, the implementation routine has no description comment on top, which makes sense to me since the code more so speaks for itself:
https://github.com/pret/poketcg/blob/43b7b92/src/engine/duel/core.asm#L7817

Although maybe it would be better if the implementation routine at least had a tl;dr description or something, I dunno.

I'm also not super concerned with standardizing, given that many different people work on the different disassemblies, and the games themselves are structured differently enough that it may be best to consider many things on an individual level. Of course, consistency and uniformity when it's easy is good.

If the code is simple or speaks for itself; then it doens't really need a comment imo.

I prefer routine input/output/description comments to go near the routine doing the majority of the "work". Especially if home is only being used as a way to jump far. If the home jump moves registers around, then it could have an additional small comment stating so, but most of the time this should be painfully obvious and simple to read.

It looks like there is conflicting feedback about this. What should I do?

SatoMew · 2023-07-23T18:47:51Z

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 · 2023-07-23T19:37:11Z

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 · 2023-07-23T19:43:34Z

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.

constants/item_constants.asm

engine/items/item_effects.asm

vulcandth · 2023-11-15T19:44:39Z

home/item.asm

-; [wWhichPokemon] = index of item within inventory
-; [wItemQuantity] = quantity to toss
-; OUTPUT:
-; clears carry flag if the item is tossed, sets carry flag if not


If the code is simple or speaks for itself; then it doens't really need a comment imo.

I prefer routine input/output/description comments to go near the routine doing the majority of the "work". Especially if home is only being used as a way to jump far. If the home jump moves registers around, then it could have an additional small comment stating so, but most of the time this should be painfully obvious and simple to read.

Rangi42 · 2023-11-15T21:48:19Z

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

constants/item_constants.asm

dannye · 2023-11-18T05:02:42Z

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 · 2023-11-18T14:18:04Z

UNUSED_ITEM_XX sounds good.

vulcandth · 2023-11-19T04:04:56Z

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 · 2023-11-19T04:50:14Z

Sure, UNUSED_ or not.

Rename SURFBOARD to SURF_ITEM, some misc changes

8a5851a

dannye reviewed Jul 23, 2023

View reviewed changes

vulcandth requested changes Nov 15, 2023

View reviewed changes

vulcandth requested changes Nov 16, 2023

View reviewed changes

constants/item_constants.asm Show resolved Hide resolved

Rangi42 closed this Nov 22, 2023

SatoMew changed the title ~~Rename SURFBOARD to SURF_ITEM~~ Rename unused items Dec 16, 2023

SatoMew mentioned this pull request Dec 17, 2023

Clean up item comments #443

Merged

SatoMew deleted the surf branch December 17, 2023 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename unused items #424

Rename unused items #424

SatoMew commented Jul 21, 2023 •

edited

Loading

dannye commented Jul 23, 2023

dannye Jul 23, 2023

SatoMew Jul 23, 2023

dannye Jul 23, 2023

SatoMew Jul 23, 2023

dannye Jul 23, 2023

vulcandth Nov 15, 2023

SatoMew Dec 16, 2023

SatoMew commented Jul 23, 2023 •

edited

Loading

dannye commented Jul 23, 2023

dannye commented Jul 23, 2023

vulcandth Nov 15, 2023

Rangi42 commented Nov 15, 2023

dannye commented Nov 18, 2023

Rangi42 commented Nov 18, 2023

vulcandth commented Nov 19, 2023

Rangi42 commented Nov 19, 2023

Rename unused items #424

Rename unused items #424

Conversation

SatoMew commented Jul 21, 2023 • edited Loading

dannye commented Jul 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SatoMew commented Jul 23, 2023 • edited Loading

dannye commented Jul 23, 2023

dannye commented Jul 23, 2023

Choose a reason for hiding this comment

Rangi42 commented Nov 15, 2023

dannye commented Nov 18, 2023

Rangi42 commented Nov 18, 2023

vulcandth commented Nov 19, 2023

Rangi42 commented Nov 19, 2023

SatoMew commented Jul 21, 2023 •

edited

Loading

SatoMew commented Jul 23, 2023 •

edited

Loading