Merge lp:~widelands-dev/widelands/bug-1525706-artifacts into lp:widelands

[GunChleoc]

Fixed checking for artifacts on a map.

The old function was semantic nonsense...

[kaputtnik (franku)]

Seems to work properly now :-)

I got one crash after several load/save actions in the editor, but i don't know if this is related to this branch or in general to bug 1543944. The crash happens after loading a map and the output says:

WidelandsMapLoader::load_map_complete() took 356ms
widelands: /home/kaputtnik/Quellcode/widelands-repo/bug-1525706-artifacts/src/logic/map.h:510: static Widelands::MapIndex Widelands::Map::get_index(const Widelands::Coords&, int16_t): Assertion `c.x < width' failed.
Abgebrochen (Speicherabzug geschrieben)

It would be nice if MapLoader and MapSaver also writes the name of the map to the output. It is hard to remember which map was saved/loaded when one often loads and saves much maps.

[GunChleoc (gunchleoc)]

This changed function is only called during saving, and it only adds a tag or not, so it's not related to the crash. Good idea of adding the map name to the output though; I will look into it.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 656. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/108226507.
Appveyor build 512. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-512.

[kaputtnik (franku)]

> Good idea of adding the map name to the output though; I will look into it.

If possible please also add the current widelands version to the output. I am always searching for it and have to start widelands (or investigate the VERSION file) to get it.

[SirVer (sirver)]

I suggest to simplify the code.

review: Needs Fixing

[kaputtnik (franku)]

Another crash in menu load map when trying to select a map. First crash was when pointing on map "Ice wars", second time when trying select map "Sun of Fire":

<error: Cannot access memory at address 0x7473656767755300>

backtrace from first crash attached to bug report.

[kaputtnik (franku)]

Oh i see now that this crash happens also in trunk r7816.

[GunChleoc (gunchleoc)]

As I said before, there are no possible map loading crashes related to this branch. This code is only executed while saving a map, and then the only thing it does is write the "artifacts" tag.

I tried and failed to just iterate over the fields - seems like there is no way to safely get the size of a std::unique_ptr<array[]>.

Why is it an array and not a vector anyway?

[kaputtnik (franku)]

I have lost overview :-S "Lost in bugs" :-D Sorry

[GunChleoc (gunchleoc)]

As long as I am not lost in translation, which would be bad for my job... :P

[kaputtnik (franku)]

Merged with trunk an looks good.

review: Approve (testing)

[GunChleoc (gunchleoc)]

I still need help with this before we can go ahead:

> I tried and failed to just iterate over the fields - seems like there is no
> way to safely get the size of a std::unique_ptr<array[]>.
>
> Why is it an array and not a vector anyway?

[GunChleoc (gunchleoc)]

I have found a safe upper limit for iterating over the fields now, so I'll be pushing something new as soon as I have compiled & tested it.

[SirVer (sirver)]

Sorry, cannot keep up with email....

I think this can be a vector and that would probably be better.

> Am 16.02.2016 um 09:48 schrieb GunChleoc <email address hidden>:
>
> I still need help with this before we can go ahead:
>
>> I tried and failed to just iterate over the fields - seems like there is no
>> way to safely get the size of a std::unique_ptr<array[]>.
>>
>> Why is it an array and not a vector anyway?
> --
> https://code.launchpad.net/~widelands-dev/widelands/bug-1525706-artifacts/+merge/285566
> Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1525706-artifacts.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~widelands-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~widelands-dev
> More help : https://help.launchpad.net/ListHelp

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

Permission denied (publickey).
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
Permission denied (publickey).
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1525706-artifacts/

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 734. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/110444117.
Appveyor build 581. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-581.

[kaputtnik (franku)]

Works :-)

review: Approve (testing)

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

ssh_exchange_identification: Connection closed by remote host
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
ssh_exchange_identification: Connection closed by remote host
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1525706-artifacts/

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

ssh_exchange_identification: Connection closed by remote host
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
ssh_exchange_identification: Connection closed by remote host
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1525706-artifacts/

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 734. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/110444117.
Appveyor build 581. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-581.

[GunChleoc (gunchleoc)]

Changing this to a vector is complicated, so I'd like to get this branch in and leave that as a problem for another day.

> Sorry, cannot keep up with email....
>
> I think this can be a vector and that would probably be better.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 768. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/111303303.
Appveyor build 614. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-614.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 768. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/111303303.
Appveyor build 614. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-614.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 769. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/111429590.
Appveyor build 615. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-615.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 787. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/112800339.
Appveyor build 633. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-633.

[TiborB (tiborb95)]

Code looks good

review: Approve

[GunChleoc (gunchleoc)]

Thanks!

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 802. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/113182182.
Appveyor build 648. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1525706_artifacts-648.

=== modified file 'src/editor/ui_menus/editor_main_menu_save_map.cc'
--- src/editor/ui_menus/editor_main_menu_save_map.cc	2016-02-22 10:23:14 +0000
+++ src/editor/ui_menus/editor_main_menu_save_map.cc	2016-03-02 17:09:33 +0000
@@ -271,7 +271,7 @@
 			map.delete_tag("seafaring");
 		}
 
-		if (map.has_artifacts(egbase.world())) {
+		if (map.has_artifacts()) {
 			map.add_tag("artifacts");
 		} else {
 			map.delete_tag("artifacts");

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2016-03-02 10:04:54 +0000
+++ src/logic/map.cc	2016-03-02 17:09:33 +0000
@@ -2129,11 +2129,12 @@
 	return false;
 }
 
-bool Map::has_artifacts(const World& world) {
-	for (int32_t i = 0; i < world.get_nr_immovables(); ++i) {
-		const ImmovableDescr& descr = *world.get_immovable_descr(i);
-		if (descr.has_attribute(descr.get_attribute_id("artifact"))) {
-			return true;
+bool Map::has_artifacts() {
+	for (MapIndex i = 0; i < max_index(); ++i) {
+		if (upcast(Immovable, immovable, fields_[i].get_immovable())) {
+			if (immovable->descr().has_attribute(immovable->descr().get_attribute_id("artifact"))) {
+				return true;
+			}
 		}
 	}
 	return false;

=== modified file 'src/logic/map.h'
--- src/logic/map.h	2016-02-16 13:15:29 +0000
+++ src/logic/map.h	2016-03-02 17:09:33 +0000
@@ -433,7 +433,9 @@
 	const PortSpacesSet& get_port_spaces() const {return port_spaces_;}
 	std::vector<Coords> find_portdock(const Widelands::Coords& c) const;
 	bool allows_seafaring();
-	bool has_artifacts(const World& world);
+
+	/// Checks whether there are any artifacts on the map
+	bool has_artifacts();
 
 protected: /// These functions are needed in Testclasses
 	void set_size(uint32_t w, uint32_t h);