Document Surface Material #1447

engineer124 · 2022-11-24T17:25:14Z

This came up from a conversation in MM but this looks to document the values stored in surfaceTypes[poly->type].data[1] & 0xF which I'm currently calling "MATERIAL", and the values of the sfxOffset based on sequence 0 using the audio extraction PR:
#1236, which I'm calling "SFX_OFFSET"

entry player_child_step_dirt
entry player_child_step_sand
entry player_child_step_stone
entry player_child_step_jabu
entry player_child_step_water
entry player_child_step_water_deep
entry player_child_step_tall_grass
entry player_child_step_lava
entry player_child_step_grass
entry player_child_step_carpet
entry player_child_step_wood
entry player_child_step_bridge
entry player_step_vines
entry player_step_iron_boots
entry player_step_unused
entry player_child_step_ice

These two sets of values (material/sfxOffset) mostly line up, but there are some values that do not.

Dragorn421 · 2022-11-24T20:36:00Z

src/code/z_actor.c

+        actor->sfx = 3;
    } else if (arg1 < 100) {
-        actor->sfx = NA_SE_PL_WALK_CONCRETE - SFX_FLAG;
+        actor->sfx = 2;
    } else {
-        actor->sfx = NA_SE_PL_WALK_SAND - SFX_FLAG;
+        actor->sfx = 1;


Can you elaborate on this change? This doesn't play a sfx?

No, this is confusing to explain, but these values are actually timer states that get fed as seq ioData into seq ioPort, that then triggers seq 0 to change the tempo of the ticking timer. This is all programmed internally in seq 0

https://github.com/engineer124/oot/blob/75ea3511865b441471945afeded76af1501a01d7/src/code/z_actor.c#L2280

I see (more or less)
Could you put a comment until this is documented, maybe something like With ACTOR_FLAG_28 the sfx value is not a sfx id, see func_80030ED8

Even more specifically, they're indices to this table here, which then applies a delay to when the next "tick" is in the timer
https://github.com/MNGoldenEagle/oot/blob/fb5bcb9e5e9bfa5e9735c9a3015aa32d896a1767/assets/sequences/sfxbanks/env.seq.inc#L740-L744
Except the indices are offset by 1 (actor->sfx = 1; // index 0) so that it passes the actor->sfx != 0 check needed to play the sfx. Then the (s8)(actor->sfx - 1) gets the true index

Yeah, I can add a temporary comment. I'll also be tackling this function in the near future as well

Dragorn421 · 2022-11-24T20:50:22Z

src/overlays/actors/ovl_En_Xc/z_en_xc.c

        func_80078914(&this->actor.projectedPos, NA_SE_VO_SK_CRASH);
    } else if (Animation_OnFrame(skelAnime, 26.0f)) {
-        func_80078914(&this->actor.projectedPos, NA_SE_PL_BOUND_GRASS);
+        func_80078914(&this->actor.projectedPos, NA_SE_PL_BOUND + SURFACE_SFX_OFFSET_GRASS);


Is this kind of change necessary? Why not just have NA_SE_PL_BOUND_GRASS ? (idk)

Yeah, this one is debatable and I'm still on the fence for it, but I feel it's good to always treat these player sfx id's as base + materialOffset + ageOffset because that's how it's designed. A huge number of sfxId's are all labelled DUMMY_ in the player bank because while they're all valid sfx, their mainly accessed through their base and offset. Just look at the player sfxId's at 0x80 - 0x8F, 0x90 - 0x9F, 0xA0 - 0xAF. But maybe long term when we properly name all sfxId's, we will be giving them all names based on combinations of base + material offset + age offset

The key thing that bothers me is as you said

these player sfx id's

so I understand it in the context of player, but this is just an actor randomly using a player sfx for its own purpose, it doesn't care all that much about the layout of the player bank?
(I'm on the fence too)

Actually this specific actor is even weirder because it does use SurfaceType_GetSfxOffset so it's a poor example
The point stands for others though

Indeed. These "player sfxIds" are called "player" because the og name for this was "Player Bank". However, as you mentioned, some actors use these for themselves (non-player actors), like walking most commonly, but other sfx as well

This actor specifically (Sheik) uses a hardcoded sfxId/material/offset because it's cutscene based, so it's all predetermined where she'll be walking (is my interpretation)

The actor uses of this kind of concept are

Fishing in Fishing_UpdateLure (who would have guessed):
func_80078914(&D_80B7AF94, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_SAND);

Ganon in BossGanon_HitByLightBall:
Audio_PlayActorSfx2(&this->actor, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_TALL_GRASS);

Nabooru in EnNb_PlayLookRightSFX and EnNb_PlayLookLeftSFX
func_80078914(&this->actor.projectedPos, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_STONE);

Ruto (child) in undocumented territory:
func_80078914(&this->actor.projectedPos, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_JABU);

Sheik in EnXc_SetThrownAroundSFX
func_80078914(&this->actor.projectedPos, NA_SE_PL_BOUND + SURFACE_SFX_OFFSET_GRASS);
func_80078914(&this->actor.projectedPos, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_GRASS);

Saria in undocumented territory:
Audio_PlaySfxGeneral(NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_DIRT, ...)

Talon in EnTa_RunWithAccelerationAndSfx
Audio_PlayActorSfx2(&this->actor, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_DIRT);

Adult Zelda (Cutscenes) in undocumented territory:
Audio_PlayActorSfx2(&this->actor, NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_DIRT);

Dragorn421 · 2022-11-24T20:52:47Z

src/code/z_message_PAL.c

@@ -3170,8 +3170,7 @@ void Message_Update(PlayState* play) {
                    R_TEXTBOX_TEXHEIGHT = 512;
                } else {
                    Message_GrowTextbox(msgCtx);
-                    // TODO: this may be NA_SE_PL_WALK_GROUND - SFX_FLAG, or not, investigate sfxId=0
-                    Audio_PlaySfxIfNotInCutscene(0);
+                    Audio_PlaySfxIfNotInCutscene(NA_SE_NONE);


Do you think this was (un)intended?

There was a textbox growing sound effect in beta (can hear it in some spaceworld era vids)

They likely just put a raw 0 in there to "comment it out" without removing the whole call

It's hard to say, I think Fig hypothesized that these were likely actual sounds in earlier releases (sw97) and that the sounds were too obnoxious or not right, so they put a NONE here to silence this sfx (maybe there was a debug conditional)? But I kinda lean into that kind of concept, so I suspect this was intentional and they were messing around with different sfx at this spot, but the final decision was to not include one

lmao. Beat me to the punch

include/sfx.h

hensldm

LGTM. Do not really have any opinion on using the sfx directly for some actors, instead of adding the surface offset.

src/code/z_actor.c

mzxrules · 2022-11-24T17:38:51Z

src/overlays/actors/ovl_Boss_Mo/z_boss_mo.c

+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_DIRT,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_DIRT,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_DIRT,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_SAND,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_STONE,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_JABU,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_WATER_SHALLOW,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_WATER_DEEP,
+        NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_TALL_GRASS,


Unless you intend to delete the other constants, I don't think it's necessary to split sound effects in cases where they are intended to be called directly.

mzxrules · 2022-11-24T17:53:15Z

src/overlays/actors/ovl_player_actor/z_player.c

@@ -1751,7 +1751,7 @@ void func_808327F8(Player* this, f32 arg1) {
    s32 sfxId;

    if (this->currentBoots == PLAYER_BOOTS_IRON) {
-        sfxId = NA_SE_PL_WALK_HEAVYBOOTS;
+        sfxId = NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_IRON_BOOTS;


Suggested change

sfxId = NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_IRON_BOOTS;

sfxId = NA_SE_PL_WALK_HEAVYBOOTS;

Same point here; NA_SE_PL_WALK_HEAVYBOOTS already expresses NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_IRON_BOOTS

mzxrules · 2022-11-24T17:53:33Z

src/overlays/actors/ovl_player_actor/z_player.c

@@ -1763,7 +1763,7 @@ void func_80832854(Player* this) {
    s32 sfxId;

    if (this->currentBoots == PLAYER_BOOTS_IRON) {
-        sfxId = NA_SE_PL_JUMP_HEAVYBOOTS;
+        sfxId = NA_SE_PL_JUMP + SURFACE_SFX_OFFSET_IRON_BOOTS;


Suggested change

sfxId = NA_SE_PL_JUMP + SURFACE_SFX_OFFSET_IRON_BOOTS;

sfxId = NA_SE_PL_JUMP_HEAVYBOOTS;

mzxrules · 2022-11-24T17:53:44Z

src/overlays/actors/ovl_player_actor/z_player.c

@@ -1775,7 +1775,7 @@ void func_808328A0(Player* this) {
    s32 sfxId;

    if (this->currentBoots == PLAYER_BOOTS_IRON) {
-        sfxId = NA_SE_PL_LAND_HEAVYBOOTS;
+        sfxId = NA_SE_PL_LAND + SURFACE_SFX_OFFSET_IRON_BOOTS;


Suggested change

sfxId = NA_SE_PL_LAND + SURFACE_SFX_OFFSET_IRON_BOOTS;

sfxId = NA_SE_PL_LAND_HEAVYBOOTS;

mzxrules · 2022-11-24T18:04:37Z

include/functions.h

-u32 SurfaceType_GetSfxType(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);
-u16 SurfaceType_GetSfxId(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);
+u32 SurfaceType_GetMaterial(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);
+u16 SurfaceType_GetSfxOffset(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);


Suggested change

u16 SurfaceType_GetSfxOffset(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);

u16 SurfaceType_GetSfxMaterial(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);

It's a personal preference, but I think the name should retain the fact that it selects a material

* material * cleanup * iron boots * climb * more docs * rename * small fix * comments * adjust bug comment * simplify comment

engineer124 added 7 commits November 23, 2022 21:54

material

382514c

cleanup

708062a

iron boots

5e2aff6

climb

4418bea

more docs

16bf172

rename

028b957

small fix

353e5c9

Dragorn421 reviewed Nov 24, 2022

View reviewed changes

Dragorn421 added Needs contributor approval labels Nov 24, 2022

engineer124 added 2 commits November 24, 2022 16:41

comments

f2f0f8f

adjust bug comment

433ffa6

hensldm approved these changes Nov 25, 2022

View reviewed changes

Dragorn421 approved these changes Nov 26, 2022

View reviewed changes

Dragorn421 removed the Needs contributor approval label Nov 26, 2022

fig02 reviewed Nov 26, 2022

View reviewed changes

src/code/z_actor.c Outdated Show resolved Hide resolved

engineer124 added 2 commits November 26, 2022 17:59

merge master

6acc60e

simplify comment

2efe3ec

fig02 approved these changes Nov 26, 2022

View reviewed changes

fig02 merged commit c7a61aa into zeldaret:master Nov 26, 2022

engineer124 deleted the material branch November 26, 2022 23:11

mzxrules reviewed Nov 27, 2022

View reviewed changes

louist103 pushed a commit to louist103/oot that referenced this pull request Jan 3, 2023

Document Surface Material (zeldaret#1447)

1961507

* material * cleanup * iron boots * climb * more docs * rename * small fix * comments * adjust bug comment * simplify comment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document Surface Material #1447

Document Surface Material #1447

engineer124 commented Nov 24, 2022

Dragorn421 Nov 24, 2022

engineer124 Nov 24, 2022 •

edited

Loading

engineer124 Nov 24, 2022

Dragorn421 Nov 24, 2022

engineer124 Nov 24, 2022

Dragorn421 Nov 24, 2022

engineer124 Nov 24, 2022

Dragorn421 Nov 25, 2022

Dragorn421 Nov 25, 2022

engineer124 Nov 25, 2022

engineer124 Nov 25, 2022 •

edited

Loading

engineer124 Nov 25, 2022

Dragorn421 Nov 24, 2022

fig02 Nov 24, 2022

engineer124 Nov 24, 2022

engineer124 Nov 24, 2022

hensldm left a comment

mzxrules Nov 24, 2022

mzxrules Nov 24, 2022

mzxrules Nov 24, 2022

mzxrules Nov 24, 2022

mzxrules Nov 24, 2022

	sfxId = NA_SE_PL_WALK_GROUND + SURFACE_SFX_OFFSET_IRON_BOOTS;
	sfxId = NA_SE_PL_WALK_HEAVYBOOTS;

	sfxId = NA_SE_PL_JUMP + SURFACE_SFX_OFFSET_IRON_BOOTS;
	sfxId = NA_SE_PL_JUMP_HEAVYBOOTS;

	sfxId = NA_SE_PL_LAND + SURFACE_SFX_OFFSET_IRON_BOOTS;
	sfxId = NA_SE_PL_LAND_HEAVYBOOTS;

	u16 SurfaceType_GetSfxOffset(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);
	u16 SurfaceType_GetSfxMaterial(CollisionContext* colCtx, CollisionPoly* poly, s32 bgId);

Document Surface Material #1447

Document Surface Material #1447

Conversation

engineer124 commented Nov 24, 2022

Choose a reason for hiding this comment

engineer124 Nov 24, 2022 • 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

engineer124 Nov 25, 2022 • 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

hensldm left a 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

engineer124 Nov 24, 2022 •

edited

Loading

engineer124 Nov 25, 2022 •

edited

Loading