EnZf OK and partially documented #988

EllipticEllipsis · 2021-09-26T03:49:43Z

It took 8 months, but it's OK and PR'd. Thanks to @petrie911 for encouraging me to do it in the first place and matching those awful platform functions that we all hate, @AngheloAlf for getting me started with some ideas about the struct variables, and @Dragorn421 for getting me to read EnWf carefully, which helped a lot with this thing.

I got a lot farther with documenting than I was expecting, but a few of the actions remain a bit elusive, and I'm a bit stuck on names for other things, both functions and struct variables. Didn't put much effort into naming all the temps since I assume we're going to try and make the fighter actors consistent anyway. I started the ball rolling on that by attempting to unify all the weird curframe things that play sound, although I didn't try to name those consistently yet either.

I'll probably carry on doing some documentation, particularly the textures in the object, but thought it was worth PRing now to get some more eyes on it.

What a mess of an actor. I've never seen setup functions that sometimes set a different function instead 1UL.

…o names)

EllipticEllipsis · 2021-09-26T03:51:27Z

Oh, almost forgot:

Yuck!

Dragorn421

Good job

(nice EnWf copypaste (or the other way around, or whatever enemy) by the dev)

src/overlays/actors/ovl_En_GeldB/z_en_geldb.c

src/overlays/actors/ovl_En_Zf/z_en_zf.c

Dragorn421 · 2021-09-26T13:38:37Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+            EnZf_SetupHopAndTaunt(this);
+            return;
+        }
+        EnZf_SetupApproachPlayer(this, globalCtx);
+        return;
+    }
+    if ((this->actor.params != ENZF_TYPE_DINOLFOS) || !EnZf_ChooseAction(globalCtx, this)) {


Suggested change

EnZf_SetupHopAndTaunt(this);

return;

}

EnZf_SetupApproachPlayer(this, globalCtx);

return;

}

if ((this->actor.params != ENZF_TYPE_DINOLFOS) || !EnZf_ChooseAction(globalCtx, this)) {

EnZf_SetupHopAndTaunt(this);

} else {

EnZf_SetupApproachPlayer(this, globalCtx);

}

} else if ((this->actor.params != ENZF_TYPE_DINOLFOS) || !EnZf_ChooseAction(globalCtx, this)) {

fits better the whole function not using early returns otherwise

I prefer early return after running the setup funcs: it makes it clear that there is no more execution happening in this function, which is rather less obvious with the elses IMO.

I get that, but the pattern should be consistent at least in a single function, in this function most setup calls aren't followed with a return

Hm, I'll have another look. IIRC a lot of these early returns are needed because the audio bit at the bottom is separate.

It does match. I'll do that until Fig tells me to put it back 1UL.

wait, i was looking at the wrong thing. line numbers changed lol

deleted my previous post, it makes sense to me here

src/overlays/actors/ovl_En_Zf/z_en_zf.c

Roman971

Nice work on this. Just a few things

src/overlays/actors/ovl_En_Zf/z_en_zf.c

Roman971 · 2021-10-11T16:51:50Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+    if ((this->actor.colorFilterTimer == 0) && (this->actor.bgCheckFlags & 1)) {
+        if (this->actor.colChkInfo.health == 0) {
+            EnZf_SetupDie(this);
+        } else if ((this->actor.params != ENZF_TYPE_DINOLFOS) || !EnZf_ChooseAction(globalCtx, this)) {


I think a few functions like this one could be improved with some early returns, but that comes down to personal preference I guess

Looking through again, I wonder if a lot of the if (!EnZf_DodgeRanged(...))s should actually be if (EnZf_DodgeRanged(...)) { return; }? It would certainly improve the indentation, and it happens in a lot of these actionFuncs

fig02

nice job, and sorry for a late review. all style and doc nitpicks, code looks good

src/overlays/actors/ovl_En_Zf/z_en_zf.c

fig02 · 2021-10-21T16:51:41Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+    if (thisx->params < ENZF_TYPE_LIZALFOS_MINIBOSS_A) { // Not minibosses
+        this->homePlatform = this->curPlatform = -1;


<= ENZF_TYPE_LIZALFOS_LONE will prob make more sense in that case. comment will prob not be needed then too

fig02 · 2021-10-21T16:52:29Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+
+    if ((this->actor.params >= ENZF_TYPE_LIZALFOS_MINIBOSS_A) /* miniboss */ &&


comment also not needed here imo, i think the context is clear enough from the conditional

src/overlays/actors/ovl_En_Zf/z_en_zf.c

fig02 · 2021-10-21T16:56:29Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+
+    if (this->actor.params >= ENZF_TYPE_LIZALFOS_MINIBOSS_A) { // miniboss


last ill comment on it, but in general i think the range checks on params dont need extra explaining cause they put the params in a pretty intuitive order.
Can leave them for now and we'll iron out how exactly we do this kind of documentation later. but just putting my thoughts out now

fig02 · 2021-10-21T17:54:28Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+    if ((globalCtx->state.frames & 0x5F) == 0) {
+        Audio_PlayActorSound2(&this->actor, NA_SE_EN_RIZA_CRY);


fig02 · 2021-10-21T17:55:32Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+
+        if ((globalCtx->gameplayFrames & 0x5F) == 0) {


last one ill point out, can check if there are anymore with a ctrl f

src/overlays/actors/ovl_En_Zf/z_en_zf.c

src/overlays/actors/ovl_En_Zf/z_en_zf.h

EllipticEllipsis · 2021-10-25T00:42:36Z

I did an experiment with a few things we could do to Zf in terms of the type and whether or not to use returns: https://github.com/EllipticEllipsis/oot/blob/438cb9ab64466ed1a885b89625bb9d7b36d10b0b/src/overlays/actors/ovl_En_Zf/z_en_zf.c Be interested to know what people think (diff with what's currently here is https://github.com/EllipticEllipsis/oot/compare/Zf...EllipticEllipsis:Zf_experiment?expand=1, but rather difficult to read) (made a new branch to avoid messing up this one).

Dragorn421 · 2021-10-03T14:11:50Z

assets/xml/objects/object_zf.xml

+        <Limb Name="gZfDinolfosTailBaseLimb" LimbType="Standard" Offset="0x64EC"/>
+        <Limb Name="gZfDinolfosScabbardRootRootLimb" LimbType="Standard" Offset="0x64F8"/>
+        <Limb Name="gZfDinolfosScabbardRootLimb" LimbType="Standard" Offset="0x6504"/>
+        <Limb Name="gZfDinolfosScabbardLimb" LimbType="Standard" Offset="0x6510"/>


For player we use "sheath", idk if we would want consistency for this detail

I'd have thought scabbard would be a better name for both anyway: it's more precise.

id say sheath is way more common colloquially, even if its slightly less accurate its something more people would recognize imo

Dragorn421 · 2021-10-09T10:01:41Z

assets/xml/objects/object_zf.xml

@@ -1,89 +1,91 @@
 <Root>
    <File Name="object_zf" Segment="6">
-        <Texture Name="object_zf_TLUT_000000" OutName="tlut_00000000" Format="rgba16" Width="16" Height="16" Offset="0x0"/>
+        <Texture Name="gZfDinolfosTLUT" OutName="zf_dinolfos_tlut" Format="rgba16" Width="16" Height="16" Offset="0x0"/>


Suggested change

<Texture Name="gZfDinolfosTLUT" OutName="zf_dinolfos_tlut" Format="rgba16" Width="16" Height="16" Offset="0x0"/>

<Texture Name="gZfDinolfosTLUT" OutName="dinolfos_tlut" Format="rgba16" Width="16" Height="16" Offset="0x0"/>

I thought we prefixed everything in the object with an object-unique prefix. That's certainly what I did for objects in the past.

hm I took a closer look in the repo and it's really inconsistent if outnames have the object-specific prefix

IMO it's not needed since the textures are already in a folder named after the object

Dragorn421 · 2021-10-09T10:01:48Z

assets/xml/objects/object_zf.xml

        <Texture Name="object_zf_Tex_000200" OutName="tex_00000200" Format="ci8" Width="8" Height="8" Offset="0x200"/>
        <Texture Name="object_zf_Tex_000240" OutName="tex_00000240" Format="ci8" Width="16" Height="16" Offset="0x240"/>
-        <Texture Name="object_zf_Tex_000340" OutName="tex_00000340" Format="rgba16" Width="8" Height="8" Offset="0x340"/>
+        <Texture Name="gZfDinolfosSpineTex" OutName="zf_dinolfos_spine" Format="rgba16" Width="8" Height="8" Offset="0x340"/>


Suggested change

<Texture Name="gZfDinolfosSpineTex" OutName="zf_dinolfos_spine" Format="rgba16" Width="8" Height="8" Offset="0x340"/>

<Texture Name="gZfDinolfosSpineTex" OutName="dinolfos_spine" Format="rgba16" Width="8" Height="8" Offset="0x340"/>

Dragorn421 · 2021-10-09T10:02:03Z

assets/xml/objects/object_zf.xml

-        <Animation Name="object_zf_Anim_009530" Offset="0x9530"/>
-        <Animation Name="object_zf_Anim_00A3D4" Offset="0xA3D4"/>
-        <Animation Name="object_zf_Anim_00B10C" Offset="0xB10C"/>
+        <Texture Name="gZfDinolfosNormalEyeTex" OutName="zf_dinolfos_normal_eye" Format="rgba16" Width="16" Height="16" Offset="0x700"/>


Suggested change

<Texture Name="gZfDinolfosNormalEyeTex" OutName="zf_dinolfos_normal_eye" Format="rgba16" Width="16" Height="16" Offset="0x700"/>

<Texture Name="gZfDinolfosNormalEyeTex" OutName="dinolfos_normal_eye" Format="rgba16" Width="16" Height="16" Offset="0x700"/>

src/overlays/actors/ovl_En_Zf/z_en_zf.c

Dragorn421 · 2021-10-28T09:15:40Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+        } else if (this->unk_3F0 != 0) {
+            this->unk_3F0--;
+        } else {
+            if (Actor_IsFacingPlayer(&this->actor, 30 * 0x10000 / 360)) {


Suggested change

if (Actor_IsFacingPlayer(&this->actor, 30 * 0x10000 / 360)) {

if (Actor_IsFacingPlayer(&this->actor, DEGF_TO_BINANG(30))) {

apparently I already suggested this, you said it wouldn't work but apparently it does (builds OK)
(use 30.0f if you want)

also in one other spot

Dragorn421 · 2021-10-28T09:27:40Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+    static Vec3f footOffset = { 300.0f, 0.0f, 0.0f };
+    static Vec3f D_80B4A2A4 = { 300.0f, -1700.0f, 0.0f }; // Sword tip?
+    static Vec3f D_80B4A2B0 = { -600.0f, 300.0f, 0.0f };  // Sword hilt?
+    static Vec3f swordQuadOffset1 = { 0.0f, 1500.0f, 0.0f };
+    static Vec3f swordQuadOffset0 = { -600.0f, -3000.0f, 1000.0f };
+    static Vec3f swordQuadOffset3 = { -600.0f, -3000.0f, -1000.0f };
+    static Vec3f swordQuadOffset2 = { 1500.0f, -3000.0f, 0.0f };


I would prefer ModelPos instead of Offset since they are positions in, well, model space

the discussion on this didn't take off
https://discord.com/channels/688807550715560050/838852326231769089/881526848209707008

I named these following GeldB/Test, iirc, so they should be renamed as well depending on what we do here.

my understanding is that its an offset from the position that it already has, not an absolute position in model space. is that not correct?

youll see plenty of things use 0,0,0 for this, and its not putting it at 0 in model space, but where the limb already is

Dragorn421 · 2021-10-28T09:31:20Z

src/overlays/actors/ovl_En_Zf/z_en_zf.c

+    if ((this->actor.bgCheckFlags & 3) && (this->hopAnimIndex != 0)) {
+        Audio_PlayActorSound2(&this->actor, NA_SE_EN_RIZA_ONGND);
+        Animation_Change(&this->skelAnime, &gZfLandingAnim, 1.0f, 0.0f, 17.0f, ANIMMODE_ONCE, 0.0f);
+        this->hopAnimIndex = 0;


since hopAnimIndex is not just used for the hop action maybe it would be better named a more generic animState ?

I'm not too convinced, I can't tell if its usage is all anim-related even. for example I don't see what its use in EnZf_SetupDamaged achieves (probably nothing afaict)

I think the point here is that the main use for this array is the hopping: it's the thing that uses all the indices. It so happens that for some reason they ended up using it for other things.

Out of the 45 uses of hopAnimIndex, only 18, maybe 20, seem relevant to hopping, so I'm not sure we can really brush off those other uses

fig02

feel like lots of the open ended conversations here are things we will figure out as we standardize actor documentation in general. so its fine on my end

fig02 · 2021-11-06T03:39:46Z

Some conflicts to fix but after that it's good to merge

* Match Destroy * Matched Init * 3 more matched * 15 functions matched, data imported * 16 matched * 17 matched * 18 matched * formatting * 19 matched * Fix unk_404 * Draw matched, 20 in total * 21 matched * 22 matched * 23 matched * 24 matched * 25 matched * 26 matched * 27 matched * 28 matched * 29 matched * 30 matched * 31 matched * 33 matched * 34 matched * 35 matched * 36 matched * 17 left * 16 left * 15 left * 14 left * 13 left * 12 left * 11 left * 10 left * spec * bss * 9 left * 8 left * 7 left * 6 left * 5 left * 4 left * 1 up to regalloc, 3 left * 2 left (+ 1 regalloc) * More naming * 1 + regalloc left * Some naming * matches but for 2 words of stack in func_80B45748 * Delete some padding in header * Merge remote-tracking branch 'upstream/master' into Zf * func_8003426C rename * more failed matching attempts * Update to new function names * OK at last * Actually OK this time, removed pragmas, spec * Begin documentation * Few more names * Some more naming, added description to z_en_geldb.c * Confirm platform categories * (Badly) named floor check functions * Make fighter actors playSpeed animation frame checks consistent (up to names) * Action enum, lot of work on the skeleton stuff in the object * Lot of function and animation naming * More naming, last of the hardcoded symbols * Some name cleanup * Remove asm * Format * Bit more cleanup * Dragorn review I * undefined syms * Correct limb names * Non-control flow review suggestions * Easy review stuff * Some more review

EllipticEllipsis added 30 commits January 24, 2021 09:13

Match Destroy

61e35c1

Matched Init

efff849

3 more matched

b8398e7

15 functions matched, data imported

60ba3a9

16 matched

7e19bea

17 matched

d7dd84b

18 matched

c7f8639

formatting

ba6ca22

19 matched

0cfcc2f

Fix unk_404

e8213d4

Draw matched, 20 in total

0d9a89d

21 matched

e5c91c7

22 matched

792fe01

23 matched

b6fe97b

24 matched

ce8d7c3

25 matched

dad6364

26 matched

a71f5bb

27 matched

5344795

28 matched

0266e71

29 matched

71f5e3b

30 matched

37deaea

31 matched

f3782cc

33 matched

5c24781

34 matched

51c98fe

35 matched

42a4d97

36 matched

6043a5c

17 left

40b47bc

16 left

c105568

15 left

dd6c0a2

14 left

f3719f2

EllipticEllipsis added 9 commits September 20, 2021 16:07

Make fighter actors playSpeed animation frame checks consistent (up t…

6c74776

…o names)

Merge remote-tracking branch 'upstream/master' into Zf

2a94ef6

Action enum, lot of work on the skeleton stuff in the object

b503f0c

Lot of function and animation naming

3c1216f

More naming, last of the hardcoded symbols

6cbfd1d

Some name cleanup

27a265c

Remove asm

a402253

Format

eadc446

Bit more cleanup

5fdbc4e

EllipticEllipsis added 3 commits September 26, 2021 07:27

Dragorn review I

2c0eabe

undefined syms

d1e107b

Merge remote-tracking branch 'upstream/master' into Zf

4d40431

Dragorn421 requested changes Sep 26, 2021

View reviewed changes

EllipticEllipsis added 2 commits October 3, 2021 05:12

Correct limb names

8132127

Merge remote-tracking branch 'upstream/master' into Zf

7a50fd7

Roman971 reviewed Oct 11, 2021

View reviewed changes

Non-control flow review suggestions

f40f575

fig02 reviewed Oct 21, 2021

View reviewed changes

EllipticEllipsis added 2 commits October 24, 2021 16:19

Merge remote-tracking branch 'upstream/master' into Zf

4f11f4d

Easy review stuff

a1f87d0

Dragorn421 requested changes Oct 28, 2021

View reviewed changes

Some more review

55cced0

fig02 approved these changes Nov 1, 2021

View reviewed changes

Roman971 approved these changes Nov 5, 2021

View reviewed changes

Merge branch 'master' into Zf

71eec95

ethteck merged commit 823a3c0 into zeldaret:master Nov 6, 2021

		if (thisx->params < ENZF_TYPE_LIZALFOS_MINIBOSS_A) { // Not minibosses
		this->homePlatform = this->curPlatform = -1;


		if ((this->actor.params >= ENZF_TYPE_LIZALFOS_MINIBOSS_A) /* miniboss */ &&


		if (this->actor.params >= ENZF_TYPE_LIZALFOS_MINIBOSS_A) { // miniboss

		if ((globalCtx->state.frames & 0x5F) == 0) {
		Audio_PlayActorSound2(&this->actor, NA_SE_EN_RIZA_CRY);

	<Texture Name="gZfDinolfosTLUT" OutName="zf_dinolfos_tlut" Format="rgba16" Width="16" Height="16" Offset="0x0"/>
	<Texture Name="gZfDinolfosTLUT" OutName="dinolfos_tlut" Format="rgba16" Width="16" Height="16" Offset="0x0"/>

	<Texture Name="gZfDinolfosSpineTex" OutName="zf_dinolfos_spine" Format="rgba16" Width="8" Height="8" Offset="0x340"/>
	<Texture Name="gZfDinolfosSpineTex" OutName="dinolfos_spine" Format="rgba16" Width="8" Height="8" Offset="0x340"/>

	<Texture Name="gZfDinolfosNormalEyeTex" OutName="zf_dinolfos_normal_eye" Format="rgba16" Width="16" Height="16" Offset="0x700"/>
	<Texture Name="gZfDinolfosNormalEyeTex" OutName="dinolfos_normal_eye" Format="rgba16" Width="16" Height="16" Offset="0x700"/>

	if (Actor_IsFacingPlayer(&this->actor, 30 * 0x10000 / 360)) {
	if (Actor_IsFacingPlayer(&this->actor, DEGF_TO_BINANG(30))) {

EnZf OK and partially documented #988

EnZf OK and partially documented #988

Conversation

EllipticEllipsis commented Sep 26, 2021

EllipticEllipsis commented Sep 26, 2021

Dragorn421 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roman971 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fig02 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

EllipticEllipsis commented Oct 25, 2021

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

fig02 left a comment

Choose a reason for hiding this comment

fig02 commented Nov 6, 2021