Restructure event flags in z64save.h #2303

mzxrules · 2024-11-20T13:31:41Z

Contributions made in this pr are licensed under CC0

I've felt for a while that z64save.h is a bit overwritten, so I made some changes to try to simplify things. _SHIFT and most _MASK defines have been removed, opting to instead create flagIds for all flags and then extracting that information from the flagId itself. The benefit is that the different get/set flag macros can now be used with more flags, and it's a little easier to follow how an event flag is being used.

Feacur

well, I tried to gather some ideas. let's see how it goes

(or for a later PR, yeah: #2303 (comment))

Feacur · 2024-11-21T22:03:54Z

include/z64save.h

-#define INFTABLE_1AD_SHIFT 13
+// INFTABLE 0x199-0x19F
+#define INFTABLE_KAKARIKO_CUCCO_INDEX (INFTABLE_199 >> 4)
+#define INFTABLE_199 0x199


given the other suggestions have been accepted, these might be renamed as

INFTABLE_INDEX_KAKARIKO_CUCCOS INFTABLE_KAKARIKO_CUCCO_*

for context: #2303 (comment)

Feacur · 2024-11-21T22:41:18Z

include/z64save.h

+#define INFTABLE_1A6 0x1A6
+#define INFTABLE_1A7 0x1A7
+#define INFTABLE_1A8 0x1A8
+#define INFTABLE_1A9 0x1A9 // different


why different though? it's still a gMapDungeonEntranceIconTex, just not overworld (?)

this special case of SCENE_ZORAS_FOUNTAIN still would use INFTABLE_OVERWORLD_ENTRANCE_ICON_INDEX. but as it is not the overworld (?) probably a more fitting name would be INFTABLE_INDEX_DUNGEON_ICONS or something?

then almost all of the INFTABLE_1A* can be INFTABLE_DUNGEON_ICON_OW_*

and INFTABLE_1A9 in its turn an INFTABLE_DUNGEON_ICON_IN_ZORAS_FOUNTAIN

for context: #2303 (comment)

It's marked "different" because it's accessed differently than the rest, and hadn't figured out if it should still be associated with the rest of the flags, but you're right in that it's an gMapDungeonEntranceIconTex all the same.

The way the system works is that when you're on the overworld, the minimap will display an icon for a dungeon? entrance when the appropriate flag is set. However, Bottom of the Well, Great Deku Tree, Jabu Jabu become inaccessible as an adult, so there's logic to prevent displaying the icon despite the flag being set. INFTABLE_1A9 is the Ice Cavern entrance. It's hardcoded in because there's already a record for Zora's Fountain (the Jabu Jabu entrance), plus it is to be displayed for both child and adult since the entrance isn't obstructed (though it is unreachable as child).

The name OVERWORLD_ENTRANCE_ICON comes from sOwEntranceIconPosX, sOwEntranceIconPosY, sOwEntranceFlag in map_data. I feel it's fine for the name to follow this naming convention for now.

got it. appreciate the explanation 🌞

Feacur · 2024-11-21T22:43:21Z

include/z64save.h

+
+
+// INFTABLE 0x1A0-0x1AF
+#define INFTABLE_OVERWORLD_ENTRANCE_ICON_INDEX (INFTABLE_1A0 >> 4)


maybe the suffix _INDEX should be attached to INFTABLE itself? like INFTABLE_INDEX_*

currently the name suggests it's an icon index, which it is not

the same is applicable to all the other instances of any table; an not only "index", but _MASK too. this way their relation with GET_EVENTCHKINF_VAR() and GET_EVENTCHKINF_MASK() should be clearer

this will definitely break, say

#define EVENTCHKINF_40_INDEX #define EVENTCHKINF_40_MASK #define EVENTCHKINF_40

into unaligned list of

#define EVENTCHKINF_INDEX_40S // as in plural, where applicable #define EVENTCHKINF_MASK_40 #define EVENTCHKINF_40 // singular, unlike encompassing "index"

but I'm personally ok with that; they will remain searchable by name just the same

different opinions are welcome, though; I'm not well literate with the codebase and the direction it should move

Idk. I can get behind EVENTCHKINF_INDEX_*, but EVENTCHKINF_MASK_* seems strange to do since the mask should be associated stronger to the flagId name, not EVENTCHKINF.

Feacur · 2024-11-21T22:44:31Z

include/z64save.h

-#define GET_EVENTINF(flag) (gSaveContext.eventInf[(flag) >> 4] & (1 << ((flag) & 0xF)))
-#define SET_EVENTINF(flag) (gSaveContext.eventInf[(flag) >> 4] |= (1 << ((flag) & 0xF)))
-#define CLEAR_EVENTINF(flag) (gSaveContext.eventInf[(flag) >> 4] &= ~(1 << ((flag) & 0xF)))
+#define ENMU_RESET_TALK_FLAGS() \


maybe these defines should keep EVENTINF_ in their names as a prefix?

Feacur · 2024-11-21T23:05:20Z

include/z64save.h

-    | EVENTCHKINF_CARPENTERS_FREE_MASK(3)    )
-#define GET_EVENTCHKINF_CARPENTERS_FREE_ALL() \
-    CHECK_FLAG_ALL(gSaveContext.save.info.eventChkInf[EVENTCHKINF_CARPENTERS_FREE_INDEX], EVENTCHKINF_CARPENTERS_FREE_MASK_ALL)
+#define EVENTCHKINF_CARPENTERS_FREED_INDEX (EVENTCHKINF_CARPENTER_0_FREED >> 4)


given the other suggestions have been accepted, these might be renamed as

EVENTCHKINF_INDEX_FREED_CARPENTERS EVENTCHKINF_FREED_CARPENTER_*

for context: #2303 (comment)

fig02 · 2024-11-21T23:22:22Z

I think ideally this PR would be solely for restructuring flags and doesnt include any renames/documentation.

Dragorn421

I agree it would be best for docs to be split from restructuring

include/z64save.h

Dragorn421 · 2024-11-22T11:23:39Z

include/z64save.h

+ * Generally, a flag will be assigned a unique id, ranging from 0x00-0xDE. Flag state can be individually accessed
+ * or modified by passing the flag id into GET_EVENTCHKINF, SET_EVENTCHKINF, and CLEAR_EVENTCHKINF
+ *
+ * In some instances where a set of flags share a common accociation, the eventChkInf variable will be accessed directly.


Suggested change

* In some instances where a set of flags share a common accociation, the eventChkInf variable will be accessed directly.

* In some instances where a set of flags share a common association, the eventChkInf entry will be accessed directly.

should GET_EVENTCHKINF_VAR not be called VAR then?

I think these can be called "page". As in each page has 16 flags in it.
But if that isnt popular, yeah I like "entry" over "variable".

tbh though even entry can make it sound like you mean a single flag. page is pretty nice at conveying that its a grouping of many things, like many words on a page (or flags in a page in this case)

I found the word that should be used here and that I didn't think of when reviewing, "element"
as in "an array has elements"

I really dislike using page here because it doesn't fit well to the existing definitions of a page. If we're talking data sizes, I wouldn't expect a page to be smaller than a single word.

I guess I also dislike entry/element in that the big picture idea is that eventChkInf, itemGetInf, infTable, eventInf should be bit arrays and that we should prioritize accessing them as such; in a bit array, one bit is one element in the array.

i looked to see if there was some special name for the type used to hold the bit array, but the only implementation I found just called it "CHAR", the type used to contain the bits.

I dont see why page has to be a word in length or why size has to do with anything at all. its just a higher level name for the grouping of each sets of flags, where each index is a page number into a "book" of flags

One of the definitions for page relates to it's size as a unit of memory. Merriam-Webster lists one definition of page as "a sizable subdivision of computer memory". In 6502 assembly for example, one page is a 256 byte aligned block of memory, with the "zero page" memory being located at memory $0000 to $00FF inclusive. A halfword of data isn't exactly a "sizable subdivision of memory".

More than that though, I've never heard of an array being referred to a book of pages (or a group of flags being a page for that matter), and I don't think the analogy makes much sense. One of the limitations of a physical book is that there is a limit to the number of pages that can be viewed at a given time. The memory that an event flag is located at is always freely accessible (to the graph thread at least).

I think its pretty clear that the context is not talking about page as a unit or subdivision of memory.

You dont even have to think about it as a book-- if you were to visualize these flags with a menu for example (like gz does) you can certainly see them as different pages that you have to click/scroll through. It just gives a physical name we can reference these as, instead of coding constructs like "elements of an array" or "variables".

I don't see anything wrong with referring to coding constructs in coding construct terms, and I'd prefer it that way because ideally we should AVOID exposing the underlying variables whenever possible, and make the GET/SET/CLEAR flag macros the main way you retrieve data from the bit arrays. It's just that with decomp we are just forced expose more than this because we can't change codegen.

Your argument also doesn't convince me. Yes, if you're creating a UI it makes sense to think of your different views as being different pages in a document. But we're talking about naming the underlying data itself, and I don't find it to be very "page-like" for the reasons I've already given. Like in gz or flg_set.c you might have an editor that flips through the variables individually for editing, but you could just as well display more than one variable at a time. Then what? Are they still pages? Do you describe the view as being a page of pages?

Dragorn421 · 2024-11-22T11:24:08Z

include/z64save.h

+ * or modified by passing the flag id into GET_EVENTCHKINF, SET_EVENTCHKINF, and CLEAR_EVENTCHKINF
+ *
+ * In some instances where a set of flags share a common accociation, the eventChkInf variable will be accessed directly.
+ * When this is the case, an EVENTCHKINF_*_INDEX constant is defined for accessing a specific eventChkInf variable.


Suggested change

* When this is the case, an EVENTCHKINF_*_INDEX constant is defined for accessing a specific eventChkInf variable.

* When this is the case, an EVENTCHKINF_*_INDEX constant is defined for accessing a specific eventChkInf entry.

Dragorn421 · 2024-11-22T11:25:12Z

include/z64save.h

+#define EVENTCHKINF_00_UNUSED 0x00
+#define EVENTCHKINF_01_UNUSED 0x01


I wouldn't add defines for unused flags at all

EVENTCHKINF_00_UNUSED and EVENTCHKINF_01_UNUSED are set when initializing the debug save in z_sram.c, so they need to be defined to locate where they are used.

oh so "unused" in that sense gotcha
maybe add a comment or don't call them "unused", that's confusing imo

fig02 · 2024-11-22T19:12:04Z

include/z64save.h

-#define ITEMGETINF_1A_MASK (1 << ITEMGETINF_1A_SHIFT)
-#define ITEMGETINF_18 ((ITEMGETINF_18_19_1A_INDEX << 4) | ITEMGETINF_18_SHIFT)
-#define ITEMGETINF_19 ((ITEMGETINF_18_19_1A_INDEX << 4) | ITEMGETINF_19_SHIFT)
-#define ITEMGETINF_1A ((ITEMGETINF_18_19_1A_INDEX << 4) | ITEMGETINF_1A_SHIFT)
+// ITEMGETINF 0x18-0x1A
+#define ITEMGETINF_GREAT_FAIRY_ITEM_INDEX (ITEMGETINF_FARORES_WIND >> 4)
+#define ITEMGETINF_FARORES_WIND 0x18
+#define ITEMGETINF_DINS_FIRE 0x19
+#define ITEMGETINF_NAYRUS_LOVE 0x1A


Going to bump my comment again: Separating documentation out to a different PR would make this easier to review and make this easier to merge quicker.

mzxrules added 3 commits November 20, 2024 05:49

Restructure save flags

dae30ac

update Sram_InitDebugSave

b26ccfb

minor fix

7254073

fig02 added Needs first approval Needs second approval labels Nov 20, 2024

Feacur reviewed Nov 21, 2024

View reviewed changes

Dragorn421 reviewed Nov 22, 2024

View reviewed changes

fig02 reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure event flags in z64save.h #2303

Restructure event flags in z64save.h #2303

mzxrules commented Nov 20, 2024

Feacur left a comment •

edited

Loading

Feacur Nov 21, 2024 •

edited

Loading

Feacur Nov 21, 2024 •

edited

Loading

mzxrules Nov 22, 2024

Feacur Nov 22, 2024

Feacur Nov 21, 2024

mzxrules Nov 22, 2024

Feacur Nov 21, 2024

Feacur Nov 21, 2024 •

edited

Loading

fig02 commented Nov 21, 2024

Dragorn421 left a comment

Dragorn421 Nov 22, 2024

mzxrules Nov 22, 2024

fig02 Nov 22, 2024

fig02 Nov 22, 2024

Dragorn421 Nov 22, 2024

mzxrules Nov 23, 2024

fig02 Nov 23, 2024

mzxrules Nov 25, 2024

fig02 Nov 25, 2024

mzxrules Nov 26, 2024

Dragorn421 Nov 22, 2024

Dragorn421 Nov 22, 2024

mzxrules Nov 22, 2024

Dragorn421 Nov 22, 2024

fig02 Nov 22, 2024



		// INFTABLE 0x1A0-0x1AF
		#define INFTABLE_OVERWORLD_ENTRANCE_ICON_INDEX (INFTABLE_1A0 >> 4)

	* In some instances where a set of flags share a common accociation, the eventChkInf variable will be accessed directly.
	* In some instances where a set of flags share a common association, the eventChkInf entry will be accessed directly.

	* When this is the case, an EVENTCHKINF_*_INDEX constant is defined for accessing a specific eventChkInf variable.
	* When this is the case, an EVENTCHKINF_*_INDEX constant is defined for accessing a specific eventChkInf entry.

		#define EVENTCHKINF_00_UNUSED 0x00
		#define EVENTCHKINF_01_UNUSED 0x01

Restructure event flags in z64save.h #2303

Are you sure you want to change the base?

Restructure event flags in z64save.h #2303

Conversation

mzxrules commented Nov 20, 2024

Feacur left a comment • edited Loading

Choose a reason for hiding this comment

Feacur Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Feacur Nov 21, 2024 • 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

Feacur Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

fig02 commented Nov 21, 2024

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

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

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

Feacur left a comment •

edited

Loading

Feacur Nov 21, 2024 •

edited

Loading

Feacur Nov 21, 2024 •

edited

Loading

Feacur Nov 21, 2024 •

edited

Loading