[zret] Port some documentation changes #1164
Conversation
|
Personally I'd be happy to have this in. Just curious is there any documentation anywhere on what the |
|
I also definitely welcome more documentation making its way into SoH! |
| @@ -76,7 +76,7 @@ OSPiHandle* osDriveRomInit(void); | |||
| void StackCheck_Init(StackEntry* entry, void* stackTop, void* stackBottom, u32 initValue, s32 minSpace, | |||
| const char* name); | |||
| void StackCheck_Cleanup(StackEntry* entry); | |||
| StackStatus StackCheck_GetState(StackEntry* entry); | |||
| s32 StackCheck_GetState(StackEntry* entry); | |||
There was a problem hiding this comment.
why would we want to use an s32 instead of an enum type here? is this a common pattern in this PR?
There was a problem hiding this comment.
It eventually becomes a u32 in the zret repo, but I'll port that PR later.. I've only ported a few PR's here.
There was a problem hiding this comment.
I want to assume that maybe it was wrong to have been using that enum?
There was a problem hiding this comment.
I want to assume that maybe it was wrong to have been using that enum?
curious as to why it would be wrong, i'd much prefer things to use named types like StackStatus than u32s, but maybe i'm missing something
There was a problem hiding this comment.
I'm not sure. I didn't actually make all these changes 🙂 just porting over documentation PR's from the decomp repo. I can ask the question on the decomp discord but not sure if we want to start in-depth reviewing the changes that come from zret -- especially since we don't have context on them
There was a problem hiding this comment.
We can still use the enums for assignment and array indexes and stuff right? I'd say it's not too big of a loss.
There was a problem hiding this comment.
i'd prefer to follow the opposite rule of "always use types that describe what's being returned", but if that'll cause merge problems when pulling in decomp stuff i guess i can understand following their pattern
I subscribe to the same philosophy -- in this case I was focused on porting PR's. Not necessarily checking them out in huge depth given that this project is based on their repo and there's lots of code to go through.
But I think only @Kenix3 can decide here how in-depth we want to get with reviewing the PR's that come directly from zret and how to deal with the changes that may be requested given that usually the porter is not the author.
There was a problem hiding this comment.
I think what is at issue is a deeper question than just whether we use enums around the codebase as return or argument types.
I think the question is: what do we do when we have a style conflict with what ZRET OOT is doing.
I can tell you that ZRET MM doesn't always match OOT.
On the matter of the specific issue at hand... I see no reason the enum can't be used for our purposes (or even decomp, because you can use it only when matching, but this is a different discussion)
So before we make a decision, I'd like us to discuss and make a decision on if we even want to change documentation from ZRET OOT. In personally in favour of it, both in general and this specific case.
There was a problem hiding this comment.
Yes. I think that’s the big question. Do we want to just take their PR’s as-is or do we want to actually try to take those changes go over them again and modify them to be “better” suited for our port.
Generally I think it’s best to modify things that make sense for our project BUT that will make porting documentation a much bigger task than it already is.. because generally the developer might not actually have a lot of context over what they’re bringing over and then might be expected to address PR review comments on a lot of code they didn’t write. It’s not impossible but I think it largely limits who can make these type of PR’s therefore making it harder for us to benefit from all that documentation.
There was a problem hiding this comment.
From my experience, the only time I've ever had issues with using enum types as parameters or return types is when you have a need to use a value of a different enum type instead. But this seems like a pretty narrow use case, the only time I've ever really run into it is with the get-item-rework PR I've been working on, where I needed some functions that were only expecting GetItemID values to also accept/return RandomizerGet values. Even then if you're in that situation you'd probably prefer that the function wasn't tied to a specific enum by it's return/parameter types.
* zret: Documentation pass on scene/room commands HarbourMasters#1226 * Update OTR scene * zret: Document the lens system HarbourMasters#1079 * zret: Misc. doc/cleanup 4 HarbourMasters#1093 * zret: Fix misc 8 HarbourMasters#1150 * zret: Document Distortions from z_camera and z_view HarbourMasters#1102
* zret: Documentation pass on scene/room commands HarbourMasters#1226 * Update OTR scene * zret: Document the lens system HarbourMasters#1079 * zret: Misc. doc/cleanup 4 HarbourMasters#1093 * zret: Fix misc 8 HarbourMasters#1150 * zret: Document Distortions from z_camera and z_view HarbourMasters#1102
No description provided.