Document EnKendoJs
#1838
Document EnKendoJs
#1838
Conversation
| bool Player_IsBackJumping(PlayState* play) { | ||
| Player* player = GET_PLAYER(play); | ||
|
|
||
| return (player->stateFlags2 & PLAYER_STATE2_80000) && (player->av1.actionVar1 == 2); | ||
| } | ||
|
|
||
| bool func_80122FCC(PlayState* play) { | ||
| bool Player_IsSideJumping(PlayState* play) { | ||
| Player* player = GET_PLAYER(play); | ||
|
|
||
| return (player->stateFlags2 & PLAYER_STATE2_80000) && |
There was a problem hiding this comment.
We are a bit more picky when player docs are involved, like how do you know these variables are related to jumping?
Ideally we would name these functions after we have a good understanding of PLAYER_STATE2_80000 and having it named too (or at least a name candidate).
There's also the usage of av1, which is an union that gets used for many different values depending of many different cases. Its usage is pretty complex, and since pretty much the only thing this function does is checking these two values then it would best to also add a new member to that union, but that implies figuring out the av1.actionVar1 usages in player code that need to be changed to this new member, which can be a massive task and definitely out of the scope of the current PR.
We try to be careful when naming stuff from big systems like Player, because something being named makes other people believe the name to be correct and base their documentation efforts on that info. I'm not saying that your naming is wrong, but just from the information presented in the PR we can't be certain we aren't missing something else and the naming should try to reflect that.
So it may be better to leave these functions alone and maybe mention this suspected behavior in a comment.
Also, if you are interested in Player then you should reach fig, he is the Player expert.
There was a problem hiding this comment.
... On the other hand, these two functions are only used by the actor in question, so I guess naming this may be just fine.
Not sure how to proceed.
I took a look at OoT and these functions don't exist over there. Also the equivalent of our PLAYER_STATE2_80000 seems to be OoT's PLAYER_STATE2_19 (not totally confident but the value at least matches)
There was a problem hiding this comment.
Yeah, if there had been other callers besides the swordsman, I wouldn't have renamed these. I won't deign to go any deeper into renaming or formalizing anything regarding these two actions or the Player actor.
CC @fig02 Would it be better to just add comments rather than rename the functions?
There was a problem hiding this comment.
I took a look at OoT and these functions don't exist over there.
Probably because OoT doesn't have an interactive tutorial requiring Link to do these moves, just signs passively explaining how to do them.
There was a problem hiding this comment.
Opinion: These names seem fine, especially if they are not in OOT we don't have to worry about aligning the naming between the two games and they are so limited in scope.
There was a problem hiding this comment.
I guess it will be fine then
| void EnKendoJs_SetupExpertCourse(EnKendoJs* this) { | ||
| SET_WEEKEVENTREG(WEEKEVENTREG_STARTED_SWORDSMAN_MINIGAME); | ||
| this->timer = 120; | ||
| this->minigameRound = 0; |
There was a problem hiding this comment.
| this->minigameRound = 0; | |
| this->minigameRound = ENKENDOJS_NOVICE_SIDE_JUMP; |
There was a problem hiding this comment.
I'm not using these constants for the expert course. The rounds in the expert course are simply the number of pairs of logs that have been raised, which is 5 in total.
| if (this->unk_290 >= 140) { | ||
| if (this->unk_284 == 5) { | ||
| if (this->timer >= 140) { | ||
| if (this->minigameRound == 5) { |
There was a problem hiding this comment.
| if (this->minigameRound == 5) { | |
| if (this->minigameRound == ENKENDOJS_NOVICE_SWORD_THRUST) { |
There was a problem hiding this comment.
Same here, not for the expert course.
AngheloAlf
added
documentation
isghj5
left a comment
isghj5
left a comment
There was a problem hiding this comment.
Looks pretty solid, here are some thoughts:
4 participants
No description provided.