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

Document Save.cutsceneIndex and adjacent data / code #2286

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

Conversation

Feacur
Copy link

@Feacur Feacur commented Nov 5, 2024

while studying the game logic flow, i saw there's a pattern between

  • Save.entranceIndex
  • Save.cutsceneIndex
  • SaveContext.sceneLayer

but values weren't yet described. so, it's time to cleanup my notes and shape them in a proper PR.
(UPD: not anymore) it's a WIP, but i'll start with a draft for visibility

P.S.:
the closest thing i see here is #1300, but

  • documented SaveContext.sceneLayer
  • it's merged already

so that it doesn't clutter previous commit. still, allowing `clangd LSP` to run format-on-save yields inconsistent results for me with the project's tooling, especially for header files
Comment on lines 377 to 379
CS_INDEX_SCRIPTED_D = 0xFFFD,
CS_INDEX_SCRIPTED_E = 0xFFFE,
CS_INDEX_SCRIPTED_F = 0xFFFF,
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
CS_INDEX_SCRIPTED_D = 0xFFFD,
CS_INDEX_SCRIPTED_E = 0xFFFE,
CS_INDEX_SCRIPTED_F = 0xFFFF,
CS_INDEX_SCRIPTED_D = 0xFFFD,

Values 0xFFF0-0xFFFC are directly tied to the scene layer system. These values are used to fetch scene layers 4-16, which are intended to contain scene command referencing a cutscene that will play immediately on scene load.

0xFFFD does not have the same behavior and thus should have a different name from the rest. It is intended to be used to trigger a scripted cutscene "manually" after scene load, and has no effect on the scene layer system. You can see this at Play_Init in z_play.c, line 332 or so, where the cutsceneIndex resets to 0 if it's 0xFFFD, before the sceneLayer is computed.

No scene has a layer count greater than 17, so values 0xFFFE/0xFFFF are never used and thus should not be enumerated.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

hmm, i do see this lines

if (gSaveContext.save.cutsceneIndex == CS_INDEX_SCRIPTED_D) {
    gSaveContext.save.cutsceneIndex = CS_INDEX_NONE;
}

and they look to me as

if (gSaveContext.save.cutsceneIndex == CS_INDEX_RESET) {
    gSaveContext.save.cutsceneIndex = CS_INDEX_NONE;
}

UPD 8cbdf80: preparing a commit, but did not apply the suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

The name needs to make sense in all parts of code, not just in the one code block. As I stated before, 0xFFFD triggers cutscenes after the scene has been loaded.

For example, z_bg_toki_sword is the Master Sword, and when you pull or drop the sword it will initiate a scripted cutscene by setting the cutscene script pointer and assigning gSaveContext.cutsceneTrigger to 1. This causes Cutscene_UpdateScripted to set the cutsceneIndex to 0xFFFD, which in turn starts the processing of the script.

The purpose of the block in Play_Init is to flush the cutsceneIndex state in the event that the PlayState is destroyed before the cutscene can complete normally. This can happen if e.g. the player falls out of bounds or dies mid-cutscene.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

ah, so CS_INDEX_TRIGGERED might do

// z_demo.c:2292
void Cutscene_SetupScripted(...)
    // if trigger flag set, isn't idling, and not in a cutscene
    if ((gSaveContext.cutsceneTrigger != 0) && (csCtx->state == CS_STATE_IDLE) && !Player_InCsMode(play))
        // then trigger
        gSaveContext.save.cutsceneIndex = CS_INDEX_TRIGGERED;

because it's a "state" not a "request"

UPD 32a2dd0

*/
typedef enum CutsceneIndex {
CS_INDEX_NONE = 0x0000,
CS_INDEX_MANUAL = 0xFFEF,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why this is called CS_INDEX_MANUAL.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

current naming is the result of this observation:

// z_demo.c:180
// i.e. CS_INDEX_MANUAL or lower
void Cutscene_UpdateManual(...)
    if (gSaveContext.save.cutsceneIndex < CS_INDEX_SCRIPTED_0)
        sManualCutsceneHandlers[csCtx->state](...);

// z_demo.c:215
// i.e. CS_INDEX_SCRIPTED_0 or higher
void Cutscene_UpdateScripted(...)
    if (gSaveContext.save.cutsceneIndex >= CS_INDEX_SCRIPTED_0)
        sScriptedCutsceneHandlers[csCtx->state](...);

Copy link
Contributor

Choose a reason for hiding this comment

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

cutsceneIndex never gets assigned to 0xFFEF though. It is only ever assigned to nextCutsceneIndex, and then the value is only used once in Play_Init where if nextCutsceneIndex is 0xFFEF then it's value is not copied over.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

ok, thanks. you are right here, i'm conflating these two together, because they are somewhat related

// `z_play.c:328`
if (gSaveContext.nextCutsceneIndex != CS_INDEX_MANUAL) {
    gSaveContext.save.cutsceneIndex = gSaveContext.nextCutsceneIndex;
    gSaveContext.nextCutsceneIndex = CS_INDEX_MANUAL;
}

i'll look closer

UPD 8cbdf80: looks like a CS_INDEX_NEXT_USED, like in "assigned to .cutseneIndex and can be set again safely"

CS_INDEX_SCRIPTED_D = 0xFFFD,
CS_INDEX_SCRIPTED_E = 0xFFFE,
CS_INDEX_SCRIPTED_F = 0xFFFF,
CS_INDEX_LAST = 0x8000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also don't understand why this is called CS_INDEX_LAST, and why it is placed all the way down here.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

currently it's the highest value i found that cutsceneIndex explicitly gets assigned with, thus the last one.

and you are right @ #2286 (comment), i've not yet finished with investigations and forgot to account for the SCENE_LAYER_CUTSCENE_FIRST

  • there's a correlation entranceIndex == baseEntranceIndex + sceneLayer
  • maximum possible sceneLayer == SCENE_LAYER_CUTSCENE_FIRST + (cutsceneIndex) & 0xF
  • entrance_table.h has the highest sceneLayer == 15 for ENTR_TEMPLE_OF_TIME_0_15
  • SCENE_LAYER_CUTSCENE_FIRST == 4, 11 == 15 - 4 == 11 == 0xB
  • thus 0xFFFB should be the last valid cutsceneIndex for the entrance table

now with this conversation, i came to think that this should be an improvement:

CS_INDEX_NONE
CS_INDEX_MANUAL // [ENTR_TEMPLE_OF_TIME_0 .. ENTR_TEMPLE_OF_TIME_0_3]
CS_INDEX_ENTRANCE_4 // for ENTR_TEMPLE_OF_TIME_0_4
...
CS_INDEX_ENTRANCE_F // for ENTR_TEMPLE_OF_TIME_0_15
CS_INDEX_SCRIPTED_C // [currently] need to come up with different name
...
CS_INDEX_LAST // [currently] may be not the last actually

probably?

UPD 8cbdf80:

CS_INDEX_RESET = 0xFFFD
CS_INDEX_STOP = 0xFFFF
CS_INDEX_LAST = 0x8000

Copy link
Contributor

Choose a reason for hiding this comment

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

tokinoma_scene actually only has layers 0-14, so the max cutscene index that loads a valid layer is actually 0xFFFA, which is what z_select goes up to. ENTR_TEMPLE_OF_TIME_0_15 is unreachable without glitches.

The cutscene count must start at 0, not 4:

  • It makes it easier to logically follow what SCENE_LAYER_CUTSCENE_FIRST + (gSaveContext.save.cutsceneIndex & 0xF); is accomplishing.

  • In MM, the game was refactored to no longer need separate layers to spawn actors at different times of day, so now only layer 0 is a default playable layer, and layers 1+ become the cutscene layers.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

UPD 2ec57c9: applied the suggestions

regarding MM: he-he, that's awesome to peek behind the scenes via decomp =)

include/z64cutscene.h Outdated Show resolved Hide resolved
typedef enum CutsceneIndex {
CS_INDEX_NONE = 0x0000,
CS_INDEX_MANUAL = 0xFFEF,
CS_INDEX_SCRIPTED_0 = 0xFFF0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't particularly like spelling out "SCRIPTED" here because I believe there's no mechanism to play unscripted cutscenes using this value.

Copy link
Author

@Feacur Feacur Nov 5, 2024

Choose a reason for hiding this comment

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

agreed, i'm compiling a commit with suggested name changes
UPD 8cbdf80

Feacur and others added 10 commits November 5, 2024 11:26
and some additional observations
reverified with
> `check_format.py ...`
> `make ...`

additionally:
- current clang-format lacks a rule for trailing commas
- compiler says about them `cfe: Warning 624`
zeldaret#2286 (comment)

reverified with
> `check_format.py ...`
> `make ...`
it gets assigned to the `nextCutsceneIndex`,
so `CS_INDEX_NONE` name was misleading
the purpose of `CS_INDEX_BARRIER` is not quite clear still
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants