Skip to content

Add comments for some music and movie file functions - #3220

Open
binary1230 wants to merge 2 commits into
Xeeynamo:masterfrom
binary1230:music-and-cutscene-comments
Open

Add comments for some music and movie file functions#3220
binary1230 wants to merge 2 commits into
Xeeynamo:masterfrom
binary1230:music-and-cutscene-comments

Conversation

@binary1230

Copy link
Copy Markdown

(resubmitted after lint and branch fix on fork)

No actual code changes, just filling in some comments around:

  1. Add descriptions for some of the movie file offsets in game_handlers.c and breadcrumbs for how to analyze with tools like jpsxdec
  2. Add comments in various cutscene audio handlers related to "has CD music started playing"

Future work:
A. The cutscene code is duplicated all over the place, I just added the comments in one section only for now. I might take a stab at de-duplicating it later.
B. We could probably safely rename a few things but... I'm not quite sure that's the only meaning of this is related to music (it PROBABLY is). Those renames would be:

  • rename func_80131F68() to something like IsMusicPlaybackStarted()
  • rename D_8013901C to something like g_IsMusicPlaying (maybe. probably. just not totally sure)

@binary1230
binary1230 marked this pull request as ready for review April 19, 2026 19:25
@binary1230
binary1230 requested a review from Xeeynamo as a code owner April 19, 2026 19:25
Comment thread src/st/sel/stream_info.c
&g_StreamInfo0,
&g_StreamInfo1,
&g_StreamInfo2,
&g_StreamInfo3,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work!
I think we could probably rename these to reference the specific cinematics you found rather than generically named

@JoshSchreuder JoshSchreuder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me, nice research! I think this opens up the door to a number of renames to make the code clearer

Comment thread include/cutscene.h
CSOP_LOAD_PORTRAIT,
CSOP_SCRIPT_UNKNOWN_20,

CSOP_SCRIPT_UNKNOWN_20, // same as CSOP_PLAY_SOUND, but always runs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably we could rename this something like CSOP_PLAY_SOUND_IGNORE_SKIP or similar

Comment thread include/cutscene.h
CSOP_WAIT_FOR_SOUND, // wait for CD audio playback to begin
// (does nothing if cutscene is being skipped)

CSOP_SCRIPT_UNKNOWN_11, // wait for CD audio playback to stop

@JoshSchreuder JoshSchreuder Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similarly here we could probably rename this as CSOP_WAIT_FOR_SOUND_BEGIN and CSOP_WAIT_FOR_SOUND_END

Comment thread src/dra/dra_bss.h
extern u8 g_SfxScriptModeCopy[4];
extern u32 g_DebugCurPal;
extern s16 D_8013901C;
extern s16 D_8013901C; // likely: "has CD music actually started playing?"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's name it and it can be iterated on later g_XaPlaying for example could work

Comment thread src/dra/sound.c
// seems like: cutscenes and dialogue tend to use this after starting a music
// track to wait until playback has actually started (or, to wait to know a
// playing track that was requested to stop has actually ended)
bool func_80131F68(void) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

name IsXaPlaying or similar?

Comment thread src/st/sel/stream_info.c
// Sectors:23113-24544, Size:2932736
// - Video: Sectors:23113-24543, Frame Count:143, Sector Frames:23113-24534
// - audio: Sectors:23120-24240
StreamInfo g_StreamInfo0 = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we know what these are now let's just give them names? g_LogoStreamInfo etc.

@Xeeynamo

Copy link
Copy Markdown
Owner

Waiting for @binary1230 answer to feedback before merging

@Xeeynamo Xeeynamo mentioned this pull request May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants