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

Audio Decomp'd - ROM is OK #1236

Closed
wants to merge 120 commits into from

Conversation

MNGoldenEagle
Copy link
Contributor

@MNGoldenEagle MNGoldenEagle commented May 22, 2022

Hope you weren't holding your breath for this, it's been a looooooooooong time coming. 😅

Lot of pieces to this. I'll explain what I can here, but can clarify more in comments or on the Discord server.

Sequences are deserialized out of the game using disassemble_sequences.py->disassemble_sequence.py. This does a fairly straightforward analysis and conversion of the binary data into its MUS textual format (documented in docs/Music Macro Language.md). Music sequences are extracted during make setup; the programmatic sequences are stored in src inside the audio folder. They've been documented pretty comprehensively, enough that it should be pretty easy to find where every sound effect is and how the Hyrule Field sequences are swapped around.

Conversely, the build process calls assemble_sequences.py->assemble_sequence.cpp, which converts the textual format into binary using a parser. This packages each sequence as an ELF file, which is included in the spec file for the Audioseq segment. The Python script also will generate the tables related to sequences that are included in the code.

Soundfonts and samples (wave files) are deserialized out of the game simultaneously using disassemble_sound.py. Soundfonts are converted from their binary representation into an XML file containing all of the instrument/drum/effect definitions, as well as the necessary envelopes. (Note: We went with calling these soundfonts since conceptually it's very close to soundfonts as used with most MIDI systems, though the OoT version is much simpler in comparison.) Once the soundfonts have been read, it'll also create the AIFF files for all samples referenced, and any unused sections are output as bin files.

To ensure that soundfonts and samples can be rebuilt matching, the soundfonts include index information detailing what instruments should appear in which order, since the instrument index doesn't always line up with the order in which their placed in the binary (particularly with sound effects). This is only needed for matching; you can omit the index otherwise. Samples are extracted in both AIFC and AIFF formats: AIFC are virtually identical to how they are represented in the ROM with the compression type set in the headers; AIFF files are decompressed and can be played in most audio players.

Rebuilding soundfonts and samples is done with assemble_sound.py, which reads the XMLs and AIFCs and converts them into the proper binary format expected by the game and packages them in ELF files, which are included in Audiobank and Audiotable respectively. The script will also generate the tables related to soundfonts and sample banks that are included in the code.

After getting everything to build, the audio sections of the ROM will match properly.

Prerequisites to build:

  • Python 3.8 or later
  • Python package makeelf (via pip3 install makeelf)
  • C++ compiler that supports C++17

Steps to build (currently):

  1. Run make clean assetclean to get a clean environment.
  2. Run make setup to get all assets.
  3. Run make to build the ROM.

Edit: I've updated the scripts so that the soundfont include files are generated during both disassembly and assembly. This makes the entire process much smoother overall.

Tremendous thanks to @waffleoRai, @zelda2774, and @engineer124 for their many contributions to this effort, and to the many others who provided valuable insight, guidance, and encouragement over this months-long effort.

MNGoldenEagle and others added 30 commits October 9, 2021 15:50
{"local":
  {"subdir":  "tools/seq64"
  ,"action":  "clone"}
,"remote":
  {"url":     "https://github.com/sauraen/seq64"
  ,"branch":  "master"
  ,"commit":  "b2c5f8348"}
,"git-subrepo":
  {"version": "0.1.0"
  ,"commit":  "???"
  ,"origin":  "???"}}
{"local":
  {"subdir":  "tools/seq64"
  ,"action":  "clone"}
,"remote":
  {"url":     "https://github.com/sauraen/seq64"
  ,"branch":  "master"
  ,"commit":  "b2c5f8348"}
,"git-subrepo":
  {"version": "0.1.0"
  ,"commit":  "???"
  ,"origin":  "???"}}
…bankToC

{"local":
  {"subdir":  "tools/AudiobankToC"
  ,"action":  "clone"}
,"remote":
  {"url":     "[email protected]:sauraen/AudiobankToC.git"
  ,"branch":  "main"
  ,"commit":  "2dc7f2617"}
,"git-subrepo":
  {"version": "0.1.0"
  ,"commit":  "???"
  ,"origin":  "???"}}
Copy link
Collaborator

@fig02 fig02 left a comment

Choose a reason for hiding this comment

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

just some surface level things glancing over this for the first time

some other comments that are more general and dont belong in a specific place:

  • checked out this branch and scrolled around the assets folder, its cool to see everything extracted.
    Would it hurt the build setup to have aifc and aiff samples separated into their own folders? When trying to just navigate around and play different samples, they are always grouped together in file explorer, and its annoying to click on an aifc file on accident.
    Thought I would be able to sort explorer by type, but that didnt work for some reason? idk
    Having all the "playable" samples separated from the compressed aifc files would be more convenient from a file browsing pov imo

  • We discussed a bit in discord, but I think it would be a good idea to consider using .seq extensions for sequences. Since that is the terminology used for these files, I find .mus to be adding unnecessary confusion.
    .mus probably comes from the modding community (seq64)? But yea I think .seq is overall consistent with the terminology we use and easy to understand

  • Some of the sequences living in src/audio is pretty unfortunate I feel. All of the other sequences go into assets, so having sequences in different places is a bit confusing.
    My understanding is that the ones currently in src are less "musical" and are more code driven. But we may want to consider consolidating sequences in general into one place.
    This brings up the wonderful question of what is an asset, and how much of this is safe to commit to the repo. We can have that discussion in discord if needed. But I think whatever solution we go with, the sequences should stay together.

src/code/audio_load.c Outdated Show resolved Hide resolved
src/audio/000_Sound_Effects.mus Outdated Show resolved Hide resolved
src/audio/sfxbanks/voice.mus.inc Outdated Show resolved Hide resolved
@MNGoldenEagle
Copy link
Contributor Author

just some surface level things glancing over this for the first time

some other comments that are more general and dont belong in a specific place:

* checked out this branch and scrolled around the assets folder, its cool to see everything extracted.
  Would it hurt the build setup to have aifc and aiff samples separated into their own folders? When trying to just navigate around and play different samples, they are always grouped together in file explorer, and its annoying to click on an aifc file on accident.
  Thought I would be able to sort explorer by type, but that didnt work for some reason? idk
  Having all the "playable" samples separated from the compressed aifc files would be more convenient from a file browsing pov imo

Yeah, I can understand this and having the AIFC/AIFF files mixed together does feel a bit... kludgy. The reason we have this (right now) is because there's no notion of specify the compression type anywhere, so if we just had the AIFF files we wouldn't be able to build matching AIFC files. So if we moved the AIFC files to build (which would be the natural place to put them), we'd need some way of expressing the compression type to use. We could potentially include a compression type in the soundfont definition; that would work, and I don't see any direct downsides to it. It might be worth discussing in Discord though.

* We discussed a bit in discord, but I think it would be a good idea to consider using .seq extensions for sequences. Since that is the terminology used for these files, I find .mus to be adding unnecessary confusion.
  .mus probably comes from the modding community (seq64)? But yea I think .seq is overall consistent with the terminology we use and easy to understand

.mus came around from some conversations with the modding community, but I'm fine with .seq and it does feel more consistent with the overall terminology we're using. The binary extension (.aseq) seems a bit odd to be fair. I borrowed that from Seq64, but we could use something else instead.

* Some of the sequences living in `src/audio` is pretty unfortunate I feel. All of the other sequences go into assets, so having sequences in different places is a bit confusing.
  My understanding is that the ones currently in `src` are less "musical" and are more code driven. But we may want to consider consolidating sequences in general into one place.
  This brings up the wonderful question of what is an asset, and how much of this is safe to commit to the repo. We can have that discussion in discord if needed. But I think whatever solution we go with, the sequences should stay together.

So when I was originally working on this, the concept of stuff being checked into assets hadn't existed yet since we didn't have the textbox MR merged yet. The primary motivator was that because this is reverse-engineered "source code" that merely queues up and plays samples rather than music, it seemed more appropriate to treat it the same as all other source code in the game. To that extent, I could move this into the assets/sequences folder instead to keep everything all in one place. I'm fine with having this conversation in Discord also; the main motivation with having it checked in is the benefit of the expressive documentation. If we're not comfortable hosting the sequences, though, the extraction tool should be able to work regardless, though a lot of that documentation will go away.

@fig02
Copy link
Collaborator

fig02 commented Jun 7, 2022

-Even if the AIFC files are kept in the assets folder with AIFF, just having them in separate folders would help navigation/browsing i think. But if we can work out a way to put the compressed version in build that would be even better.

-Yeah I think .seq would make sense for sequences, and we can do .seq.bin for the assembled binary versions or something like that. More opinions welcome from anyone

-And yeah, can understand wanting to commit them with the documentation. I think having them live in the assets folder with the other sequences, even though those specific ones would be committed, would be a great improvement over having them in two separate places.

All of these can be discussed in more detail in discord, since others will have to weigh in as well

@MNGoldenEagle MNGoldenEagle changed the title [WIP] Audio Decomp'd - ROM not yet OK due to BSS issues Audio Decomp'd - ROM is OK Jun 25, 2022
@Dragorn421
Copy link
Collaborator

Dragorn421 commented Aug 25, 2022

Some notes on building this (well I couldn't build it but the extraction seems to work and that's what I was after in my case so enough for me for now, anyway:)

1) `tools/assemble_sequence` is not built

Because $(CPP_PROGRAMS) isn't a prerequisite of the default rule all in tools/Makefile

I was also surprised when fixing that, that $(CC) compiled a C++ program. May be because my CC resolved to clang and clang is smart idk. I think the compiler for C++ programs should be explicitly for C++ anyway even if this does work with gcc (I would be even more surprised)

2) Can't build due to linking tables

So everything runs fine until linking, then the various tables are apparently missing from the object files ld gets to see? :

mips-linux-gnu-ld -T build/undefined_syms.txt -T build/ldscript.txt --no-check-sections --accept-unknown-input-arch --emit-relocs -Map build/z64.map -o zelda_ocarina_mq_dbg.elf
mips-linux-gnu-ld: build/src/code/audio_load.o: in function `AudioLoad_Init':
src/code/audio_load.c:1208: undefined reference to `gSequenceTable'
mips-linux-gnu-ld: src/code/audio_load.c:1208: undefined reference to `gSequenceTable'
mips-linux-gnu-ld: src/code/audio_load.c:1209: undefined reference to `gSoundFontTable'
mips-linux-gnu-ld: src/code/audio_load.c:1209: undefined reference to `gSoundFontTable'
mips-linux-gnu-ld: src/code/audio_load.c:1210: undefined reference to `gSampleBankTable'
mips-linux-gnu-ld: src/code/audio_load.c:1210: undefined reference to `gSampleBankTable'
mips-linux-gnu-ld: src/code/audio_load.c:1211: undefined reference to `gSequenceFontTable'
mips-linux-gnu-ld: src/code/audio_load.c:1211: undefined reference to `gSequenceFontTable'
make: *** [Makefile:292: zelda_ocarina_mq_dbg.elf] Error 1

Even though gSequenceTable is indeed a global symbol in .data in build/assets/data/SequenceTable.o

$ mips-linux-gnu-objdump -x build/assets/data/SequenceTable.o

build/assets/data/SequenceTable.o:     file format elf32-tradbigmips
build/assets/data/SequenceTable.o
architecture: mips:3000, flags 0x00000010:
HAS_SYMS
start address 0x00000000
private flags = 0: [no abi set] [mips1] [not 32bitmode]

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .data         000006f0  00000000  00000000  0000011d  2**4
                  CONTENTS, ALLOC, LOAD, DATA
SYMBOL TABLE:
00000000 l    d  .data  000006f0 .data
00000000 g     O .data  00000000 _SequenceTable_start
00000000 g     O .data  000006f0 gSequenceTable
000006f0 g     O .data  00000000 _SequenceTable_end

and .data of that file is indeed linked by the linker script:

$ grep SequenceTable build/ldscript.txt
            build/assets/data/SequenceTable.o (.text)
            build/assets/data/SequenceTable.o (.data)
            build/assets/data/SequenceTable.o (.rodata)
            build/assets/data/SequenceTable.o (.rodata.str1.4)
            build/assets/data/SequenceTable.o (.rodata.cst4)
            build/assets/data/SequenceTable.o (.rodata.cst8)
            build/assets/data/SequenceTable.o (.sdata)
            build/assets/data/SequenceTable.o (.ovl)
            build/assets/data/SequenceTable.o (.sbss)
            build/assets/data/SequenceTable.o (.scommon)
            build/assets/data/SequenceTable.o (.bss)
            build/assets/data/SequenceTable.o (COMMON)

I have no idea what's going on here.

Hack solution to get OK despite this symbol issue, add this to undefined_syms.txt:

gSequenceTable = 0x80155500;
gSoundFontTable = 0x801550d0;
gSampleBankTable = 0x80155bf0;
gSequenceFontTable = 0x80155340;

EDIT2: so I looked on the makeelf repo and this is a bug in makeelf, see this issue v3l0c1r4pt0r/makeelf#22 opened by some person whose pseudonym is "MNGoldenEagle" idk if that rings a bell to anybody. Sounds like someone had a similar issue before, what were the odds

Changing the following in makeelf (elf.ELF.append_symbol):
symtab_hdr.sh_info = sym_id + 1
to
symtab_hdr.sh_info = 2
(this "2" is a magic value that Just Worked for me, this isn't a proper fix at all)
seems to fix the linker not seeing symbols.

But now it's seeing too many symbols and they clash with each other :)

mips-linux-gnu-ld -T build/undefined_syms.txt -T build/ldscript.txt --no-check-sections --accept-unknown-input-arch --emit-relocs -Map build/z64.map -o zelda_ocarina_mq_dbg.elf
mips-linux-gnu-ld: build/assets/samplebanks/5_Goron_City.o:(.data+0x0): multiple definition of `Goron_Drum'; build/assets/samplebanks/0_Sound_Effects.o:(.data+0x3aa6e0): first defined here
mips-linux-gnu-ld: build/assets/samplebanks/5_Goron_City.o:(.data+0x7b70): multiple definition of `Tom_Drum'; build/assets/samplebanks/0_Sound_Effects.o:(.data+0x335740): first defined here
mips-linux-gnu-ld: build/assets/samplebanks/5_Goron_City.o:(.data+0x8630): multiple definition of `Resonant_Bass_Marimba'; build/assets/samplebanks/0_Sound_Effects.o:(.data+0x35d100): first defined here
mips-linux-gnu-ld: build/assets/samplebanks/6_Spirit_Temple.o:(.data+0xe120): multiple definition of `Tom_Drum'; build/assets/samplebanks/0_Sound_Effects.o:(.data+0x335740): first defined here
mips-linux-gnu-ld: build/assets/samplebanks/6_Spirit_Temple.o:(.data+0x12eb0): multiple definition of `Gong'; build/assets/samplebanks/0_Sound_Effects.o:(.data+0x39a6b0): first defined here
mips-linux-gnu-ld: build/assets/samplebanks/6_Spirit_Temple.o:(.data+0x1d3e0): multiple definition of `Bass_Slap_'; build/assets/samplebanks/5_Goron_City.o:(.data+0x2540): first defined here
mips-linux-gnu-ld: build/assets/samplebanks/6_Spirit_Temple.o:(.data+0x22ee0): multiple definition of `Windchimes'; build/assets/samplebanks/0_Sound_Effects.o:(.data+0x3a4be0): first defined here

maybe this hack made too many symbols visible but it doesn't look like it, from having rebuild audio assets with

        # added in elf.ELF.append_symbol
        if sym_binding != STB.STB_GLOBAL and sym_name not in {".data",}:
            print(sym_binding)
            raise Exception()

which would raise for any non-global symbol not called .data, yet this didn't raise when building

@fig02 fig02 added the Needs discussion There is a debate or other required discussion preventing changes from being made label Aug 27, 2022
Copy link
Collaborator

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

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

I read through the current Music Macro Language.md, very nice and useful document, even incomplete


**.define** *symbol* *value*

**Description:** Defines a symbol that may be used elsewhere in the sequence script. The symbol may consist of any Unicode letter, number, or underscores. The value must be a primitive value. Symbols must be defined only once in a sequence, similar to
Copy link
Collaborator

Choose a reason for hiding this comment

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

end of paragraph is cut/missing

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.

**Parameters:**

* `symbol` - The symbol being defined in the sequence.
* `value` - A primitive that will the symbol represents.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `value` - A primitive that will the symbol represents.
* `value` - A primitive that the symbol represents.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


**Section Metainstructions**

There are metainstructions specific to certain sections that will be documented in those sections specifically. These will be described in those sections as appropriate. Note that metainstructions are always distinguished by starting with a period, whereas instructions do not.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
There are metainstructions specific to certain sections that will be documented in those sections specifically. These will be described in those sections as appropriate. Note that metainstructions are always distinguished by starting with a period, whereas instructions do not.
There are metainstructions specific to certain sections that will be documented in those sections specifically. Note that metainstructions are always distinguished by starting with a period, whereas instructions do not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


Tracks are composed of three different types of instructions: sequence, channel, and note layer. The sequence contains instructions for controlling the overall sequence itself. Channels are defined at the sequence level, with at most sixteen channels running at the same time. Each channel can then reference zero or more note layers, where the actual note instructions will be referenced. Most of the instructions are specific to these subsections and are not interchangeable, aside from the *Branching instructions*, which are supported with all three. A sequence *always* starts with sequence instructions.

Tracks are executed by the **sequence player**, as defined in `audio_seqplayer.c`. The sequence player is capable of executing one track at a time, with sixteen simultaneous channels, each channel typically supporting four note layers. A tick counter allows the player to track the execution time of a sequence. Ticks increment based by the tempo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Tracks are executed by the **sequence player**, as defined in `audio_seqplayer.c`. The sequence player is capable of executing one track at a time, with sixteen simultaneous channels, each channel typically supporting four note layers. A tick counter allows the player to track the execution time of a sequence. Ticks increment based by the tempo.
Tracks are executed by the **sequence player**, as defined in `audio_seqplayer.c`. The sequence player is capable of executing one track at a time, with sixteen simultaneous channels, each channel typically supporting four note layers. A tick counter allows the player to track the execution time of a sequence. Ticks increment based on the tempo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


Tracks are executed by the **sequence player**, as defined in `audio_seqplayer.c`. The sequence player is capable of executing one track at a time, with sixteen simultaneous channels, each channel typically supporting four note layers. A tick counter allows the player to track the execution time of a sequence. Ticks increment based by the tempo.

All instructions in a track section are executed simultaneously unless a delay command is specified, in which case subsequent instructions will only run after that number of ticks have elapsed while playing the sequence.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
All instructions in a track section are executed simultaneously unless a delay command is specified, in which case subsequent instructions will only run after that number of ticks have elapsed while playing the sequence.
All instructions in a track section are executed simultaneously unless a delay command is specified, in which case subsequent instructions will only run after that number of ticks has elapsed while playing the sequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


.data

.array shortvel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.array shortvel
.array shortvels

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.

**Example:**
```
mutebhv 0x20 # Sets mute behavior to reduce sequence volume when muted
mutescale 40 # Reduce volume by 31.5% when muted
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mutescale 40 # Reduce volume by 31.5% when muted
mutescale 40 # Scale volume by 31.5% when muted

"reduce by 30%" would be "scale by 70%"

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


**initchan** *mask*

**Description:** Initializes the indicated channels on the sequence player and prepares them for playing audio. This configure's the channels with the default soundfont, assigned mute behavior, and configured note allocation policy. Each bit on the bitmask represents a different channel, with the most significant bit representing channel 16 and the least significant bit representing channel 1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**Description:** Initializes the indicated channels on the sequence player and prepares them for playing audio. This configure's the channels with the default soundfont, assigned mute behavior, and configured note allocation policy. Each bit on the bitmask represents a different channel, with the most significant bit representing channel 16 and the least significant bit representing channel 1.
**Description:** Initializes the indicated channels on the sequence player and prepares them for playing audio. This configures the channels with the default soundfont, assigned mute behavior, and configured note allocation policy. Each bit on the bitmask represents a different channel, with the most significant bit representing channel 16 and the least significant bit representing channel 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


**Description:** Initializes the indicated channels on the sequence player and prepares them for playing audio. This configure's the channels with the default soundfont, assigned mute behavior, and configured note allocation policy. Each bit on the bitmask represents a different channel, with the most significant bit representing channel 16 and the least significant bit representing channel 1.

It is not strictly necessary to use `initchan` for the channels to be used, however the previous mentioned settings will not be configured on the channel and will be in an undefined state. For obvious reasons, this is not recommended.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
It is not strictly necessary to use `initchan` for the channels to be used, however the previous mentioned settings will not be configured on the channel and will be in an undefined state. For obvious reasons, this is not recommended.
It is not strictly necessary to use `initchan` for the channels to be used, however the previously mentioned settings will not be configured on the channel and will be in an undefined state. For obvious reasons, this is not recommended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.


It's recommended to avoid using this command while audio is playing on the sequence, as this can result in undefined behavior.

In general, it is not necessary to use this command, as by default the sequence player will use any free notes in the global note pool. This command also is not useful for addressing issues with notes cutting out on channels in a sequence, as this applies to the sequence player itself. Howver, it can be useful for preventing other sequences from stealing notes from this sequence.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In general, it is not necessary to use this command, as by default the sequence player will use any free notes in the global note pool. This command also is not useful for addressing issues with notes cutting out on channels in a sequence, as this applies to the sequence player itself. Howver, it can be useful for preventing other sequences from stealing notes from this sequence.
In general, it is not necessary to use this command, as by default the sequence player will use any free notes in the global note pool. This command also is not useful for addressing issues with notes cutting out on channels in a sequence, as this applies to the sequence player itself. However, it can be useful for preventing other sequences from stealing notes from this sequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.

@Dragorn421
Copy link
Collaborator

Linking this here for reference

Zelda64 Audio Files Format by Tharo
https://hackmd.io/@qTHcM61qSZeJxGFR4GgSxg/rypABUFP9

Copy link
Contributor

@engineer124 engineer124 left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay in review, great work on this again! This review round mainly focuses on naming

Comment on lines +76 to +81
0xF1: ['allocnotelist', 'u8'],
0xF0: ['freenotelist'],
0xDF: ['transpose', 's8'],
0xDE: ['rtranspose', 's8'],
0xDD: ['tempo', 'u8'],
0xDC: ['tempochg', 's8'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking ahead (outside this PR), have you thought of a way to use these commands to better document audio_seqplayer.c? As a start, perhaps we could define three sets of defines for the three different types of commands, which we could use for the interpreter, and have a brief doc for each command in the case: where that command is called? Is there any way to have overlap for macros, or would we need to make a complete separate set of macro’s for audio_seqplayer.c

Copy link
Contributor

Choose a reason for hiding this comment

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

TBD.

Comment on lines +2 to +5
<Sequence Name="000_Sound_Effects" Extract="false"/>
<Sequence Name="001_Ambient_Effects" Extract="false"/>
<Sequence Name="002_Hyrule_Field_Program" Extract="false"/>
<Sequence Name="003_Hyrule_Field_Intro"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not particularly committed to matching original names if there's a better/more common name for a sequence. As long as the sequence names are clear and specific to the sequences they represent.

Also, I'm the one who made all the current sequence names in master, and can confirm I don't have a particular attachment to them. Any of them can be renamed if there's a better name.

Should probably take the same approach that we did naming scenes recently

Comment on lines +1 to +2
.table table_player_kid
entry player_child_step_dirt
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
.table table_player_kid
entry player_child_step_dirt
.table table_player_child
entry player_child_step_dirt

And all references to this table

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.

Comment on lines +129 to +131
entry player_arrow_light_charge
.table table_player_old
entry player_adult_step_dirt
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
entry player_arrow_light_charge
.table table_player_old
entry player_adult_step_dirt
entry player_arrow_light_charge
.table table_player_adult
entry player_adult_step_dirt

Spacing this out would be good

Copy link
Contributor

Choose a reason for hiding this comment

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

Applied in #1509.

Comment on lines +850 to +865
entry sword_glow # FX0: Master sword glow
entry sheik_transform # FX1: Sheik's transformation to Zelda
entry sage_seal # FX2: Sages accumulating their power
entry nayru_magic # FX3: Nayru's magic establishing order
entry farore_magic # FX4: Farore's magic creating life
entry din_magic # FX5: Din's building of the earth
entry lava_erupt # FX6: Lava erupting from Volvagia's pit
entry bongo_hurl_link # FX7: Link screaming while attacked by invisible Bongo Bongo
entry bongo_hover # FX8: Bongo Bongo hovering menacingly
entry bongo_death # FX9: Bongo Bongo disintegrating
entry trial_warp # FXA: Warping from trial barrier
entry trial_destroy # FXB: Destroying the trial barrier
entry dispel_barrier # FXC: Dispelling the Tower barrier
entry tower_collapse # FXD: Ganon's Tower's collapse
entry link_scream # FXE: Child Link screaming (unused)
entry rainfall # FXF: Rain with thunder effects
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
entry sword_glow # FX0: Master sword glow
entry sheik_transform # FX1: Sheik's transformation to Zelda
entry sage_seal # FX2: Sages accumulating their power
entry nayru_magic # FX3: Nayru's magic establishing order
entry farore_magic # FX4: Farore's magic creating life
entry din_magic # FX5: Din's building of the earth
entry lava_erupt # FX6: Lava erupting from Volvagia's pit
entry bongo_hurl_link # FX7: Link screaming while attacked by invisible Bongo Bongo
entry bongo_hover # FX8: Bongo Bongo hovering menacingly
entry bongo_death # FX9: Bongo Bongo disintegrating
entry trial_warp # FXA: Warping from trial barrier
entry trial_destroy # FXB: Destroying the trial barrier
entry dispel_barrier # FXC: Dispelling the Tower barrier
entry tower_collapse # FXD: Ganon's Tower's collapse
entry link_scream # FXE: Child Link screaming (unused)
entry rainfall # FXF: Rain with thunder effects
entry sword_glow # FX0: Master sword glow
entry sheik_transform # FX1: Sheik's transformation to Zelda
entry sage_seal # FX2: Sages accumulating their power
entry farore_magic # FX3: Farore's magic creating life
entry nayru_magic # FX4: Nayru's magic establishing order
entry din_magic # FX5: Din's building of the earth
entry lava_erupt # FX6: Lava erupting from Volvagia's pit
entry bongo_hurl_link # FX7: Link screaming while attacked by invisible Bongo Bongo
entry bongo_hover # FX8: Bongo Bongo hovering menacingly
entry bongo_emerges # FX9: Bongo Bongo emerging from the well
entry trial_warp # FXA: Warping from trial barrier
entry trial_destroy # FXB: Destroying the trial barrier
entry dispel_barrier # FXC: Dispelling the Tower barrier
entry tower_collapse # FXD: Ganon's Tower's collapse
entry link_scream # FXE: Child Link screaming (unused)
entry rainfall # FXF: Rain with thunder effects

This is what I found from looking in code, some small fixes

Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed bongo_death to bongo_emerges in #1509, but you can't swap around farore_magic and nayru_magic as that breaks decomp.

Comment on lines +135 to +137
0xDB: ['transpose', 's8'],
0xDA: ['env', 'addr'],
0xD9: ['releaserate', 'u8'],
Copy link
Contributor

Choose a reason for hiding this comment

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

env is also used as an abbreviation for environment (think env sound bank), so I think it's worth having envelope here, which is still shorter than it's surrounding commands

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, applied in #1509.

0xC3: ['short'],
0xC2: ['dyntbl', 'addr'],
0xC1: ['instr', 'u8'],
0xBD: ['randptr', 'u16', 'u16'],
Copy link
Contributor

Choose a reason for hiding this comment

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

making a note that randptr is the one instruction that changes OP in MM compared to OoT, any thoughts how to handle this? (doesn't need addressing here)

Copy link
Contributor

Choose a reason for hiding this comment

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

TBD. What does OP mean in this context?

Comment on lines +965 to +967
**cdelay** *ticks*

**Description:** Delays processing of the channel script by the provided number of ticks (up to 16). A delay of 0 is effectively a no-op.
Copy link
Contributor

Choose a reason for hiding this comment

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

why c in cdelay? Does that mean channel? Would chandelay make more sense for consistency? Is that c the same in stcio and ldcio?

Copy link
Contributor

Choose a reason for hiding this comment

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

TBD.

Comment on lines +217 to +219
0x00: ['notedvg', 'bits:6', 'var', 'u8', 'u8'],
0x40: ['notedv', 'bits:6', 'var', 'u8'],
0x80: ['notevg', 'bits:6', 'u8', 'u8'],
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 think these are documented in MML yet. It would be good to get these in since dvg/dv/vg means very little without documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

TBD.

Comment on lines +86 to +89
{ { SectionType::Seq }, 0xF1, "allocnotelist", { Arg::u8 } },
{ { SectionType::Seq }, 0xF0, "freenotelist", {} },
{ { SectionType::Seq }, 0xDF, "transpose", { Arg::s8 } },
{ { SectionType::Seq }, 0xDE, "rtranspose", { Arg::s8 } },
Copy link
Contributor

Choose a reason for hiding this comment

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

All these seq instructions are basically copied twice: once in c++ (assemble_sequence.cpp) and once in python (disassemble_sequence.py). Is there any way to consolidate them into one place?

Copy link
Contributor

@playerskel playerskel Mar 30, 2023

Choose a reason for hiding this comment

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

One could port the assembler to Python or the disassembler to C++, but keeping it as-is is probably easiest. Doing one pass of disassembly + assembly + decomp check should probably catch most inconsistency bugs, except for instructions that aren't used in the game of course.

Comment on lines +1 to +9
.table table_system
entry system_menu_open
entry system_menu_close
entry system_puzzle_solved
entry system_rupee
entry system_timer_ding
entry system_error
entry system_error
entry system_treasure_appear
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
.table table_system
entry system_menu_open
entry system_menu_close
entry system_puzzle_solved
entry system_rupee
entry system_timer_ding
entry system_error
entry system_error
entry system_treasure_appear
.table table_system
entry system_menu_open # NA_SE_SY_WIN_OPEN
entry system_menu_close # NA_SE_SY_WIN_CLOSE
entry system_puzzle_solved # NA_SE_SY_CORRECT_CHIME
entry system_rupee # NA_SE_SY_GET_RUPY
entry system_timer_ding # NA_SE_SY_MESSAGE_WOMAN
entry system_error # NA_SE_SY_MESSAGE_MAN
entry system_error # NA_SE_SY_ERROR
entry system_treasure_appear # NA_SE_SY_TRE_BOX_APPEAR

This is more of an idea, but it would be really nice if the sfx enums were mapped to the table entries here so it can be much faster finding the sfx, and It'll be easier syncing the names once we find user-friendly names for all the sfx enum

Copy link
Contributor

Choose a reason for hiding this comment

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

TBD. Could do.

Comment on lines +1030 to +1037
stseq 0, layer_6576 # set semitone = sound id - 119
ldseq addr_657B
stseq 0, layer_6576+2
ldlayer 0, layer_6574
end

.layer layer_6574
transpose 2
Copy link
Contributor

Choose a reason for hiding this comment

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

In the "fonts", there are three distinct types of sound: "Instruments", "drums", "sound effects".

Currently, semitone and transpose are named after how they are used for "Instruments", and they are decent name.

However, these two variables have completely different meaning when it comes to sound effects (this particular code is part of the infamous "Mweep" sfx").

"semitone" for sound effects actually stores the lower bit of the sfxId (the index to the list of soundeffects in the font, and NOT the id's for sfx that are exposed externally to audio)

"transpose" for sound effects stores the upper bit of the sfxId.

You can see the code for this here (which needs updated docs, probably unions to represent this different functionality):

sfxId = (layer->transposition << 6) + semitone;

Now regarding the instructions here, would it make sense to have different named commands representing the same OP instruction, since it has different meaning depending on if it's processing an instrument/drum/sound effect?

Copy link
Contributor

@engineer124 engineer124 Feb 7, 2023

Choose a reason for hiding this comment

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

Particularly, semitone and transpose are set so that it constructs this enum value:
https://github.com/MNGoldenEagle/oot/blob/fb5bcb9e5e9bfa5e9735c9a3015aa32d896a1767/assets/xml/soundfonts/Sound%20Effects%201.xml#L235

Here's the steps below on how it achieves that:

There's the "external" mweep sfxId:

/* 0x687A */ DEFINE_SFX(NA_SE_VO_KZ_MOVE, 0x30, 1, 1, SFX_FLAG_15)

Which the table entry ("sound id as you have in the comment above") is extracted from that via the mask & 0x1FF, i.e. 0x7A or 122.

Then, stseq 0, layer_6576 # set semitone = sound id - 119 is evaluated to 3

So the semitone = 3 and transposition = 2

Those are the 2 values needed to construct the internal sfx:
sfxId = (layer->transposition << 6) + semitone;
sfxId = (2 << 6) + 3;
sfxId = 131

Looking back at:
https://github.com/MNGoldenEagle/oot/blob/fb5bcb9e5e9bfa5e9735c9a3015aa32d896a1767/assets/xml/soundfonts/Sound%20Effects%201.xml#L235
The <Effect Name="King Zora Scoots" Enum="MWINK"/> is the 132'nd sound effect entry in that font (index 131). In fact, the enum MWINK is valued at 131. So the sfxId = 131 or sfxId = MWINK is set to get that entry.

So is there any way to have transposition and semitone set so that it uses the enum MWINK and not have it hardcoded? I don't see any easy solution but just wanted to walk through things

Copy link
Contributor

Choose a reason for hiding this comment

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

TBD. Interesting stuff though.

@fig02
Copy link
Collaborator

fig02 commented Sep 19, 2023

Superseded by #1509

Thanks again for all of your hard work in getting this process started!

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.

8 participants