Script/Text Pointer Macros for Map Scripts

[vulcandth]

Fixes #376

Was looking through through some of the comments for #270 and noticed that there was a lack of constants for script/text pointers. So I came up with some macros to define a constant and pointer at the same time. This would also allow giving more appropriate names to the text labels. Anyways, this is currently a proof of concept. Please provide feedback. If we reach something we all like, I can apply this to the rest of the map scripts. For now, I just did AgathasRoom and ChampionsRoom.

[dannye]

I like the look of this.

[vulcandth]

@Rangi42 Sorry, hate to ping you; but whenever you have time, no rush! I'd like for your opinion on this as well before I go through the great effort of applying this to ALL of the map scripts. Want to make sure everyone likes what they see.

[Rangi42]

Assuming we come up with meaningful constant names, this looks good.

I'm not sure if the constant should be first or the pointer label. It's an issue that was also brought up in https://github.com/pret/pokecrystal/issues/748 and https://github.com/pret/pokecrystal/issues/755.

[vulcandth]

So I've been slowly working through this PR in my spare time... and I'm starting to worry that it could become rather difficult to review in a single PR.

Here are the main changes I have planned:

1. Rename map _ScriptPointers => _SceneScriptPointers (These scripts functions as scenes)
2. Rename map _TextPointers => _TextScriptPointers (These aren't always just text, and sometimes contain their own scripts.)
3. Introduce the following macros:

MACRO def_scene_script_pointers
\1_SceneScriptPointers:
	const_def
ENDM

MACRO def_text_script_pointers
\1_TextScriptPointers:
	const_def 1
ENDM

MACRO dw_const
	dw \1
	const \2
ENDM

4. Provide proper script pointer / constant labels to ALL of the map scripts.... (This is the big one).. For example:

	def_scene_script_pointers AgathasRoom
	dw_const AgathasRoomDefaultSceneScript,         SCENE_AGATHASROOM_DEFAULT
	dw_const DisplayEnemyTrainerTextAndStartBattle, SCENE_AGATHASROOM_AGATHA_START_BATTLE
	dw_const AgathasRoomAgathaEndBattleSceneScript, SCENE_AGATHASROOM_AGATHA_END_BATTLE
	dw_const AgathasRoomPlayerIsMovingSceneScript,  SCENE_AGATHASROOM_PLAYER_IS_MOVING
	dw_const AgathasRoomStubSceneScript,            SCENE_AGATHASROOM_STUB

Would you find it easier to review if this was broken up into several PRs? (Obviously not all at the same time.)

Like say... PR with the Pointer Renames if you agree with those. Then a PR for introducing the macros and applying them to a couple maps with meaningful labels and constants. Then we would just do a PR with a few maps at a time until they are all done.

This is also all assuming that you think the above is the way to go, and don't think this needs to go in another direction.

[dannye]

So I've been slowly working through this PR in my spare time... and I'm starting to worry that it could become rather difficult to review in a single PR.

We're used to reading large diffs when it's worth it :p

Here are the main changes I have planned:

1. Rename map _ScriptPointers => _SceneScriptPointers (These scripts functions as scenes)

2. Rename map _TextPointers => _TextScriptPointers (These aren't always just text, and sometimes contain their own scripts.)

I don't think this is a good idea. Scripts can execute code and/or print text. Texts can print text and/or execute code. The important difference between them is their default mode; code or text processing. Putting "Script" in each name makes it harder to distinguish while reading. "Scene" also doesn't seem like an appropriate descriptor for all script routines.

Your 3) and 4) sound good to me, aside from simplifying proposed macro names (ie, def_scene_script_pointers -> def_script_pointers).

Would you find it easier to review if this was broken up into several PRs?

Just one PR is good 👍

[Rangi42]

One time I added an object_event with a text ID higher than the bg_event ones, and it broke a sign: https://github.com/Rangi42/redstarbluestar/issues/3 I had to put object events first, then bg events: https://github.com/Rangi42/redstarbluestar/commit/ccb78660456ed4f711552dd873358b2b80c31a18 I'd like to better understand how this works, and ideally add some asserts to guarantee it.

[vulcandth]

One time I added an object_event with a text ID higher than the bg_event ones, and it broke a sign: Rangi42/redstarbluestar#3 I had to put object events first, then bg events: Rangi42/redstarbluestar@ccb7866 I'd like to better understand how this works, and ideally add some asserts to guarantee it.

Looks like what causes this occurs in DisplayTextID::

https://github.com/pret/pokered/blob/6b5be9129cabe72b9f775b02db1bf7e7169153da/home/text_script.asm#L30-L36

The function compares hSpriteIndexOrTextID to wNumSprites. If hSpriteIndexOrTextID is less than or equal to the number of sprites on the map; then it performs .spriteHandling; otherwise it skips to .skipSpriteHandling. Looks like one of the main reasons for doing this.. is sprites need the following:

https://github.com/pret/pokered/blob/6b5be9129cabe72b9f775b02db1bf7e7169153da/home/text_script.asm#L41

Edit: I am still analyzing some of the differences in .spriteHandling and .skipSpriteHandling and how it plays out.

[Rangi42]

Should the def_script_pointers and def_text_pointers macros actually be defining the labels too? A couple reasons why I think maybe they shouldn't:

1) Explicit labels show that a new context has started. Compare:

CeladonGymResetScripts:
	xor a ; SCRIPT_CELADONGYM_DEFAULT
	ld [wJoyIgnore], a
	ld [wCeladonGymCurScript], a
	ld [wCurMapScript], a
	ret

	def_script_pointers CeladonGym
	dw_const CheckFightingMapTrainers,              SCRIPT_CELADONGYM_DEFAULT
	dw_const DisplayEnemyTrainerTextAndStartBattle, SCRIPT_CELADONGYM_START_BATTLE
	dw_const EndTrainerBattle,                      SCRIPT_CELADONGYM_END_BATTLE
	dw_const CeladonGymErikaPostBattleScript,       SCRIPT_CELADONGYM_ERIKA_POST_BATTLE

with:

CeladonGymResetScripts:
	xor a ; SCRIPT_CELADONGYM_DEFAULT
	ld [wJoyIgnore], a
	ld [wCeladonGymCurScript], a
	ld [wCurMapScript], a
	ret

CeladonGym_ScriptPointers:
	def_script_pointers
	dw_const CheckFightingMapTrainers,              SCRIPT_CELADONGYM_DEFAULT
	dw_const DisplayEnemyTrainerTextAndStartBattle, SCRIPT_CELADONGYM_START_BATTLE
	dw_const EndTrainerBattle,                      SCRIPT_CELADONGYM_END_BATTLE
	dw_const CeladonGymErikaPostBattleScript,       SCRIPT_CELADONGYM_ERIKA_POST_BATTLE

2) It's more consistent with other def macros. Like in Gen 2, the *_MapScripts and *_MapEvents labels are explicitly defined for every map. Or the Gen 1 trainers have explicit *TrainerHeaders labels:

CeladonGymTrainerHeaders:
	def_trainers 2
CeladonGymTrainerHeader0:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_0, 2, CeladonGymBattleText2, CeladonGymEndBattleText2, CeladonGymAfterBattleText2
CeladonGymTrainerHeader1:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_1, 2, CeladonGymBattleText3, CeladonGymEndBattleText3, CeladonGymAfterBattleText3
CeladonGymTrainerHeader2:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_2, 4, CeladonGymBattleText4, CeladonGymEndBattleText4, CeladonGymAfterBattleText4
CeladonGymTrainerHeader3:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_3, 4, CeladonGymBattleText5, CeladonGymEndBattleText5, CeladonGymAfterBattleText5
CeladonGymTrainerHeader4:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_4, 2, CeladonGymBattleText6, CeladonGymEndBattleText6, CeladonGymAfterBattleText6
CeladonGymTrainerHeader5:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_5, 2, CeladonGymBattleText7, CeladonGymEndBattleText7, CeladonGymAfterBattleText7
CeladonGymTrainerHeader6:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_6, 3, CeladonGymBattleText8, CeladonGymEndBattleText8, CeladonGymAfterBattleText8
	db -1 ; end

Although speaking of those, they could use local labels to be more terse. Consider:

CeladonGymTrainers:
	def_trainers 2
.Lass17:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_0, 2, CeladonGymBattleText2, CeladonGymEndBattleText2, CeladonGymAfterBattleText2
.Beauty1:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_1, 2, CeladonGymBattleText3, CeladonGymEndBattleText3, CeladonGymAfterBattleText3
.JrTrainerF11:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_2, 4, CeladonGymBattleText4, CeladonGymEndBattleText4, CeladonGymAfterBattleText4
.Beauty2:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_3, 4, CeladonGymBattleText5, CeladonGymEndBattleText5, CeladonGymAfterBattleText5
.Lass18:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_4, 2, CeladonGymBattleText6, CeladonGymEndBattleText6, CeladonGymAfterBattleText6
.Beauty3:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_5, 2, CeladonGymBattleText7, CeladonGymEndBattleText7, CeladonGymAfterBattleText7
.CooltrainerF1:
	trainer EVENT_BEAT_CELADON_GYM_TRAINER_6, 3, CeladonGymBattleText8, CeladonGymEndBattleText8, CeladonGymAfterBattleText8
	db -1 ; end

(This PR doesn't need to update the trainer labels; I can do that separately, but I'd like to hear more opinions on that change.)

3) Script and text labels are different. The *_TextPointers labels do need to follow a consistent naming pattern, since the map_header macro refers to \1_TextPointers (as well as \1_Blocks and \1_Script). But the *_ScriptPointers labels are all referred to locally, like ld de, CeladonGym_ScriptPointers, in the same file as they're defined. (Alongside ld hl, CeladonGymTrainerHeaders.) Not hiding the label name in a macro makes it easier to see the connection between the load and the definition.

[vulcandth]

When I started the PR, I honestly just liked how clean it looked if the macros defined the labels... And a few other minor reasons.

Nowadays ax6 has rubbed off on me a little and I'm more cautious of obscuring stuff too much in macros on the main repos.

So yeah, we can pull the labels out of the macros. I agree with your points.

[vulcandth]

I modified the first post with a task list; Since I plan to return to this shortly. If anyone wants to help.. just mark which maps you plan on working on [Edit my post].. so we are not duplicating work.

[Rangi42]

For all the Silph workers whose text just varies depending on whether you saved Silph, I don't think they need dialog-specific labels; just things like SilphCo7FWorkerM1BeforeText and SilphCo7FWorkerM1AfterText would do. Likewise for other NPCs who just switch between two texts based on an event.

[kqesar]

Hello,

I don't know if it's the right place to ask this but could we merge this PR ?

Merge this PR give the possibility to update our forks and continue to contribute to de-obsucate the code with the new macro of @vulcandth

Actually with the big update of this PR, contribute to help to de-obscucate it is not a good idea :/

Thank you in advance

PS: If my request is not in the right place i will remove my comment

Regards

[vulcandth]

For all the Silph workers whose text just varies depending on whether you saved Silph, I don't think they need dialog-specific labels; just things like SilphCo7FWorkerM1BeforeText and SilphCo7FWorkerM1AfterText would do. Likewise for other NPCs who just switch between two texts based on an event.

I don't have a strong argument against this other than... I don't think it really matters all that much.. and i've already done the work by giving them dialog-specific labels and it be a minor pain to go back and fix them. If you think its best I will of course go back and fix it lol... but I can't say i'm not slightly reluctant lol.

[vulcandth]

Alright, that's all I can spot. I'll do another pass checking for hSpriteIndex/hSpriteIndexOrTextID and w*CurScript to see if there are any more spots we should be using the new constants.

I did a pretty thorough review for this after I finished using several regex's to help identify potential ones I might have missed; but feel free!

[Rangi42]

I mainly want to review the Seafoam Islands and Silph Co names. (At least some are still unidentified, e.g. SeafoamIslands5Script_467a5.)

[Rangi42]

Amazing work, @vulcandth ! :partying_face: