Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Camera docs: KeepOn4 #1456

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Dragorn421
Copy link
Collaborator

@Dragorn421 Dragorn421 commented Dec 8, 2022

Document the Camera_KeepOn4 function, which handles some varied player interactions, and neighboring code/logic.

Researching past convos for context, I noticed engineer already did documentation on what I made the KEEPON4_ITEM_TYPE_ enum. Since my doc is independent from it, it will be fruitful to compare the two. I haven't done so yet

This PR has room for improvement but it's also a good start as it touches a few other systems


If it helps getting a grasp on what Camera_KeepOn4 does, engineer made this to show its different "item type" modes:


Notes/research/comments from @engineer124

https://discord.com/channels/688807550715560050/838852326231769089/882911530964901910

I’m in the process of documenting the Item2 setting of camera, this is the camera setting that zooms into Link’s face. This setting is implemented through the function Camera_KeepOn4. There are many variations of this setting that have similar behaviour (same camera function), but have values slightly altered in a giant switch-case based on a parameter called “item type”.

This “item type” names comes from a debug string in camera: “camera: item: item type changed %d -> %d\n”, is stored indirectly in camera->data2 through the last argument of func_80835EA4 (func from z_player), the exception being links death which sets camera->data2 directly instead of interfacing through this function.

Doing research on “item type”, here’s what the following values are:
• 0x1: Reading ruto’s letter, dropping fish
• 0x2: Drinking from a bottle
• 0x3: Releasing fairy
• 0x4: Show empty item, catch item in bottle, FW Return
• 0x8: Grabbing ruto’s letter from underwater (or any generic GI from surfacing from the water)
• 0x9: Receive item from npc, grabbing freestanding item, get item from chest, get most items
• 0x5: Releasing bugs and blue fire
• 0xA: Casting FW and Nayru’s Love
• 0xC: Link's Death cutscene
• 0x5A: Ocarina without target
• 0x5B: Ocarina with target (Great Fairy Fountain, botw, frogs) (Not zora river, not skull kid, not oca game)
• 0x51: Immediately after playing epona,
• 0xB: Navi talking to you

I was brainstorming a possible enum for this, although feedback/thoughts would be great. Also, is it possible to have multiple enums for the same value or do they have to be combined. Even if we can have multiple enums, is that bad practice?

typedef enum {
    /* 0x01 */ CAM_ITEM_BOTTLE_READ_RUTO = 0x1,
    /* 0x01 */ CAM_ITEM_BOTTLE_DROP_FISH = 0x1,
    /* 0x02 */ CAM_ITEM_BOTTLE_DRINK,
    /* 0x03 */ CAM_ITEM_BOTTLE_RELEASE, // fairy
    /* 0x04 */ CAM_ITEM_BOTTLE_CATCH, // bugs, fish, fairy, blue fire
    /* 0x04 */ CAM_ITEM_FARORE_RETURN = 0x4,
    /* 0x04 */ CAM_ITEM_SHOW_ITEM = 0x4,
    /* 0x05 */ CAM_ITEM_BOTTLE_DROP_BUGS_FIRE, 
    /* 0x08 */ CAM_ITEM_GI_WATER = 0x8,
    /* 0x09 */ CAM_ITEM_GI_GENERIC,
    /* 0x0A */ CAM_ITEM_CAST_FARORE_NAYRU,
    /* 0x0B */ CAM_ITEM_NAVI_TALK,
    /* 0x0C */ CAM_ITEM_DEATH,
    /* 0x51 */ CAM_ITEM_CALL_EPONA, // camera pans to epona after playing epona’s song
    /* 0x5A */ CAM_ITEM_OCARINA_NO_TARGET, 
    /* 0x5B */ CAM_ITEM_OCARINA_TARGET, // player->unk_6A8 != NULL
} CameraItemType;

Missed one, and forgot to highlight both 2 & 3 have the same behaviour. Both 0xB & 0xC do a sweep before settling on link, and 0x51 releases the camera after a few seconds


Trying to explain the `a - b > 0 ? a + c : a - c` construct seen in z_camera

https://discord.com/channels/688807550715560050/688851317593997489/1049866797760925746

A botched explanation of this construct that comes up a bit in z_camera.c

// note: typically at ~ playerPos
baseYaw = (s16)((s16)(playerPosRot->rot.y - 0x7FFF) - atToEyeNextDir.yaw) > 0
                ? (s16)(playerPosRot->rot.y - 0x7FFF) + CAM_DEG_TO_BINANG(roData->yawTarget)
                : (s16)(playerPosRot->rot.y - 0x7FFF) - CAM_DEG_TO_BINANG(roData->yawTarget);

(specifically, this is from the KEEPON4_FLAG_1 case (in my wip KeepOn4 docs branch))

What it does is based on the target relative angle roData->yawTarget, compute a target angle in world space baseYaw off a base world angle (playerPosRot->rot.y - 0x7FFF) (here, an angle towards player), picking the side of that angle to offset towards in a way the camera crosses the base direction

In-game, this can be experienced for example by repeatedly using ocarina, the camera switches side every usage

The screenshots illustrate the angles going into the calculation (note it makes more sense to look at it assuming CameraAt and Player are at nearly the same position, which is typically the case, the drawing doesn't stack them though for readability)
Both screenshots use yawTarget (roData->yawTarget) = 170° which is the default, used for example by ocarina
The first screenshot shows that when the eye is to the left of the player, the new eye ends up to its right
And similarly, the second screenshot shows that when the eye is to the right of the player, the new eye ends up to its left

this->subCamId = OnePointCutscene_Init(play, 1100, -101, NULL, CAM_ID_MAIN);
} else {
func_80835EA4(play, 10);
func_80835EA4(play, KEEPON4_ITEM_TYPE_10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func_80835EA4(play, KEEPON4_ITEM_TYPE_10);
func_80835EA4(play, CAM_ITEM_TYPE_10);

I feel since this is external to camera, it should have a CAM_ prefix to it. Maybe add onto it the setting it's associated with, TURN_AROUND, but I'm not sure it's necessary. I don't think CAM_ITEM_TYPE_ conflicts with anything, although that alone might not be clear enough.

Also, now that you understand this function much more, do you think CAM_SET_TURN_AROUND is decent, or is there something better than TURN_AROUND that could be used for this? (And possibly be incorporated into enums/function names i.e. func_80835EA4)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah TURN_AROUND isn't the best name tbh (neither is item_type, just got that from the debug string as placeholder, not to mention "keepon4")

I may want to refer to this whole thing (turn_around, keepon4, item_type) as "player action camera" but I'm not sure that's not too generic of a name for it, it may encompass more things I'm not aware of

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed about "TURN_AROUND", never felt quite right to me.

I feel the list I had is pretty comprehensive of all use-cases of this camera settings:
• 0x1: Reading ruto’s letter, dropping fish
• 0x2: Drinking from a bottle
• 0x3: Releasing fairy
• 0x4: Show empty item, catch item in bottle, FW Return
• 0x8: Grabbing ruto’s letter from underwater (or any generic GI from surfacing from the water)
• 0x9: Receive item from npc, grabbing freestanding item, get item from chest, get most items
• 0x5: Releasing bugs and blue fire
• 0xA: Casting FW and Nayru’s Love
• 0xC: Link's Death cutscene
• 0x5A: Ocarina without target
• 0x5B: Ocarina with target (Great Fairy Fountain, botw, frogs) (Not zora river, not skull kid, not oca game)
• 0x51: Immediately after playing epona,
• 0xB: Navi talking to you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commonality between them is that the camera turns around and faces Link from the front instead of being behind Link

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really tempted by "focus player action" but that overlaps with onepointdemo stuff (not that there is a name that doesn't though...)

I get that interpretation of "turn around", but it's more refering to the data KeepOn4 typically uses than what it can actually do
(i.e. if it used angles that made it not face Link's front, it wouldn't)

Maybe if this stays open for 6 more months I'll have an idea by then but I don't plan to find good names for CAM_SET_TURN_AROUND/CAM_ITEM_TYPE/KeepOn4 in this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this, and maybe instead of "turn around", what about "inverted camera"? I know "inverted cam" is the trick used to back-walk with the camera facing Link, maybe that term could be expanded to KeepOn4 and the associated player functions/enums. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not too sure, the glitch we call "inverted camera" is called as such cause the camera is "inverted" from how its supposed to be while targeting or in "parralel" mode. Im not sure it carries over well to a case where the camera is intended to be facing player.

I tbh find "turn around" to be quite intuitive. Its not only the fact that it faces link, but also includes the little swinging or "turning around" motion it does when getting there.
But if we want to move away from it, I would probably suggest going down the path of "front of player" "face player front", or something along those lines

@@ -835,56 +841,72 @@ typedef struct {
{ swingPitchAdj, CAM_DATA_SWING_PITCH_ADJ }, \
{ fov, CAM_DATA_FOV }, \
{ atLerpStepScale, CAM_DATA_AT_LERP_STEP_SCALE }, \
{ yawUpdateRateTarget, CAM_DATA_YAW_UPDATE_RATE_TARGET }, \
{ initTimer, CAM_DATA_YAW_UPDATE_RATE_TARGET }, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the second value (CAM_DATA_YAW_UPDATE_RATE_TARGET) also need to be updated?

{ fov, CAM_DATA_FOV }, \
{ interfaceField, CAM_DATA_INTERFACE_FIELD }, \
{ yawUpdateRateTarget, CAM_DATA_YAW_UPDATE_RATE_TARGET }, \
{ unk_22, CAM_DATA_UNK_22 }
{ initTimer, CAM_DATA_UNK_22 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps UNK_22 was suppose to be the init timer, and the one above is wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

Comment on lines +851 to +852
/* 0x0C */ f32 yawTarget; // degrees, usage varies with KEEPON4_FLAG_EYE_
/* 0x10 */ f32 atOffsetPlayerForwards; // distance to offset at by in the player's forwards direction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"distance to offset at by, in the player's forwards direction"
will help alot with the word soup here. i think putting at and eye in backticks in general helps the reader quickly know its referring to the camera component, and not the regular word

Comment on lines +859 to +860
typedef enum {
/* 1 */ CAM_ITEM_TYPE_1=1, // drop fish from bottle and something with ruto's letter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id space out all the value assignments here, = 1 etc

{ fov, CAM_DATA_FOV }, \
{ interfaceField, CAM_DATA_INTERFACE_FIELD }, \
{ yawUpdateRateTarget, CAM_DATA_YAW_UPDATE_RATE_TARGET }, \
{ unk_22, CAM_DATA_UNK_22 }
{ initTimer, CAM_DATA_UNK_22 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

Comment on lines +143 to +147
#define PLAYER_MAGIC_SPELL(playerItemActionMagicSpell) ((playerItemActionMagicSpell) - PLAYER_IA_MAGIC_SPELL_15)
#define PLAYER_MAGIC_SPELL_MAX (1 + PLAYER_MAGIC_SPELL(PLAYER_IA_DINS_FIRE))

#define PLAYER_BOTTLE(playerItemActionBottle) ((playerItemActionBottle) - PLAYER_IA_BOTTLE)
#define PLAYER_BOTTLE_MAX (1 + PLAYER_BOTTLE(PLAYER_IA_BOTTLE_FAIRY))
Copy link
Collaborator

@fig02 fig02 Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can prob afford _IA_ in the middle to make it clear its dealing with item actions

Comment on lines 1390 to +1403
Color_RGB8 sBottleColors[] = {
{ 255, 255, 255 }, // Empty
{ 80, 80, 255 }, // Fish
{ 255, 100, 255 }, // Fire
{ 0, 0, 255 }, // Bug
{ 255, 0, 255 }, // Poe
{ 255, 0, 255 }, // Big Poe
{ 200, 200, 100 }, // Letter
{ 255, 0, 0 }, // Red Potion
{ 0, 0, 255 }, // Blue Potion
{ 0, 255, 0 }, // Green Potion
{ 255, 255, 255 }, // Milk
{ 255, 255, 255 }, // Half Milk
{ 80, 80, 255 }, // Fairy
{ 255, 255, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE)
{ 80, 80, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_FISH)
{ 255, 100, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_FIRE)
{ 0, 0, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_BUG)
{ 255, 0, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_POE)
{ 255, 0, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_BIG_POE)
{ 200, 200, 100 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_RUTOS_LETTER)
{ 255, 0, 0 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_POTION_RED)
{ 0, 0, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_POTION_BLUE)
{ 0, 255, 0 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_POTION_GREEN)
{ 255, 255, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_MILK_FULL)
{ 255, 255, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_MILK_HALF)
{ 80, 80, 255 }, // PLAYER_BOTTLE(PLAYER_IA_BOTTLE_FAIRY)
Copy link
Collaborator

@fig02 fig02 Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally think these can stay as they were. it adds alot of noise just to get the same information out, and i dont think the fact that its an item action value matters too much in this context

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but also I'm on a quest to PLAYER_IA_ shiftability, so commenting where its ordering comes up is useful to me (but if we just don't want that in the repo it's ok)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think maybe a note next to this array (and any other similar scenarios) that says "These are adjusted item action values (PLAYER_IA_) " or something that tells you the information and would come up in a search of PLAYER_IA_ would help. But could get some more opinions too

Comment on lines +1526 to +1532
static u8 sMagicSpellCosts[] = {
12, // PLAYER_MAGIC_SPELL(PLAYER_IA_MAGIC_SPELL_15)
24, // PLAYER_MAGIC_SPELL(PLAYER_IA_MAGIC_SPELL_16)
24, // PLAYER_MAGIC_SPELL(PLAYER_IA_MAGIC_SPELL_17)
12, // PLAYER_MAGIC_SPELL(PLAYER_IA_FARORES_WIND)
24, // PLAYER_MAGIC_SPELL(PLAYER_IA_NAYRUS_LOVE)
12, // PLAYER_MAGIC_SPELL(PLAYER_IA_DINS_FIRE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would perfer simple comments here as well

@fig02 fig02 added Needs discussion There is a debate or other required discussion preventing changes from being made and removed Needs contributor approval labels Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs discussion There is a debate or other required discussion preventing changes from being made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants