Document Player's Face and z_actor FaceChange functions #1777

engineer124 · 2024-12-23T01:10:08Z

This PR does a few things:

Documents enums for various things related to links face
Documents eyes and mouth textures
Documents the FaceChange system in z_actor
Changes FaceChange functions to use a struct instead of an array (looks much nicer)
Introduce an AVOID_UB for player's eyes and mouth textures similar to OoT.

There's a few questions/things to address in this PR still, so I'll be doing a self review to address some points, and I'd be open to others feedback.

See this PR for reference:
zeldaret/oot#1928

engineer124 · 2024-12-23T01:10:39Z

src/code/z_player_lib.c

+    // PLAYER_FORM_DEKU
+    {
+        NULL, // PLAYER_EYES_OPEN
+        NULL, // PLAYER_EYES_HALF
+        NULL, // PLAYER_EYES_CLOSED
+        NULL, // PLAYER_EYES_RIGHT
+        NULL, // PLAYER_EYES_LEFT
+        NULL, // PLAYER_EYES_UP
+        NULL, // PLAYER_EYES_DOWN
+        NULL, // PLAYER_EYES_WINCING


The biggest question is, in Player_DrawImpl, the eye and mouth textures are always placed on segment 8/9, even if it’s not read from.
For example, for the deku transformation, the data at gLinkHumanEyesOpenTex inside object_link_nuts has segmant 8 pointing to it. However, that’s just vtx data at that location. However, gsDPLoadTextureBlock(0x08000000, is never called from the head DL of the deku form, so it doesn’t cause any issues.
The question is, is putting NULL here okay to signify that this data isn’t read from. i.e does
gSPSegment(&gfx[0], 0x08, Lib_SegmentedToVirtual(NULL)); break anything?

As far as I know, gSPSegment(&gfx[0], 0x08, Lib_SegmentedToVirtual(NULL)); should be harmless

engineer124 · 2024-12-23T01:12:49Z

include/z64player.h

+    /* 3 */ PLAYER_EYES_RIGHT,
+    /* 4 */ PLAYER_EYES_LEFT,


In OoT, PLAYER_EYES_RIGHT and PLAYER_EYES_LEFT are swapped but it’s the same texture as MM. So I think it’s wrong in OoT and swapped. But it’s also a matter of perspective too. Could someone else verify which is left and which is right?

We name assets from the actor's perspective.
Just checked and the eyes textures do follow the actor's pov

Might want to bring this up so OoT is aware they might have the eyes swapped.

engineer124 · 2024-12-23T01:13:01Z

include/z64player.h

+typedef struct PlayerFaceIndices {
+    /* 0x0 */ u8 eyeIndex;
+    /* 0x1 */ u8 mouthIndex;
+} PlayerFaceIndices; // size = 0x2


In OoT, they opted to use an enum and a 2D array instead of a struct. i.e.

typedef enum PlayerFacePart { /* 0 */ PLAYER_FACEPART_EYES, /* 1 */ PLAYER_FACEPART_MOUTH, /* 2 */ PLAYER_FACEPART_MAX } PlayerFacePart;

I like the struct version better, but open to opinions

I agree the struct looks nicer.

I would agree that the struct looks nicer, but do you know if this option was discussed in OoT? I would much prefer to have the same implementation for this.

engineer124 · 2024-12-23T01:13:59Z

include/z64player.h

+    /* 0x1 */ u8 mouthIndex;
+} PlayerFaceIndices; // size = 0x2
+
+typedef enum PlayerEyes {


In OoT, they also opted to not use a macro to access the eyes and mouth data from animations i.e. they don’t use GET_EYE_INDEX_FROM_JOINT_TABLE, GET_MOUTH_INDEX_FROM_JOINT_TABLE, and GET_APPEARANCE_FROM_JOINT_TABLE. Just noting that here.

engineer124 · 2024-12-23T01:14:14Z

src/code/z_actor.c

+ * @param blinkIntervalRandRange  The range for a random number of frames that can be added to `blinkIntervalBase`
+ * @param blinkDuration  The number of frames it takes for a single blink to occur
+ */
+s16 FaceChange_UpdateBlinkingAlt(FaceChange* faceChange, s16 blinkIntervalBase, s16 blinkIntervalRandRange,


I need help coming up with a better name for FaceChange_UpdateBlinkingAlt. The only difference between this function and FaceChange_UpdateBlinking is the condition to catch where the “eyes half open” face is needed. i.e. } else if (faceChange->timer - blinkDuration == 0) {

Unfortunately, I also can not think of a good name to distinguish them. Kind of as you mentioned, the only difference I see is the length of eyeHalf. The original is for 2 frames, while Alt is for 1 frame.

engineer124 · 2024-12-23T01:14:58Z

src/overlays/actors/ovl_En_Test3/z_en_test3.c

+    // The first 6 faces defined must be default blinking faces. See relevant code in `Player_UpdateCommon`.
+    { PLAYER_EYES_OPEN, PLAYER_MOUTH_CLOSED },   // PLAYER_FACE_NEUTRAL
+    { PLAYER_EYES_HALF, PLAYER_MOUTH_CLOSED },   // PLAYER_FACE_NEUTRAL_BLINKING_HALF
+    { PLAYER_EYES_CLOSED, PLAYER_MOUTH_CLOSED }, // PLAYER_FACE_NEUTRAL_BLINKING_CLOSED


I was unsure whether to reuse the player face enums/structs for Kafei or to make a near identical copy for kafei. There is nothing specific tying these together.

I think I'm inclined to just reuse the enums/structs. Don't really have any motivation other than I don't really see why we need to duplicate them.

engineer124 · 2024-12-23T01:15:05Z

src/overlays/actors/ovl_En_Test3/z_en_test3.c

-static KafeiFace sFaceExpressions[] = {
-    { 0, 0 }, { 1, 0 }, { 2, 0 }, { 0, 0 }, { 1, 0 }, { 2, 0 }, { 4, 0 }, { 5, 1 }, { 7, 2 }, { 0, 2 },
-    { 3, 0 }, { 4, 0 }, { 2, 2 }, { 1, 1 }, { 0, 2 }, { 0, 0 }, { 0, 0 }, { 0, 0 }, { 0, 0 }, { 0, 0 },
+static PlayerFaceIndices sKafeiFaces[PLAYER_FACE_MAX] = {


Similary, for the face data, sKafeiFaces is entirely identical to OoT’s sPlayerFaces, but MM’s sPlayerFaces has just 1 value changed in the last entry of sPlayerFaces.

engineer124 · 2024-12-23T01:15:24Z

src/overlays/actors/ovl_En_Toto/z_en_toto.c

    EnToto* this = (EnToto*)thisx;
    s32 pad;

    OPEN_DISPS(play->state.gfxCtx);

    Gfx_SetupDL25_Opa(play->state.gfxCtx);
-    gSPSegment(POLY_OPA_DISP++, 0x08, Lib_SegmentedToVirtual(sp4C[this->blinkInfo.eyeTexIndex]));
+    gSPSegment(POLY_OPA_DISP++, 0x08, Lib_SegmentedToVirtual(eyeTextures[this->faceChange.face]));


It’s worth noting that toto for some reason uses the face change system and FaceChange_UpdateBlinkingAlt to handle it’s blinking. The only time it’s used outside of player. So I’m not sure if that should affect the name of FaceChange.

I don't think so. Name seems fine.

AngheloAlf · 2024-12-23T01:24:49Z

include/z64player.h

+typedef struct PlayerFaceIndices {
+    /* 0x0 */ u8 eyeIndex;
+    /* 0x1 */ u8 mouthIndex;
+} PlayerFaceIndices; // size = 0x2


I agree the struct looks nicer.

AngheloAlf · 2024-12-23T01:29:23Z

include/z64player.h

+    /* 3 */ PLAYER_EYES_RIGHT,
+    /* 4 */ PLAYER_EYES_LEFT,


We name assets from the actor's perspective.
Just checked and the eyes textures do follow the actor's pov

AngheloAlf · 2024-12-23T01:37:50Z

src/code/z_player_lib.c

+ * Therefore, the segment will point at gargage data, but this does not cause issues as the data is not read from.
+ */
+#ifndef AVOID_UB
+static TexturePtr sEyeTextures[PLAYER_EYES_MAX] = {


Huh, I didn't knew this happened in MM too, otherwise I would have added this when decompiling player_lib. I guess I didn't notice because most forms doesn't have proper eye sets.

src/code/z_player_lib.c

AngheloAlf · 2024-12-23T01:41:16Z

src/code/z_player_lib.c

+        gLinkGoronEyesOpenTex,      // PLAYER_EYES_OPEN
+        gLinkGoronEyesHalfTex,      // PLAYER_EYES_HALF
+        gLinkGoronEyesClosedTex,    // PLAYER_EYES_CLOSED
+        gLinkGoronEyesSurprisedTex, // PLAYER_EYES_RIGHT
+        NULL,                       // PLAYER_EYES_LEFT
+        NULL,                       // PLAYER_EYES_UP
+        NULL,                       // PLAYER_EYES_DOWN
+        NULL,                       // PLAYER_EYES_WINCING


I don't think goron should have eye textures. Half are NULLs and some others don't even match the offset in the object, which makes this an actual change instead of a shiftability fix

If you look at the xmls, then these do actually match the offsets:

<Texture Name="gLinkGoronEyesOpenTex" Offset="0x0" <Texture Name="gLinkGoronEyesHalfTex" Offset="0x800" <Texture Name="gLinkGoronEyesClosedTex" Offset="0x1000" <Texture Name="gLinkGoronEyesSurprisedTex" Offset="0x1800"

and

<Texture Name="gLinkHumanEyesOpenTex" Offset="0x0" <Texture Name="gLinkHumanEyesHalfTex" Offset="0x800" <Texture Name="gLinkHumanEyesClosedTex" Offset="0x1000" <Texture Name="gLinkHumanEyesRightTex" Offset="0x1800"

And to manage the NULL entries, there's special code in Player_DrawImpl i.e.

if (playerForm == PLAYER_FORM_GORON) { // Goron does not have the eye textures to look in different directions if ((eyeIndex >= PLAYER_EYES_RIGHT) && (eyeIndex <= PLAYER_EYES_DOWN)) { eyeIndex = PLAYER_EYES_OPEN; } else if (eyeIndex == PLAYER_EYES_WINCING) { // Goron form puts a surpised expression where the eyes-right normally goes eyeIndex = PLAYER_EYES_RIGHT; } }

which maps all 8 eyeIndex values to valid eyeIndex values:

PLAYER_EYES_OPEN -> PLAYER_EYES_OPEN (gLinkGoronEyesOpenTex) PLAYER_EYES_HALF -> PLAYER_EYES_HALF (gLinkGoronEyesHalfTex) PLAYER_EYES_CLOSED -> PLAYER_EYES_CLOSED (gLinkGoronEyesClosedTex) PLAYER_EYES_RIGHT -> PLAYER_EYES_OPEN (gLinkGoronEyesOpenTex) PLAYER_EYES_LEFT -> PLAYER_EYES_OPEN (gLinkGoronEyesOpenTex) PLAYER_EYES_UP -> PLAYER_EYES_OPEN (gLinkGoronEyesOpenTex) PLAYER_EYES_DOWN -> PLAYER_EYES_OPEN (gLinkGoronEyesOpenTex) PLAYER_EYES_WINCING -> PLAYER_EYES_RIGHT (gLinkGoronEyesSurprisedTex)

Oh I see. It is so silly to do half of it normally and to special case the other half.

AngheloAlf · 2024-12-23T01:44:09Z

src/code/z_player_lib.c

+    // PLAYER_FORM_DEKU
+    {
+        NULL, // PLAYER_EYES_OPEN
+        NULL, // PLAYER_EYES_HALF
+        NULL, // PLAYER_EYES_CLOSED
+        NULL, // PLAYER_EYES_RIGHT
+        NULL, // PLAYER_EYES_LEFT
+        NULL, // PLAYER_EYES_UP
+        NULL, // PLAYER_EYES_DOWN
+        NULL, // PLAYER_EYES_WINCING


As far as I know, gSPSegment(&gfx[0], 0x08, Lib_SegmentedToVirtual(NULL)); should be harmless

AngheloAlf · 2024-12-26T13:58:23Z

src/code/z_player_lib.c

+        gLinkGoronEyesOpenTex,      // PLAYER_EYES_OPEN
+        gLinkGoronEyesHalfTex,      // PLAYER_EYES_HALF
+        gLinkGoronEyesClosedTex,    // PLAYER_EYES_CLOSED
+        gLinkGoronEyesSurprisedTex, // PLAYER_EYES_RIGHT
+        NULL,                       // PLAYER_EYES_LEFT
+        NULL,                       // PLAYER_EYES_UP
+        NULL,                       // PLAYER_EYES_DOWN
+        NULL,                       // PLAYER_EYES_WINCING


Oh I see. It is so silly to do half of it normally and to special case the other half.

tom-overton

Looks good, just some stuff in the comments I noticed

tom-overton · 2024-12-27T02:42:04Z

src/code/z_player_lib.c

+ * from human Link's object are used here.
+ *
+ * Note that some player forms do not use the eyes and mouth textures loaded into segments 0x08 and 0x09 respectively.
+ * Therefore, the segment will point at gargage data, but this does not cause issues as the data is not read from.


Suggested change

* Therefore, the segment will point at gargage data, but this does not cause issues as the data is not read from.

* Therefore, the segment will point at garbage data, but this does not cause issues as the data is not read from.

tom-overton · 2024-12-27T04:26:02Z

src/code/z_player_lib.c

-TexturePtr sPlayerMouthTextures[PLAYER_MOUTH_MAX] = {
+/**
+ * Link's eyes and mouth textures are placed at the exact same place in all player form's respective object files.
+ * This allows the array to only contain the symbols for one file and have it apply to both. This is a problem for


Is the "both" in this line an OoT copy-paste? There are more than two object files for the player in MM. Maybe it should be "...apply to all of them?"

engineer124 added 6 commits December 22, 2024 22:28

player face docs

f6376a9

more docs

bcee8b5

cleanup

6bb09c8

toto

564029c

more comments

a5a9bf8

fix bss, names

9b8176d

engineer124 commented Dec 23, 2024

View reviewed changes

engineer124 added documentation Improvements or additions to documentation Needs-second-approval Second approval Needs-first-approval First approval labels Dec 23, 2024

better comment

ee9db14

AngheloAlf reviewed Dec 23, 2024

View reviewed changes

PR, fix comment

1093bdd

AngheloAlf approved these changes Dec 26, 2024

View reviewed changes

AngheloAlf removed the Needs-first-approval First approval label Dec 26, 2024

tom-overton approved these changes Dec 27, 2024

View reviewed changes

AngheloAlf added Merge-ready All reviewers satisfied, just waiting for CI Waiting-for-author Author needs fix to conflicts or address reviews and removed Needs-second-approval Second approval labels Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document Player's Face and z_actor FaceChange functions #1777

Document Player's Face and z_actor FaceChange functions #1777

engineer124 commented Dec 23, 2024 •

edited

Loading

engineer124 Dec 23, 2024 •

edited

Loading

AngheloAlf Dec 23, 2024

engineer124 Dec 23, 2024

AngheloAlf Dec 23, 2024

hensldm Dec 23, 2024

engineer124 Dec 23, 2024

AngheloAlf Dec 23, 2024

hensldm Dec 23, 2024

engineer124 Dec 23, 2024

engineer124 Dec 23, 2024

hensldm Dec 23, 2024

engineer124 Dec 23, 2024

hensldm Dec 23, 2024

engineer124 Dec 23, 2024

engineer124 Dec 23, 2024

hensldm Dec 23, 2024

AngheloAlf Dec 23, 2024

AngheloAlf Dec 23, 2024

AngheloAlf Dec 23, 2024

AngheloAlf Dec 23, 2024

engineer124 Dec 23, 2024

AngheloAlf Dec 26, 2024

AngheloAlf Dec 23, 2024

AngheloAlf Dec 26, 2024

tom-overton left a comment

tom-overton Dec 27, 2024

tom-overton Dec 27, 2024

	* Therefore, the segment will point at gargage data, but this does not cause issues as the data is not read from.
	* Therefore, the segment will point at garbage data, but this does not cause issues as the data is not read from.

Document Player's Face and z_actor FaceChange functions #1777

Are you sure you want to change the base?

Document Player's Face and z_actor FaceChange functions #1777

Conversation

engineer124 commented Dec 23, 2024 • edited Loading

engineer124 Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-overton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineer124 commented Dec 23, 2024 •

edited

Loading

engineer124 Dec 23, 2024 •

edited

Loading