Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
You trigger those by conquering a foreign military site and then again by destroying it. I ran a setup on Golden Peninsula where I gave the opponent the "Village" starting condition. so that I could easily conquer something. -- https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/smaller_building_statistics. ___ 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/smaller_building_statistics into lp:widelands
Review: Approve compile, revied, debug for coverage, testplay
I testplayed this now on a seafaring Map:
* I was unable to reach the following lies of code:
* BuildingStatisticsMenu::foreign_tribe_building_is_valid()
...
if (descr.type() == MapObjectType::CONSTRUCTIONSITE ||
descr.type() == MapObjectType::DISMANTLESITE) {
return false;
// I think this can never be reached I can not build a forign building
// A dismantled site cannot be reached
* BuildingStatisticsMenu::reset()
...
if (building_buttons_[current_building_type_] != nullptr) {
set_current_building_type(current_building_type_); // no idea what this
shall be
} else {
* I found that some missing soldiers Arrows where incorrect for a foreign
military building.
* I played on a lot of different resolutions
Despite these Issues this can go in:
@bunnybot merge
--
https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/smaller_building_statistics.
___
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/smaller_building_statistics into lp:widelands
I did some testplaying with a debugger now and gained some coverage of that code: * Everything worked as expected so far. * The Handling of the forward / backward buttons has room for improvement: * no need so use large switch blocks -> call the repective function directly. * Maybe I can do some refactoring there, later. * I could not trigger foreign_tribe_building_is_valid, yet, no Enemy t conquer found so far :-) * I need a another map _with_ seafaring, my current one does not allow this * I was unable to trigger BuildingStatisticsMenu::reset() no idea why this was never called. Gun: any idea? -- https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/smaller_building_statistics. ___ 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/smaller_building_statistics into lp:widelands
fetched it again, no idea if I have time at the weekend. -- https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/smaller_building_statistics. ___ 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/smaller_building_statistics into lp:widelands
Review: Resubmit I know - I have taken care of it. Branch is ready; I'll put it up for review as soon as Launchpad has finished parsing it. https://code.launchpad.net/~widelands-dev/widelands/allows_seafaring_performance -- https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/smaller_building_statistics. ___ 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/smaller_building_statistics into lp:widelands
Mhh, scripts might want to add seafaring later in some scenario. Hopefully this will not break such scripts. Anyway, I do not have that much time today for a debugging session, lets see what I can do perhaps next weekend. -- 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. ___ 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/smaller_building_statistics into lp:widelands
I have changed my mind about the seafaring check - since this is so expensive, we should generally only recalculate it on map changes. I'll create another branch for it, to be merged before this branch. -- 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. ___ 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/smaller_building_statistics into lp:widelands
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 +
> +++ src/ui_basic/tabpanel.cc 2018-04-07 17:36:19 +
> @@ -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 +
> +++ src/wui/building_statistics_menu.cc 2018-04-07 17:36:19 +
> @@ -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 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(_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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
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 +
> +++ src/wui/building_statistics_menu.cc 2018-04-07 17:36:19 +
> @@ -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 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(_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, _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],
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
One question inline, will take a closer look later.
just brached this and will have a look.
Not sure who will be the payer for the other tribes military sites ;-) ?
Diff comments:
>
> === modified file 'src/ui_basic/tabpanel.cc'
> --- src/ui_basic/tabpanel.cc 2017-08-08 17:39:40 +
> +++ src/ui_basic/tabpanel.cc 2018-04-07 17:36:19 +
> @@ -225,6 +225,9 @@
> }
>
> bool TabPanel::remove_last_tab(const std::string& tabname) {
> + if (tabs_.empty()) {
> + return false;
> + }
This can happen only in some rare, scripted scenarios, but yes.
> if (tabs_.back()->get_name() == tabname) {
> tabs_.pop_back();
> if (active_ > tabs_.size() - 1) {
--
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.
___
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
