Name numerous event info flags

[JordanLongstaff]

A lot of event info flag names copied over from SoH.

[mzxrules]

I'm concerned that SoH's version of these flags may be modified to match their own code modifications.

[mzxrules]

Is EVENTCHKINF_OBTAINED_KOKIRI_EMERALD even a real flag?

[JordanLongstaff]

This and the other flag were pulled in from SoH. Not that they see much real use, but they're there.

I'll revert this one only because of skepticism considering there's another flag for this, and this flag is never used.

[Feacur]

as an addendum:
i would say that 0x07 and 0x09 are related to that event

// z_door_warp1.c:507
Flags_SetEventChkInf(EVENTCHKINF_07);
Flags_SetEventChkInf(EVENTCHKINF_09);
Item_Give(play, ITEM_KOKIRI_EMERALD);

in one of my fork's branches with some documentation process it looks like that:

[fig02]

If they do exist, where are they referenced? Currently no define exists for them because it is never referenced in code, unless I am missing something.
If it doesnt ever get used in code, how can we know what its name should be?

[Pepe20129]

I can confirm that it isn't a real flag, it isn't even referenced anywhere in SoH either.

[fig02]

Thanks.
This goes back to my comment in my previous review, it would be helpful if each flag is carefully examined for accuracy, instead of just blindly copying from SOH.

[mzxrules]

Is EVENTCHKINF_LEARNED_SARIAS_SONG a real flag?

[Pepe20129]

It is not.

[mzxrules]

I don't like having defines for these

[JordanLongstaff]

Is this a deal-breaker? There are plenty of other shift defines elsewhere in this section.

[mzxrules]

Yep, and I don't particularly like it's usage those cases either.

I feel there's no reason to create defines for these because the values are only ever relevant to exactly one section of code (this file). It just makes the code harder to follow logically for 0 gain.

[Pepe20129]

I think it can make understanding the code easier (a comment would also work).

[mzxrules]

I find this particular name odd. I'm not sure it should necessarily be associated with obtaining the Kokiri Emerald.

[Pepe20129]

It is set when you're given the kokiri emerald and the deku tree dies but it's only referenced by deku tree related stuff so I agree, I think naming it something like EVENTCHKINF_DEKU_TREE_DEAD or EVENTCHKINF_COMPLETED_DEKU_TREE would be better.

[fig02]

In the same vein as the above, it would be good if we can verify all of the names are accurate to the code that they correspond to. For example:

[fig02]

EVENTCHKINF_RETURNED_TO_TEMPLE_OF_TIME_WITH_ALL_MEDALLIONS
The cutscene starting only checks for Spirit and Shadow medallions. Even though it can be considered a check for all medallions, assuming no glitches/sequence breaks, it would still be good to keep this accurate.

It can probably be named after starting the Light Arrow Cutscene, which is what the relevant code is doing anyway

[Feacur]

as an addendum:

// z_demo.c:2415
       LINK_IS_ADULT && !Flags_GetEventChkInf(EVENTCHKINF_C4) &&
       (gEntranceTable[((void)0, gSaveContext.save.entranceIndex)].sceneId == SCENE_TEMPLE_OF_TIME)) {
Flags_SetEventChkInf(EVENTCHKINF_C4);

looks like "if haven't seen this cutscene, mark it and never show again"

[Pepe20129]

Probably something along the lines of EVENTCHKINF_STARTED_LIGHT_ARROW_CUTSCENE would be better.

[fig02]

I think it may be helpful for you and for reviewers if this was split into smaller batches. This way you can check accuracy as you port each flag name over in smaller amounts, and the review process can be easier on people trying to verify your requested changes.

[JordanLongstaff]

Oops. didn't mean to push yet. I only did a few changes.