[Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/reed-compatibility into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/reed-compatibility. ___ 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
Review: Approve -- https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/reed-compatibility. ___ 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
@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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways. Travis build 5164. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/542121335. -- 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
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/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/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/reed-compatibility into lp:widelands
I have added a second if statement for the test variable to simulate the effect of the goto and break out of the outer loop. Thanks for the review! @bunybot 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
Continuous integration builds have changed state: Travis build 5154. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/541619525. Appveyor build 4936. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_reed_compatibility-4936. -- 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
> So ... you created a diff of 800 lines by passing objects around so you can > call a method in only a handful of places?? I like it! :) Lol yep. It will save a bit of memory though if the objects are created only once. For triggering the bug, you need a savegame created with bzr9118 or older that has reed or wheat fields on 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/reed-compatibility into lp:widelands
So ... you created a diff of 800 lines by passing objects around so you can
call a method in only a handful of places?? I like it! :)
The changes are mostly looking good to me. Replacing the GOTO might be broken,
though, see my diff comment below.
I haven't tested the changes, mostly because I have no sensible idea how to do
so. But based on the code I guess it shouldn't break anything.
Diff comments:
>
> === modified file 'src/map_io/map_buildingdata_packet.cc'
> --- src/map_io/map_buildingdata_packet.cc 2019-05-18 11:58:43 +
> +++ src/map_io/map_buildingdata_packet.cc 2019-05-26 03:34:22 +
> @@ -615,14 +618,15 @@
> if
> (worker_descr.can_act_as(working_position.first)) {
> while (wp->worker ||
> wp->worker_request) {
> ++wp;
> - if (!--count)
> - goto
> end_working_position;
> + if (!--count) {
> + continue;
> + }
While I appreciate getting rid of GOTO, I am not sure if the logic change here
is intentional. I think the continue will influence the while() loop while I
guess that you want to influence the for() loop?
A possible replacement would be setting a flag and break-ing within the
while(), then afterwards check for the flag, resetting it and using continue.
> }
> found_working_position = true;
> break;
> - } else
> + } else {
> wp += count;
> - end_working_position:;
> + }
> }
>
> if (!found_working_position)
>
> === modified file 'src/map_io/map_flagdata_packet.h'
> --- src/map_io/map_flagdata_packet.h 2019-02-23 11:00:49 +
> +++ src/map_io/map_flagdata_packet.h 2019-05-26 03:34:22 +
> @@ -21,7 +21,14 @@
> #define WL_MAP_IO_MAP_FLAGDATA_PACKET_H
>
> #include "map_io/map_data_packet.h"
> +#include "map_io/tribes_legacy_lookup_table.h"
>
> -MAP_DATA_PACKET(MapFlagdataPacket)
> +namespace Widelands {
> +class MapFlagdataPacket {
> +public:
> + void read(FileSystem&, EditorGameBase&, bool, MapObjectLoader&, const
> TribesLegacyLookupTable& tribes_lookup_table);
> + void write(FileSystem&, EditorGameBase&, MapObjectSaver&);
> +};
> +}
Maybe provide a second macro for the extended version? Just an idea, this way
is okay as well.
>
> #endif // end of include guard: WL_MAP_IO_MAP_FLAGDATA_PACKET_H
--
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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
Continuous integration builds have changed state: Travis build 5058. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/537337141. Appveyor build 4838. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_reed_compatibility-4838. -- 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands
GunChleoc has proposed merging lp:~widelands-dev/widelands/reed-compatibility
into lp:widelands.
Commit message:
Fix savegame compatibility for reed, buildings, players view and economy
requests.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
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.
=== modified file 'src/economy/expedition_bootstrap.cc'
--- src/economy/expedition_bootstrap.cc 2019-02-23 11:00:49 +
+++ src/economy/expedition_bootstrap.cc 2019-05-26 03:34:22 +
@@ -203,7 +203,7 @@
}
void ExpeditionBootstrap::load(
- Warehouse& warehouse, FileRead& fr, Game& game, MapObjectLoader& mol, uint16_t packet_version) {
+ Warehouse& warehouse, FileRead& fr, Game& game, MapObjectLoader& mol, const TribesLegacyLookupTable& tribes_lookup_table, uint16_t packet_version) {
static const uint16_t kCurrentPacketVersion = 7;
assert(queues_.empty());
@@ -214,7 +214,7 @@
uint8_t num_queues = fr.unsigned_8();
for (uint8_t i = 0; i < num_queues; ++i) {
WorkersQueue* wq = new WorkersQueue(warehouse, INVALID_INDEX, 0);
-wq->read(fr, game, mol);
+wq->read(fr, game, mol, tribes_lookup_table);
wq->set_callback(input_callback, this);
if (wq->get_index() == INVALID_INDEX) {
@@ -232,7 +232,7 @@
uint8_t num_queues = fr.unsigned_8();
for (uint8_t i = 0; i < num_queues; ++i) {
WaresQueue* wq = new WaresQueue(warehouse, INVALID_INDEX, 0);
- wq->read(fr, game, mol);
+ wq->read(fr, game, mol, tribes_lookup_table);
wq->set_callback(input_callback, this);
if (wq->get_index() == INVALID_INDEX) {
=== modified file 'src/economy/expedition_bootstrap.h'
--- src/economy/expedition_bootstrap.h 2019-02-23 11:00:49 +
+++ src/economy/expedition_bootstrap.h 2019-05-26 03:34:22 +
@@ -88,7 +88,7 @@
* packet, and there in the warehouse data packet.
*/
void
- load(Warehouse& warehouse, FileRead& fr, Game& game, MapObjectLoader& mol, uint16_t version);
+ load(Warehouse& warehouse, FileRead& fr, Game& game, MapObjectLoader& mol, const TribesLegacyLookupTable& tribes_lookup_table, uint16_t version);
/** Save this into a file.
*
=== modified file 'src/economy/input_queue.cc'
--- src/economy/input_queue.cc 2019-02-23 11:00:49 +
+++ src/economy/input_queue.cc 2019-05-26 03:34:22 +
@@ -130,7 +130,7 @@
constexpr uint16_t kCurrentPacketVersion = 3;
-void InputQueue::read(FileRead& fr, Game& game, MapObjectLoader& mol) {
+void InputQueue::read(FileRead& fr, Game& game, MapObjectLoader& mol, const TribesLegacyLookupTable& tribes_lookup_table) {
uint16_t const packet_version = fr.unsigned_16();
try {
@@ -140,25 +140,26 @@
if (packet_version == 1 || packet_version == kCurrentPacketVersion) {
if (fr.unsigned_8() == 0) {
assert(type_ == wwWARE);
-index_ = owner().tribe().ware_index(fr.c_string());
+index_ = owner().tribe().ware_index(tribes_lookup_table.lookup_ware(fr.c_string()));
} else {
assert(type_ == wwWORKER);
-index_ = owner().tribe().worker_index(fr.c_string());
+index_ = owner().tribe().worker_index(tribes_lookup_table.lookup_worker(fr.c_string()));
}
max_size_ = fr.unsigned_32();
max_fill_ = fr.signed_32();
consume_interval_ = fr.unsigned_32();
if (fr.unsigned_8()) {
request_.reset(new Request(owner_, 0, InputQueue::request_callback, type_));
-request_->read(fr, game, mol);
+request_->read(fr, game, mol, tribes_lookup_table);
} else {
request_.reset();
}
read_child(fr, game, mol);
} else if (packet_version == 2) {
+ // TODO(GunChleoc): Savegame compatibility, get rid after Build 21
assert(type_ == wwWARE);
- index_ = owner().tribe().ware_index(fr.c_string());
+ index_ = owner().tribe().ware_index(tribes_lookup_table.lookup_ware(fr.c_string()));
max_size_ = fr.unsigned_32();
max_fill_ = fr.signed_32();
// No read_child() call here, doing it manually since there is no child-version number
@@ -166,7 +167,7 @@
consume_interval_ = fr.unsigned_32();
if (fr.unsigned_8()) {
request_.reset(new Request(owner_, 0, InputQueue::request_callback, type_));
-request_->read(fr, game, mol);
+request_->read(fr, game, mol, tribes_lookup_table);
} else {
request_.reset();
}
=== modified file 'src/economy/input_queue.h'
--- src/economy/input_queue.h 2019-02-27 19:00:36 +
+++ src/economy/input_queue.h 2019-05-26 03:34:22 +
@@ -184,7 +184,7 @@
* @param game The game this queue will be part of.
* @param mol The game/map loader that handles the lading. Required to pass to Request::read().
*/
- void read(FileRead& f, Game& g, MapObjectLoader& mol);
+ void read(FileRead& f, Game& g, MapObjectLoader& mol, const TribesLegacyLookupTable& tribes_lookup_table);
/**
* Writes the st
