search

pret / pokecrystal

Disassembly of Pokémon Crystal
https://pret.github.io/pokecrystal/
2.1k stars 803 forks source link

Avoid repeating TM/HM move names twice, in constants and data #738

Closed Rangi42 closed 4 years ago

Rangi42 commented 4 years ago

https://github.com/pret/pokered/pull/261 makes the add_tm and add_hm macros define TM##_MOVE and HM##_MOVE constants equal to the move ID, so that the moves data table can just use those. This avoid having to type each name twice, and prevents misleading names like add_tm ICE_BEAM corresponding to db SNORE.

@mid-kid @entrpntr What do you think of taking this approach in pokegold and pokecrystal?

Rangi42 commented 4 years ago

An independent change to consider porting from https://github.com/pret/pokered/pull/261: lining up the TM/HM learnsets in columns instead of one long line.

Pros: easier to read without scrolling or word-wrap.

Cons: larger diffs if you change the learnsets, that wouldn't just highlight the added/removed moves in the one line.

entrpntr commented 4 years ago

Seems fine to me. Would it make sense to do this for move tutor moves as well? It's slightly different since there are also script constants involved, but the hardcoded move index values do show up here, in addition to data/moves/tmhm_moves.asm.

Rangi42 commented 4 years ago

Yes, they would create constants MT01_MOVE through MT03_MOVE. And you're right, avoiding the repetition in engine/events/move_tutor.asm would be nice.

mid-kid commented 4 years ago

Wait, so now when I want to define a move that will be a TM later, I need to go to the items table? And to make a TM for an existing move, I'd have to rename the existing move constant everywhere? That sounds counter-intuitive, I'd rather not. If you want it to be clearer, consider exposing the TM_ constant prefix from the macros, so you know you're referencing a TM ID and not a move.

No real opinion on the learnset column thing but I don't think word wrap is a strong enough reason to have to maintain alignment when editing that or adding/removing them. Maybe if it were a list with one item per line instead, that'd be ok.

Rangi42 commented 4 years ago

Right now if you wanted to make Fissure TM-learnable without creating a TM_FISSURE item yet, you'd still have to do enum FISSURE_TMNUM for compatibility with the tmhm base data macro, and the items table is the place you'd do it since that's where the *_TMNUM enum is ongoing.

With this proposal, you could still do just that. Instead of add_tm FISSURE, do enum FISSURE_TMNUM, and in the tmhm_moves.asm table, put db FISSURE instead of db TM##_MOVE.

All this does is it defines TM01_MOVE as an alias for DYNAMICPUNCH when you create the TM_DYNAMICPUNCH and item and DYNAMICPUNCH_TMNUM flag with add_tm DYNAMICPUNCH. So yes, if DYNAMICPUNCH were undefined, it would fail, but I wouldn't expect anyone to be defining learnability for a move that doesn't exist yet. (And if they are, then a placeholder DYNAMICPUNCH = 1 would be sufficient.)

I do slightly prefer the current single-line tmhm declaration. One item per line IMO would be too lengthy; it's useful to see all the learnable TMs on-screen at once.

mid-kid commented 4 years ago

Ah, I see, so this is a change that only affects how data/moves/tmhm_moves.asm is written. In that case, that sounds good, but since we're partially removing the need to edit data/moves/tmhm_moves.asm, could that table simply be generated in full? As in, when adding or removing TMs, you wouldn't need to edit that table at all.

Rangi42 commented 4 years ago

Yeah, that would be convenient.