-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 - sounds now affected by artifacts/CBMs that slow down time. #79812
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
[JSON & C++ formatters](https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/c++/DEVELOPER_TOOLING.md)
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 804 to 805 in ea12b66
dbg(D_WARNING) << "Mix_RegisterEffect failed: " << Mix_GetError(); | |
Mix_HaltChannel(channel); |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 808 to 809 in ea12b66
if (!failed && angle.has_value()) { | |
if (Mix_SetPosition(channel, static_cast<Sint16>(to_degrees(*angle)), 1) == 0) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Line 811 in ea12b66
dbg(D_INFO) << "Mix_SetPosition failed: " << Mix_GetError(); |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 814 to 815 in ea12b66
if (failed) { | |
on_finish(-1, handler); |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Line 871 in ea12b66
// if consume_passed_chunk == true, s will be freed, if not it will be unaffected by this call. |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Line 882 in ea12b66
// result->alen = n_samples_out * 4; |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 924 to 925 in ea12b66
bool error = sound_effect_handler::make_audio(static_cast<int>(channel::any), effect_to_play, 0, volume, false, selected_sound_effect, std::nullopt, std::nullopt); | |
if (error) { |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Line 952 in ea12b66
bool is_pitched = (pitch_min > 0) && (pitch_max > 0); |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Line 956 in ea12b66
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 958 to 959 in ea12b66
double pitch_mod = rng_float(pitch_min, pitch_max); | |
effect_to_play = do_pitch_shift( effect_to_play, static_cast<float>(pitch_mod) ); |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 962 to 965 in ea12b66
bool error = sound_effect_handler::make_audio(static_cast<int>(channel::any), effect_to_play, 0, volume, destroy_sound, selected_sound_effect, std::make_optional(angle), std::nullopt); | |
if (error) | |
dbg(D_ERROR) << "Failed to play sound effect: " << Mix_GetError() << " id:" << id | |
<< " variant:" << variant << " season:" << season; |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Line 995 in ea12b66
if (is_pitched) effect_to_play = do_pitch_shift(effect_to_play, static_cast<float>(pitch)); |
[JSON & C++ formatters] reported by reviewdog 🐶
Cataclysm-DDA/src/sdlsound.cpp
Lines 997 to 998 in ea12b66
volume = selected_sound_effect.volume * get_option<int>("AMBIENT_SOUND_VOLUME") * volume / (100 * 100); | |
bool failed = sound_effect_handler::make_audio(static_cast<int>(channel), effect_to_play, loops, volume, destroy_sound, selected_sound_effect, std::nullopt, fade_in_duration); |
oh, don't worry about the commit called "this commit should not get pushed" |
Some quick notes: You have some errant changes in iexamine.h. You need to run astyle on your changes. You have large chunks of commented out code, those should be removed unless there is some extraordinary reason to keep them |
I ran astyle on sdlsound.cpp (I thought it ran that automatically?) and got rid of the commented-out code I missed. I think I also undid the unintentional formatting change to iexamine.h. |
Please resolve clang errors https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/13485364959/job/37689203934?pr=79812#step:7:476 and |
nb. if the commit stack doesn't get cleaned up, mergers should squash this one. The commits adding and removing cc-sounds will add to the internal git bloat of the repo pretty badly. |
How do I squash commits? I'm not particularly experienced with Git and it didn't occur to me that git will track me doing that. Also, do I need to worry about some mapgen test failing if I didn't touch anything outside sounds? |
No, unfortunately that test is just known to be flaky. It's not failing for a required build so nothing to consider on your end. |
Ok, thanks! |
Summary
Features "Makes sound effects play in slow motion while the player has slowed down time"
Purpose of change
Time-slowing artifacts and the time dilation CBM are really cool, but the way they just "add moves" can make them feel a bit unimmersive/mechanical. This PR also makes it simpler for other people to add other cool post-processing to sound effects.
Describe the solution
Significantly modified both overloads of play_variant_sound() and play_ambient_variant_sound() to use SDL_Mixer's RegisterEffect() method to allow arbitrary modification of the sound data before it plays. For this change specifically, it just stretches out the samples when time is slowed down, resulting in longer sound length and lower pitch. A new struct sound_effect_handler for every currently playing sound effect helps facilitate this. As a positive side effect, while modifying those functions I was able to remove a lot of duplicated code. I also noticed that while in the past (for many years) sound effects have occasionally not played when many were simultaneously playing, but after I made my changes that was no longer an issue, so that's cool.
To determine if time was slowed down, I just see if the player has more than twice as many moves as they should have given their speed.
(I CTRL-Fed through every piece of code that modified the player's amount of moves; other than unit tests, CBMs, and artifacts nothing increases the player's moves above their current speed so this should be fine. If something more than doubled the player's speed in a single turn without changing the amount of moves they had, this would fail, but I doubt that would happen)
Performance shouldn't be a real concern since registered effects are called by SDL_Mixer on a separate audio thread, so it shouldn't be able to affect actual gameplay performance.
Describe alternatives you've considered
One could simply play dramatic sounds when time is slowed down by an artifact/CBM.
Also, this change does not affect music since it's played in a different way. Maybe it should?
Testing
I tested this with CC_Sounds. I went around shooting, closing doors (both normally and violently spamming), and fighting zombies. While doing so I repeatedly slowed down time using the time dilation CBM.
It's possible there's still a rare crash caused by this, but I think I fixed it. I'm not sure how to confirm that, though.
Additional context
Strange artifacts occur if the sound_speed_factor isn't a power of two like 0.25 or 0.5. This is fine for this PR, but if future code wants to do something like smoothly changing the playback speed over time, they'll have to make changes.