Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
Transient failure on Travis @bunnybot merge force -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
@bunnybot merge -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
Thanks a lot for the review!
SDL_Mixer already uses threading for playing the sounds.
The only other spot where we could use some threading would be when registering
sounds, but then the threads would fight over hard disk access and probably
make things slower rather than faster. I already have another branch in the
works that will speed up finding the files for animations and sounds by not
using regular expressions, which will greatly speed up loading the tribes and
world.
Diff comments:
>
> === modified file 'src/economy/road.h'
> --- src/economy/road.h2019-02-27 17:19:00 +
> +++ src/economy/road.h2019-04-09 04:52:18 +
> @@ -131,8 +131,7 @@
> void cleanup(EditorGameBase&) override;
>
> void draw(uint32_t gametime,
> - TextToDraw draw_text,
> - const Vector2f& point_on_dst,
> + TextToDraw draw_text, const Vector2f&, const Coords&,
Road::draw doesn't do anything. The variables have names on those map objects
that do draw themselves.
> float scale,
> RenderTarget* dst) override;
>
>
> === modified file 'src/editor/editorinteractive.cc'
> --- src/editor/editorinteractive.cc 2019-02-27 17:19:00 +
> +++ src/editor/editorinteractive.cc 2019-04-09 04:52:18 +
> @@ -405,6 +405,10 @@
> is_painting_ = false;
> }
>
> +bool EditorInteractive::player_hears_field(const Widelands::Coords&) const {
Will add one in interactive_base.h
> + return true;
> +}
> +
> void EditorInteractive::on_buildhelp_changed(const bool value) {
> toggle_buildhelp_->set_perm_pressed(value);
> }
>
> === modified file 'src/sound/sound_handler.cc'
> --- src/sound/sound_handler.cc2019-02-23 11:00:49 +
> +++ src/sound/sound_handler.cc2019-04-09 04:52:18 +
> @@ -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",
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
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.cc2019-02-23 11:00:49 +
> +++ src/sound/sound_handler.cc2019-04-09 04:52:18 +
> @@ -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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
Review, found so far:
* restructered the sound directory wihht more subdirectories
* Help now shows --nosoundStarts the game with sound disabled.
* Using FXset one could have different soundscapes for testing or scenarions,
interesting
* I assuem the random must be a _local_ random, as sounds are per player.
* More coommetns please, see inline comments
For the wishlist: a helpscreen explainng the sounds :-)
Checekd upto "TODO(GunChleoc): Compatibility code to avoid getting bug reports
about unread sections. Remove after Build 21." sorry can nott do more tonight
Diff comments:
>
> === modified file 'src/economy/road.h'
> --- src/economy/road.h2019-02-27 17:19:00 +
> +++ src/economy/road.h2019-04-09 04:52:18 +
> @@ -131,8 +131,7 @@
> void cleanup(EditorGameBase&) override;
>
> void draw(uint32_t gametime,
> - TextToDraw draw_text,
> - const Vector2f& point_on_dst,
> + TextToDraw draw_text, const Vector2f&, const Coords&,
mmh, i like those named varibales better, Now I must guess what "const Coords&"
may be used for?
> float scale,
> RenderTarget* dst) override;
>
>
> === modified file 'src/editor/editorinteractive.cc'
> --- src/editor/editorinteractive.cc 2019-02-27 17:19:00 +
> +++ src/editor/editorinteractive.cc 2019-04-09 04:52:18 +
> @@ -405,6 +405,10 @@
> is_painting_ = false;
> }
>
> +bool EditorInteractive::player_hears_field(const Widelands::Coords&) const {
Did I tell you, that I like comments?
> + return true;
> +}
> +
> void EditorInteractive::on_buildhelp_changed(const bool value) {
> toggle_buildhelp_->set_perm_pressed(value);
> }
--
https://code.launchpad.net/~widelands-dev/widelands/cleanup-soundhandler/+merge/365001
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
That message should come up every time that the song finishes playing and starts again. Yep, definitely a big code review. Probably best do it with a diff program like Meld and add your remarks as NOCOM comments to the code. I'll merge trunk now to make the diff smaller. -- https://code.launchpad.net/~widelands-dev/widelands/cleanup-soundhandler/+merge/365001 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
I played the first 3 Tutorial on this branhc and everyhting was fine. When hanging around in the config I get > Songset: Loaded song "music/menu_00.ogg" again and again, is this intended, or should this happen only once I will try to do a code review. But this seems to be a _bigger_ task :-) -- https://code.launchpad.net/~widelands-dev/widelands/cleanup-soundhandler/+merge/365001 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands
The new UI classes and changes to WLApplication are missing from the diff, because it is too long :( -- https://code.launchpad.net/~widelands-dev/widelands/cleanup-soundhandler/+merge/365001 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/cleanup-soundhandler into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
