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 EvosMovesPointerTable to EvosAttacksPointers to sync with pokecrystal #315 #359

Closed
wants to merge 1 commit into from

Conversation

kqesar
Copy link
Contributor

@kqesar kqesar commented May 15, 2022

This PR have for goal to sync the name of pointer for evo attacks like pokecrystal.

Related to the issue #315

@Rangi42
Copy link
Member

Rangi42 commented May 15, 2022

Good idea, thanks! evos_moves.asm should also be renamed to evos_attacks.asm.

@kqesar kqesar force-pushed the sync_move_pointers_pokecrystal branch from 1a60a02 to c9d5314 Compare May 15, 2022 21:36
@kqesar
Copy link
Contributor Author

kqesar commented May 16, 2022

I have updated my PR to take in consideration your suggestion @Rangi42 :)

EvosMovesPointerTable:
table_width 2, EvosMovesPointerTable
EvosAttacksPointers:
table_width 2, EvosAttacksPointers
dw RhydonEvosMoves
Copy link
Member

Choose a reason for hiding this comment

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

The problem now is that the rest of this file continues to use "EvosMoves" (RhydonEvosMoves etc).

@@ -122,15 +122,15 @@ RedrawPartyMenu_::
ld c, a
add hl, bc
ld de, wEvosMoves
Copy link
Member

Choose a reason for hiding this comment

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

and wEvosMoves as well

@dannye
Copy link
Member

dannye commented May 16, 2022

Sorry for being contrarian (again), but I argue that pokecrystal should be updated to match pokered in this case.

Both repos already primarily use "move" (data/moves/ etc) over "attack" to refer to pokemon moves. The mainline games do as well (move relearner, move deleter, etc).
Plus, "attack" is a stat like defense etc. so better to not let that word get confused with moves.

@kqesar
Copy link
Contributor Author

kqesar commented May 16, 2022

I can update pokecrystal instead of pokered if you prefer

My wish is to contribute according to your recommendations, it is not a concern to rectify my PRs

@dannye
Copy link
Member

dannye commented May 16, 2022

We can wait for @Rangi42 to chime in too, in case she feels strongly one way or the other.

@Rangi42
Copy link
Member

Rangi42 commented May 17, 2022

Sorry for being contrarian (again), but I argue that pokecrystal should be updated to match pokered in this case.

Both repos already primarily use "move" (data/moves/ etc) over "attack" to refer to pokemon moves. The mainline games do as well (move relearner, move deleter, etc). Plus, "attack" is a stat like defense etc. so better to not let that word get confused with moves.

This is a good point. One thing is that we have NUM_MOVES EQU 4 (how many a mon can learn) and NUM_ATTACKS EQU (the number of different moves). (That's the case in the Gen 1 and 2 repos.) It would probably make sense for those to be renamed NUM_MON_MOVES and NUM_MOVES, but I wonder if changing the meaning of NUM_MOVES would be too confusing.

@dannye
Copy link
Member

dannye commented May 17, 2022

This is a good point. One thing is that we have NUM_MOVES EQU 4 (how many a mon can learn) and NUM_ATTACKS EQU (the number of different moves). (That's the case in the Gen 1 and 2 repos.) It would probably make sense for those to be renamed NUM_MON_MOVES and NUM_MOVES, but I wonder if changing the meaning of NUM_MOVES would be too confusing.

Hmm good point also. Swapping out those names might cause some confusion but probably worth it imo. Those suggested names are much better.

@iimarckus
Copy link
Member

Reusing an old name is gratuitously unfriendly and discourages people from updating their repos.

@dannye
Copy link
Member

dannye commented May 17, 2022

Reusing an old name is gratuitously unfriendly and discourages people from updating their repos.

Yeah that's fair. I think NUM_MOVES can be renamed to NUM_MON_MOVES at the very least because that's just all around better.
Maybe NUM_ATTACKS should just stay the way it is. It isn't horrible after all.

@kqesar kqesar force-pushed the sync_move_pointers_pokecrystal branch from 0b020c8 to e1c6af9 Compare May 17, 2022 16:58
@kqesar kqesar requested a review from dannye May 17, 2022 17:01
@Rangi42
Copy link
Member

Rangi42 commented Jun 6, 2022

@kqesar Thanks for getting this synchronization started. I'm closing this PR since we decided to go the other way and use "moves" instead of "attacks" in all four repos (except for keeping NUM_ATTACKS and renaming DEF NUM_MOVES EQU 4 to DEF NUM_MON_MOVES EQU 4, so there just won't be a "NUM_MOVES" constant).

@Rangi42 Rangi42 closed this Jun 6, 2022
@kqesar kqesar deleted the sync_move_pointers_pokecrystal branch November 24, 2023 19:11
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.

4 participants