widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Some more commetns inline, I think this deserves some time in the debugger.
Mostly for me to better understand the widelands internal structures.

Maybe this will make things slower, Not sure about this, lets seee.

Diff comments:

> 
> === 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.
The functions comment must state this: index - denotes a building not normally belonging to the players tribe.

> +		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) {

Better extract nrbuildings, compiler may not be able to deduce this is const during the loop.
Extract const Widelands::Player& player for own_building_is_valid / foreign_tribe_building_is_valid as this is an ivariant

> +		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.