pret / pokecrystal

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

Use named constants for warp IDs? #1097

Open Rangi42 opened 7 months ago

Rangi42 commented 7 months ago

For example, here's how some would look:

; Route32.asm:
    def_warp_events ROUTE_32
    warp_event 11, 73, ROUTE_32_POKECENTER_1F, LEFT
    warp_event  4,  2, ROUTE_32_RUINS_OF_ALPH_GATE, UPPER
    warp_event  4,  3, ROUTE_32_RUINS_OF_ALPH_GATE, LOWER
    warp_event  6, 79, UNION_CAVE_1F

; Route32RuinsOfAlphGate.asm:
    def_warp_events ROUTE_32_RUINS_OF_ALPH_GATE
    warp_event  0,  4, RUINS_OF_ALPH_OUTSIDE, UPPER
    warp_event  0,  5, RUINS_OF_ALPH_OUTSIDE, LOWER
    warp_event  9,  4, ROUTE_32, UPPER
    warp_event  9,  5, ROUTE_32, LOWER

; UnionCave1F.asm:
    def_warp_events UNION_CAVE_1F
    warp_event  5, 19, UNION_CAVE_B1F, A
    warp_event  3, 33, UNION_CAVE_B1F, B
    warp_event 17, 31, ROUTE_33
    warp_event 17,  3, ROUTE_32

And here are the constants those macros would be defining and using:

; Route32.asm:
    def_warp_events ROUTE_32
    ; DEF WARP__ROUTE_32__ROUTE_32_POKECENTER_1F__LEFT EQU 1
    warp_event 11, 73, ROUTE_32_POKECENTER_1F, LEFT ; db WARP__ROUTE_32_POKECENTER_1F__ROUTE_32__LEFT
    ; DEF WARP__ROUTE_32__ROUTE_32_RUINS_OF_ALPH_GATE__UPPER EQU 2
    warp_event  4,  2, ROUTE_32_RUINS_OF_ALPH_GATE, UPPER ; db WARP__ROUTE_32_RUINS_OF_ALPH_GATE__ROUTE_32__UPPER
    ; DEF WARP__ROUTE_32__ROUTE_32_RUINS_OF_ALPH_GATE__LOWER EQU 3
    warp_event  4,  3, ROUTE_32_RUINS_OF_ALPH_GATE, LOWER ; db WARP__ROUTE_32_RUINS_OF_ALPH_GATE__ROUTE_32__LOWER
    ; DEF WARP__ROUTE_32__UNION_CAVE_1F EQU 4
    warp_event  6, 79, UNION_CAVE_1F ; db WARP__UNION_CAVE_1F__ROUTE_32

; Route32RuinsOfAlphGate.asm:
    def_warp_events ROUTE_32_RUINS_OF_ALPH_GATE
    ; DEF WARP__ROUTE_32_RUINS_OF_ALPH_GATE__RUINS_OF_ALPH_OUTSIDE__UPPER EQU 1
    warp_event  0,  4, RUINS_OF_ALPH_OUTSIDE, UPPER ; db WARP__RUINS_OF_ALPH_OUTSIDE__ROUTE_32_RUINS_OF_ALPH_GATE__UPPER
    ; DEF WARP__ROUTE_32_RUINS_OF_ALPH_GATE__RUINS_OF_ALPH_OUTSIDE__LOWER EQU 2
    warp_event  0,  5, RUINS_OF_ALPH_OUTSIDE, LOWER ; db WARP__RUINS_OF_ALPH_OUTSIDE__ROUTE_32_RUINS_OF_ALPH_GATE__LOWER
    ; DEF WARP__ROUTE_32_RUINS_OF_ALPH_GATE__ROUTE_32__UPPER EQU 3
    warp_event  9,  4, ROUTE_32, UPPER ; db WARP__ROUTE_32__ROUTE_32_RUINS_OF_ALPH_GATE__UPPER
    ; DEF WARP__ROUTE_32_RUINS_OF_ALPH_GATE__ROUTE_32__LOWER EQU 4
    warp_event  9,  5, ROUTE_32, LOWER ; db WARP__ROUTE_32__ROUTE_32_RUINS_OF_ALPH_GATE__LOWER

; UnionCave1F.asm:
    def_warp_events UNION_CAVE_1F
    ; DEF WARP__UNION_CAVE_1F__UNION_CAVE_B1F__A EQU 1
    warp_event  5, 19, UNION_CAVE_B1F, A ; db WARP__UNION_CAVE_B1F__UNION_CAVE_1F__A
    ; DEF WARP__UNION_CAVE_1F__UNION_CAVE_B1F__B EQU 2
    warp_event  3, 33, UNION_CAVE_B1F, B ; db WARP__UNION_CAVE_B1F__UNION_CAVE_1F__A
    ; DEF WARP__UNION_CAVE_1F__ROUTE_33 EQU 3
    warp_event 17, 31, ROUTE_33 ; db WARP__ROUTE_33__UNION_CAVE_1F
    ; DEF WARP__UNION_CAVE_1F__ROUTE_32 EQU 4
    warp_event 17,  3, ROUTE_32 ; db WARP__ROUTE_32__UNION_CAVE_1F

(The macros would explicitly check for a "-1" argument to warp to "the last map", like elevators do.)

Pros: People wouldn't get confused about what the number means. Rearranging warps would Just Work automatically.

Cons: More obscure magic going on behind the scenes. Warps to the same map, like in Saffron Gym, would need some kind of exception. (Maybe a warp_event_self x, y, from id, to id?)

Rangi42 commented 7 months ago

Never mind, this also wouldn't neatly handle the case where multiple warps share a destination (e.g. two carpet sides inside a building going to one door outside).

mid-kid commented 7 months ago

I do think it'd be good to have named constants for warps regardless.

mid-kid commented 7 months ago

I think last time, I shot something like this down out of concerns for greppability, I now think that's a non-issue when it comes to this, as you can simply look for the map (script) name.

Rangi42 commented 7 months ago

In that case, let's try and work out how non-injective and self-warps could work.

Currently:

; SafronCity.asm:
    def_warp_events
    warp_event 26,  3, FIGHTING_DOJO, 1
    warp_event 34,  3, SAFFRON_GYM, 1
    ...

; SaffronGym.asm:
    def_warp_events
    warp_event  8, 17, SAFFRON_CITY, 2
    warp_event  9, 17, SAFFRON_CITY, 2
    warp_event 11, 15, SAFFRON_GYM, 18
    warp_event 19, 15, SAFFRON_GYM, 19
    ...
    warp_event 19, 17, SAFFRON_GYM, 3
    warp_event 19,  9, SAFFRON_GYM, 4
    ...
mid-kid commented 7 months ago

I think something like warp_def x, y, source_name, target_map, target_name could work

Rangi42 commented 7 months ago

How about this:

; SafronCity.asm:
    def_warp_events SAFFRON_CITY
    warp_event 26,  3, FIGHTING_DOJO, LEFT
    warp_event 34,  3, SAFFRON_GYM, LEFT
    ...

; SaffronGym.asm:
    def_warp_events SAFFRON_GYM
    warp_event  8, 17, SAFFRON_CITY, LEFT
    warp_event  9, 17, SAFFRON_CITY, RIGHT, LEFT
    warp_event 11, 15, SAFFRON_GYM, PANEL_18, PANEL_3
    warp_event 19, 15, SAFFRON_GYM, PANEL_19, PANEL_4
    ...
    warp_event 19, 17, SAFFRON_GYM, PANEL_3, PANEL_18
    warp_event 19,  9, SAFFRON_GYM, PANEL_4, PANEL_19
    ...

Basically, the def_warp_events macro now takes an arg to save as SOURCE. Then we'd have:

In all those cases, an ID of -1 is used literally and does not cause any definitions. So elevators would work like this:

; GoldenrodDeptStore1F.asm:
    def_warp_events GOLDENROD_DEPT_STORE_1F
    ...
    warp_event  2,  0, GOLDENROD_DEPT_STORE_ELEVATOR, LEFT

; GoldenrodDeptStore2F.asm:
    def_warp_events GOLDENROD_DEPT_STORE_2F
    ...
    warp_event  2,  0, GOLDENROD_DEPT_STORE_ELEVATOR, LEFT

; GoldenrodDeptStoreElevator.asm:
    def_warp_events GOLDENROD_DEPT_STORE_ELEVATOR
    warp_event  1,  3, GOLDENROD_DEPT_STORE_1F, LEFT, -1
    warp_event  2,  3, GOLDENROD_DEPT_STORE_1F, RIGHT, -1
Rangi42 commented 7 months ago

@dannye Thoughts on this system as applied to pokered?

Rangi42 commented 7 months ago

Bikeshedding: Maybe the order should be X, Y, TARGET, TGT_ID, SRC_ID instead. Or even X, Y, SRC_ID, TARGET, TGT_ID?

Rangi42 commented 7 months ago

@vulcandth Curious what you think too, you did all those other pokered map event constants.

mid-kid commented 7 months ago

I prefer explicit over implicit in this case. I don't think it's gonna be clear to the casual eye what the 3-arg and 4-arg variants do. (And map scripts are prime material for casuals to wander into)

Instead, I'd consider whether we should follow the example of the object event defines at the top of the file.

Rangi42 commented 7 months ago

Okay, interesting. Because previously I've proposed the opposite, to define object event constants along with their events, not at the opposite end of the file.

Could you outline how you see the Route32+Route32RuinsOfAlphGate+UnionCave1F example working that way?

Rangi42 commented 7 months ago

(Despite all the above tinkering, I still kind of want to just stick with the current raw-number system for its simplicity, even if it takes more manual counting to work with.)

Rangi42 commented 4 months ago

I think something like warp_def x, y, source_name, target_map, target_name could work

@mid-kid I think you're right. Also in chat recently I was liking the idea of lowercase map identifiers, otherwise it's too much SHOUTING on one line.

So the above example would be:

; Route32.asm:
    def_warp_events ROUTE_32
    warp_event 11, 73, pokecenter, ROUTE_32_POKECENTER_1F, left
    warp_event  4,  2, alph_top, ROUTE_32_RUINS_OF_ALPH_GATE, top_right
    warp_event  4,  3, alph_bottom, ROUTE_32_RUINS_OF_ALPH_GATE, bottom_right
    warp_event  6, 79, union_cave, UNION_CAVE_1F, route_32

; Route32RuinsOfAlphGate.asm:
    def_warp_events ROUTE_32_RUINS_OF_ALPH_GATE
    warp_event  0,  4, top_left, RUINS_OF_ALPH_OUTSIDE, route_32_top
    warp_event  0,  5, bottom_left, RUINS_OF_ALPH_OUTSIDE, route_32_bottom
    warp_event  9,  4, top_right, ROUTE_32, alph_top
    warp_event  9,  5, bottom_right, ROUTE_32, alph_bottom

; UnionCave1F.asm:
    def_warp_events UNION_CAVE_1F
    warp_event  5, 19, a, UNION_CAVE_B1F, a
    warp_event  3, 33, b, UNION_CAVE_B1F, b
    warp_event 17, 31, route_33, ROUTE_33, union_cave
    warp_event 17,  3, route_32, ROUTE_32, union_cave

(This format also has an advantage for Polished Map: it could easily distinguish between the old 4-arg and new 5-arg formats, and still be able to Shift+click a warp to open its target map.)

DamienDoury commented 4 months ago

Here is my take:

I think some kind of label system would be a good replacement for warp IDs. Labels could be moved around easily, and are easy to identify, which would help with code readability and code searches.

Here is an example: In AzaleaTown.asm

def_warp_events
AzaleaTown_Warps:
.SlowpokeWellEntrance:
warp_event 31,  7, SlowpokeWellB1F_Warps.Exit
.PokecenterEntrance
warp_event 15,  9, AzaleaPokecenter1F.LeftExit
...

And in SlowpokeWellB1F:

def_warp_events
SlowpokeWellB1F_Warps:
.Exit:
warp_event 17, 15, AzaleaTown_Warps.SlowpokeWellEntrance
...

Feel free to edit this idea to your liking. The label should be converted to an index at compile time, so it stays 100% compatible with vanilla.

Rangi42 commented 4 months ago

Thank you for writing up your proposal!

Basically instead of this:

    def_warp_events <CUR_MAP>
    warp_event <X>, <Y>, <Warp1Name>, <DEST_MAP_1>, <DestWarp1Name>
    warp_event <X>, <Y>, <Warp2Name>, <DEST_MAP_2>, <DestWarp2Name>

You're proposing this:

<CurMap>_Warps:
    def_warp_events
    .<Warp1Name>:
    warp_event <X>, <Y>, <DestMap1>_Warps.<DestWarp1Name>
    .<Warp2Name>:
    warp_event <X>, <Y>, <DestMap2>_Warps.<DestWarp2Name>

This doesn't seem all that different to me from the way object_event constants are defined separately from the events themselves, except this is using labels instead of constants. For instance, we could imagine:

    warp_const_def <CUR_MAP>
    def_warp_events
    warp_const <Warp1Name>
    warp_event <X>, <Y>, <DEST_MAP_1>, <DestWarp1Name>
    warp_const <Warp2Name>
    warp_event <X>, <Y>, <DEST_MAP_2>, <DestWarp2Name>

Or even separate them into two lists, like how object_const_def are at the top and def_object_events at the bottom:

    warp_const_def <CUR_MAP>
    warp_const <Warp1Name>
    warp_const <Warp2Name>

    def_warp_events
    warp_event <X>, <Y>, <DEST_MAP_1>, <DestWarp1Name>
    warp_event <X>, <Y>, <DEST_MAP_2>, <DestWarp2Name>

Separating the constants from the other event data kind of makes sense for object_events, because they have so much data and the lines are too long already; but warp_events wouldn't be all that long, even if done in one line.

(Also, given that the warp_event macro expands to use a map_id, we have to get the all-caps CUR_MAP and DEST_MAP in there somehow; so if the label idea is just to reduce SHOUTING, well, it would still have to put those in caps.)

DamienDoury commented 4 months ago

I think we shouldn't separate the labels and data into 2 lists, as it makes editing warps more prone to errors. By separating them, we are kinda going back to an index system.

For example, if someone is not careful in a big list of warps, they could remove the warp label at index 6 and warp data at index 7.

In my opinion, it's important that the label stays next to the data.

That's why I think

    warp_const_def <CUR_MAP>
    def_warp_events
    warp_const <Warp1Name>
    warp_event <X>, <Y>, <DEST_MAP_1>, <DestWarp1Name>
    warp_const <Warp2Name>
    warp_event <X>, <Y>, <DEST_MAP_2>, <DestWarp2Name>

Is a better solution. Also, words in CAPS is not an issue to me.