Document Magic #1199

engineer124 · 2022-04-21T02:18:14Z

This documents the different magic variables and functions in the game, primarily from z_parameter.

A few notes:

There are two variables to store the magicCapacity. One of them is always the true capacity, either 0, 0x30, or 0x60. The second one is the magicCapacity that's "drawn", which exists as a separate variable specifically for when you enter the first scene from a new load, and the magic bar grows from 0 width to the width of the current bar. I went with magicCapacity for the fixed value and magicCapacityDrawn as the drawn value. Other than that first scene load, they're always the same and are used interchangeably throughout the repo in terms of checking for magic capacity. Maybe another name could be magicCapacityTarget for the fixed values and magicCapacity for the drawn values
I'm not sure if the different types of border changes should be encapsulated in the enums or comments, since the enum names are getting long as is
I think the enum names for MagicBarChange could be improved, but I went with a more verbose name to start just to help reviewers understand how magic is working
Some of the comments for the magic bar enums are repeated both where the enum is defined and where the switch is, I wasn't sure which of the two the comments should be. So I put them in both for now

Dragorn421

Great job! 🪄 🎩 🐰

include/z64.h

include/z64save.h

src/code/z_parameter.c

include/z64save.h

fig02 · 2022-04-22T17:41:29Z

include/z64save.h

+    /* 02 */ MAGIC_BAR_CONSUME_NOW_ALT, // Identical behaviour to MAGIC_BAR_CONSUME_NOW. Unused
+    /* 03 */ MAGIC_BAR_CONSUME_LENS, // Lens consumption
+    /* 04 */ MAGIC_BAR_CONSUME_WAIT_PREVIEW, // Sets consume target but waits to consume. Draws yellow magic to target consumption
+    /* 05 */ MAGIC_BAR_ADD // Sets a target to add magic
+} MagicBarChange;


would call the enum ChangeType instead of just "Change". may need to update variables to reflect that.
also normally we would have a common prefix for these values that all fit under the same umbrella, but its a bit weird with add being different from all the other consume types i guess

in general I think we can probably afford to make all the magic related enums just start with MAGIC_ and omit the bar? that goes for the enum above too.
Then we can do MAGIC_CHANGE_CONSUME_NOW and MAGIC_CHANGE_ADD etc, with MAGIC telling you its for the magic system, and CHANGE telling you its for the change type, followed by the actual type name

For your first point:
I also had CHANGE_ in the enum earlier, but then decided to take it out because the names were getting quite long and it seems like some of the names had reduncency in them i.e.

typedef enum MagicBarChange { /* 00 */ MAGIC_BAR_CHANGE_CONSUME_NOW, /* 01 */ MAGIC_BAR_CHANGE_CONSUME_WAIT_NO_PREVIEW, /* 02 */ MAGIC_BAR_CHANGE_CONSUME_NOW_ALT, /* 03 */ MAGIC_BAR_CHANGE_CONSUME_LENS, /* 04 */ MAGIC_BAR_CHANGE_CONSUME_WAIT_PREVIEW, /* 05 */ MAGIC_BAR_CHANGE_ADD } MagicBarChange;

The terms CHANGE_CONSUME and CHANGE_ADD feel redundant, and there's only one that's different than the others ADD (I'm fine with MagicBarChangeType though)

For your second point, I messed around a bit with magic vs. magicBar for many of these names and enums. The reason I leaned more towards MagicBar when I could is because magic felt a bit too general, like magic spell or magic amount, it's hard to articulate. I felt MagicBar was really clear in what it was referring to, does that make sense?

include/z64save.h

engineer124 · 2022-04-23T08:16:56Z

Thoughts on consistently using the Magic_ prefix?
Interface_UpdateMagicBar -> Magic_UpdateBar
Interface_DrawMagicBar -> Magic_DrawBar
I know the magic bar itself is part of the interface system, but it's also part of the magic system.

Also, maybe Magic_ResetMagicBarAction could be simplified to Magic_ResetBar to keep it simple?

engineer124 · 2022-04-24T01:45:36Z

I've made the suggestions from the discord discussion, using the Magic_ prefix and dropping the bar everywhere. I've also changed ACTION -> STATE, and renamed lensMagicDepletionTimer to just magicConsuptionTimer. Other discussions are still open though

include/z64.h

src/code/z_parameter.c

include/z64save.h

src/code/z_parameter.c

Dragorn421

minor things besides Magic_Reset

include/z64save.h

src/code/z_parameter.c

Dragorn421

Great job on this! 🧙 🎈

I'm still not convinced by Magic_Reset but I don't have a strong suggestion

include/functions.h

EllipticEllipsis

Nice job, this is actually comprehensible now. Couple of things.

EllipticEllipsis · 2022-05-14T22:02:12Z

include/z64save.h

+    /* 0x003A */ u8 isMagicAcquired;
    /* 0x003B */ char unk_3B[0x01];
-    /* 0x003C */ u8 doubleMagic;
+    /* 0x003C */ u8 isDoubleMagicAcquired;


I didn't realise this was used extensively in the repo, but now I'm wondering if we want to name variables like this interrogatively... does the is actually do anything here, that the plain past participle doesn't?

I think in this specific instance, doubleMagicAcquired alone can be inferred to be a boolean-like value (it still uses the 0/1 in calculating magicCapacity so it's not a true bool), but as you mentioned, is is just a common and fast way to identify that something is boolean without thinking about it, at least that was my understanding of the convention

Also as a side note, not related to this comment but something I wanted to note:
doubleDefense -> isDoubleDefenseAcquired (or without the is) would be consistent with isDoubleMagicAcquired but I refrained from changing it for now

If we were going by the game it should be "half damage" (or possibly "enhanced defence") anyway:

"I'm going to enhance your \n" COLOR(LIGHTBLUE) "defensive power" COLOR(DEFAULT) "." [...] "Your " COLOR(LIGHTBLUE) "defensive power" COLOR(DEFAULT) " is enhanced!" [...] "Your defensive power has been \n" "enhanced! Damage inflicted by \n" "enemies will be " COLOR(RED) "reduced by half" COLOR(DEFAULT) "."

(I suppose we could say the same thing for magic, actually, it never explicitly says it doubles, although that's obvious from the bar size)

imo this is a case where the common colloquial name makes more sense. no one says half damage or enhanced defense

I had a look into this, since I was curious. I looked at 4 guidebooks (3 English, 1 the official one, 1 Japanese) and the first page of Google results for walkthroughs, including 4 GameFAQs guides. A large majority say one or more of

half damage

enhanced defense

doubled health

and only 2 (of about 20) say double/doubled defense. I was pretty surprised by this myself, since the tiny speedrunning corner of the world does seem to call always call it "double defense", probably because of the alliteration.

Just to add a different perspective, I come from the randomizer community and never heard of any of those terms, it's only ever been "double defense" for me

Other related names in decomp: FAIRY_UPGRADE_HALF_DAMAGE (hi elliptic), gGreatFairyDoubleDefenceCs (hi louis)

I would be okay with normalizing all of that to "half damage", with a comment on the save context struct member like // double defense

"double defense" probably is so popular because it rolls so well off the tongue

src/code/z_parameter.c

include/z64save.h

EllipticEllipsis · 2022-05-14T22:12:49Z

src/code/z_parameter.c

+
+    switch (gSaveContext.magicState) {
+        case MAGIC_STATE_GROW_METER:
+            // Step magicCapacityDrawn to what is magicCapacity


As here: it's basically the same as MAGIC_STATE_FILL, but for the bar itself, right?

In a sense. The way this block is designed is that it just moves the bar drawn to where the true capacity is. In theory, it could be moved wider or smaller (i.e. it doesn't specifically have to grow), it's just that in practice it only grows based on how this case is used

So it could be UPDATE_WIDTH, even.

WIDTH could apply to a lot of things so I think it'd be important to specify meter in there, but yeah I'd be fine with that. Could maybe even be SYNC_METER_WIDTH in the sense that it "sync"s the magicCapacityDrawn to the true magicCapacity just as an idea

Yeah, something along those lines makes sense to me.

src/code/z_parameter.c

Roman971 · 2022-05-16T15:26:07Z

src/overlays/actors/ovl_Boss_Ganon/z_boss_ganon.c

@@ -1191,7 +1191,7 @@ void BossGanon_SetupTowerCutscene(BossGanon* this, GlobalContext* globalCtx) {
        this->csTimer = 0;
        this->csState = 100;
        this->unk_198 = 1;
-        gSaveContext.magic = gSaveContext.unk_13F4;
+        gSaveContext.magic = gSaveContext.magicCapacityDrawn;


magicCapacityDrawn seems misleading since it's used for things other than drawing (like here and in a few other places), and it seems to be closer to the "effective" capacity at a specific point in time (eg. in Magic_RequestChange).
So as you mentioned originally I think it would be better to name this one magicCapacity and the other one something like magicCapacityTarget or magicCapacityMax

I'd be okay with either, but I have a slight preference to the way I have it now but it’s hard to articulate why.

But I interpret it as the point of magicCapacity being to store the “true” value in a sense while magicCapacityDrawn gives flexibility on how the capacity is drawn from 0 -> magicCapacity or the previous magicCapacity -> new magicCapacity. Most checks when comparing against magic capacity seems to be against magicCapacity instead of magicCapacityDrawn, but either work due to them being mostly the same. But I still has a sense that magicCapacity is the true value and calling that magicCapacityTarget or magicCapacityMax seems a bit funny to me.

Sure magicCapacityDrawn is used more to draw, but it also contains info on the magicCapacity which is what it’s used for, and is still in the name, so I don't find it misleading imo.

With the current names, you also get a meaningful distinction between the two variables instead of something like magicCapacityMax and magicCapacity which seems too similar.

There also doesn’t seemed to be a logic as to whether devs used magicCapacity vs magicCapacityDrawn (outside z_parameter) since they basically contain the same info 99.5% of the time and the devs names were awful so I’d understand if they mixed them up a little.

To note, I followed through on my suggestion of changing MAGIC_STATE_GROW_METER to MAGIC_STATE_SYNC_METER_WIDTH, but if these variables were to change, maybe it's be worth changing it to something like MAGIC_STATE_SYNC_CAPACITIES or something

I agree they are both used as the capacity in different instances, but to me it's misleading to have magicCapacityDrawn if it gets used in actual game logic. Personally I would interpret that name as something that is used for drawing on the hud, which means I could change its value without affecting the rest of the game, but that would be a big mistake in this case.

Regarding the names I suggested, it's true that magicCapacity and magicCapacityMax/magicCapacityTarget are pretty similar, but I would argue there is no real way around that since they are used for similar purposes like you mentioned, and we can't fix that on our end unfortunately.

Also, looking at the code again, magicCapacityDrawn is actually used more than magicCapacity, even when only accounting for game logic purposes. Besides debug prints, magicCapacity is only read in one place for case MAGIC_STATE_FILL in Magic_Update, while magicCapacityDrawn is used everywhere else, including when adding magic through Magic_RequestChange. So imo it makes more sense to treat it as the "true" capacity in almost all cases (hence magicCapacity). And maybe we could have the other one as magicFillCapacity since that's only thing it's really used for?

Right, I see what you mean.

There's also the process of obtaining magic and double magic:
For obtaining normal magic,
gSaveContext.magicCapacity = MAGIC_NORMAL_METER;
Then in Interface_Update:

if (gSaveContext.isMagicAcquired && (gSaveContext.magicLevel == 0)) { gSaveContext.magicLevel = gSaveContext.isDoubleMagicAcquired + 1; gSaveContext.magicState = MAGIC_STATE_SYNC_METER_WIDTH;

Which is true because the magicLevel was 0 before, and this magicState is the one that steps magicCapacityDrawn -> magicCapacity (it happens when the hud is hidden so you don't actually visually see the magic bar grown).

And when obtaining double magic:

gSaveContext.magicCapacity = MAGIC_DOUBLE_METER; gSaveContext.magicLevel = 0;

They explicitly set magicLevel to 0 so that the same code in Interface_Update runs i.e. (gSaveContext.magicLevel == 0) passes, and the state becomes MAGIC_STATE_SYNC_METER_WIDTH. to step magicCapacityDrawn -> magicCapacity

In both cases, magicCapacity is acting like a target, and magicCapacityDrawn does feel like the true magic capacity.

So I kinda like:
magicCapacity -> magicCapacityTarget
magicCapacityDrawn -> magicCapacity
as originally had. Then maybe:
MAGIC_STATE_SYNC_METER_WIDTH -> MAGIC_STATE_SYNC_CAPACITY

Thoughts?

Or maybe something like:
MAGIC_STATE_SYNC_METER_WIDTH ->
MAGIC_STATE_STEP_CAPACITY
OR
MAGIC_STATE_UPDATE_CAPACITY

src/code/z_parameter.c

Dragorn421

yay figured it out for real this time
comments, mostly on comments

include/z64save.h

src/code/z_parameter.c

src/overlays/actors/ovl_Bg_Dy_Yoseizo/z_bg_dy_yoseizo.c

src/overlays/gamestates/ovl_file_choose/z_file_choose.c

src/overlays/gamestates/ovl_select/z_select.c

* Magic docs WIP * More docs, first round finished * Better docs * More renaming * Simpler name * Another small adjustment * rm if(1) * Better names again after in-game testing * Change comments * change comment * Big rename based on all the suggestions * Small touch-up * More PR Suggestions * RESTORE_IDLE -> RESET * More docs * Capitalization * PR suggestions * Make declaration consistent * Health_ChangeBy (amount) * PR Suggestions * Missed one * More PR Suggestions * Change comment * Add another clarity comment * Discord discussions on `magicFillTarget` * Comments * grammar * More comment clarity * Another bad comment * PR suggestions, improved comments * One more comment * one more thing * bar -> meter

engineer124 added 10 commits April 19, 2022 10:51

Magic docs WIP

7782db3

More docs, first round finished

b5e6ac8

Better docs

bf2858b

More renaming

bdda1e5

Simpler name

6bd46b2

Another small adjustment

354c594

rm if(1)

e359fce

Better names again after in-game testing

80de240

Change comments

522ee08

change comment

764a102

Dragorn421 reviewed Apr 22, 2022

View reviewed changes

include/z64.h Outdated Show resolved Hide resolved

include/z64save.h Outdated Show resolved Hide resolved

src/code/z_parameter.c Outdated Show resolved Hide resolved

include/z64save.h Outdated Show resolved Hide resolved

fig02 reviewed Apr 22, 2022

View reviewed changes

engineer124 added 4 commits April 24, 2022 00:44

Merge branch 'master' into magic

9125e97

Big rename based on all the suggestions

e2eada8

Merge branch 'master' into magic

f05f8c2

Small touch-up

26adeef

Dragorn421 reviewed Apr 24, 2022

View reviewed changes

engineer124 added 5 commits May 7, 2022 12:48

Merge branch 'master' into magic

bd3174d

More PR Suggestions

0146061

RESTORE_IDLE -> RESET

aa10f47

More docs

fe133ce

Capitalization

9779f9e

Dragorn421 reviewed May 7, 2022

View reviewed changes

PR suggestions

727a5bd

Dragorn421 approved these changes May 7, 2022

View reviewed changes

fig02 reviewed May 7, 2022

View reviewed changes

include/functions.h Outdated Show resolved Hide resolved

engineer124 added 2 commits May 7, 2022 21:33

Make declaration consistent

5919acc

Health_ChangeBy (amount)

d647ba1

fig02 approved these changes May 7, 2022

View reviewed changes

EllipticEllipsis reviewed May 14, 2022

View reviewed changes

Roman971 reviewed May 16, 2022

View reviewed changes

engineer124 added 3 commits May 18, 2022 14:32

Merge branch 'master' into magic

d992fb5

PR Suggestions

2ce2812

Missed one

e0776f5

fig02 approved these changes May 20, 2022

View reviewed changes

engineer124 added 10 commits May 21, 2022 23:13

Merge branch 'master' into magic

a920403

More PR Suggestions

743bb67

Change comment

ad2fbd9

Add another clarity comment

ca12b3d

Merge branch 'master' into magic

6a9468a

Discord discussions on magicFillTarget

b9e8936

Comments

af5d5c9

grammar

34a7451

More comment clarity

72306ba

Another bad comment

c540364

Dragorn421 reviewed May 22, 2022

View reviewed changes

PR suggestions, improved comments

c7225d1

Dragorn421 approved these changes May 22, 2022

View reviewed changes

engineer124 added 3 commits May 23, 2022 12:30

One more comment

a7a0d00

one more thing

2bd92ab

bar -> meter

b34a259

Dragorn421 approved these changes May 23, 2022

View reviewed changes

Roman971 approved these changes May 23, 2022

View reviewed changes

Roman971 merged commit e68f321 into zeldaret:master May 23, 2022

engineer124 deleted the magic branch May 23, 2022 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document Magic #1199

Document Magic #1199

engineer124 commented Apr 21, 2022

Dragorn421 left a comment

fig02 Apr 22, 2022

engineer124 Apr 22, 2022

engineer124 commented Apr 23, 2022

engineer124 commented Apr 24, 2022

Dragorn421 left a comment

Dragorn421 left a comment

EllipticEllipsis left a comment

EllipticEllipsis May 14, 2022

engineer124 May 14, 2022

engineer124 May 14, 2022

EllipticEllipsis May 14, 2022 •

edited

Loading

fig02 May 14, 2022

EllipticEllipsis May 15, 2022 •

edited

Loading

engineer124 May 16, 2022

Dragorn421 May 16, 2022 •

edited

Loading

EllipticEllipsis May 14, 2022

engineer124 May 14, 2022

EllipticEllipsis May 16, 2022

engineer124 May 16, 2022 •

edited

Loading

EllipticEllipsis May 16, 2022

Roman971 May 16, 2022

engineer124 May 18, 2022

engineer124 May 18, 2022

Roman971 May 20, 2022

engineer124 May 21, 2022

engineer124 May 21, 2022

Dragorn421 left a comment

Document Magic #1199

Document Magic #1199

Conversation

engineer124 commented Apr 21, 2022

Dragorn421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineer124 commented Apr 23, 2022

engineer124 commented Apr 24, 2022

Dragorn421 left a comment

Choose a reason for hiding this comment

Dragorn421 left a comment

Choose a reason for hiding this comment

EllipticEllipsis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EllipticEllipsis May 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EllipticEllipsis May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dragorn421 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineer124 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dragorn421 left a comment

Choose a reason for hiding this comment

EllipticEllipsis May 14, 2022 •

edited

Loading

EllipticEllipsis May 15, 2022 •

edited

Loading

Dragorn421 May 16, 2022 •

edited

Loading

engineer124 May 16, 2022 •

edited

Loading