Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update event macro comments #1558

Merged
merged 22 commits into from
Nov 23, 2021
Merged

Conversation

GriffinRichards
Copy link
Member

Main goal was to update the event macro comments, which in many cases were either missing, using outdated language, or plain incorrect. Ended up down a couple rabbit holes along the way. Notable changes aside from comment updates below:

  • warp macros now handle different numbers of optional arguments. In addition to the original warp MAP, warpId, x, y, the following are now also valid: warp MAP, x, y, warp MAP, warpId, and warp MAP.

  • macros which use a value 0-2 to refer to STR_VAR_# may now also refer directly to the STR_VAR name, i.e. buffermovename 1, MOVE_TACKLE can now also be buffermovename STR_VAR_2, MOVE_TACKLE.

  • A few macros like checkitem and updatemoneybox now have formerly required arguments as optional with default values.

  • Some macros have changed names, either because they were incorrect or inconsistent with their function name. The used ones are listed below

Old name New name
getpricereduction getpokenewsactive
trainerbattlebegin dotrainerbattle
setobjectpriority setobjectsubpriority
resetobjectpriority resetobjectsubpriority
gotoram trywondercardscript
setflashradius setflashlevel
warpteleport2 warpspinenter
warpsootopolislegend warpwhitefade
buffercontesttypestring buffercontestname
  • Documents field_tasks.c
  • Documents some tv.c
  • Adds slot machine id constants
  • Adds PARTY_NOTHING_CHOSEN constant for party menu script functions
  • Adds WARP_ID_NONE for the special warp id to indicate coordinates should be used.
  • StringGetEnd10, StringCopy10, and StringCopy7 now have names indicating what they are limiting and use the corresponding constant.
  • Dropped some unnecessary Overworld_ function name prefixes
  • Clarify "object event sprite" when referring to "virtual objects," the sprites used in place of object events.
  • Removes orphaned declarations for old sub_ functions.

asm/macros/event.inc Outdated Show resolved Hide resolved
@mrgriffin
Copy link
Collaborator

While we're making sweeping event macro changes, maybe it would make sense to provide combined compare + goto_if_*/call_if_* macros in a similar vein to goto_if_set etc*?

It appears that practically** all raw compares in the scripts are immediately followed by a branching operation, using sed to extract the command immediately following compare we have:

pokeemerald$ find data -name '*.inc' | xargs sed -nr '/^\s+compare/{N;s/^\s+(\w+).*\n\s+(\w+).*$/\1 \2/p}' | sort -n | uniq -c
    963 compare call_if_eq
     13 compare call_if_ge
      2 compare call_if_gt
      2 compare call_if_le
     11 compare call_if_lt
     14 compare call_if_ne
   1696 compare goto_if_eq
     56 compare goto_if_ge
     45 compare goto_if_gt
     18 compare goto_if_le
     33 compare goto_if_lt
    116 compare goto_if_ne
     13 compare vgoto_if_eq
      2 compare vgoto_if_ne

An example implementation could be:

.macro goto_if_lt a:req, b, c
.ifnb \c
  compare \a, \b
  goto_if 0, \c
.else
  goto_if 0, \a
.endif
.endm

Which is used as follows:

goto_if_eq VAR_RESULT, TRUE, AbandonedShip_CaptainsOffice_EventScript_CanYouDeliverScanner

@ Equivalent to
compare VAR_RESULT, TRUE
goto_if_eq AbandonedShip_CaptainsOffice_EventScript_CanYouDeliverScanner

* compare also appears in things like case—but I'm not proposing to remove the old compare/branching macros, just streamline their most common use.
** I'm assuming I've missed some examples of, e.g., compare + goto_if_lt + goto_if_gt to form a three-way branch, because otherwise it appears that all compares are immediately followed by a branch.

@GriffinRichards
Copy link
Member Author

GriffinRichards commented Nov 18, 2021

While we're making sweeping event macro changes, maybe it would make sense to provide combined compare + goto_if_*/call_if_* macros in a similar vein to goto_if_set etc*?

It's the kind of thing that had we done this earlier in the project I wouldn't think twice about it, but now reformatting a major portion of the scripts makes me nervous. In any case I'd say yes to at least providing it as an option. I'm traveling today so I'm not at my computer but if it can be used for all instances of goto_if/call_if I'd be more on board. For instance checkflag never gets used in a script because goto_if_set etc. are always used instead. If we can't do it everywhere then I'm less inclined to use it in the scripts because having a core script macro written with two different formats could be confusing.

@mrgriffin
Copy link
Collaborator

mrgriffin commented Nov 18, 2021

if it can be used for all instances of goto_if/call_if I'd be more on board.

I think it might be possible to replace all instances, but I'd be hesitant to break compatibility with scripts people have already written for their hacks, and even more hesitant if Poryscript compiles to goto_if_eq instead of goto_if etc. Although, it's just a simple sed script to migrate, so maybe the compatibility break wouldn't be the worst thing ever?

Regarding all uses of the existing goto_if_*/call_if_*, I have this (grepping for those commands and the line prior, filtering out the divider that occurs between different files, and then the same process of pairing the commands from the start of each line):

pokeemerald$ git grep -h -B1 '\(goto_if\|call_if\)_\(eq\|ne\|lt\|le\|gt\|ge\)' -- data/\*.inc | grep -v -e '--' | sed -nr '/^\s+compare/{N;s/^\s+(\w+).*\n\s+(\w+).*$/\1 \2/p}' | sort -n | uniq -c
    963 compare call_if_eq
     13 compare call_if_ge
      2 compare call_if_gt
      2 compare call_if_le
     11 compare call_if_lt
     14 compare call_if_ne
   1696 compare goto_if_eq
     56 compare goto_if_ge
     45 compare goto_if_gt
     18 compare goto_if_le
     33 compare goto_if_lt
    116 compare goto_if_ne
     13 compare vgoto_if_eq
      2 compare vgoto_if_ne

So, the same results as having checked for "what follows a compare?"

I wouldn't be surprised if GF used a combined command when they wrote their scripts, but I can't confirm whether that's true or not.

@GriffinRichards
Copy link
Member Author

Alright well that does sound like it could be used to replace all direct uses of compare. I don't think this would break compatibility? The macros would still handle the original single argument case the same way, as in your example.

@mrgriffin
Copy link
Collaborator

Oh! Sorry, I misunderstood your suggestion, I thought you meant the macro should only support the 3-argument version. Yeah, I think having an (unused by pret's master) 1-argument option would cover our bases well :)

@GriffinRichards
Copy link
Member Author

GriffinRichards commented Nov 19, 2021

Reformatted the scripts to use single statement conditional jumps (e.g. goto_if_eq VAR_RESULT, TRUE, MyScript).
compare is now only used as a sub-macro command, never directly. The old format compare VAR_RESULT, TRUE followed by goto_if_eq MyScript is still valid. Where this really shines is sequences of comparisons:

-       compare VAR_BRINEY_LOCATION, 1
-	goto_if_eq EventScript_MoveMrBrineyToHouse
-	compare VAR_BRINEY_LOCATION, 2
-	goto_if_eq EventScript_MoveMrBrineyToDewford
-	compare VAR_BRINEY_LOCATION, 3
-	goto_if_eq EventScript_MoveMrBrineyToRoute109
+	goto_if_eq VAR_BRINEY_LOCATION, 1, EventScript_MoveMrBrineyToHouse
+	goto_if_eq VAR_BRINEY_LOCATION, 2, EventScript_MoveMrBrineyToDewford
+	goto_if_eq VAR_BRINEY_LOCATION, 3, EventScript_MoveMrBrineyToRoute109

BuffelSaft pushed a commit to BuffelSaft/pokeemerald that referenced this pull request Nov 22, 2021
@GriffinRichards GriffinRichards merged commit e0fae87 into pret:master Nov 23, 2021
@GriffinRichards GriffinRichards deleted the update-macros branch November 23, 2021 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants