Rename and document Adult Ruto functions #2288

JordanLongstaff · 2024-11-07T21:29:47Z

No description provided.

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

JordanLongstaff · 2024-11-09T06:49:11Z

Thanks for the tips. Most of this was info I didn't see. 👍

cadmic

Very nice work!

cadmic · 2024-11-11T21:43:22Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2_cutscene_data.inc.c

@@ -2,7 +2,7 @@
 #include "z64cutscene_commands.h"

 // clang-format off
-static CutsceneData D_80AF411C[] = {
+static CutsceneData gWaterMedallionCS[] = {


nit: gWaterMedallionCs is more consistent with existing names (e.g. gIceCavernSerenadeCs)

The rest of the named CS data variables in csdis_re.py end in CS, not Cs.

That is a tool for processing assets, external to the games code. When considering naming conventions of code for the game we are speaking within the context of src/ and related header files

Oh sorry, I see what you are saying. The symbol names within HARDCODED_SYM_ROM.
Those should change to conform to rest of the codebases style (also what is this script even doing?)

I didn't realize the naming was inconsistent sorry, disregard

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

mzxrules

Again, the bodies of EnRu2_UpdateEyes and EnRu1_UpdateEyes are identical, so the variables shouldn't be named differently.

fig02 · 2024-11-14T16:45:02Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+s32 EnRu2_GetSwitchFlag(EnRu2* this) {
    s32 params_shift = PARAMS_GET_U(this->actor.params, 8, 8);

    return params_shift;


This isnt related to your changes, but params_shift doesnt even fit our style regarding variable names haha.
You can name it switchFlag. Actually, is a temp even required?

Additionally, if you feel like doing so, it would be helpful to give the params macro a helpful name.
For example
#define ENRU2_GET_SWITCH_FLAG(thisx) PARAMS_GET_U(thisx->params, 8, 8)

fig02 · 2024-11-14T16:45:25Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+s32 EnRu2_GetType(EnRu2* this) {
    s32 params = PARAMS_GET_U(this->actor.params, 0, 8);

    return params;
 }


same as a above but named type, and also making ENRU2_GET_TYPE()

fig02 · 2024-11-14T16:48:22Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+/**
+ * Gradually increases Ruto's Y-coordinate as she rises up through the blue warp in the Chamber of Sages.
+ */


worth noting this is changing her model Y offset while her actual position is unaffected. The shape.YOffset field is also in terms of model space, which is why the values are so high.

I would reword the comment to say "Model Y Offset" instead of "Y-coordinate".
And maybe consider changing the function name to RaiseModel, but maybe this implementation detail doesnt have to be mentioned in the name

fig02 · 2024-11-14T16:50:56Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+/**
+ * Sets up Ruto's actor in the Chamber of Sages. Note: this gets called every time the Chamber of Sages
+ * is loaded, regardless of story progress (or which Temple you just finished).
+ */


Regarding the note, this is less a consequence of the actors implementation and has more to do with the fact that all sage actors are loaded when you enter chamber of sages, right?

The comment may be better worded as
Note: All sages actors are present in the Chamber of Sages regardless of which dungeon was just completed. This function runs unconditionally, even if it is not relevant for Ruto

fig02 · 2024-11-14T16:56:28Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+/**
+ * Occurs in the Chamber of Sages (regardless of which medallion you've earned). Sets up the cutscene data for
+ * Ruto to give Link the Water Medallion, but this only happens if she should do so. Otherwise, this function
+ * does nothing except loop endlessly until Link leaves the Chamber of Sages.
+ */


I think this can be a bit more concise. I restate the note mentioned in EnRu2_SpawnWaterMedallion since the reader might not have seen that comment, and its arguably more important here due to the endless loop

Suggested change

/**

* Occurs in the Chamber of Sages (regardless of which medallion you've earned). Sets up the cutscene data for

* Ruto to give Link the Water Medallion, but this only happens if she should do so. Otherwise, this function

* does nothing except loop endlessly until Link leaves the Chamber of Sages.

*/

/**

* Sets up the Water Medallion Cutscene if coming from Water Temple.

* All sage actors are present in the Chamber of Sages regardless of which dungeon was just completed.

* This function will loop endlessly if the current sage cutscene is not for the Water Medallion.

*/

fig02 · 2024-11-14T17:22:27Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

-s32 func_80AF383C(EnRu2* this, PlayState* play) {
+s32 EnRu2_IsPlayerInRange(EnRu2* this, PlayState* play) {


In range for what? it has a very specific position check.
Seems to be for starting the encounter, so EnRu2_IsPlayerInRangeForEncounter

fig02 · 2024-11-14T17:23:24Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+/**
+ * Checks if Link is close enough to Ruto and conditionally triggers the encounter cutscene in the Water Temple.
+ */
+void EnRu2_TriggerEncounterInRange(EnRu2* this, PlayState* play) {


This function spends most of its time waiting doing nothing while it waits for link, so CheckToStartEncounter or similar seems nice to me

fig02 · 2024-11-14T17:24:35Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

+/**
+ * Triggers the encounter cutscene in the Water Temple, unconditionally. Appears to be unused.
+ */
+void EnRu2_TriggerEncounter(EnRu2* this, PlayState* play) {
+    this->action = ENRU2_WATER_TEMPLE_ENCOUNTER_BEGIN;
    OnePointCutscene_Init(play, 3130, -99, &this->actor, CAM_ID_MAIN);
 }


This isnt really triggering the encounter right, that already happened above. This is more like SetupBeginEncounter.
Also want to note inconsistency between ENRU2_WATER_TEMPLE_ENCOUNTER_BEGIN and EnRu2_BeginEncounter

No, the difference between this and the other function is that this is unconditional whereas the other one requires Link to be in the same room. Both of them trigger the cutscene where Link meets Ruto, but this one is unused.

fig02 · 2024-11-14T17:26:26Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

-    switch (func_80AF26A0(this)) {
+    switch (EnRu2_GetType(this)) {
        case 2:


An enum for these type values could be helpful if you want to make it

fig02 · 2024-11-14T17:26:54Z

src/overlays/actors/ovl_En_Ru2/z_en_ru2.c

-            func_80AF2E1C(this, play);
+            EnRu2_InitCrossingArms(this, play);


What is crossing arms? Like what cutscne does this correspond to? Might be helpful to name it after that, so the corresponding type name is also clear.

Two places. One is when you complete the Water Trial, the other is during the "Seal Ganon" cutscene.
It's named after the pose she strikes where she crosses her arms in an X.

JordanLongstaff added 4 commits November 7, 2024 16:27

Rename and document Adult Ruto functions

8476fbe

Rename struct fields also

324f492

Add enums for Ruto actions and draw configs

d9c2f82

Proper name for Water Medallion cutscene data

edd9879

mzxrules reviewed Nov 9, 2024

View reviewed changes

Suggested changes

6f3db36

cadmic reviewed Nov 11, 2024

View reviewed changes

fig02 reviewed Nov 11, 2024

View reviewed changes

More suggested changes

da7260b

fig02 added Needs first approval Needs second approval labels Nov 12, 2024

cadmic approved these changes Nov 12, 2024

View reviewed changes

fig02 removed the Needs first approval label Nov 12, 2024

Dragorn421 mentioned this pull request Nov 13, 2024

OoT64 decomp: unofficial "contributor agreement" Dragorn421/oot-licensing#3

Open

mzxrules suggested changes Nov 13, 2024

View reviewed changes

Bring back naming consistency

3413e2f

fig02 reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename and document Adult Ruto functions #2288

Rename and document Adult Ruto functions #2288

JordanLongstaff commented Nov 7, 2024

JordanLongstaff commented Nov 9, 2024 •

edited

Loading

cadmic left a comment

cadmic Nov 11, 2024

JordanLongstaff Nov 12, 2024

fig02 Nov 12, 2024

fig02 Nov 12, 2024

cadmic Nov 12, 2024

mzxrules left a comment

fig02 Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

JordanLongstaff Nov 14, 2024

fig02 Nov 14, 2024

fig02 Nov 14, 2024

JordanLongstaff Nov 14, 2024

		s32 func_80AF383C(EnRu2* this, PlayState* play) {
		s32 EnRu2_IsPlayerInRange(EnRu2* this, PlayState* play) {

		func_80AF2E1C(this, play);
		EnRu2_InitCrossingArms(this, play);

Rename and document Adult Ruto functions #2288

Are you sure you want to change the base?

Rename and document Adult Ruto functions #2288

Conversation

JordanLongstaff commented Nov 7, 2024

JordanLongstaff commented Nov 9, 2024 • edited Loading

cadmic 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

mzxrules 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

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

JordanLongstaff commented Nov 9, 2024 •

edited

Loading