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.

Todos at the end:

• Remove ; person comments
• Ensure all w<Map>CurScript are using the new SCRIPT_* constants. (See Script/Text Pointer Macros for Map Scripts #367 (comment) for the last constant related to this item.)

Consider changes SilphCoWorker text to something like: SilphCo7FWorkerM1BeforeText and SilphCo7FWorkerM1AfterText

[dannye]

I like the look of this.

[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 pret/pokecrystal#748 and pret/pokecrystal#755.

[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 👍

[dannye]

Awesome progress!

[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.

[dannye]

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.

[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!

[dannye]

Final round from me :)

[dannye]

Looks ready to merge to me! @Rangi42 anything else you want to check for?

[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 ! 🥳

[Mord5]

Isn't this guy a Jr Trainer?

[vulcandth]

Isn't this guy a Jr Trainer?

It's based on the sprite. He uses the CooltrainerM sprite.

[Rangi42]

Hmm. Given that we have things like FIGHTINGDOJO_HITMONLEE_POKE_BALL, I'd expect trainers' labels and constants to match their real class. That's already the case for specific characters, e.g. OPP_ERIKA has TEXT_CELADONGYM_ERIKA, despite using SPRITE_SILPH_WORKER_F.

[Rangi42]

All such cases: https://pastebin.com/vb93gijw (There's just one case not in there which already goes this way: in PokemonMansionB1F.asm, an OPP_BURGLAR has TEXT_POKEMONMANSIONB1F_BURGLAR, despite using SPRITE_SUPER_NERD. All the rest are like TEXT_POKEMONMANSION2F_SUPER_NERD, TEXT_CINNABARGYM_SUPER_NERD2, etc.) I've updated #420 with this.