Restructure event flags in z64save.h (2/?): Carpenters #2385
Conversation
include/z64save.h
Outdated
| #define GET_EVENTCHKINF_CARPENTERS_FREE_ALL() \ | ||
| CHECK_FLAG_ALL(gSaveContext.save.info.eventChkInf[EVENTCHKINF_INDEX_CARPENTERS_FREE], EVENTCHKINF_CARPENTERS_FREE_MASK_ALL) | ||
| // Carpenters rescued from Gerudo Fortress | ||
| #define EVENTCHKINF_INDEX_CARPENTERS_RESCUED EVENTCHKINF_INDEX(EVENTCHKINF_CARPENTER_0_RESCUED) |
There was a problem hiding this comment.
I understand why you did it this way, but it feels weird that only EVENTCHKINF_CARPENTER_0_RESCUED is used here and not the other carpenter flag IDs. Maybe it would be better to hardcode 0x9 here?
There was a problem hiding this comment.
Personally I prefer hardcoding all of the index values
There was a problem hiding this comment.
oh, why did you decide not to here?
There was a problem hiding this comment.
because I've been indecisive on which way to go. From now on it'll be consistent.
There was a problem hiding this comment.
It'll be consistent with not hardcoding the values?
| s32 EnGe1_CheckCarpentersFreed(void); | ||
| s32 EnGe1_CheckAllCarpentersRescued(void); |
There was a problem hiding this comment.
imo dont even need "check" in this case, "if all carpenters resuced" reads nicely
There was a problem hiding this comment.
same would apply to ge2 if you do change it
There was a problem hiding this comment.
eh, I think it's better as-is. AllCarpentersRescued in a vacuum reads more towards being the event of having rescued all carpenters.
fig02
added
Met Review Requirements
and removed
Needs first approval
Needs second approval
labels
|
even tho this has met merge requirements, there are a couple of comments. please comment and lmk if you plan to address them or not |
Contributions made in this pr are licensed under CC0
Continuation of #2303