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

[GunChleoc]

Make dismantle button independent of buildable/enhanced. This fixes missing Dismantle buttons in Empire scenario 4. Cleaned up Buildcost code.

[Notabilis (notabilis27)]

From what I can say without knowing the scenario it works as intended, with some building gaining and some building loosing the possibility/button to dismantle them. Code is looking good to me as well.

Unrelated, but noticed this when testing: The empire "outpost enhanced to barrier" gives more materials when dismantling than a directly constructed barrier. I think this is on purpose, though.

review: Approve (diff, testing)

[hessenfarmer (stephan-lutz)]

For me this is a new feature and should therefore be postponed to B21, which would give me some time to test which i haven't currently and to properly rework the scenario to the new feature

[GunChleoc (gunchleoc)]

I'll create another branch then that only loads the backend bits (r8907), without the UI - we can expect bug reports for the log message, so I'll want that clean for Build 20.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4198. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/452676639.
Appveyor build 3994. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_empire04_unused_key_return_on_dismantle-3994.

[hessenfarmer (stephan-lutz)]

could you please merge trunk so we will get Appveyor builds to test this.

I would start with the complete thing (this branch) for testing and try the other only in case this is not delivering the correct result.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4251. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/456892857.
Appveyor build 4045. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_empire04_unused_key_return_on_dismantle-4045.

[hessenfarmer (stephan-lutz)]

Have tested this branch now and it is not working as expected (and stated in the dialogs) for the "special" forester which has been designed to be dismantable but delivering no wares. So we could either keep the text and ensure this in the logic or have this logic and change the text both we should do for b21.
So I'll disapprove this with needs fixing. and approve the other branch to get rid of the error log message.

review: Needs Fixing

[GunChleoc (gunchleoc)]

If dismantling doesn't return any wares, the player will choose to destroy the building anyway, because all that dismantling will do for the player is to take longer. So, I think we should change the text for build 21.

[hessenfarmer (stephan-lutz)]

I am ok with that.

Unfortunately I overlooked some code issues in the toher branch which Arty found. we should make sure we will fix them with this branch at the latest.

Perhaps it will be even recommendable to do it now in a fixing round

[Arty (artydent)]

I got ninja'd yesterday when I reviewed the code. :-)

As for the "needs positive return on dismantle" condition, I don't have a strong opinion, but I'd prefer to not have it, even though in most circumstances dismantling without return would be useless. My reasons:

1) The condition - even though it makes sense - doesn't really add anything. The game would work fine without it. And regular buildings (and most special builidings in campaigns) have proper return on dismantle amounts anyway, so I wouldn't expect "false" bug reports about this.

2) Not having the condition allows for a little more flavour in missions. Like in emp04. Or maybe there'll be a mission where destroying would come with a fire hazard to neighboring buildings, possibly destroying them, too. (Not sure this is possibly atm, i.e., whether we can detect the difference between dismantling and destroying via mission scripts. Just a general idea.)

3) If the building is not far from a warehouse with a builder available, dismantling without return is even a little faster than destroying. Difference is minor though.

[GunChleoc (gunchleoc)]

Arty has convinced me, so I have changed the design. Needs retesting.

review: Needs Resubmitting

[hessenfarmer (stephan-lutz)]

Have tested it. Unfortunately it doesn't work. an empty table is not triggering the dismantle button.

I just remembered myself why I had the forester dismantable with no wares. It was the only way to distinguish its building window from the building window of the farms which we need to catch for one objective.

furthermore some indents seem to be wrong in the diff.

[Arty (artydent)]

Reviewed the code and tested quite a lot. Looks pretty good, but still doesn't work quite right. Not hard to fix though.

1) Now that you changed the design to requiring the buildcost table to be empty instead of listing wares with amount 0, the dismantle button does not appear for such empty tables. This is easily fixed in ‘wui/buildingwindow.cc’ by removing a ware check, i.e., removing lines 287 and 297 (and unindenting the block inbetween). I already tested this, it works fine.

2) Having no items shown in the box when hovering over the dismantle button looks good enough for me, but I can imagine that some might find this not nice. Maybe adding a little text “nothing” or so might be good. And (at a later point) maybe even a little icon.

3) See diff comments.

review: Needs Fixing

[hessenfarmer (stephan-lutz)]

thanks Arty for your comments and solutions. I would be happy to test this again after the fixes.

Nice spot with the resulting farm1 issue after this finally works.

[hessenfarmer (stephan-lutz)]

Tested the branch. Works like it should do. no difference found in empire 04.

ALthough the message for no wares returned looks unfamiliar and is to long for my taste.

For me we should have the same headings as for wares returned. and a short message like "no wares" instead of the waremap to richtext.

resulting in:

Dismantle
Returns:
"no wares"

anyhow as this is a matter of taste I will approve this.
Question is do we want to have it for b20? Problem is that it will have at least one new string to translate.

[hessenfarmer (stephan-lutz)]

Perhaps it might be worth having it without the string . (showing nothing beneath the Headings)

review: Approve

[GunChleoc (gunchleoc)]

We already had to add some strings for other bug fixes. I think breaking the string freeze for smaller strings is OK, as long as we don't overdo it.

I have addressed all the points in the code review now. We can't check for duplicate keys in Lua tables, because the backend (Eris) or maybe Lua itself already squashes them. So, I have removed all those checks.

review: Needs Resubmitting

[hessenfarmer (stephan-lutz)]

Ok I will test this tonight and merge if ok.

in the diff here on Launchpad there are some differing indentations? are they intentional?

Anyhow I have marked them in the diff comments

[GunChleoc (gunchleoc)]

Indentations will be fixed by bunnybot. I have set my IDE back to tabs from spaces, so it will be a bit less crazy in the future.

The dismantle tooltip now only says "Dismantle" when there are no wares returned. I have tried out some alternatives and I think that looks best.

[hessenfarmer (stephan-lutz)]

Agreed. now it looks consistent with the rest of the window. Thanks for fixing.

@bunnybot merge

review: Approve (test)

=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua'
--- data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua	2018-08-22 06:48:57 +0000
+++ data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua	2018-11-23 09:22:27 +0000
@@ -14,7 +14,6 @@
       granite = 1
    },
    return_on_dismantle_on_enhanced = {
-      planks = 0,
    },
 
    animations = {

=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/farm1.lua'
--- data/campaigns/emp04.wmf/scripting/tribes/farm1.lua	2018-09-09 17:58:55 +0000
+++ data/campaigns/emp04.wmf/scripting/tribes/farm1.lua	2018-11-23 09:22:27 +0000
@@ -9,13 +9,6 @@
    size = "big",
    enhancement = "empire_farm2",
 
-   return_on_dismantle = {
-      planks = 0,
-      granite = 0,
-      marble = 0,
-      marble_column = 0
-   },
-
    animations = {
       idle = {
          pictures = path.list_files(dirname .. "idle_??.png"),

=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua'
--- data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua	2018-08-09 20:54:08 +0000
+++ data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua	2018-11-23 09:22:27 +0000
@@ -15,7 +15,6 @@
    },
 
    return_on_dismantle = {
-      planks = 0,
    },
 
    animations = {

=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/mill2.lua'
--- data/campaigns/emp04.wmf/scripting/tribes/mill2.lua	2018-08-22 06:48:57 +0000
+++ data/campaigns/emp04.wmf/scripting/tribes/mill2.lua	2018-11-23 09:22:27 +0000
@@ -15,9 +15,6 @@
       marble = 1
    },
    return_on_dismantle_on_enhanced = {
-      log = 0,
-      granite = 0,
-      marble = 0
    },
 
    animations = {

=== modified file 'src/logic/map_objects/buildcost.cc'
--- src/logic/map_objects/buildcost.cc	2018-04-07 16:59:00 +0000
+++ src/logic/map_objects/buildcost.cc	2018-11-23 09:22:27 +0000
@@ -23,8 +23,6 @@
 #include <memory>
 
 #include "base/wexception.h"
-#include "io/fileread.h"
-#include "io/filewrite.h"
 #include "logic/game_data_error.h"
 #include "logic/map_objects/tribes/tribe_descr.h"
 #include "logic/map_objects/tribes/tribes.h"
@@ -37,32 +35,32 @@
 Buildcost::Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes)
    : std::map<DescriptionIndex, uint8_t>() {
 	for (const std::string& warename : table->keys<std::string>()) {
-		int32_t value = INVALID_INDEX;
-		try {
-			DescriptionIndex const idx = tribes.safe_ware_index(warename);
-			if (count(idx)) {
-				throw GameDataError(
-				   "A buildcost item of this ware type has already been defined: %s", warename.c_str());
-			}
-			value = table->get_int(warename);
-			const uint8_t ware_count = value;
-			if (ware_count != value) {
-				throw GameDataError("Ware count is out of range 1 .. 255");
-			}
-			insert(std::pair<DescriptionIndex, uint8_t>(idx, ware_count));
-		} catch (const WException& e) {
-			throw GameDataError("[buildcost] \"%s=%d\": %s", warename.c_str(), value, e.what());
-		}
+		// Check ware name
+		if (!tribes.ware_exists(warename)) {
+			throw GameDataError("Buildcost: Unknown ware: %s", warename.c_str());
+		}
+
+		// Read value
+		const int32_t value = table->get_int(warename);
+		if (value < 1) {
+			throw GameDataError("Buildcost: Ware count needs to be > 0 in \"%s=%d\".\nEmpty buildcost tables are allowed if you wish to have an amount of 0.", warename.c_str(), value);
+		} else if (value > 255) {
+			throw GameDataError("Buildcost: Ware count needs to be <= 255 in \"%s=%d\".", warename.c_str(), value);
+		}
+
+		// Add
+		insert(std::pair<DescriptionIndex, uint8_t>(tribes.safe_ware_index(warename), value));
 	}
 }
 
 /**
  * Compute the total buildcost.
  */
-uint32_t Buildcost::total() const {
-	uint32_t sum = 0;
-	for (const_iterator it = begin(); it != end(); ++it)
+Widelands::Quantity Buildcost::total() const {
+	Widelands::Quantity sum = 0;
+	for (const_iterator it = begin(); it != end(); ++it) {
 		sum += it->second;
+	}
 	return sum;
 }
 
@@ -79,8 +77,9 @@
 
 	for (;;) {
 		std::string name = fr.c_string();
-		if (name.empty())
+		if (name.empty()) {
 			break;
+		}
 
 		DescriptionIndex index = tribe.ware_index(name);
 		if (!tribe.has_ware(index)) {

=== modified file 'src/logic/map_objects/buildcost.h'
--- src/logic/map_objects/buildcost.h	2018-04-07 16:59:00 +0000
+++ src/logic/map_objects/buildcost.h	2018-11-23 09:22:27 +0000
@@ -23,12 +23,11 @@
 #include <map>
 #include <memory>
 
+#include "io/fileread.h"
+#include "io/filewrite.h"
 #include "logic/widelands.h"
 #include "scripting/lua_table.h"
 
-class FileRead;
-class FileWrite;
-
 namespace Widelands {
 
 class TribeDescr;
@@ -39,7 +38,7 @@
 	Buildcost();
 	Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes);
 
-	uint32_t total() const;
+	Quantity total() const;
 
 	void save(FileWrite& fw, const TribeDescr& tribe) const;
 	void load(FileRead& fw, const TribeDescr& tribe);

=== modified file 'src/logic/map_objects/tribes/building.cc'
--- src/logic/map_objects/tribes/building.cc	2018-11-19 21:38:03 +0000
+++ src/logic/map_objects/tribes/building.cc	2018-11-23 09:22:27 +0000
@@ -59,7 +59,9 @@
                              const EditorGameBase& egbase)
    : MapObjectDescr(init_type, table.get_string("name"), init_descname, table),
      egbase_(egbase),
-     buildable_(false),
+     buildable_(table.has_key("buildcost")),
+     can_be_dismantled_(table.has_key("return_on_dismantle") || table.has_key("return_on_dismantle_on_enhanced")),
+	 destructible_(table.has_key("destructible") ? table.get_bool("destructible") : true),
      size_(BaseImmovable::SMALL),
      mine_(false),
      port_(false),
@@ -106,8 +108,6 @@
 	}
 
 	// Parse build options
-	destructible_ = table.has_key("destructible") ? table.get_bool("destructible") : true;
-
 	if (table.has_key("enhancement")) {
 		const std::string enh = table.get_string("enhancement");
 
@@ -141,7 +141,6 @@
 		return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
 	}
 	if (table.has_key("buildcost")) {
-		buildable_ = true;
 		if (!table.has_key("return_on_dismantle")) {
 			throw wexception(
 			   "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"", name().c_str());
@@ -151,15 +150,13 @@
 
 	if (table.has_key("enhancement_cost")) {
 		enhanced_building_ = true;
-		try {
-			enhance_cost_ = Buildcost(table.get_table("enhancement_cost"), egbase_.tribes());
-			return_enhanced_ =
-			   Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
-		} catch (const WException& e) {
-			throw wexception("The enhanced building '%s' must define \"enhancement_cost\""
-			                 "and \"return_on_dismantle_on_enhanced\": %s",
-			                 name().c_str(), e.what());
+		if (!table.has_key("return_on_dismantle_on_enhanced")) {
+			throw wexception("The enhanced building '%s' has an \"enhancement_cost\" but no \"return_on_dismantle_on_enhanced\"",
+			                 name().c_str());
 		}
+		enhance_cost_ = Buildcost(table.get_table("enhancement_cost"), egbase_.tribes());
+		return_enhanced_ =
+		   Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
 	}
 
 	needs_seafaring_ = table.has_key("needs_seafaring") ? table.get_bool("needs_seafaring") : false;
@@ -315,11 +312,13 @@
 	const BuildingDescr& tmp_descr = descr();
 	if (tmp_descr.is_destructible()) {
 		caps |= PCap_Bulldoze;
-		if (tmp_descr.is_buildable() || tmp_descr.is_enhanced())
+		if (tmp_descr.can_be_dismantled()) {
 			caps |= PCap_Dismantle;
+		}
 	}
-	if (tmp_descr.enhancement() != INVALID_INDEX)
+	if (tmp_descr.enhancement() != INVALID_INDEX) {
 		caps |= PCap_Enhancable;
+	}
 	return caps;
 }
 

=== modified file 'src/logic/map_objects/tribes/building.h'
--- src/logic/map_objects/tribes/building.h	2018-09-25 06:32:35 +0000
+++ src/logic/map_objects/tribes/building.h	2018-11-23 09:22:27 +0000
@@ -73,6 +73,9 @@
 	bool is_buildable() const {
 		return buildable_;
 	}
+    bool can_be_dismantled() const {
+		return can_be_dismantled_;
+	}
 	bool is_destructible() const {
 		return destructible_;
 	}
@@ -172,8 +175,9 @@
 	const EditorGameBase& egbase_;
 
 private:
-	bool buildable_;     // the player can build this himself
-	bool destructible_;  // the player can destruct this himself
+	const bool buildable_;     // the player can build this himself
+    const bool can_be_dismantled_; // the player can dismantle this building
+	const bool destructible_;  // the player can destruct this himself
 	Buildcost buildcost_;
 	Buildcost return_dismantle_;  // Returned wares on dismantle
 	Buildcost enhance_cost_;      // cost for enhancing

=== modified file 'src/logic/map_objects/tribes/worker_descr.cc'
--- src/logic/map_objects/tribes/worker_descr.cc	2018-09-03 17:53:31 +0000
+++ src/logic/map_objects/tribes/worker_descr.cc	2018-11-23 09:22:27 +0000
@@ -68,21 +68,20 @@
 		items_table = table.get_table("buildcost");
 		for (const std::string& key : items_table->keys<std::string>()) {
 			try {
-				if (buildcost_.count(key)) {
-					throw GameDataError(
-					   "a buildcost item of this ware type has already been defined: %s", key.c_str());
-				}
 				if (!tribes.ware_exists(tribes.ware_index(key)) &&
 				    !tribes.worker_exists(tribes.worker_index(key))) {
 					throw GameDataError("\"%s\" has not been defined as a ware/worker type (wrong "
 					                    "declaration order?)",
 					                    key.c_str());
 				}
-				int32_t value = items_table->get_int(key);
-				uint8_t const count = value;
-				if (count != value)
-					throw GameDataError("count is out of range 1 .. 255");
-				buildcost_.insert(std::pair<std::string, uint8_t>(key, count));
+				const int32_t value = items_table->get_int(key);
+				if (value < 1) {
+					throw GameDataError("Buildcost: Ware/Worker count needs to be > 0 in \"%s=%d\".\nEmpty buildcost tables are allowed if you wish to have an amount of 0.", key.c_str(), value);
+				} else if (value > 255) {
+					throw GameDataError("Buildcost: Ware/Worker count needs to be <= 255 in \"%s=%d\".", key.c_str(), value);
+				}
+
+				buildcost_.insert(std::pair<std::string, uint8_t>(key, value));
 			} catch (const WException& e) {
 				throw GameDataError("[buildcost] \"%s\": %s", key.c_str(), e.what());
 			}

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2018-11-11 20:42:54 +0000
+++ src/wlapplication.cc	2018-11-23 09:22:27 +0000
@@ -1064,7 +1064,7 @@
 			log("\n%s\n%s\n", messagetitle.c_str(), message.c_str());
 
 			UI::WLMessageBox mmb(
-			   &mm, messagetitle, message, UI::WLMessageBox::MBoxType::kOk, UI::Align::kLeft);
+			   &mm, messagetitle, richtext_escape(message), UI::WLMessageBox::MBoxType::kOk, UI::Align::kLeft);
 			mmb.run<UI::Panel::Returncodes>();
 
 			message.clear();

=== modified file 'src/wui/buildingwindow.cc'
--- src/wui/buildingwindow.cc	2018-09-04 15:48:47 +0000
+++ src/wui/buildingwindow.cc	2018-11-23 09:22:27 +0000
@@ -220,7 +220,7 @@
 					      if (canceled.bootstrap == pd->expedition_bootstrap()) {
 						      update_expedition_button(true);
 					      }
-					   });
+				      });
 			}
 		} else if (upcast(const Widelands::ProductionSite, productionsite, building)) {
 			if (!is_a(Widelands::MilitarySite, productionsite)) {
@@ -282,14 +282,17 @@
 		}
 
 		if (capscache_ & Widelands::Building::PCap_Dismantle) {
-			const Widelands::Buildcost wares =
-			   Widelands::DismantleSite::count_returned_wares(building);
-			if (!wares.empty()) {
+			if (building->descr().can_be_dismantled()) {
+				const Widelands::Buildcost wares =
+				   Widelands::DismantleSite::count_returned_wares(building);
+
+				const std::string dismantle_text =
+				   (wares.empty() ? _("Dismantle") : std::string(_("Dismantle")) + "<br><font size=11>" + _("Returns:") + "</font><br>" +
+				   waremap_to_richtext(owner.tribe(), wares));
+
 				UI::Button* dismantlebtn =
 				   new UI::Button(capsbuttons, "dismantle", 0, 0, 34, 34, UI::ButtonStyle::kWuiMenu,
-				                  g_gr->images().get(pic_dismantle),
-				                  std::string(_("Dismantle")) + "<br><font size=11>" + _("Returns:") +
-				                     "</font><br>" + waremap_to_richtext(owner.tribe(), wares));
+				                  g_gr->images().get(pic_dismantle), dismantle_text);
 				dismantlebtn->sigclicked.connect(
 				   boost::bind(&BuildingWindow::act_dismantle, boost::ref(*this)));
 				capsbuttons->add(dismantlebtn);