Use RS commands to define struct offset constants

[Rangi42]

We're currently not using this feature of rgbds. It's similar to our current const and enum macros, with the counter _RS instead of const_value or __enum__.

• rsreset sets _RS = 0
• rsset N sets _RS = N
• FOO rb N sets FOO = _RS and does _RS += N
• FOO rw N sets FOO = _RS and does _RS += N * 2

Their intended use is for defining struct offsets. We have no consistent pattern for those:

• a const_def: the most common; e.g. ITEMATTR_* in constants/item_data_constants.asm
• a sequence of EQUs: easier to have non-sequential values; e.g. NPCTRADE_* in constants/npc_trade_constants.asm
• label differences from a canonical struct usage: avoids redundantly defining a struct macro and offset constants; e.g. BASE_* in constants/pokemon_data_constants.asm

@ISSOtm also points out that using the rs commands is faster to build than using const or enum macros.

I think that at least the const_def and EQUs patterns could be replaced by usage of _RS, and possibly the label offsets as well.

The above examples rewritten:

	rsreset
ITEMATTR_PRICE       rw 1
ITEMATTR_EFFECT      rb 1
ITEMATTR_PARAM       rb 1
ITEMATTR_PERMISSIONS rb 1
ITEMATTR_POCKET      rb 1
ITEMATTR_HELP        rb 1
ITEMATTR_STRUCT_LENGTH EQU _RS

	rsreset
NPCTRADE_DIALOG  rb 1
NPCTRADE_GIVEMON rb 1
NPCTRADE_GETMON  rb 1
NPCTRADE_NICK    rb MON_NAME_LENGTH
NPCTRADE_DVS     rw 1
NPCTRADE_ITEM    rb 1
NPCTRADE_OT_ID   rw 1
NPCTRADE_OT_NAME rb NAME_LENGTH
NPCTRADE_GENDER  rb 1
NPCTRADE_PADDING rb 1

	rsreset
BASE_DEX_NO rb 1
BASE_STATS  EQU _RS
BASE_HP     rb 1
BASE_ATK    rb 1
...etc
BASE_DATA_SIZE EQU _RS

I don't think the base data case is actually an improvement; there's exactly one base data struct in WRAM, so defining constants based on its labels is convenient and avoids repetition. For the other two cases, I think the rs commands have advantages. Their definitions parallel the data definitions in macros like item_attribute and npctrade, and it indicates that this particular set of constants is meant as offsets to a struct.

[Rangi42]

A more drastic proposal would be to replace other const_def and enum usages with RS since it's faster to build.

For instance, this:

	const_def
	const ITEM_POCKET     ; 0
	const BALL_POCKET     ; 1
	const KEY_ITEM_POCKET ; 2
	const TM_HM_POCKET    ; 3
NUM_POCKETS EQU const_value

could become this:

	rsreset
ITEM_POCKET     rb 1 ; 0
BALL_POCKET     rb 1 ; 1
KEY_ITEM_POCKET rb 1 ; 2
TM_HM_POCKET    rb 1 ; 3
NUM_POCKETS EQU _RS

Since EQUS "macros" are faster than full-fledged parameterized MACROs, there could be some convenience definitions involved, like const_def EQUS "rsset" or const EQUS "rb 1" or const_value EQUS "_RS". Thus:

	const_def
ITEM_POCKET     const ; 0
BALL_POCKET     const ; 1
KEY_ITEM_POCKET const ; 2
TM_HM_POCKET    const ; 3
NUM_POCKETS EQU const_value

This would make constant series definitions more similar to our groups of non-sequential EQU definitions, in that the name being defined is always first and unindented.

I don't personally think it's worth breaking the long convention of const definitions, but it's something to consider.

[Rangi42]

Since we sometimes define a const series and an enum series in parallel, even if const_def were replaced with RS commands, enum would still use __enum_value__ and its associated macros.

[mid-kid]

Big +1 on using rsset for structures but I think it hinders readability vs const. Implementing const using rsset is fine, however.
Currently, structures are a mishmash of const and simply EQU, and it's a huge mess.

Can we talk for a hot second about how dumb the const vs enum split is, by the way? The only reason it's necessary is because in some places we need to keep track of two values. However, in constants/map_constants.asm, const isn't even used beyond const_def, and in constants/item_constants.asm, the usage of enum could be worked away into the add_tm/add_hm macros without many changes either. I can't think of any other place where this is done, but in those situations similar macros could be used.
My proposal here is to move the additional functionality of enum into what const does, and only keep const.

[Rangi42]

Yeah, they are redundant. And if add_tm/add_hm/add_mt end up more complex anyway from #738, then keeping the parallel enumeration wholly inside them would be even more appropriate.

[Rangi42]

Related: if const_def gets enum's functionality of having a custom increment value, then when we have constant series that descend from a maximum, they can go in natural order. For example, this:

	const_def -7
	const CREDITS_THEEND
	const CREDITS_WAIT2
	const CREDITS_MUSIC
	const CREDITS_CLEAR
	const CREDITS_SCENE
	const CREDITS_WAIT
	const CREDITS_END

would be more natural as this:

	const_def $ff, -1
	const CREDITS_END
	const CREDITS_WAIT
	const CREDITS_SCENE
	const CREDITS_CLEAR
	const CREDITS_MUSIC
	const CREDITS_WAIT2
	const CREDITS_THEEND

[aaaaaa123456789]

I'll be happy to see structures defined using rs and friends. I don't think it's too great for constants, though — const FOO is extremely clear and obvious.

As for const vs. enum, it'd probably be better to track down the (hopefully few) locations where they are both necessary, solve those problems somehow, and get rid of enum for the rest of the repo.

[Rangi42]

While we're planning to refactor the const/enum definitions, I'd like to add this macro: const_skip.

const_skip: MACRO
if \1 < const_value
fail "Can't skip backwards"
else
const_value = \1
endc
ENDM

This would be useful for constant sequences with large gaps. It avoids needing a bunch of unused placeholder values, but also ties the whole series together under a single const_def, and lets you safely introduce new constants in the gaps without shifting or aliasing over the ones further ahead.

For example, the ; held item effects in constants/item_data_constants.asm could replace the const_def 10, const_def 20, etc, with const_skip 10, const_skip 20, etc.

Also, in constants/event_constants.asm, there are four runs of unused constants: EVENT_0C2 to EVENT_0C7 ($0C8 == 200), EVENT_105 to EVENT_257 ($258 == 600), EVENT_341 to EVENT_3E7 (0x3E8 == 1000), and EVENT_5CC to EVENT_63F ($640 == 1600). I think those could be better expressed with const_skip 200, const_skip 600, etc, with comments like ; skips 339 unused events from $105 to $257. This applies even more to pokered's constants/event_constants.asm, which declares 2,560 EVENT_* constants but only uses 504.

[aaaaaa123456789]

const_skip is misleading — I'd expect const_skip 2 to mean "two values were skipped". The macro is fine, but it's probably better to name it better.

[Rangi42]

How about const_continue?

[Rangi42]

Another const upgrade, as @aaaaaa123456789 mentioned from Prism: const skip would increment the const_value without defining a new symbol. This would work for one-off unused constants like SUBSTATUS_UNKNOWN_1, MAPOBJECT_E and MAPOBJECT_F, etc.

[Rangi42]

Also, the SPRITE constants would be another use case for const_skip (or const_continue or whatever): the Pokémon sprites skip ahead to $80, and the variable sprites to $F0.

[mid-kid]

const_next for setting the value of the "next" sequential entry.
const_skip for incrementing the const_value, not a big fan of special-casing arguments.

[i0brendan0]

Was checking this out through email updates and I personally like const_skip # to skip # many, and then const_skip_to # to skip to # as there might be cases that one or both of them would be used. Just a thought. Obviously you guys might have better ideas.

Shucks. Looks like you guys already decided. I’ll just leave this here I guess.

[Rangi42]

The const_skip and const_next discussion has been implemented in #740. The usage of RS commands is still pending.

[Rangi42]

Something else to consider for refactoring structs: using local labels for their members. So party_struct wTempMon would define wTempMon.Species, wTempMon.Item, etc. This would make it clear that those labels are created via a macro, and "wTempMonSpecies" doesn't appear anywhere in wram.asm.

[aaaaaa123456789]

This seems disruptive for little benefit... while dot notation might be familiar for a class of users, this is something that is very quickly learned, and changing it will break compatibility with every repository using the current format.