widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Review: Approve reiew, compile, testplay

I played some internet game with this brnahc and it was ok, felt a bit slow, though.

Completed the review, code is fine, please check some nits:
 * some functions should be inlined.
 * Once this is in r21 I would like to do a performace review
 * Mabye we could make more of this run in a seperate thread / cpu?

This can go in in r21 anytime now.



Diff comments:

> 
> === modified file 'src/sound/sound_handler.cc'
> --- src/sound/sound_handler.cc	2019-02-23 11:00:49 +0000
> +++ src/sound/sound_handler.cc	2019-04-09 04:52:18 +0000
> @@ -199,476 +176,461 @@
>  		fx_lock_ = nullptr;
>  	}
>  
> -	songs_.clear();
> -	fxs_.clear();
> -
>  	Mix_Quit();
>  	SDL_QuitSubSystem(SDL_INIT_AUDIO);
>  }
>  
> -/** Read the main config file, load background music and systemwide sound fx
> - *
> +/// Prints an error and disables and shuts down the sound system.
> +void SoundHandler::initialization_error(const char* const msg, bool quit_sdl) {
> +	log("WARNING: Failed to initialize sound system: %s\n", msg);
> +	SoundHandler::disable_backend();
> +	if (quit_sdl) {
> +		SDL_QuitSubSystem(SDL_INIT_AUDIO);
> +	}
> +	return;
> +}
> +
> +/**
> + * Load the sound options from g_options. If an option is not available, use the defaults set by the constructor.
>   */
>  void SoundHandler::read_config() {
> -	Section& s = g_options.pull_section("global");
> -
> -	if (nosound_) {
> -		set_disable_music(true);
> -		set_disable_fx(true);
> -	} else {
> -		set_disable_music(s.get_bool("disable_music", false));
> -		set_disable_fx(s.get_bool("disable_fx", false));
> -		music_volume_ = s.get_int("music_volume", kDefaultMusicVolume);
> -		fx_volume_ = s.get_int("fx_volume", kDefaultFxVolume);
> -	}
> -
> -	random_order_ = s.get_bool("sound_random_order", true);
> -
> -	register_song("music", "intro");
> -	register_song("music", "menu");
> -	register_song("music", "ingame");
> -}
> -
> -/** Load systemwide sound fx into memory.
> - * \note This loads only systemwide fx. Worker/building fx will be loaded by
> - * their respective conf-file parsers
> - */
> -void SoundHandler::load_system_sounds() {
> -	load_fx_if_needed("sound", "click", "click");
> -	load_fx_if_needed("sound", "create_construction_site", "create_construction_site");
> -	load_fx_if_needed("sound", "message", "message");
> -	load_fx_if_needed("sound/military", "under_attack", "military/under_attack");
> -	load_fx_if_needed("sound/military", "site_occupied", "military/site_occupied");
> -	load_fx_if_needed("sound", "lobby_chat", "lobby_chat");
> -	load_fx_if_needed("sound", "lobby_freshmen", "lobby_freshmen");
> -}
> -
> -/**
> - * Returns 'true' if the playing of sounds is disabled due to sound driver problems.
> - */
> -bool SoundHandler::is_backend_disabled() const {
> -	return is_backend_disabled_;
> -}
> -
> -/** Load a sound effect. One sound effect can consist of several audio files
> +	// TODO(GunChleoc): Compatibility code to avoid getting bug reports about unread sections. Remove after Build 21.
> +	if (g_options.get_section("sound") == nullptr) {
> +		Section& global = g_options.pull_section("global");
> +
> +		for (auto& option : sound_options_) {
> +			switch (option.first) {
> +			case SoundType::kMusic:
> +				option.second.volume = global.get_int("music_volume", option.second.volume);
> +				option.second.enabled = !global.get_bool("disable_music", !option.second.enabled);
> +				break;
> +			case SoundType::kChat:
> +				option.second.volume = global.get_int("fx_volume", option.second.volume);
> +				option.second.enabled = global.get_bool("sound_at_message", option.second.enabled);
> +				break;
> +			default:
> +				option.second.volume = global.get_int("fx_volume", option.second.volume);
> +				option.second.enabled = !global.get_bool("disable_fx", !option.second.enabled);
> +				break;
> +			}
> +		}
> +		save_config();
> +	}
> +
> +	// This is the code that we want to keep
> +	Section& sound = g_options.pull_section("sound");
> +	for (auto& option : sound_options_) {
> +		option.second.volume = sound.get_int(("volume_" + option.second.name).c_str(), option.second.volume);
> +		option.second.enabled = sound.get_bool(("enable_" + option.second.name).c_str(), option.second.enabled);
> +	}
> +}
> +
> +/// Save the current sound options to g_options
> +void SoundHandler::save_config() {
> +	Section& sound = g_options.pull_section("sound");
> +	for (auto& option : sound_options_) {
> +		const int volume = option.second.volume;
> +		const std::string& name = option.second.name;
> +		const bool enabled = option.second.enabled;
> +
> +		const std::string enable_name = "enable_" + name;
> +		sound.set_bool(enable_name.c_str(), enabled);
> +
> +		const std::string volume_name = "volume_" + name;
> +		sound.set_int(volume_name.c_str(), volume);
> +	}
> +}
> +
> +/// Read the sound options from g_options and apply them
> +void SoundHandler::load_config() {
> +	read_config();
> +	for (auto& option : sound_options_) {
> +		set_volume(option.first, option.second.volume);
> +		set_enable_sound(option.first, option.second.enabled);
> +	}
> +}
> +
> +/**
> + * Returns 'true' if the playing of sounds is disabled due to sound driver problems, or because disable_backend() was used.
> + */
> +bool SoundHandler::is_backend_disabled() {

This and the flooowing one should be inline

> +	return SoundHandler::backend_is_disabled_;
> +}
> +
> +/**
> + * Disables all sound.
> + */
> +void SoundHandler::disable_backend() {
> +	SoundHandler::backend_is_disabled_ = true;
> +}
> +
> +/** Register a sound effect. One sound effect can consist of several audio files
>   * named EFFECT_XX.ogg, where XX is between 00 and 99.
>   *
>   * Subdirectories of and files under FILENAME_XX can be named anything you want.
>   *
> - * \param dir        The relative directory where the audio files reside in data/sound
> - * \param filename   Name from which filenames will be formed
> - *                   (BASENAME_XX.ogg);
> - *                   also the name used with \ref play_fx
> - */
> -void SoundHandler::load_fx_if_needed(const std::string& dir,
> -                                     const std::string& basename,
> -                                     const std::string& fx_name) {
> -	assert(g_fs);
> -
> -	if (!g_fs->is_directory(dir)) {
> -		throw wexception("SoundHandler: Can't load files from %s, not a directory!", dir.c_str());
> -	}
> -
> -	if (nosound_ || fxs_.count(fx_name) > 0)
> -		return;
> -
> -	fxs_.insert(std::make_pair(fx_name, std::unique_ptr<FXset>(new FXset())));
> -
> -	boost::regex re(basename + "_\\d+\\.ogg");
> -	FilenameSet files = filter(g_fs->list_directory(dir), [&re](const std::string& fn) {
> -		return boost::regex_match(FileSystem::fs_filename(fn.c_str()), re);
> -	});
> -
> -	for (const std::string& path : files) {
> -		assert(!g_fs->is_directory(path));
> -		load_one_fx(path, fx_name);
> -	}
> -}
> -
> -/** Add exactly one file to the given fxset.
> - * \param path      the effect to be loaded
> - * \param fx_name   the fxset to add the file to
> - * The file format must be ogg. Otherwise this call will complain and
> - * not load the file.
> - * \note The complete audio file will be loaded into memory and stays there
> - * until the game is finished.
> - */
> -void SoundHandler::load_one_fx(const std::string& path, const std::string& fx_name) {
> -	if (nosound_ || is_backend_disabled_) {
> -		return;
> -	}
> -
> -	FileRead fr;
> -	if (!fr.try_open(*g_fs, path)) {
> -		log("WARNING: Could not open %s for reading!\n", path.c_str());
> -		return;
> -	}
> -
> -	if (Mix_Chunk* const m =
> -	       Mix_LoadWAV_RW(SDL_RWFromMem(fr.data(fr.get_size(), 0), fr.get_size()), 1)) {
> -		// Make sure that requested FXset exists
> -
> -		assert(fxs_.count(fx_name) > 0);
> -
> -		fxs_[fx_name]->add_fx(m);
> -	} else
> -		log("SoundHandler: loading sound effect \"%s\" for FXset \"%s\" "
> -		    "failed: %s\n",
> -		    path.c_str(), fx_name.c_str(), Mix_GetError());
> -}
> -
> -/** Find out whether to actually play a certain effect right now or rather not
> - * (to avoid "sonic overload").
> - */
> -// TODO(unknown): What is the selection algorithm? cf class documentation
> -bool SoundHandler::play_or_not(const std::string& fx_name,
> -                               int32_t const stereo_pos,
> + * \param type       The category of the FxSet to create
> + * \param fx_path    The relative path and base filename from which filenames will be formed
> + *                   (<datadir>/fx_path_XX.ogg). If an effect with the same 'type' and 'fx_path' already exists, we assume that it is already registered and skip it.
> + * \returns  An ID for the effect that can be used to identify it in \ref play_fx.
> + */
> +
> +FxId SoundHandler::register_fx(SoundType type, const std::string& fx_path) {
> +	if (SoundHandler::is_backend_disabled() || g_sh == nullptr) {
> +		return kNoSoundEffect;
> +	}
> +	return g_sh->do_register_fx(type, fx_path);
> +}
> +
> +/// Non-static implementation of register_fx
> +FxId SoundHandler::do_register_fx(SoundType type, const std::string& fx_path) {
> +	assert(!SoundHandler::is_backend_disabled());
> +	if (fx_ids_[type].count(fx_path) == 0) {
> +		const FxId new_id = fxs_[type].size();
> +		fx_ids_[type].insert(std::make_pair(fx_path, new_id));
> +		fxs_[type].insert(std::make_pair(new_id, std::unique_ptr<FXset>(new FXset(fx_path, rng_.rand()))));
> +		return new_id;
> +	} else {
> +		return fx_ids_[type].at(fx_path);
> +	}
> +}
> +
> +/**
> + * Find out whether to actually play a certain effect right now or rather not
> + * (to avoid "sonic overload"). Based on priority and on when it was last played.
> + * System sounds and sounds with priority "kFxPriorityAlwaysPlay" always return 'true'.
> + */
> +bool SoundHandler::play_or_not(SoundType type, const FxId fx_id,

Should debug this to find out how often this is called, might be a bit expensive

>                                 uint8_t const priority) {
> -	bool allow_multiple = false;  //  convenience for easier code reading
> -	float evaluation;             // Temporary to calculate single influences
> -	float probability;            // Weighted total of all influences
> -
> -	if (nosound_)
> -		return false;
> -
> -	// Probability that this fx gets played; initially set according to priority
> -
> -	//  float division! not integer
> -	probability = (priority % PRIO_ALLOW_MULTIPLE) / 128.0f;
> -
> -	// TODO(unknown): what to do with fx that happen offscreen?
> -	// TODO(unknown): reduce volume? reduce priority? other?
> -	if (stereo_pos == -1) {
> -		return false;
> -	}
> -
> -	// TODO(unknown): check for a free channel
> -
> -	if (priority == PRIO_ALWAYS_PLAY) {
> -		// TODO(unknown): if there is no free channel, kill a running fx and complain
> -		return true;
> -	}
> -
> -	if (priority >= PRIO_ALLOW_MULTIPLE)
> -		allow_multiple = true;
> -
> -	// Find out if an fx called fx_name is already running
> -	bool already_running = false;
> -
> -	// Access to active_fx_ is protected because it can
> -	// be accessed from callback
> -	if (fx_lock_)
> -		SDL_LockMutex(fx_lock_);
> -
> -	// starting a block, so I can define a local type for iterating
> -	{
> +	assert(!SoundHandler::is_backend_disabled() && is_sound_enabled(type));
> +	assert(priority >= kFxPriorityLowest);
> +
> +	if (fxs_[type].count(fx_id) == 0) {
> +		return false;
> +	}
> +
> +	switch (type) {

A simple if() would be clearer

> +	case SoundType::kAmbient:
> +		break;
> +	default:
> +		// We always play UI, chat and system sounds
> +		return true;
> +	}
> +
> +	// We always play important sounds
> +	if (priority == kFxPriorityAlwaysPlay) {
> +		return true;
> +	}
> +
> +	// Do not run multiple instances of the same sound effect if the priority is too low
> +	bool too_many_playing = false;
> +	if (priority < kFxPriorityAllowMultiple) {
> +		lock_fx();
> +		// Find out if an fx called 'fx_name' is already running
>  		for (const auto& fx_pair : active_fx_) {
> -			if (fx_pair.second == fx_name) {
> -				already_running = true;
> +			if (fx_pair.second == fx_id) {
> +				too_many_playing = true;
>  				break;
>  			}
>  		}
>  	}
>  
> -	if (fx_lock_)
> -		SDL_UnlockMutex(fx_lock_);
> +	release_fx_lock();
>  
> -	if (!allow_multiple && already_running)
> +	if (too_many_playing) {
>  		return false;
> +	}
>  
>  	// TODO(unknown): long time since any play increases weighted_priority
>  	// TODO(unknown): high general frequency reduces weighted priority
>  	// TODO(unknown): deal with "coupled" effects like throw_net and retrieve_net
>  
> -	uint32_t const ticks_since_last_play = SDL_GetTicks() - fxs_[fx_name]->last_used_;
> -
> -	//  reward an fx for being silent
> -	if (ticks_since_last_play > SLIDING_WINDOW_SIZE) {
> -		evaluation = 1;  //  arbitrary value; 0 -> no change, 1 -> probability = 1
> +	uint32_t const ticks_since_last_play = fxs_[type][fx_id]->ticks_since_last_play();
> +
> +	// Weighted total probability that this fx gets played; initially set according to priority
> +	//  float division! not integer
> +	float probability = (priority % kFxPriorityAllowMultiple) / static_cast<float>(kFxPriorityAllowMultiple);
> +
> +	// How many milliseconds in the past to consider
> +	constexpr uint32_t kSlidingWindowSize = 20000;
> +
> +	if (ticks_since_last_play > kSlidingWindowSize) { //  reward an fx for being silent
> +		const float evaluation = 1.0f;  //  arbitrary value; 0 -> no change, 1 -> probability = 1
>  
>  		//  "decrease improbability"
> -		probability = 1 - ((1 - probability) * (1 - evaluation));
> +		probability = 1.0f - ((1.0f - probability) * (1.0f - evaluation));
>  	} else {  // Penalize an fx for playing in short succession
> -		evaluation = static_cast<float>(ticks_since_last_play) / SLIDING_WINDOW_SIZE;
> +		const float evaluation = static_cast<float>(ticks_since_last_play) / kSlidingWindowSize;
>  		probability *= evaluation;  //  decrease probability
>  	}
>  
>  	// finally: the decision
>  	// float division! not integer
> -	return (rng_.rand() % 255) / 255.0f <= probability;
> +	return (rng_.rand() % kFxPriorityAlwaysPlay) / static_cast<float>(kFxPriorityAlwaysPlay) <= probability;
>  }
>  
> -/** \overload
> - * \param fx_name  The identifying name of the sound effect, see \ref load_fx .
> - * \param stereo_position  position in widelands' game window, see
> - *                         \ref stereo_position
> +/**
> + * \param type             The categorization of the sound effect to be played
> + * \param fx_id            The ID of the sound effect, see \ref register_fx
>   * \param priority         How important is it that this FX actually gets
>   *                         played? (see \ref FXset::priority_)
> + * \param stereo_position  Position in widelands' game window
> + * \param distance         Distance in widelands' game window
>   */
> -void SoundHandler::play_fx(const std::string& fx_name,
> +void SoundHandler::play_fx(SoundType type, const FxId fx_id,
> +						   uint8_t const priority,
>                             int32_t const stereo_pos,
> -                           uint8_t const priority) {
> -	if (nosound_ || is_backend_disabled_)
> -		return;
> -
> -	assert(stereo_pos >= -1);
> -	assert(stereo_pos <= 254);
> -
> -	if (get_disable_fx())
> -		return;
> -
> -	if (fxs_.count(fx_name) == 0) {
> -		log("SoundHandler: sound effect \"%s\" does not exist!\n", fx_name.c_str());
> +                           int distance) {
> +	if (SoundHandler::is_backend_disabled() || !is_sound_enabled(type)) {
> +		return;
> +	}
> +
> +	assert(stereo_pos >= kStereoLeft);
> +	assert(stereo_pos <= kStereoRight);
> +
> +	if (fx_id == kNoSoundEffect) {
> +		throw wexception("SoundHandler: Trying to play sound effect that was never registered. Maybe you registered it before instantiating g_sh?\n");
> +	}
> +
> +	if (fxs_[type].count(fx_id) == 0) {
> +		log("SoundHandler: Sound effect %d does not exist!\n", fx_id);
>  		return;
>  	}
>  
>  	// See if the FX should be played
> -	if (!play_or_not(fx_name, stereo_pos, priority))
> +	if (!play_or_not(type, fx_id, priority)) {
>  		return;
> +	}
>  
>  	//  retrieve the fx and play it if it's valid
> -	if (Mix_Chunk* const m = fxs_[fx_name]->get_fx()) {
> +	if (Mix_Chunk* const m = fxs_[type][fx_id]->get_fx(rng_.rand())) {
>  		const int32_t chan = Mix_PlayChannel(-1, m, 0);
>  		if (chan == -1) {
>  			log("SoundHandler: Mix_PlayChannel failed: %s\n", Mix_GetError());
>  		} else {
> -			Mix_SetPanning(chan, 254 - stereo_pos, stereo_pos);
> -			Mix_Volume(chan, get_fx_volume());
> +			Mix_SetPanning(chan, kStereoRight - stereo_pos, stereo_pos);
> +			Mix_SetDistance(chan, distance);
> +			Mix_Volume(chan, get_volume(type));
>  
> -			// Access to active_fx_ is protected
> -			// because it can be accessed from callback
> -			if (fx_lock_)
> -				SDL_LockMutex(fx_lock_);
> -			active_fx_[chan] = fx_name;
> -			if (fx_lock_)
> -				SDL_UnlockMutex(fx_lock_);
> +			lock_fx();
> +			active_fx_[chan] = fx_id;
> +			release_fx_lock();
>  		}
> -	} else
> -		log("SoundHandler: sound effect \"%s\" exists but contains no files!\n", fx_name.c_str());
> -}
> -
> -/** Load a background song. One "song" can consist of several audio files named
> +	} else {
> +		log("SoundHandler: Sound effect %d exists but contains no files!\n", fx_id);
> +	}
> +}
> +
> +/// Removes the given FXset from memory
> +void SoundHandler::remove_fx_set(SoundType type) {
> +	fxs_.erase(type);
> +	fx_ids_.erase(type);
> +}
> +
> +/**
> + * Register a background songset. A songset can consist of several audio files named
>   * FILE_XX.ogg, where XX is between 00 and 99.
>   * \param dir        The directory where the audio files reside.
>   * \param basename   Name from which filenames will be formed
> - *                   (BASENAME_XX.ogg); also the name used with \ref play_fx .
> - * This just registers the song, actual loading takes place when
> - * \ref Songset::get_song() is called, i.e. when the song is about to be
> + *                   (BASENAME_XX.ogg); also the name used with \ref change_music .
> + * This just registers the songs, actual loading takes place when
> + * \ref Songset::get_song() is called, i.e. when a song is about to be
>   * played. The song will automatically be removed from memory when it has
>   * finished playing.
>   */
> -void SoundHandler::register_song(const std::string& dir, const std::string& basename) {
> -	if (nosound_ || is_backend_disabled_)
> +void SoundHandler::register_songs(const std::string& dir, const std::string& basename) {
> +	if (SoundHandler::is_backend_disabled()) {
>  		return;
> -	assert(g_fs);
> -
> -	FilenameSet files;
> -
> -	files = filter(g_fs->list_directory(dir), [&basename](const std::string& fn) {
> -		const std::string only_filename = FileSystem::fs_filename(fn.c_str());
> -		return boost::starts_with(only_filename, basename) && boost::ends_with(only_filename, ".ogg");
> -	});
> -
> -	for (const std::string& filename : files) {
> -		assert(!g_fs->is_directory(filename));
> -		if (songs_.count(basename) == 0) {
> -			songs_.insert(std::make_pair(basename, std::unique_ptr<Songset>(new Songset())));
> -		}
> -		songs_[basename]->add_song(filename);
> +	}
> +	if (songs_.count(basename) == 0) {
> +		songs_.insert(std::make_pair(basename, std::unique_ptr<Songset>(new Songset(dir, basename))));
>  	}
>  }
>  
> -/** Start playing a songset.
> +/**
> + * Start playing a songset.
>   * \param songset_name  The songset to play a song from.
> - * \param fadein_ms     Song will fade from 0% to 100% during fadein_ms
> - *                      milliseconds starting from now.
> - * \note When calling start_music() while music is still fading out from
> - * \ref stop_music()
> - * or \ref change_music() this function will block until the fadeout is complete
> + * \note When calling start_music() while music is still fading out from \ref stop_music() or \ref change_music(),
> + * this function will block until the fadeout is complete
>   */
> -void SoundHandler::start_music(const std::string& songset_name, int32_t fadein_ms) {
> -	if (get_disable_music() || nosound_ || is_backend_disabled_)
> +void SoundHandler::start_music(const std::string& songset_name) {
> +	if (SoundHandler::is_backend_disabled() || !is_sound_enabled(SoundType::kMusic)) {
>  		return;
> -
> -	if (fadein_ms == 0)
> -		fadein_ms = 250;  //  avoid clicks
> -
> -	if (Mix_PlayingMusic())
> -		change_music(songset_name, 0, fadein_ms);
> -
> -	if (songs_.count(songset_name) == 0)
> +	}
> +
> +	if (Mix_PlayingMusic()) {
> +		change_music(songset_name, kMinimumMusicFade);
> +	}
> +
> +	if (songs_.count(songset_name) == 0) {
>  		log("SoundHandler: songset \"%s\" does not exist!\n", songset_name.c_str());
> -	else {
> -		if (Mix_Music* const m = songs_[songset_name]->get_song()) {
> -			Mix_FadeInMusic(m, 1, fadein_ms);
> +	} else {
> +		if (Mix_Music* const m = songs_[songset_name]->get_song(rng_.rand())) {
> +			Mix_FadeInMusic(m, 1, kMinimumMusicFade);
>  			current_songset_ = songset_name;
> -		} else
> +		} else {
>  			log("SoundHandler: songset \"%s\" exists but contains no files!\n", songset_name.c_str());
> +		}
>  	}
> -	Mix_VolumeMusic(music_volume_);
>  }
>  
> -/** Stop playing a songset.
> +/**
> + * Stop playing a songset.
>   * \param fadeout_ms Song will fade from 100% to 0% during fadeout_ms
>   *                   milliseconds starting from now.
>   */
> -void SoundHandler::stop_music(int32_t fadeout_ms) {
> -	if (get_disable_music() || nosound_)
> +void SoundHandler::stop_music(int fadeout_ms) {
> +	if (SoundHandler::is_backend_disabled()) {
>  		return;
> -
> -	if (fadeout_ms == 0)
> -		fadeout_ms = 250;  //  avoid clicks
> -
> -	Mix_FadeOutMusic(fadeout_ms);
> +	}
> +
> +	if (Mix_PlayingMusic()) {
> +		Mix_FadeOutMusic(std::max(fadeout_ms, kMinimumMusicFade));
> +	}
>  }
>  
> -/** Play an other piece of music.
> +/**
> + * Play a new piece of music.
>   * This is a member function provided for convenience. It is a wrapper around
>   * \ref start_music and \ref stop_music.
>   * \param fadeout_ms  Old song will fade from 100% to 0% during fadeout_ms
>   *                    milliseconds starting from now.
> - * \param fadein_ms   New song will fade from 0% to 100% during fadein_ms
> - *                    milliseconds starting from now.
>   * If songset_name is empty, another song from the currently active songset will
>   * be selected
>   */
>  void SoundHandler::change_music(const std::string& songset_name,
> -                                int32_t const fadeout_ms,
> -                                int32_t const fadein_ms) {
> -	if (nosound_)
> +                                int const fadeout_ms) {
> +	if (SoundHandler::is_backend_disabled()) {
>  		return;
> -
> -	std::string s = songset_name;
> -
> -	if (s == "")
> -		s = current_songset_;
> -	else
> -		current_songset_ = s;
> -
> -	if (Mix_PlayingMusic())
> +	}
> +
> +	if (!songset_name.empty()) {
> +		current_songset_ = songset_name;
> +	}
> +
> +	if (Mix_PlayingMusic()) {
>  		stop_music(fadeout_ms);
> -	else
> -		start_music(s, fadein_ms);
> -}
> -
> -bool SoundHandler::get_disable_music() const {
> -	return disable_music_;
> -}
> -bool SoundHandler::get_disable_fx() const {
> -	return disable_fx_;
> -}
> -int32_t SoundHandler::get_music_volume() const {
> -	return music_volume_;
> -}
> -int32_t SoundHandler::get_fx_volume() const {
> -	return fx_volume_;
> -}
> -
> -/** Normal set_* function, but the music must be started/stopped accordingly
> - * Also, the new value is written back to the config file right away. It might
> - * get lost otherwise.
> - */
> -void SoundHandler::set_disable_music(bool const disable) {
> -	if (is_backend_disabled_ || disable_music_ == disable)
> -		return;
> -
> -	if (disable) {
> -		stop_music();
> -		disable_music_ = true;
>  	} else {
> -		disable_music_ = false;
>  		start_music(current_songset_);
>  	}
> -
> -	g_options.pull_section("global").set_bool("disable_music", disable);
> -}
> -
> -/** Normal set_* function
> - * Also, the new value is written back to the config file right away. It might
> - * get lost otherwise.
> - */
> -void SoundHandler::set_disable_fx(bool const disable) {
> -	if (is_backend_disabled_)
> -		return;
> -
> -	disable_fx_ = disable;
> -
> -	g_options.pull_section("global").set_bool("disable_fx", disable);
> -}
> -
> -/**
> - * Normal set_* function.
> - * Set the music volume between 0 (muted) and \ref get_max_volume().
> - * The new value is written back to the config file.
> - *
> - * \param volume The new music volume.
> - */
> -void SoundHandler::set_music_volume(int32_t volume) {
> -	if (!is_backend_disabled_ && !nosound_) {
> -		music_volume_ = volume;
> +}
> +
> +/// Returns the currently playing songset
> +const std::string SoundHandler::current_songset() const {
> +	return current_songset_;
> +}
> +
> +/// Returns whether we want to hear sounds of the given 'type'
> +bool SoundHandler::is_sound_enabled(SoundType type) const {
> +	assert(sound_options_.count(type) == 1);
> +	return sound_options_.at(type).enabled;
> +}
> +
> +/// Returns the volume that the given 'type' of sound is to be played at
> +int32_t SoundHandler::get_volume(SoundType type) const {
> +	assert(sound_options_.count(type) == 1);
> +	return sound_options_.at(type).volume;
> +}
> +
> +/**
> + * Sets that we want to / don't want to hear the given 'type' of sounds. If the type is \ref SoundType::kMusic, start/stop the music as well.
> + */
> +void SoundHandler::set_enable_sound(SoundType type, bool const enable) {
> +	if (SoundHandler::is_backend_disabled()) {
> +		return;
> +	}
> +	assert(sound_options_.count(type) == 1);
> +
> +	SoundOptions& sound_options = sound_options_.at(type);
> +	sound_options.enabled = enable;
> +
> +	// Special treatment for music
> +	switch (type) {
> +	case SoundType::kMusic:
> +		if (enable) {
> +			if (!Mix_PlayingMusic()) {
> +				start_music(current_songset_);
> +			}
> +		} else {
> +			stop_music();
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +/**
> + * Sets the music or sound 'volume' for the given 'type' between 0 (muted) and \ref get_max_volume().
> + */
> +void SoundHandler::set_volume(SoundType type, int32_t volume) {
> +	if (SoundHandler::is_backend_disabled()) {
> +		return;
> +	}
> +
> +	assert(sound_options_.count(type) == 1);
> +	assert(volume >= 0 && volume <= get_max_volume());
> +
> +	sound_options_.at(type).volume = volume;
> +
> +	// Special treatment for music
> +	switch (type) {
> +	case SoundType::kMusic:
>  		Mix_VolumeMusic(volume);
> -		g_options.pull_section("global").set_int("music_volume", volume);
> -	}
> -}
> -
> -/**
> - * Normal set_* function
> - * Set the FX sound volume between 0 (muted) and \ref get_max_volume().
> - * The new value is written back to the config file.
> - *
> - * \param volume The new music volume.
> - */
> -void SoundHandler::set_fx_volume(int32_t volume) {
> -	if (!is_backend_disabled_ && !nosound_) {
> -		fx_volume_ = volume;
> +		break;
> +	default:
>  		Mix_Volume(-1, volume);
> -		g_options.pull_section("global").set_int("fx_volume", volume);
> +		break;
>  	}
>  }
>  
> -/** Callback to notify \ref SoundHandler that a song has finished playing.
> - * Usually, another song from the same songset will be started.
> - * There is a special case for the intro screen's music: only one song will be
> - * played. If the user has not clicked the mouse or pressed escape when the song
> - * finishes, Widelands will automatically go on to the main menu.
> +/**
> + * Returns the max value for volume settings. We use a function to hide
> + * SDL_mixer constants outside of sound_handler.
> + */
> +int32_t SoundHandler::get_max_volume() const {
> +	return MIX_MAX_VOLUME;
> +}
> +
> +/**
> + * Callback to notify \ref SoundHandler that a song has finished playing.
> + * Pushes an SDL_Event with type = SDL_USEREVENT and user.code = CHANGE_MUSIC.
>   */
>  void SoundHandler::music_finished_callback() {
>  	// DO NOT CALL SDL_mixer FUNCTIONS OR SDL_LockAudio FROM HERE !!!
>  
> +	assert(!SoundHandler::is_backend_disabled());
> +	// Trigger that we want a music change and leave the specifics to the application.
>  	SDL_Event event;
> -	if (g_sound_handler.current_songset_ == "intro") {
> -		// Special case for splashscreen: there, only one song is ever played
> -		event.type = SDL_KEYDOWN;
> -		event.key.state = SDL_PRESSED;
> -		event.key.keysym.sym = SDLK_ESCAPE;
> -	} else {
> -		// Else just play the next song - see general description for
> -		// further explanation
> -		event.type = SDL_USEREVENT;
> -		event.user.code = CHANGE_MUSIC;
> -	}
> +	event.type = SDL_USEREVENT;
> +	event.user.code = CHANGE_MUSIC;
>  	SDL_PushEvent(&event);
>  }
>  
> -/** Callback to notify \ref SoundHandler that a sound effect has finished
> - * playing.
> +/**
> + * Callback to notify \ref SoundHandler that a sound effect has finished
> + * playing. Removes the finished sound fx from the list of currently playing ones.
>   */
>  void SoundHandler::fx_finished_callback(int32_t const channel) {
>  	// DO NOT CALL SDL_mixer FUNCTIONS OR SDL_LockAudio FROM HERE !!!
>  
> +	assert(!SoundHandler::is_backend_disabled());
>  	assert(0 <= channel);
> -	g_sound_handler.handle_channel_finished(static_cast<uint32_t>(channel));
> +	g_sh->lock_fx();
> +	g_sh->active_fx_.erase(static_cast<uint32_t>(channel));
> +	g_sh->release_fx_lock();
>  }
>  
> -/** Remove a finished sound fx from the list of currently playing ones
> - * This is part of \ref fx_finished_callback
> - */
> -void SoundHandler::handle_channel_finished(uint32_t channel) {
> -	// Needs locking because active_fx_ may be accessed
> -	// from this callback or from main thread
> -	if (fx_lock_)
> +/// Lock the SDL mutex. Access to 'active_fx_' is protected by mutex because it can be accessed both from callbacks or from the main thread.
> +void SoundHandler::lock_fx() {
> +	if (fx_lock_) {
>  		SDL_LockMutex(fx_lock_);
> -	active_fx_.erase(channel);
> -	if (fx_lock_)
> +	}
> +}
> +
> +/// Release the SDL mutex
> +void SoundHandler::release_fx_lock() {
> +	if (fx_lock_) {
>  		SDL_UnlockMutex(fx_lock_);
> +	}
>  }


-- 
https://code.launchpad.net/~widelands-dev/widelands/cleanup-soundhandler/+merge/365001
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cleanup-rendertarget.