widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Thanks for the review :)

And you were right about the efficiency concerns, the seafaring check is causing a slowdown. I have reduced it to be checked once every 2 minutes.

Diff comments:

> 
> === modified file 'src/ui_basic/tabpanel.cc'
> --- src/ui_basic/tabpanel.cc	2017-08-08 17:39:40 +0000
> +++ src/ui_basic/tabpanel.cc	2018-04-07 17:36:19 +0000
> @@ -225,6 +225,9 @@
>  }
>  
>  bool TabPanel::remove_last_tab(const std::string& tabname) {
> +	if (tabs_.empty()) {
> +		return false;
> +	}

This crashes when you try to remove a tab when there are no tabs. It happened to me before I moved the tab deletion from init() to reset()

>  	if (tabs_.back()->get_name() == tabname) {
>  		tabs_.pop_back();
>  		if (active_ > tabs_.size() - 1) {
> 
> === modified file 'src/wui/building_statistics_menu.cc'
> --- src/wui/building_statistics_menu.cc	2017-12-16 10:48:12 +0000
> +++ src/wui/building_statistics_menu.cc	2018-04-07 17:36:19 +0000
> @@ -315,6 +206,189 @@
>  	productivity_labels_.clear();
>  }
>  
> +void BuildingStatisticsMenu::reset() {
> +	update();  // In case a building got removed, make sure to deselect it first
> +
> +	const int last_selected_tab = tab_assignments_[tab_panel_.active()];
> +
> +	tab_panel_.remove_last_tab("building_stats_ports");
> +	tab_panel_.remove_last_tab("building_stats_mines");
> +	tab_panel_.remove_last_tab("building_stats_big");
> +	tab_panel_.remove_last_tab("building_stats_medium");
> +	tab_panel_.remove_last_tab("building_stats_small");
> +
> +	// Clean state if buildings disappear from list
> +	const DescriptionIndex nr_buildings = iplayer().egbase().tribes().nrbuildings();
> +	building_buttons_.clear();
> +	building_buttons_.resize(nr_buildings);
> +	owned_labels_.clear();
> +	owned_labels_.resize(nr_buildings);
> +	productivity_labels_.clear();
> +	productivity_labels_.resize(nr_buildings);
> +
> +	// Ensure that defunct buttons disappear
> +	for (int tab_index = 0; tab_index < kNoOfBuildingTabs; ++tab_index) {
> +		if (tabs_[tab_index] != nullptr) {
> +			tabs_[tab_index]->die();
> +		}
> +	}
> +
> +	init(last_selected_tab);
> +
> +	// Reset navigator
> +	building_name_.set_text("");
> +	if (has_selection_) {
> +		if (building_buttons_[current_building_type_] != nullptr) {
> +			set_current_building_type(current_building_type_);
> +		} else {
> +			has_selection_ = false;
> +		}
> +	}
> +}
> +
> +void BuildingStatisticsMenu::init(int last_selected_tab) {
> +	// We want to add player tribe's buildings in correct order
> +	const Widelands::Player& player = iplayer().player();
> +	const TribeDescr& tribe = player.tribe();
> +	std::vector<DescriptionIndex> buildings_to_add[kNoOfBuildingTabs];
> +	// Add the player's own tribe's buildings.
> +	for (DescriptionIndex index : tribe.buildings()) {
> +		if (own_building_is_valid(index)) {
> +			buildings_to_add[find_tab_for_building(*tribe.get_building_descr(index))].push_back(index);
> +		}
> +	}
> +
> +	// We want to add other tribes' buildings on the bottom. Only add the ones that the player owns.
> +	for (DescriptionIndex index = 0; index < iplayer().egbase().tribes().nrbuildings(); ++index) {
> +		if (foreign_tribe_building_is_valid(index)) {
> +			buildings_to_add[find_tab_for_building(*tribe.get_building_descr(index))].push_back(index);
> +		}
> +	}
> +
> +	// Now create the tab contents and add the building buttons
> +	int row_counters[kNoOfBuildingTabs];
> +	for (int tab_index = 0; tab_index < kNoOfBuildingTabs; ++tab_index) {
> +		int current_column = 0;
> +		tabs_[tab_index] = new UI::Box(&tab_panel_, 0, 0, UI::Box::Vertical);
> +		UI::Box* row = new UI::Box(tabs_[tab_index], 0, 0, UI::Box::Horizontal);
> +		row_counters[tab_index] = 0;
> +
> +		for (const Widelands::DescriptionIndex id : buildings_to_add[tab_index]) {
> +			const BuildingDescr& descr = *iplayer().egbase().tribes().get_building_descr(id);
> +			add_button(id, descr, row);
> +			++current_column;
> +			if (current_column == 1) {
> +				++row_counters[tab_index];
> +			} else if (current_column == kColumns) {
> +				tabs_[tab_index]->add(row, UI::Box::Resizing::kFullSize);
> +				tabs_[tab_index]->add_space(6);
> +				row = new UI::Box(tabs_[tab_index], 0, 0, UI::Box::Horizontal);
> +				current_column = 0;
> +			}
> +		}
> +		// Add final row
> +		if (current_column != 0) {
> +			tabs_[tab_index]->add(row, UI::Box::Resizing::kFullSize);
> +		}
> +	}
> +
> +	// Show the tabs that have buttons on them
> +	int tab_counter = 0;
> +	auto add_tab = [this, row_counters, &tab_counter, last_selected_tab](
> +	   int tab_index, const std::string& name, const std::string& image, const std::string& descr) {
> +		if (row_counters[tab_index] > 0) {
> +			tab_panel_.add(name, g_gr->images().get(image), tabs_[tab_index], descr);
> +			if (last_selected_tab == tab_index) {
> +				tab_panel_.activate(tab_counter);
> +			}
> +			tab_assignments_[tab_counter] = tab_index;
> +			row_counters_[tab_counter] = row_counters[tab_index];
> +			++tab_counter;
> +		}
> +	};
> +	add_tab(BuildingTab::Small, "building_stats_small",
> +	        "images/wui/fieldaction/menu_tab_buildsmall.png", _("Small buildings"));
> +	add_tab(BuildingTab::Medium, "building_stats_medium",
> +	        "images/wui/fieldaction/menu_tab_buildmedium.png", _("Medium buildings"));
> +	add_tab(BuildingTab::Big, "building_stats_big", "images/wui/fieldaction/menu_tab_buildbig.png",
> +	        _("Big buildings"));
> +	add_tab(BuildingTab::Mines, "building_stats_mines",
> +	        "images/wui/fieldaction/menu_tab_buildmine.png", _("Mines"));
> +	add_tab(BuildingTab::Ports, "building_stats_ports",
> +	        "images/wui/fieldaction/menu_tab_buildport.png", _("Ports"));
> +
> +	update();
> +}
> +
> +bool BuildingStatisticsMenu::own_building_is_valid(Widelands::DescriptionIndex index) const {
> +	const Widelands::Player& player = iplayer().player();
> +	const BuildingDescr& descr = *player.tribe().get_building_descr(index);
> +	// Skip seafaring buildings if not needed
> +	if (descr.needs_seafaring() && !iplayer().game().map().allows_seafaring() &&
> +	    player.get_building_statistics(index).empty()) {
> +		return false;
> +	}
> +	if (descr.type() == MapObjectType::CONSTRUCTIONSITE ||
> +	    descr.type() == MapObjectType::DISMANTLESITE) {
> +		return false;
> +	}
> +	// Only add allowed buildings or buildings that are owned by the player.
> +	if ((player.is_building_type_allowed(index) && (descr.is_buildable() || descr.is_enhanced())) ||
> +	    !player.get_building_statistics(index).empty()) {
> +		return true;
> +	}
> +	return false;
> +}
> +
> +bool BuildingStatisticsMenu::foreign_tribe_building_is_valid(
> +   Widelands::DescriptionIndex index) const {
> +	const Widelands::Player& player = iplayer().player();
> +	if (!player.tribe().has_building(index) && !player.get_building_statistics(index).empty()) {

player.tribe().has_building(index) is always false is this context and can be dropped. -> not true, because this function is called for all buildings in tribes().

> +		const BuildingDescr& descr = *iplayer().egbase().tribes().get_building_descr(index);
> +		if (descr.type() == MapObjectType::CONSTRUCTIONSITE ||
> +		    descr.type() == MapObjectType::DISMANTLESITE) {
> +			return false;
> +		}
> +		return true;
> +	}
> +	return false;
> +}
> +
> +int BuildingStatisticsMenu::find_tab_for_building(const Widelands::BuildingDescr& descr) const {
> +	assert(descr.type() != MapObjectType::CONSTRUCTIONSITE);
> +	assert(descr.type() != MapObjectType::DISMANTLESITE);
> +	if (descr.get_ismine()) {
> +		return BuildingTab::Mines;
> +	} else if (descr.get_isport()) {
> +		return BuildingTab::Ports;
> +	} else {
> +		switch (descr.get_size()) {
> +		case BaseImmovable::SMALL:
> +			return BuildingTab::Small;
> +		case BaseImmovable::MEDIUM:
> +			return BuildingTab::Medium;
> +		case BaseImmovable::BIG:
> +			return BuildingTab::Big;
> +		default:
> +			throw wexception(
> +			   "Building statictics: Found building without a size: %s", descr.name().c_str());
> +		}
> +	}
> +	NEVER_HERE();
> +}
> +
> +void BuildingStatisticsMenu::update_building_list() {
> +	for (DescriptionIndex index = 0; index < iplayer().egbase().tribes().nrbuildings(); ++index) {

Good idea. I've even turned nrbuildings into a member variable, because the value never changes after the game is loaded.

> +		const bool should_have_this_building =
> +		   own_building_is_valid(index) || foreign_tribe_building_is_valid(index);
> +		const bool has_this_building = building_buttons_[index] != nullptr;
> +		if (should_have_this_building != has_this_building) {
> +			reset();
> +			return;
> +		}
> +	}
> +}
> +
>  /**
>   * Adds 3 buttons per building type.
>   *


-- 
https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands.