Update mixer example to run sound effect in addition to music #666
Conversation
There was a problem hiding this comment.
It would have been nicer to make the sound effects optional, so that you don't have to have a sound effect on top of a music to test a basic example of mixer.
If there are sound effects passed as parameters: play them, otherwise, just play the music like it used to be. How does that sound?
|
|
||
| // Number of mixing channels available for sound effect `Chunk`s to play | ||
| // simultaneously. | ||
| sdl2::mixer::allocate_channels(4); |
There was a problem hiding this comment.
Does this mean that you can play 4 sound effects + the usual music with that? Is that why iy wouldn't work with sdl2::mixer::allocate_channel(0)?
There was a problem hiding this comment.
Yes, exactly. If you allocate_channel(0) you cannot play sound effects.
See: https://www.libsdl.org/projects/SDL_mixer/docs/SDL_mixer_26.html
Set the number of channels being mixed. This can be called multiple times, even with sounds playing. If numchans is less than the current number of channels, then the higher channels will be stopped, freed, and therefore not mixed any longer. It's probably not a good idea to change the size 1000 times a second though.
If any channels are deallocated, any callback set by Mix_ChannelFinished will be called when each channel is halted to be freed. Note: passing in zero WILL free all mixing channels, however music will still play.
The rust-sdl2 docs for that function should probably be updated.
There was a problem hiding this comment.
Indeed, it should probably be updated, but I intended to rewrite it completely, and add tons of meaningful comments with it as well (#646). But in the meantime a small comment about what this does would be much needed indeed.
|
@Cobrand Making sound effects optional sounds good (and I actually considered it for a little bit). I'll work on adding that. |
examples/mixer_demo.rs
Outdated
| } else { | ||
| if args.len() == 2 { | ||
| demo(Path::new(&args[1]), Path::new("")); | ||
| } else if args.len() == 3 { |
There was a problem hiding this comment.
This is a pretty un-rusty way to do this, in Rust we have nice things like options instead of empty strings, so let's use these instead! Make demo have this signature: demo(music_path: &Path, sound_path: Option<&Path>), and just pass args.get(2) to actually have an Option<&Path>. That effectively means merging the branches args.len() == 2 and args.len() == 3 as one here.
In demo, if the option is None, don't output anything. If it's Some(path) but the path is wrong, output an error, and if it's a correct sound file, just output the same thing.
There was a problem hiding this comment.
Trying to use .get(2) directly as the input seems to not work since we need to re-wrap into a new Option<T>.
25 | demo(Path::new(&args[1]), args.get(2));
| ^^^^^^^^^^^ expected struct `std::path::Path`, found struct `std::string::String`
|
= note: expected type `std::option::Option<&std::path::Path>`
found type `std::option::Option<&std::string::String>`
Let me know if there is a more idiomatic way to do this than what I did, which was to de-structure and re-structure.
examples/mixer_demo.rs
Outdated
| } else if args.len() == 3 { | ||
| demo(Path::new(&args[1]), Path::new(&args[2])); | ||
| if args.len() > 2 { | ||
| let sound_file_path; |
There was a problem hiding this comment.
let sound_file = args.get(2).map(|sound_file| Path::new(sound_file)) doesn't work here? Much easier to read and shorter as well.
There was a problem hiding this comment.
Thanks for the tips! (Been doing too much C++ lately where functional programming just isn't really ergonomic).
examples/mixer_demo.rs
Outdated
| match play_res { | ||
| Ok(_) => { println!("played sound")}, | ||
| Err(e) => { println!("{:?}", e) }, | ||
| match sound_file { |
There was a problem hiding this comment.
if let Some(sound_file_path) = sound_file { and get rid of the None branch ;)
|
Side note, should the file be renamed |
|
I thought I renamed all XXX_demo.rs into XXX-demo.rs, but it looks like I missed this one. Please do rename! |
|
Looks great! Please squash of all these commits into a single one that makes sense, and I'll merge that. |
|
@Cobrand I've used GitHubs "Squash and Merge" button before as a repo owner. Is that an option? For the message I'd just use the title of the PR if you like that. |
|
I think I tried it at some point but I refrained from using it for some reason, but I don't remember why. I'll squash this via github and we'll this how it goes. |
Closes #665