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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands has been updated. Status: Needs review => Merged For more details, see: 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
gt; + 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.
___
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
}
> +
> + // 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], 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()
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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
Continuous integration builds have changed state: Travis build 3357. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/363552376. Appveyor build 3163. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_smaller_building_statistics-3163. -- 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands has been updated. Commit message changed to: Building Statistics now only show relevant buildings - Military sites not belonging to the tribe are omitted, unless the payer currently owns one - Seafaring and allowed buildings are dynamically checked - Show "under construction" navigation for buildings being enhanced - Added "needs_seafaring" to port buildings For more details, see: 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands.
Commit message:
Building Statistics now only show relevant buildings
- Military sites not belonging to the tribe are omitted, unless the payer
currently owns one
- Seafaring and allowed buildings are dynamically checked
- Show "under construction" navigation for buildings being enhanced
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1636511 in widelands: "Buildings being upgraded not showing up in
Building Statistics box."
https://bugs.launchpad.net/widelands/+bug/1636511
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828
Fixes the attached bug.
Since the Frisians were added, the building statistics got too big due to all
the military sites - the navigation became impossible to reach on 800x600
resolution without moving the window around. So, I have implemented a dynamic
approach that will only show allowed and currently owned buildings.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands.
=== modified file 'data/tribes/buildings/warehouses/atlanteans/port/init.lua'
--- data/tribes/buildings/warehouses/atlanteans/port/init.lua 2017-09-02 19:53:03 +
+++ data/tribes/buildings/warehouses/atlanteans/port/init.lua 2018-04-07 16:30:59 +
@@ -8,6 +8,7 @@
helptext_script = dirname .. "helptexts.lua",
icon = dirname .. "menu.png",
size = "port",
+ needs_seafaring = true,
buildcost = {
log = 3,
=== modified file 'data/tribes/buildings/warehouses/barbarians/port/init.lua'
--- data/tribes/buildings/warehouses/barbarians/port/init.lua 2017-09-02 19:53:03 +
+++ data/tribes/buildings/warehouses/barbarians/port/init.lua 2018-04-07 16:30:59 +
@@ -8,6 +8,7 @@
helptext_script = dirname .. "helptexts.lua",
icon = dirname .. "menu.png",
size = "port",
+ needs_seafaring = true,
buildcost = {
log = 3,
=== modified file 'data/tribes/buildings/warehouses/empire/port/init.lua'
--- data/tribes/buildings/warehouses/empire/port/init.lua 2017-09-02 19:53:03 +
+++ data/tribes/buildings/warehouses/empire/port/init.lua 2018-04-07 16:30:59 +
@@ -8,6 +8,7 @@
helptext_script = dirname .. "helptexts.lua",
icon = dirname .. "menu.png",
size = "port",
+ needs_seafaring = true,
buildcost = {
log = 3,
=== modified file 'data/tribes/buildings/warehouses/frisians/port/init.lua'
--- data/tribes/buildings/warehouses/frisians/port/init.lua 2018-02-16 14:53:04 +
+++ data/tribes/buildings/warehouses/frisians/port/init.lua 2018-04-07 16:30:59 +
@@ -8,6 +8,7 @@
helptext_script = dirname .. "helptexts.lua",
icon = dirname .. "menu.png",
size = "port",
+ needs_seafaring = true,
buildcost = {
brick = 6,
=== 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 16:30:59 +
@@ -225,6 +225,9 @@
}
bool TabPanel::remove_last_tab(const std::string& tabname) {
+ if (tabs_.empty()) {
+ return false;
+ }
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 16:30:59 +
@@ -120,118 +120,11 @@
low_production_(33),
has_selection_(false) {
- for (int i = 0; i < kNoOfBuildingTabs; ++i) {
- row_counters_[i] = 0;
- tabs_[i] = new UI::Box(_panel_, 0, 0, UI::Box::Vertical);
- }
-
- tab_panel_.add("building_stats_small",
- g_gr->images().get("images/wui/fieldaction/menu_tab_buildsmall.png"),
- tabs_[BuildingTab::Small], _("Small buildings"));
- tab_panel_.add("building_stats_medium",
- g_gr->images().get("images/wui/fieldaction/menu_tab_buildmedium.png"),
- tabs_[BuildingTab::Medium], _("Medium buildings"));
- tab_panel_.add("building_stats_big",
- g_gr->images().get("images/wui/fieldaction/menu_tab_buildbig.png"),
- tabs_[BuildingTab::Big], _("Big buildings"));
- tab_panel_.add("building_stats_mines",
- g_gr->images().get("images/wui/fieldaction/menu_tab_buildmine.png"),
- tabs_[BuildingTab::Mines], _("Mines"));
-
- // Only show the ports tab for seafaring maps
- if (iplayer().game().map().allows_seafaring()) {
- tab_panel_.add("building_stats_ports",
- g_gr->ima
