Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
Inputqueues again @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/reed-compatibility 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/bug-1826744-lobby-commands into lp:widelands
Review: Approve LGTM :) But we need the server to be in place to test this better. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1826744-lobby-commands/+merge/368285 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1826744-lobby-commands. ___ 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: 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/unify-program-parsers into lp:widelands
Thanks for the review! It should be OK. There were some merge conflicts, which
is why there might be some small additional stuff, but nothing serious.
I have pushed a commit to address your comments.
Diff comments:
>
> === modified file 'src/logic/editor_game_base.cc'
> --- src/logic/editor_game_base.cc 2019-04-26 05:52:49 +
> +++ src/logic/editor_game_base.cc 2019-06-06 06:00:01 +
> @@ -113,50 +113,57 @@
> * throws an exception if something goes wrong
> */
> void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const
> type) {
> - // should only be called when a map was already loaded
> - assert(map_.filesystem());
> -
> - g_fs->ensure_directory_exists(kTempFileDir);
> -
> - std::string filename = kTempFileDir + g_fs->file_separator() +
> timestring() + "_mapdata";
> - std::string complete_filename = filename + kTempFileExtension;
> -
> - // if a file with that name already exists, then try a few name
> modifications
> - if (g_fs->file_exists(complete_filename)) {
> - int suffix;
> - for (suffix = 0; suffix <= 9; suffix++) {
> - complete_filename = filename + "-" +
> std::to_string(suffix) + kTempFileExtension;
> - if (!g_fs->file_exists(complete_filename))
> - break;
> - }
> - if (suffix > 9) {
> - throw
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all
> considered "
> - "filenames a file already existed");
> - }
> - }
> -
> - // create tmp_fs_
> - tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> -
> - // save necessary map data (we actually save the whole map)
> - std::unique_ptr wms(new
> Widelands::MapSaver(*tmp_fs_, *this));
> - wms->save();
> -
> - // swap map fs
> - std::unique_ptr mapfs(tmp_fs_->make_sub_file_system("."));
> - map_.swap_filesystem(mapfs);
> - mapfs.reset();
> -
> - // This is just a convenience hack:
> - // If tmp_fs_ is a zip filesystem then - because of the way zip
> filesystems are currently
> - // implemented -
> - // the file is still in zip mode right now, which means that the file
> isn't finalized yet, i.e.,
> - // not even a valid zip file until zip mode ends. To force ending the
> zip mode (thus finalizing
> - // the file)
> - // we simply perform a (otherwise useless) filesystem request.
> - // It's not strictly necessary, but this way we get a valid zip file
> immediately istead of
> - // at some unkown later point (when an unzip operation happens or a
> filesystem object destructs).
> - tmp_fs_->file_exists("binary");
> + if (!map_.filesystem()) {
> + return;
It's the same behaviour as in trunk, but you're right - it's not even needed.
Removed the whole if clause.
> + }
> +
> + // save map data to temporary file and reassign map fs
> + try {
> + g_fs->ensure_directory_exists(kTempFileDir);
> +
> + std::string filename = kTempFileDir + g_fs->file_separator() +
> timestring() + "_mapdata";
> + std::string complete_filename = filename + kTempFileExtension;
> +
> + // if a file with that name already exists, then try a few name
> modifications
> + if (g_fs->file_exists(complete_filename)) {
> + int suffix;
> + for (suffix = 0; suffix <= 9; suffix++) {
> + complete_filename = filename + "-" +
> std::to_string(suffix) + kTempFileExtension;
> + if (!g_fs->file_exists(complete_filename))
> + break;
> + }
> + if (suffix > 9) {
> + throw
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all
> considered "
> + "filenames a
> file already existed");
> + }
> + }
> +
> + // create tmp_fs_
> + tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename,
> type));
> +
> + // save necessary map data (we actually save the whole map)
> + std::unique_ptr wms(new
> Widelands::MapSaver(*tmp_fs_, *this));
> + wms->save();
> +
> + // swap map fs
> + std::unique_ptr
> mapfs(tmp_fs_->make_sub_file_system("."));
> + map_.swap_filesystem(mapfs);
> + mapfs.reset();
> +
> + // This is just a convenience hack:
> + // If tmp_fs_ is a zip filesystem then - because of the way zip
> filesystems are currently
> + // implemented -
> + // the file is still in zip mode right now, which means that
> the file isn't finalized yet, i.e.,
> + // not
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
Done. Thanks for the review! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/reed-compatibility 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/reed-compatibility into lp:widelands
This code is pretty old, so I'm sure we would have had bug reports for it in the past if it was broken. It should be fine if iterating starts with the most advanced working position, because then we won't put a master miner i a miner's slot and end up stumped when we have to put a miner in a master miner's slot, which is not allowed. I'll reinstate the goto to make sure we can't possibly have any accidental semantic changes, so that we can get this bugfix in. -- https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/reed-compatibility 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/ferry into lp:widelands
I'll need to set aside a weekend for this, so it can take some time until I get around to it. -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ferry. ___ 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/unify-program-parsers into lp:widelands
Looking mostly good, two comments in the diff.
I went through it commit by commit while ignoring the merges of trunk and
list-directories-in-cpp. I hope that is okay? I am a bit unsure about that
since there seem to be changes in the full diff that I can't find in the single
commits.
Diff comments:
>
> === modified file 'src/logic/editor_game_base.cc'
> --- src/logic/editor_game_base.cc 2019-04-26 05:52:49 +
> +++ src/logic/editor_game_base.cc 2019-06-06 06:00:01 +
> @@ -113,50 +113,57 @@
> * throws an exception if something goes wrong
> */
> void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const
> type) {
> - // should only be called when a map was already loaded
> - assert(map_.filesystem());
> -
> - g_fs->ensure_directory_exists(kTempFileDir);
> -
> - std::string filename = kTempFileDir + g_fs->file_separator() +
> timestring() + "_mapdata";
> - std::string complete_filename = filename + kTempFileExtension;
> -
> - // if a file with that name already exists, then try a few name
> modifications
> - if (g_fs->file_exists(complete_filename)) {
> - int suffix;
> - for (suffix = 0; suffix <= 9; suffix++) {
> - complete_filename = filename + "-" +
> std::to_string(suffix) + kTempFileExtension;
> - if (!g_fs->file_exists(complete_filename))
> - break;
> - }
> - if (suffix > 9) {
> - throw
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all
> considered "
> - "filenames a file already existed");
> - }
> - }
> -
> - // create tmp_fs_
> - tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> -
> - // save necessary map data (we actually save the whole map)
> - std::unique_ptr wms(new
> Widelands::MapSaver(*tmp_fs_, *this));
> - wms->save();
> -
> - // swap map fs
> - std::unique_ptr mapfs(tmp_fs_->make_sub_file_system("."));
> - map_.swap_filesystem(mapfs);
> - mapfs.reset();
> -
> - // This is just a convenience hack:
> - // If tmp_fs_ is a zip filesystem then - because of the way zip
> filesystems are currently
> - // implemented -
> - // the file is still in zip mode right now, which means that the file
> isn't finalized yet, i.e.,
> - // not even a valid zip file until zip mode ends. To force ending the
> zip mode (thus finalizing
> - // the file)
> - // we simply perform a (otherwise useless) filesystem request.
> - // It's not strictly necessary, but this way we get a valid zip file
> immediately istead of
> - // at some unkown later point (when an unzip operation happens or a
> filesystem object destructs).
> - tmp_fs_->file_exists("binary");
> + if (!map_.filesystem()) {
> + return;
Is silently doing nothing the right behavior here? Shouldn't it throw a
wrong_gamestate or something like that?
> + }
> +
> + // save map data to temporary file and reassign map fs
> + try {
> + g_fs->ensure_directory_exists(kTempFileDir);
> +
> + std::string filename = kTempFileDir + g_fs->file_separator() +
> timestring() + "_mapdata";
> + std::string complete_filename = filename + kTempFileExtension;
> +
> + // if a file with that name already exists, then try a few name
> modifications
> + if (g_fs->file_exists(complete_filename)) {
> + int suffix;
> + for (suffix = 0; suffix <= 9; suffix++) {
> + complete_filename = filename + "-" +
> std::to_string(suffix) + kTempFileExtension;
> + if (!g_fs->file_exists(complete_filename))
> + break;
> + }
> + if (suffix > 9) {
> + throw
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all
> considered "
> + "filenames a
> file already existed");
> + }
> + }
> +
> + // create tmp_fs_
> + tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename,
> type));
> +
> + // save necessary map data (we actually save the whole map)
> + std::unique_ptr wms(new
> Widelands::MapSaver(*tmp_fs_, *this));
> + wms->save();
> +
> + // swap map fs
> + std::unique_ptr
> mapfs(tmp_fs_->make_sub_file_system("."));
> + map_.swap_filesystem(mapfs);
> + mapfs.reset();
> +
> + // This is just a convenience hack:
> + // If tmp_fs_ is a zip filesystem then - because of the way zip
> filesystems are currently
> + // implemented -
> + // the file is still in zip
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
Sorry, I am having second thoughts about the goto-part. Actually, I am not only unsure about the goto-replacement but over the whole code part independent of your changes. :-/ If I understand it right, the code iterates over the possible jobs in the production site. If there is a job the loaded worker can work on, we iterate over all working positions of the site and check if *any* of these is empty, so we can assign the worker. Shouldn't we only check the working positions that are for the required job? Otherwise, the code could break with production sites which have multiple types of worker positions (Mines probably? These need a miner and a master miner). Maybe I am just confused and the code is fine, but it would be good if someone could check it. -- https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/reed-compatibility 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/ferry into lp:widelands
Now, how do we get progress here? Will try to play this again on Cavisson -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ferry. ___ 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
