Merge lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui into lp:widelands

[GunChleoc]

Allow return on dismantle values without buildcost.

UI fix has been postponed until after Build 20, c.f.

https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273

[hessenfarmer (stephan-lutz)]

I am not sure whether this fix or the other branch has any influence on the Scenario, and I can't test this at least until next monday.
In the current implementation I managed to have the buildings in the right state some should be dismantable, some not. Some should be enhanceable, some not or only after a quest has been fulfillled. This state of the Scenario most probably is affected by these changes. If so the text is not matching the gameplay properly, so I recommend to postpone the whole thing to b21.

If you wan't to have it anyway it is your decision. I will have a look as soon as I have time for this.

[GunChleoc (gunchleoc)]

There is no dismantle button in this version, just like in trunk. I have checked.

We can wait with merging until you have a little time - there are still other bug to fix for Build 20.

[hessenfarmer (stephan-lutz)]

Ok I will try to find some time next week.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4196. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/452675533.
Appveyor build 3992. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_empire04_unused_key_return_on_dismantle_no_ui-3992.

[GunChleoc (gunchleoc)]

AppVeyor build is ready: https://ci.appveyor.com/project/widelands-dev/widelands/builds/20346183

[hessenfarmer (stephan-lutz)]

ok tested this branch. No changes to the scenario were seen. Error log messages were gone.

review: Approve (test review)

[hessenfarmer (stephan-lutz)]

@bunnybot merge

[Arty (artydent)]

Code looks good in principle, but haven't tested yet whether it really works as intended.

Two minor nits:
1) see diff comment
2) All the new lines (except when they were copied over) are indented with spaces instead of tabs.

[GunChleoc (gunchleoc)]

Thanks for the review - I have addressed your comment in the other branch

https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273

=== modified file 'src/logic/map_objects/tribes/building.cc'
--- src/logic/map_objects/tribes/building.cc	2018-09-23 11:10:56 +0000
+++ src/logic/map_objects/tribes/building.cc	2018-11-16 06:07:58 +0000
@@ -133,17 +133,22 @@
 		}
 	}
 
+    // We define a building as buildable if it has a "buildcost" table.
+    // A buildable building must also define "return_on_dismantle".
+    // However, we support "return_on_dismantle" without "buildable", because this is used by custom scenario buildings.
+    if (table.has_key("return_on_dismantle")) {
+        return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
+    }
 	if (table.has_key("buildcost")) {
 		buildable_ = true;
-		try {
-			buildcost_ = Buildcost(table.get_table("buildcost"), egbase_.tribes());
-			return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
-		} catch (const WException& e) {
-			throw wexception(
-			   "A buildable building must define \"buildcost\" and \"return_on_dismantle\": %s",
-			   e.what());
-		}
+        if (!table.has_key("return_on_dismantle")) {
+            throw wexception(
+			   "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"",
+			   name().c_str());
+        }
+		buildcost_ = Buildcost(table.get_table("buildcost"), egbase_.tribes());
 	}
+
 	if (table.has_key("enhancement_cost")) {
 		enhanced_building_ = true;
 		try {
@@ -151,9 +156,9 @@
 			return_enhanced_ =
 			   Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
 		} catch (const WException& e) {
-			throw wexception("An enhanced building must define \"enhancement_cost\""
+			throw wexception("The enhanced building '%s' must define \"enhancement_cost\""
 			                 "and \"return_on_dismantle_on_enhanced\": %s",
-			                 e.what());
+			                 name().c_str(), e.what());
 		}
 	}