Document z_horse.c and related actors #2278
Conversation
include/regs.h
Outdated
| @@ -118,6 +118,8 @@ | |||
| #define R_MESSAGE_DEBUGGER_TEXTID YREG(79) | |||
| #define R_C_UP_ICON_X YREG(88) | |||
| #define R_C_UP_ICON_Y YREG(89) | |||
| #define R_ENTER_RIDING_HORSE AREG(6) | |||
| #define R_DEBUG_FORCE_EPONA_OBTAINED DREG(1) //If set, overrides EVENTCHKINF_EPONA_OBTAINED state giving Epona | |||
There was a problem hiding this comment.
nit: space between // and If
src/code/z_actor.c
Outdated
| @@ -1130,7 +1131,7 @@ void func_8002DE04(PlayState* play, Actor* actorA, Actor* actorB) { | |||
| actorA->flags &= ~ACTOR_FLAG_13; | |||
| } | |||
|
|
|||
| void func_8002DE74(PlayState* play, Player* player) { | |||
| void Actor_SetCameraHorseSetting(PlayState* play, Player* player) { | |||
There was a problem hiding this comment.
I find the wording of this function name awkward ("set" appears twice, and what's a "camera horse"?). Some suggestions:
Actor_SetHorseCamera
Actor_RequestHorseCamera
Actor_RequestHorseCameraSetting
There was a problem hiding this comment.
Interesting. Player_SetCameraHorseSetting is the MM name for the function, but the MM Camera system names seem out of date.
Actor_RequestHorseCameraSetting might be best, but I'm not confident on word ordering.
src/code/z_horse.c
Outdated
| /** | ||
| * Determines when and where a horse should spawn for the player | ||
| */ | ||
| void Horse_Set(PlayState* play, Player* player) { |
There was a problem hiding this comment.
maybe Horse_Spawn for all of these would be more specific? ditto with e.g. Actor_InitPlayerHorse -> Actor_SpawnHorse or something
There was a problem hiding this comment.
I'm against calling it Actor_SpawnHorse and somewhat against using the word Spawn at all because it doesn't fit the same sort of operation that Actor_Spawn does. With Actor_Spawn, there is a reasonable expectation that the function will always return a new instance to an actor, whereas "Horse_Set" will only create a horse if the rather complex set of conditions to do so are met.
There was a problem hiding this comment.
I think Set does a worse job of capturing "create a horse if the rather complex set of conditions to do so are met" though, compared to Spawn. Maybe something like Horse_TrySpawn?
There was a problem hiding this comment.
perhaps everything here should be called Epona instead of Horse, since IIUC these don't apply to untamed horses or Ingo's horse?
There was a problem hiding this comment.
z_horse.c is an extension of z_en_horse.c, which is both Epona and the unnamed rideable horse with a saddle.
Horse_SetCutscene also contains a condition that spawns either Epona or the saddled horse depending on which one the player chooses to race with (i.e. the line with paramFlag = 0x8000). Because of this, Horse_SetCheck will also affect whether the saddled horse will spawn for the player.
For these reasons, I feel it is cleaner to just continue to refer to the system as Horse rather than picking out specific functions that happen to be Epona only.
| #define EVENTINF_HORSES_05 ((EVENTINF_HORSES_INDEX << 4) | EVENTINF_HORSES_05_SHIFT) | ||
| #define EVENTINF_HORSES_06 ((EVENTINF_HORSES_INDEX << 4) | EVENTINF_HORSES_06_SHIFT) | ||
| // EVENTINF 0x00-0x03 reserved for IngoRaceState | ||
| #define EVENTINF_INGORACE_STATE_MASK (0xF << 0) |
There was a problem hiding this comment.
nit: maybe INGORACE -> INGO_RACE since I think of it as two words (and could remove STATE if it's getting long)
There was a problem hiding this comment.
INGORACE matches the pre-existing spellings INGORACE and MALONRACE used in z_en_horse_game.c, which I mostly left untouched.
There was a problem hiding this comment.
The inconsistency between IngoRaceState and INGORACE_STATE still bugs me but it's whatever
There was a problem hiding this comment.
I would change it now with the understanding that horse_game will also change later. INGORACE as one word is just worse.
src/code/z_horse.c
Outdated
| @@ -41,49 +58,58 @@ typedef struct HorseSpawn { | |||
| /* 0x0A */ s16 type; | |||
| } HorseSpawn; | |||
|
|
|||
| void func_8006D0EC(PlayState* play, Player* player) { | |||
| void Horse_SetNormal(PlayState* play, Player* player) { | |||
There was a problem hiding this comment.
maybe "SpawnGameplay" for the opposite of a "cutscene" spawn? Initially I thought this was talking about normal vectors based on the name
| #define EVENTINF_HORSES_05 ((EVENTINF_HORSES_INDEX << 4) | EVENTINF_HORSES_05_SHIFT) | ||
| #define EVENTINF_HORSES_06 ((EVENTINF_HORSES_INDEX << 4) | EVENTINF_HORSES_06_SHIFT) | ||
| // EVENTINF 0x00-0x03 reserved for IngoRaceState | ||
| #define EVENTINF_INGORACE_STATE_MASK (0xF << 0) |
There was a problem hiding this comment.
The inconsistency between IngoRaceState and INGORACE_STATE still bugs me but it's whatever
Contributions made in this pr are licensed under CC0