Add custom music support to Jukebox #19
Conversation
|
Hey there! This looks like something we would be willing to merge. @DarylPinto will take a look within a few days hopefully. I agree with your suggestions: I think the music would be better placed at What is the behavior if the HPS offset is not detected exactly? |
Gotcha, I'll make the changes.
It just falls back to the previous behavior and plays the vanilla stage music. If |
|
Hey there, I'm the author of Jukebox! Thanks for implementing this feature - it's a great idea, and I'm sure people will get a kick out of adding their own songs. I have some feedback about the implementation details that I'll put into a follow-up comment. Feel free to reach out to me on Discord in the |
jukebox/src/utils.rs
Outdated
| 71324300 | 183962648 => "final_destination", | ||
| 67606252 | 192481656 => "battlefield", | ||
| 260526192 => "yoshis_story", | ||
| 86968172 => "fountain_of_dreams", | ||
| 135559532 => "dreamland", | ||
| 156902380 | 160483852 => "pokemon_stadium", | ||
| // Misc | ||
| 116194572 | 114173548 | 112087660 => "menu", | ||
| 227529496 => "target_test", | ||
| // Other | ||
| 258226448 | 180321356 => "yoshis_island", | ||
| 138041836 => "yoshis_island_2", | ||
| 129809196 => "kongo_jungle", | ||
| 102329228 => "brinstar_depths", | ||
| 40067628 => "fourside", | ||
| 12767116 | 119550124 => "big_blue", | ||
| 163012812 => "poke_floats", | ||
| 79192940 | 24668300 => "kingdom", | ||
| 83914380 => "kingdom_2", | ||
| 264873488 => "brinstar", | ||
| 144477996 | 140261388 => "onett", | ||
| 124345132 => "mute_city", | ||
| 168831468 => "rainbow_cruise", | ||
| 94385004 => "jungle_japes", | ||
| 173785356 => "temple", | ||
| 56785644 => "green_greens", | ||
| 250920912 => "venom", | ||
| 10628844 | 75159404 => "icicle_mountain", | ||
| 17422220 => "princess_peachs_castle", | ||
| 45632108 => "kongo_jungle", | ||
| 172460780 | 54558348 => "great_bay", | ||
| 21447116 => "corneria", | ||
| 36096908 => "flatzone", | ||
| _ => "", |
There was a problem hiding this comment.
While this approach works and is something that was used in very early versions of jukebox, it can be a quite brittle. I highly encourage making use of what is known as the FST (Filesystem Table). It's essentially a table of contents that lists every file on the disc, and it's offset in the ISO. Fortunately, I've already written code to work with this in an earlier version of Jukebox: https://github.com/project-slippi/slippi-rust-extensions/blob/ec8db587/jukebox/src/fst/mod.rs#L20-L89
In this code I walk the FST and produce a hashmap of TrackIDs (an arbitrary enum) and the associated song's offset and length in bytes: HashMap::<TrackId, (Offset, Length)>
In your case, you would want to modify this so the hashmap that's produced is something like HashMap::<Offset, StageDirPath> where Offset: usize and StageDirPath: std:path::Path
{
0x123456: Path("/path/to/User/Slippi/Music/yoshis_story"),
0xABCDEF: Path("/path/to/User/Slippi/Music/final_destination"),
...
}
For example: you load into Yoshi's Story, you receive 0x123456 from start_song() and can look up the associated custom music directory in the hashmap with:
let stage_dir = custom_song_path_map.get(0x123456);Obviously the FST doesn't contain a list of your custom folder paths in it (since we are making them up), but it does contain the names of specific files. You will need an intermediary match that associates each hps filename with it's corresponding stage_dir:
match filename {
"ystory.hps" => Some(Path::new("/path/to/User/Slippi/Music/yoshis_story")),
"sp_end.hps" => Some(Path::new("/path/to/User/Slippi/Music/final_destination"))
...
_ => None
}You can use this match as a starting point: https://github.com/project-slippi/slippi-rust-extensions/blob/ec8db587/jukebox/src/tracks.rs#L59-L108
I know this FST stuff is a lot of esoteric mumbo jumbo, so feel free to message me on Discord and I'll be happy to help you along.
There was a problem hiding this comment.
Ooh I like this, thanks! I was really worried about edge cases, this is great.
jukebox/src/lib.rs
Outdated
| let mut custom_song_path = None; | ||
| if let Some(stage) = hps_to_stage(real_hps_offset) { | ||
| let stage_dir = Path::new(&jukebox_path).join(stage); | ||
| if let Ok(entries) = read_dir(&stage_dir) { | ||
| // Get all files in folder | ||
| let files: Vec<_> = entries | ||
| .filter_map(|entry| { | ||
| if let Ok(entry) = entry { | ||
| let path = entry.path(); | ||
| if path.is_file() { | ||
| if let Some(extension) = path.extension().and_then(|extension| extension.to_str()) { | ||
| if ["mp3", "wav", "ogg", "flac"].contains(&extension.to_lowercase().as_str()) { | ||
| return Some(path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Decode the Hps into audio | ||
| let audio = match hps.decode() { | ||
| Ok(audio) => audio, | ||
| Err(e) => { | ||
| tracing::error!(target: Log::Jukebox, error = ?e, "Failed to decode hps into audio. Cannot play song."); | ||
| Dolphin::add_osd_message( | ||
| Color::Red, | ||
| OSDDuration::Normal, | ||
| "Invalid music data found in ISO. This music will not play.", | ||
| ); | ||
| continue; | ||
| }, | ||
| }; | ||
| // Choose a random file from the stage folder if available | ||
| if !files.is_empty() { | ||
| if let Some(random_file) = files.choose(&mut rand::thread_rng()) { | ||
| custom_song_path = Some(random_file.clone()) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Append stage audio to sink | ||
| if let Some(custom_song_path) = custom_song_path { | ||
| match File::open(custom_song_path) { | ||
| Ok(custom_song_file) => { | ||
| if let Ok(custom_song) = rodio::Decoder::new(BufReader::new(custom_song_file)) { | ||
| sink.append(custom_song.repeat_infinite()); | ||
| } | ||
| }, | ||
| Err(e) => { | ||
| tracing::error!(target: Log::Jukebox, error = ?e, "Failed to open custom song. Cannot play song."); | ||
| continue; | ||
| }, | ||
| } |
There was a problem hiding this comment.
A few notes about this:
- I strongly suggest moving this logic into it's own function to keep
start()terse and easy to read. - In a function you can greatly improve readability by reducing nesting. Instead of:
you can simply do:
if let Some(foo) = get_foo() {
let foo = get_foo()?;
- You should call as much of this as possible before the main loop as to avoid reading from the disk unnecessarily every time a new song loads.
There was a problem hiding this comment.
I actually was originally using the try operator but I removed it so it just falls back on the default behavior instead of erroring out if anything goes wrong with the custom audio. Do you agree or should I just assume that if the right folders are present, any issues down the line should actually early return?
Edit: Given that I'm moving it to a separate function this might not even be an issue, you can probably ignore this comment
| @@ -84,8 +84,11 @@ impl SlippiEXIDevice { | |||
| initial_dolphin_music_volume, | |||
| } = config | |||
| { | |||
| // TODO: Consider passing in user path directly and appending | |||
| let jukebox_path = self.config.paths.user_json.replace("user.json", "Jukebox"); | |||
There was a problem hiding this comment.
There might be a better way to do this. Thoughts @ryanmcgrath?
There was a problem hiding this comment.
This'll be easier once #18 is merged, since that migrates these kinds of things to true path types. Dunno if it's worth mucking with it here but I'll defer to whatever Nikki wants.
jukebox/src/lib.rs
Outdated
| .filter_map(|entry| { | ||
| if let Ok(entry) = entry { | ||
| let path = entry.path(); | ||
| if path.is_file() { | ||
| if let Some(extension) = path.extension().and_then(|extension| extension.to_str()) { | ||
| if ["mp3", "wav", "ogg", "flac"].contains(&extension.to_lowercase().as_str()) { | ||
| return Some(path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| }) |
There was a problem hiding this comment.
This can be simplified using ? early returns for a less nested structure:
.filter_map(|entry| {
let path = entry.ok()?.path();
if !path.is_file() {
return None;
}
let extension = path.extension()?.to_str()?.to_lowercase();
match extension.as_str() {
"mp3" | "wav" | "ogg" | "flac" => Some(path),
_ => None,
}
})The logic might not be correct, so be sure to verify
Thanks for all the comments! I'm a bit swamped right now but I should have it up to date within a couple of days. |
Adds the ability for users to use their own music with Jukebox.
Usage
In order to load custom music, all the user has to do is:
musicdirectory in the same directory as their Melee ISO<stage>directories within themusicfolder<stage>directory with audio filesE.g:
musicpokemon_stadiumsong1.oggsong2.mp3Jukebox will then pick a track at random when loading a stage. The naming convention for stage directories is fully lowercase, no symbols, using
_as a separator.While fully functional, I have a few questions. This is my first PR here so I made it as unobtrusive as possible, but there are some things to consider:
musicdirectory tojukebox?User/Slippi/Jukebox? I went with the ISO path because it was already available and made enough sense. Also I don't know if putting it inside the Slippi folder could make the files get lost when resetting the emulator, switching mainline/beta, etc. Could also potentially be a config option.I've been using this for a couple of days with no issues.