Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/constructionsite_options into lp:widelands
Code LGTM :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/371170 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Yes, that fixed it :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Should be fixed now. I´m unable to compile and test at the moment though. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
The hero/rookie buttons now oscillate their state, so that needs fixing. The memory leak seems to be gone. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Review: Resubmit Repositioned the buttons. I also pushed a revision that should fix the memory leak you found (not tested). -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
I did some testing, looks good. I noticed that the heroes/rookies buttons order is reversed in comparison to the finished military buildings. Do you want to swap them before we merge this branch? -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Changes look good :) -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Just found another issue in trainingsite-constructionsite saveloading which I´d like to fix with this branch -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Review: Approve Yep, that makes sense. Code LGTM, not tested yet. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Replied to the diff comment.
Also found and fixed another flaw in the inputqueuedisplay.
Diff comments:
>
> === modified file 'src/logic/playercommand.cc'
> --- src/logic/playercommand.cc2019-06-23 11:41:17 +
> +++ src/logic/playercommand.cc2019-06-23 14:34:42 +
> @@ -1131,39 +1139,43 @@
> PlayerImmovable& imm,
> const DescriptionIndex index,
> const WareWorker type,
> - const uint32_t max_fill)
> + const uint32_t max_fill,
> + bool cs_setting)
> : PlayerCommand(init_duetime, init_sender),
> serial_(imm.serial()),
> index_(index),
> type_(type),
> - max_fill_(max_fill) {
> + max_fill_(max_fill),
> + is_constructionsite_setting_(cs_setting) {
> }
>
> void CmdSetInputMaxFill::execute(Game& game) {
> MapObject* mo = game.objects().get_object(serial_);
> - if (upcast(ConstructionSite, cs, mo)) {
> - if (upcast(ProductionsiteSettings, s, cs->get_settings())) {
> - switch (type_) {
> - case wwWARE:
> - for (auto& pair : s->ware_queues) {
> - if (pair.first == index_) {
> - assert(pair.second.max_fill >=
> max_fill_);
> - pair.second.desired_fill =
> max_fill_;
> - return;
> - }
> - }
> - NEVER_HERE();
> - case wwWORKER:
> - for (auto& pair : s->worker_queues) {
> - if (pair.first == index_) {
> - assert(pair.second.max_fill >=
> max_fill_);
> - pair.second.desired_fill =
> max_fill_;
> - return;
> - }
> + if (is_constructionsite_setting_) {
> + if (upcast(ConstructionSite, cs, mo)) {
This cannot be asserted because the constructionsite might have been destroyed
or completed in the short interval between sending and executing the
playercommand
> + if (upcast(ProductionsiteSettings, s,
> cs->get_settings())) {
> + switch (type_) {
> + case wwWARE:
> + for (auto& pair : s->ware_queues) {
> + if (pair.first == index_) {
> +
> assert(pair.second.max_fill >= max_fill_);
> +
> pair.second.desired_fill = max_fill_;
> + return;
> + }
> + }
> + NEVER_HERE();
> + case wwWORKER:
> + for (auto& pair : s->worker_queues) {
> + if (pair.first == index_) {
> +
> assert(pair.second.max_fill >= max_fill_);
> +
> pair.second.desired_fill = max_fill_;
> + return;
> + }
> + }
> + NEVER_HERE();
> }
> NEVER_HERE();
> }
> - NEVER_HERE();
> }
> } else if (upcast(Building, b, mo)) {
> if (b->owner().player_number() == sender()) {
--
https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
1 small nit.
Diff comments:
>
> === modified file 'src/logic/playercommand.cc'
> --- src/logic/playercommand.cc2019-06-23 11:41:17 +
> +++ src/logic/playercommand.cc2019-06-23 14:34:42 +
> @@ -1131,39 +1139,43 @@
> PlayerImmovable& imm,
> const DescriptionIndex index,
> const WareWorker type,
> - const uint32_t max_fill)
> + const uint32_t max_fill,
> + bool cs_setting)
> : PlayerCommand(init_duetime, init_sender),
> serial_(imm.serial()),
> index_(index),
> type_(type),
> - max_fill_(max_fill) {
> + max_fill_(max_fill),
> + is_constructionsite_setting_(cs_setting) {
> }
>
> void CmdSetInputMaxFill::execute(Game& game) {
> MapObject* mo = game.objects().get_object(serial_);
> - if (upcast(ConstructionSite, cs, mo)) {
> - if (upcast(ProductionsiteSettings, s, cs->get_settings())) {
> - switch (type_) {
> - case wwWARE:
> - for (auto& pair : s->ware_queues) {
> - if (pair.first == index_) {
> - assert(pair.second.max_fill >=
> max_fill_);
> - pair.second.desired_fill =
> max_fill_;
> - return;
> - }
> - }
> - NEVER_HERE();
> - case wwWORKER:
> - for (auto& pair : s->worker_queues) {
> - if (pair.first == index_) {
> - assert(pair.second.max_fill >=
> max_fill_);
> - pair.second.desired_fill =
> max_fill_;
> - return;
> - }
> + if (is_constructionsite_setting_) {
> + if (upcast(ConstructionSite, cs, mo)) {
You can replace the if with an assert here. Same below.
> + if (upcast(ProductionsiteSettings, s,
> cs->get_settings())) {
> + switch (type_) {
> + case wwWARE:
> + for (auto& pair : s->ware_queues) {
> + if (pair.first == index_) {
> +
> assert(pair.second.max_fill >= max_fill_);
> +
> pair.second.desired_fill = max_fill_;
> + return;
> + }
> + }
> + NEVER_HERE();
> + case wwWORKER:
> + for (auto& pair : s->worker_queues) {
> + if (pair.first == index_) {
> +
> assert(pair.second.max_fill >= max_fill_);
> +
> pair.second.desired_fill = max_fill_;
> + return;
> + }
> + }
> + NEVER_HERE();
> }
> NEVER_HERE();
> }
> - NEVER_HERE();
> }
> } else if (upcast(Building, b, mo)) {
> if (b->owner().player_number() == sender()) {
--
https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/369210
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
@bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
inputqueues again @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Tested and still working - thanks for this great feature! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/constructionsite_options. ___ 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/constructionsite_options into lp:widelands
Review: Approve
LGTM now :)
I'd like to retest this due to the code changes made during review.
Diff comments:
>
> === modified file 'src/logic/map_objects/tribes/militarysite.cc'
> --- src/logic/map_objects/tribes/militarysite.cc 2019-05-26 17:21:15
> +
> +++ src/logic/map_objects/tribes/militarysite.cc 2019-05-28 15:09:01
> +
> @@ -982,6 +982,13 @@
> return false;
> }
>
> +const BuildingSettings* MilitarySite::create_building_settings() const {
> + MilitarysiteSettings* settings = new MilitarysiteSettings(descr());
> + settings->desired_capacity = std::min(settings->max_capacity,
> soldier_control_.soldier_capacity());
> + settings->prefer_heroes = soldier_preference_ ==
> SoldierPreference::kHeroes;
Yes I did! So, this is correct :)
> + return settings;
> +}
> +
> // setters
>
> void MilitarySite::set_soldier_preference(SoldierPreference p) {
--
https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/constructionsite_options.
___
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/constructionsite_options into lp:widelands
No problem. I have very little time for Widelands at the moment anyway :)
Diff comments:
>
> === added file 'src/logic/map_objects/tribes/building_settings.cc'
> --- src/logic/map_objects/tribes/building_settings.cc 1970-01-01 00:00:00
> +
> +++ src/logic/map_objects/tribes/building_settings.cc 2019-05-28 15:09:01
> +
> @@ -0,0 +1,346 @@
> +/*
> + * Copyright (C) 2002-2019 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +#include "logic/map_objects/tribes/building_settings.h"
> +
> +#include "io/fileread.h"
> +#include "io/filewrite.h"
> +#include "logic/game.h"
> +#include "logic/game_data_error.h"
> +#include "logic/map_objects/tribes/militarysite.h"
> +#include "logic/map_objects/tribes/productionsite.h"
> +#include "logic/map_objects/tribes/trainingsite.h"
> +#include "logic/map_objects/tribes/tribe_descr.h"
> +#include "logic/map_objects/tribes/warehouse.h"
> +
> +namespace Widelands {
> +
> +ProductionsiteSettings::ProductionsiteSettings(const ProductionSiteDescr&
> descr)
> + : BuildingSettings(descr.name()), stopped(false) {
> + for (WareRange i(descr.input_wares()); i; ++i) {
Done
> + ware_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> + for (WareRange i(descr.input_workers()); i; ++i) {
> + worker_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> +}
> +
> +MilitarysiteSettings::MilitarysiteSettings(const MilitarySiteDescr& descr)
> + : BuildingSettings(descr.name()),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()),
> + prefer_heroes(descr.prefers_heroes_at_start_) {
> +}
> +
> +TrainingsiteSettings::TrainingsiteSettings(const TrainingSiteDescr& descr)
> + : ProductionsiteSettings(descr),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()) {
> +}
> +
> +WarehouseSettings::WarehouseSettings(const WarehouseDescr& wh, const
> TribeDescr& tribe)
> + : BuildingSettings(wh.name()),
> launch_expedition_allowed(wh.get_isport()), launch_expedition(false) {
> + for (const DescriptionIndex di : tribe.wares()) {
> + ware_preferences.emplace(di, StockPolicy::kNormal);
> + }
> + for (const DescriptionIndex di : tribe.workers()) {
> + worker_preferences.emplace(di, StockPolicy::kNormal);
has_demand_check() returns true only for soldiers and 2nd carriers, but a
warehouse allows stock policies for all workers except the ones listed in
worker_types_without_cost()
> + }
> + for (const DescriptionIndex di : tribe.worker_types_without_cost()) {
> + worker_preferences.erase(di);
> + }
> +}
> +
> +void ProductionsiteSettings::apply(const BuildingSettings& bs) {
> + BuildingSettings::apply(bs);
> + if (upcast(const ProductionsiteSettings, s, )) {
> + stopped = s->stopped;
> + for (auto& pair : ware_queues) {
> + for (const auto& other : s->ware_queues) {
> + if (pair.first == other.first) {
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> + break;
> + }
> + }
> + }
> + for (auto& pair : worker_queues) {
> + for (const auto& other : s->worker_queues) {
> + if (pair.first == other.first) {
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> + break;
> + }
> + }
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/constructionsite_options into lp:widelands
The changes look good - there are still some open comments, though. Sorry it
took me a while to get back to this.
Diff comments:
>
> === added file 'src/logic/map_objects/tribes/building_settings.cc'
> --- src/logic/map_objects/tribes/building_settings.cc 1970-01-01 00:00:00
> +
> +++ src/logic/map_objects/tribes/building_settings.cc 2019-05-28 15:09:01
> +
> @@ -0,0 +1,346 @@
> +/*
> + * Copyright (C) 2002-2019 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +#include "logic/map_objects/tribes/building_settings.h"
> +
> +#include "io/fileread.h"
> +#include "io/filewrite.h"
> +#include "logic/game.h"
> +#include "logic/game_data_error.h"
> +#include "logic/map_objects/tribes/militarysite.h"
> +#include "logic/map_objects/tribes/productionsite.h"
> +#include "logic/map_objects/tribes/trainingsite.h"
> +#include "logic/map_objects/tribes/tribe_descr.h"
> +#include "logic/map_objects/tribes/warehouse.h"
> +
> +namespace Widelands {
> +
> +ProductionsiteSettings::ProductionsiteSettings(const ProductionSiteDescr&
> descr)
> + : BuildingSettings(descr.name()), stopped(false) {
> + for (WareRange i(descr.input_wares()); i; ++i) {
The WareRange struct still exists and is being used in productionsites - please
get rid of it completely.
> + ware_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> + for (WareRange i(descr.input_workers()); i; ++i) {
> + worker_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> +}
> +
> +MilitarysiteSettings::MilitarysiteSettings(const MilitarySiteDescr& descr)
> + : BuildingSettings(descr.name()),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()),
> + prefer_heroes(descr.prefers_heroes_at_start_) {
> +}
> +
> +TrainingsiteSettings::TrainingsiteSettings(const TrainingSiteDescr& descr)
> + : ProductionsiteSettings(descr),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()) {
> +}
> +
> +WarehouseSettings::WarehouseSettings(const WarehouseDescr& wh, const
> TribeDescr& tribe)
> + : BuildingSettings(wh.name()),
> launch_expedition_allowed(wh.get_isport()), launch_expedition(false) {
> + for (const DescriptionIndex di : tribe.wares()) {
> + ware_preferences.emplace(di, StockPolicy::kNormal);
Duh, forget that I mentioned it - I got confused with the economy settings
window.
> + }
> + for (const DescriptionIndex di : tribe.workers()) {
> + worker_preferences.emplace(di, StockPolicy::kNormal);
This has not been cleaned up yet
> + }
> + for (const DescriptionIndex di : tribe.worker_types_without_cost()) {
> + worker_preferences.erase(di);
> + }
> +}
> +
> +void ProductionsiteSettings::apply(const BuildingSettings& bs) {
> + BuildingSettings::apply(bs);
> + if (upcast(const ProductionsiteSettings, s, )) {
> + stopped = s->stopped;
> + for (auto& pair : ware_queues) {
> + for (const auto& other : s->ware_queues) {
> + if (pair.first == other.first) {
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> + break;
> + }
> + }
> + }
> + for (auto& pair : worker_queues) {
> + for (const auto& other : s->worker_queues) {
> + if (pair.first == other.first) {
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/constructionsite_options into lp:widelands
Implemented your comments
Diff comments:
>
> === added file 'src/logic/map_objects/tribes/building_settings.cc'
> --- src/logic/map_objects/tribes/building_settings.cc 1970-01-01 00:00:00
> +
> +++ src/logic/map_objects/tribes/building_settings.cc 2019-05-28 15:09:01
> +
> @@ -0,0 +1,346 @@
> +/*
> + * Copyright (C) 2002-2019 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +#include "logic/map_objects/tribes/building_settings.h"
> +
> +#include "io/fileread.h"
> +#include "io/filewrite.h"
> +#include "logic/game.h"
> +#include "logic/game_data_error.h"
> +#include "logic/map_objects/tribes/militarysite.h"
> +#include "logic/map_objects/tribes/productionsite.h"
> +#include "logic/map_objects/tribes/trainingsite.h"
> +#include "logic/map_objects/tribes/tribe_descr.h"
> +#include "logic/map_objects/tribes/warehouse.h"
> +
> +namespace Widelands {
> +
> +ProductionsiteSettings::ProductionsiteSettings(const ProductionSiteDescr&
> descr)
> + : BuildingSettings(descr.name()), stopped(false) {
> + for (WareRange i(descr.input_wares()); i; ++i) {
> + ware_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> + for (WareRange i(descr.input_workers()); i; ++i) {
> + worker_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> +}
> +
> +MilitarysiteSettings::MilitarysiteSettings(const MilitarySiteDescr& descr)
> + : BuildingSettings(descr.name()),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()),
> + prefer_heroes(descr.prefers_heroes_at_start_) {
> +}
> +
> +TrainingsiteSettings::TrainingsiteSettings(const TrainingSiteDescr& descr)
> + : ProductionsiteSettings(descr),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()) {
> +}
> +
> +WarehouseSettings::WarehouseSettings(const WarehouseDescr& wh, const
> TribeDescr& tribe)
> + : BuildingSettings(wh.name()),
> launch_expedition_allowed(wh.get_isport()), launch_expedition(false) {
> + for (const DescriptionIndex di : tribe.wares()) {
> + ware_preferences.emplace(di, StockPolicy::kNormal);
Why? A warehouse allows to set stock policies for all wares.
> + }
> + for (const DescriptionIndex di : tribe.workers()) {
> + worker_preferences.emplace(di, StockPolicy::kNormal);
> + }
> + for (const DescriptionIndex di : tribe.worker_types_without_cost()) {
> + worker_preferences.erase(di);
> + }
> +}
> +
> +void ProductionsiteSettings::apply(const BuildingSettings& bs) {
> + BuildingSettings::apply(bs);
> + if (upcast(const ProductionsiteSettings, s, )) {
> + stopped = s->stopped;
> + for (auto& pair : ware_queues) {
> + for (const auto& other : s->ware_queues) {
> + if (pair.first == other.first) {
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> + break;
> + }
> + }
> + }
> + for (auto& pair : worker_queues) {
> + for (const auto& other : s->worker_queues) {
> + if (pair.first == other.first) {
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> + break;
> + }
> + }
> + }
> + }
> +}
> +
> +void TrainingsiteSettings::apply(const BuildingSettings& bs) {
> + ProductionsiteSettings::apply(bs);
> + if (upcast(const
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/constructionsite_options into lp:widelands
Review: Needs Fixing
I finally got around to doing the code review. This branch still has a lot of
room for streamlining the code and the performance.
Diff comments:
>
> === added file 'src/logic/map_objects/tribes/building_settings.cc'
> --- src/logic/map_objects/tribes/building_settings.cc 1970-01-01 00:00:00
> +
> +++ src/logic/map_objects/tribes/building_settings.cc 2019-05-28 15:09:01
> +
> @@ -0,0 +1,346 @@
> +/*
> + * Copyright (C) 2002-2019 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +#include "logic/map_objects/tribes/building_settings.h"
> +
> +#include "io/fileread.h"
> +#include "io/filewrite.h"
> +#include "logic/game.h"
> +#include "logic/game_data_error.h"
> +#include "logic/map_objects/tribes/militarysite.h"
> +#include "logic/map_objects/tribes/productionsite.h"
> +#include "logic/map_objects/tribes/trainingsite.h"
> +#include "logic/map_objects/tribes/tribe_descr.h"
> +#include "logic/map_objects/tribes/warehouse.h"
> +
> +namespace Widelands {
> +
> +ProductionsiteSettings::ProductionsiteSettings(const ProductionSiteDescr&
> descr)
> + : BuildingSettings(descr.name()), stopped(false) {
> + for (WareRange i(descr.input_wares()); i; ++i) {
Why not simply use
for (const auto& WareAmount amount : descr.input_wares()) {
}
And get rid of the WareRange struct?
> + ware_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> + for (WareRange i(descr.input_workers()); i; ++i) {
> + worker_queues.push_back(std::make_pair(i.current->first,
> + InputQueueSetting{i.current->second,
> i.current->second, kPriorityNormal}));
> + }
> +}
> +
> +MilitarysiteSettings::MilitarysiteSettings(const MilitarySiteDescr& descr)
> + : BuildingSettings(descr.name()),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()),
> + prefer_heroes(descr.prefers_heroes_at_start_) {
> +}
> +
> +TrainingsiteSettings::TrainingsiteSettings(const TrainingSiteDescr& descr)
> + : ProductionsiteSettings(descr),
> + max_capacity(descr.get_max_number_of_soldiers()),
> + desired_capacity(descr.get_max_number_of_soldiers()) {
> +}
> +
> +WarehouseSettings::WarehouseSettings(const WarehouseDescr& wh, const
> TribeDescr& tribe)
> + : BuildingSettings(wh.name()),
> launch_expedition_allowed(wh.get_isport()), launch_expedition(false) {
> + for (const DescriptionIndex di : tribe.wares()) {
> + ware_preferences.emplace(di, StockPolicy::kNormal);
Use WareDescr::has_demand_check to not add wares like log, wheat.
> + }
> + for (const DescriptionIndex di : tribe.workers()) {
> + worker_preferences.emplace(di, StockPolicy::kNormal);
If you use WorkerDescr::has_demand_check() here, you can get rid of the loop
below where you erase them.
> + }
> + for (const DescriptionIndex di : tribe.worker_types_without_cost()) {
> + worker_preferences.erase(di);
> + }
> +}
> +
> +void ProductionsiteSettings::apply(const BuildingSettings& bs) {
> + BuildingSettings::apply(bs);
> + if (upcast(const ProductionsiteSettings, s, )) {
> + stopped = s->stopped;
> + for (auto& pair : ware_queues) {
> + for (const auto& other : s->ware_queues) {
> + if (pair.first == other.first) {
These 2 data structures should have the same keys always - so you should be
able to get the element from the second map directly and get rid of the inner
loop. Same below for the workers.
> + pair.second.priority =
> other.second.priority;
> + pair.second.desired_fill =
> std::min(pair.second.max_fill, other.second.desired_fill);
> + break;
> + }
> + }
> + }
> + for (auto& pair : worker_queues) {
> + for (const auto& other : s->worker_queues) {
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/constructionsite_options into lp:widelands
OK, let's leave the design as it is :) This doesn't compile: ow.cc.o -c ../src/wui/dismantlesitewindow.cc In file included from ../src/wui/dismantlesitewindow.h:25:0, from ../src/wui/dismantlesitewindow.cc:20: ../src/wui/buildingwindow.h: In member function ‘void BuildingWindow::set_building_descr_for_help(const Widelands::BuildingDescr&)’: ../src/wui/buildingwindow.h:99:30: error: ‘void Widelands::BuildingDescr::operator=(const Widelands::BuildingDescr&)’ is private within this context building_descr_for_help_ = d; ^ Once that has been fixed, we still need to look at the code. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
> "Launch expedition" should be "Start an expedition" like on the button for > the finished port. > The target of the help button does not get updated when enhancing, e.g. > construction site for deeper coal mine will show the coal mine instead. > ASan Should all be fixed now > It would be nice if the tab was remembered after using the "enhance" button. This is exactly the same as bug 1818857 so it should be fixed independent from this branch. There will likely be one catch-all fix for both flavours of the bug. > It would be nice if the extra options like "Start an expedition" or "Stop" > would have the same UI buttons as the finished sites. I can live with the > checkboxes though. What I could do is to render the appropriate icon next to the checkbox if you like. But unless we change the other UI elements in such a way that they show clearly what the current state is, there is no alternative to the checkboxes IMHO, since the current icons are only acceptable because one can see the active state elsewhere (stats string / expedition tab), which can not happen here. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Also, this: = ==6265==ERROR: LeakSanitizer: detected memory leaks Direct leak of 2744 byte(s) in 49 object(s) allocated from: #0 0x7ff86f576458 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xe0458) #1 0x55643637edfd in Widelands::MilitarySite::create_building_settings() const ../src/logic/map_objects/tribes/militarysite.cc:978 #2 0x556435cb4c9b in Widelands::Player::enhance_or_dismantle(Widelands::Building*, unsigned char) ../src/logic/player.cc:763 #3 0x556435cb47a3 in Widelands::Player::dismantle_building(Widelands::Building*) ../src/logic/player.cc:743 #4 0x5564366afc07 in Widelands::CmdDismantleBuilding::execute(Widelands::Game&) ../src/logic/playercommand.cc:701 #5 0x5564366a690e in Widelands::CmdQueue::run_queue(int, unsigned int&) ../src/logic/cmd_queue.cc:122 #6 0x556435c8be44 in Widelands::Game::think() ../src/logic/game.cc:599 #7 0x55643605fe76 in InteractiveBase::think() ../src/wui/interactive_base.cc:475 #8 0x5564360b3bde in InteractivePlayer::think() ../src/wui/interactive_player.cc:228 #9 0x556435eec265 in UI::Panel::do_think() ../src/ui_basic/panel.cc:483 #10 0x556435ee95ca in UI::Panel::do_run() ../src/ui_basic/panel.cc:184 #11 0x556435983b5d in UI::Panel::Returncodes UI::Panel::run() ../src/ui_basic/panel.h:104 #12 0x556435c8b669 in Widelands::Game::run(UI::ProgressWindow*, Widelands::Game::StartGameType, std::__cxx11::basic_string, std::allocator > const&, bool, std::__cxx11::basic_string, std::allocator > const&) ../src/logic/game.cc:569 #13 0x55643596f976 in WLApplication::new_game() ../src/wlapplication.cc:1354 #14 0x55643596da4a in WLApplication::mainmenu_singleplayer() ../src/wlapplication.cc:1208 #15 0x55643596cb95 in WLApplication::mainmenu() ../src/wlapplication.cc:1114 #16 0x556435963b7b in WLApplication::run() ../src/wlapplication.cc:470 #17 0x55643595f8ce in main ../src/main.cc:44 #18 0x7ff86c9a9b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
It would be nice if the tab was remembered after using the "enhance" button. The target of the help button does not get updated when enhancing, e.g. construction site for deeper coal mine will show the coal mine instead. "Launch expedition" should be "Start an expedition" like on the button for the finished port. It would be nice if the extra options like "Start an expedition" or "Stop" would have the same UI buttons as the finished sites. I can live with the checkboxes though. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
OK, will change it like this -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Would it be possible to move the tabs for Warehouses' and Ports' to the top, instead of having to tab rows? -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
I am getting a similar error when loading our homepage reference screenshot savegame Build, so we already have a change to saveloading in trunk where we overlooked having to increase the packet number. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Trying to load a savgame from trunk prints now in a window (and console): Game data error buildingdata: building 524547: not found Loading in trunk works fine. Save game: https://bugs.launchpad.net/widelands/+bug/1597310/+attachment/5264357/+files/construction_site_settings.wgf -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Seems to be fixed. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Since the bug is not reproducible, I can´t be certain, but it should most likely be gone now -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
This seems to be the same issue as the crash with animation ID -432781024, but with a more usable file number. Will push a fix soonish -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
And another assertion. Assertion failed: (animation_index < animations.size()), function draw, file /Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/constructionsite.cc, line 85. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Oops… should be fixed now -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Looks like this branch breaks savegame compatibility. When trying to load a game which runs fine in trunk i get: Writing Buildingdata Data ... widelands: /home/kaputtnik/Quellcode/widelands-repo/constructionsite_options/src/map_io/map_buildingdata_packet.cc:974: void Widelands::MapBuildingdataPacket::write_constructionsite(const Widelands::ConstructionSite&, FileWrite&, Widelands::Game&, Widelands::MapObjectSaver&): Assertion `constructionsite.settings_' failed. Abgebrochen (Speicherabzug geschrieben) -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Assert fail fixed :) -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Assertion failed: (military_site_->capacity_ != capacity), function set_soldier_capacity, file /Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/militarysite.cc, line 85. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Took care of the warnings and fixed the input queue layout. > src/graphic/animation.cc:451] Requested unknown animation with id: -432781024 A backtrace would be helpful… Did you get this while an (enhanced?) constructionsite was being built, or when a constructionsite was being completed? I got similar crashes before, but I thought I had caught all corner cases now… -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
With this branch I get crashes after a while. Not sure if the culprit is this branch or something in trunk, though. /Users/toni/Launchpad/widelands-repo/working_tree/src/graphic/animation.cc:451] Requested unknown animation with id: -432781024 ==54196==ERROR: AddressSanitizer: SEGV on unknown address 0x (pc 0x bp 0x7ffee6344650 sp 0x7ffee63445a8 T0) -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Your buttons are a little too big. The buttons for in-/decreasing the wares are 24x24 while yours are 25x30. Also, some wares are covered by the button; see the fish in the tavern (I guess that is because of your buttons being to big) -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Four warnings left: src/logic/playercommand.cc:2288:6: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default] default: src/logic/playercommand.cc:2384:6: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default] default: src/wui/constructionsitewindow.cc:197:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default] default: src/logic/map_objects/tribes/building_settings.cc:176:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default] default: -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
hmm. I would prefer icons but you are completely right. I opened a topic in the forum: https://wl.widelands.org/forum/topic/4518/?page=1#post-27957 -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
Some comments regarding the UI. I think we should keep the UI consistent. Instead of checkboxes please use the icons/buttons that are used in the building's window. The same applies for the priority settings. We use traffic lights instead of buttons. -- https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/constructionsite_options 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/constructionsite_options into lp:widelands
I get these compiler warnings a couple of times.
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:42:8:
warning: 'Widelands::BuildingSettings' has virtual functions but non-virtual
destructor [-Wnon-virtual-dtor]
struct BuildingSettings {
^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:57:8:
warning: 'Widelands::ProductionsiteSettings' has virtual functions but
non-virtual destructor [-Wnon-virtual-dtor]
struct ProductionsiteSettings : public BuildingSettings {
^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:74:8:
warning: 'Widelands::MilitarysiteSettings' has virtual functions but
non-virtual destructor [-Wnon-virtual-dtor]
struct MilitarysiteSettings : public BuildingSettings {
^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:86:8:
warning: 'Widelands::TrainingsiteSettings' has virtual functions but
non-virtual destructor [-Wnon-virtual-dtor]
struct TrainingsiteSettings : public ProductionsiteSettings {
^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:97:8:
warning: 'Widelands::WarehouseSettings' has virtual functions but non-virtual
destructor [-Wnon-virtual-dtor]
struct WarehouseSettings : public BuildingSettings {
^
5 warnings generated.
--
https://code.launchpad.net/~widelands-dev/widelands/constructionsite_options/+merge/367428
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/constructionsite_options 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
