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

rename func_8002F368, a getter for struct Player -> exchangeItemId #2358

Merged
merged 1 commit into from
Dec 14, 2024

Conversation

Feacur
Copy link
Contributor

@Feacur Feacur commented Dec 13, 2024

this PR invites anyone to discuss and decide on the name

my initial thought was to name it simply Player_GetExchangeItemId. I've reconsidered to settle for Actor_GetPlayerExchangeItemId now, though. arguably, the former might be not the best name, as discussed with @fig02 @ #2295 (comment)

reasoning

all in all it seems that the general consensus is:

  • prefix indicates a caller
    • Actor_ for common functions
    • %EntityName%_ for entity-specific ones
  • the rest descibes its purpose, obviously
    • it shouldn't necesarily be a getter

so, following this rule, Actor_GetPlayerExchangeItemId seems to fit:

  • multiple different entities request exchange info
  • player is still being mentioned here, because the state is completely encapsulated inside its data

P.S.

existing functions considered:

  • Actor_FindNearby: an actor looks up another one
  • BgDyYoseizo_Vanish: Great Fairy looks up an ocarina spot, besides the main purpose
  • Actor_GetProjectileActor: an actor looks up a projectile
  • BgGanonOtyuka_WaitToFall: Ganon looks up a falling platform
  • ObjBean_CheckForHorseTrample: Bean plant looks up a horse proximity

Comment on lines +1771 to 1774
s8 Actor_GetPlayerExchangeItemId(PlayState* play) {
Player* player = GET_PLAYER(play);

return player->exchangeItemId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking maybe Actor_GetOfferedExchangeItem or something. As in, get the exchange item that player is offering to the actor.

Dont have to do with this one right away, am curious on other opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I slightly prefer "Player" to "Offered", I feel it makes me think less. Dunno about dropping Id, it seems inconsistent rn (e.g. EXCH_ITEM_ODD_MUSHROOM and not EXCH_ITEM_ID_ODD_MUSHROOM) so idk

Copy link
Collaborator

Choose a reason for hiding this comment

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

so, the name as it is now is what you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either Actor_GetPlayerExchangeItemId or Actor_GetPlayerExchangeItem I guess, but I don't really care

Comment on lines +1771 to 1774
s8 Actor_GetPlayerExchangeItemId(PlayState* play) {
Player* player = GET_PLAYER(play);

return player->exchangeItemId;
Copy link
Contributor

Choose a reason for hiding this comment

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

I slightly prefer "Player" to "Offered", I feel it makes me think less. Dunno about dropping Id, it seems inconsistent rn (e.g. EXCH_ITEM_ODD_MUSHROOM and not EXCH_ITEM_ID_ODD_MUSHROOM) so idk

@fig02 fig02 merged commit 0f27d2f into zeldaret:main Dec 14, 2024
1 check passed
@Feacur Feacur deleted the doc/func_8002F368 branch December 14, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants