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

Proposed by GunChleoc
Status: Merged
Merged at revision: 8932
Proposed branch: lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle
Merge into: lp:widelands
Diff against target: 339 lines (+62/-71)
11 files modified
data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua (+0/-1)
data/campaigns/emp04.wmf/scripting/tribes/farm1.lua (+0/-7)
data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua (+0/-1)
data/campaigns/emp04.wmf/scripting/tribes/mill2.lua (+0/-3)
src/logic/map_objects/buildcost.cc (+21/-22)
src/logic/map_objects/buildcost.h (+3/-4)
src/logic/map_objects/tribes/building.cc (+13/-14)
src/logic/map_objects/tribes/building.h (+6/-2)
src/logic/map_objects/tribes/worker_descr.cc (+8/-9)
src/wlapplication.cc (+1/-1)
src/wui/buildingwindow.cc (+10/-7)
To merge this branch: bzr merge lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle
Reviewer Review Type Date Requested Status
hessenfarmer test Approve
GunChleoc Needs Resubmitting
Arty Needs Fixing
Notabilis diff, testing Approve
Review via email: mp+358273@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Notabilis (notabilis27) wrote :

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)
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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.

8909. By hessenfarmer

merged trunk

8910. By GunChleoc

Merged trunk.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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
Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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

Revision history for this message
Arty (artydent) wrote :

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.

8911. By GunChleoc

Merged trunk.

8912. By GunChleoc

Improved exception handling

8913. By GunChleoc

Make "can be dismantled" independent of amount of returned wares.

8914. By GunChleoc

Clean up Buildcost code.

8915. By GunChleoc

Remove buildcost entries with amount 0, leaving empty tables.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

review: Needs Resubmitting
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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.

Revision history for this message
Arty (artydent) wrote :

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
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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.

8916. By GunChleoc

Made buildable_, can_be_dismantled_ and destructible_ const

8917. By GunChleoc

Fix crash in richtext renderer

8918. By GunChleoc

Show dismantle button if building can be dismantled but returned wares are empty

8919. By GunChleoc

Merged trunk.

8920. By GunChleoc

Fix string concatenation

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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

review: Approve
8921. By GunChleoc

Remove checks for duplicate keys in lua tables, because the backend already squashes the keys and we can't check this.

8922. By GunChleoc

Improved text for dismantling with no wares

Revision history for this message
GunChleoc (gunchleoc) wrote :

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
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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

8923. By GunChleoc

Improved tooltip if dismantle returns nothing

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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

@bunnybot merge

review: Approve (test)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua'
2--- data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua 2018-08-22 06:48:57 +0000
3+++ data/campaigns/emp04.wmf/scripting/tribes/brewery2.lua 2018-11-23 09:22:27 +0000
4@@ -14,7 +14,6 @@
5 granite = 1
6 },
7 return_on_dismantle_on_enhanced = {
8- planks = 0,
9 },
10
11 animations = {
12
13=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/farm1.lua'
14--- data/campaigns/emp04.wmf/scripting/tribes/farm1.lua 2018-09-09 17:58:55 +0000
15+++ data/campaigns/emp04.wmf/scripting/tribes/farm1.lua 2018-11-23 09:22:27 +0000
16@@ -9,13 +9,6 @@
17 size = "big",
18 enhancement = "empire_farm2",
19
20- return_on_dismantle = {
21- planks = 0,
22- granite = 0,
23- marble = 0,
24- marble_column = 0
25- },
26-
27 animations = {
28 idle = {
29 pictures = path.list_files(dirname .. "idle_??.png"),
30
31=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua'
32--- data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua 2018-08-09 20:54:08 +0000
33+++ data/campaigns/emp04.wmf/scripting/tribes/foresters_house1.lua 2018-11-23 09:22:27 +0000
34@@ -15,7 +15,6 @@
35 },
36
37 return_on_dismantle = {
38- planks = 0,
39 },
40
41 animations = {
42
43=== modified file 'data/campaigns/emp04.wmf/scripting/tribes/mill2.lua'
44--- data/campaigns/emp04.wmf/scripting/tribes/mill2.lua 2018-08-22 06:48:57 +0000
45+++ data/campaigns/emp04.wmf/scripting/tribes/mill2.lua 2018-11-23 09:22:27 +0000
46@@ -15,9 +15,6 @@
47 marble = 1
48 },
49 return_on_dismantle_on_enhanced = {
50- log = 0,
51- granite = 0,
52- marble = 0
53 },
54
55 animations = {
56
57=== modified file 'src/logic/map_objects/buildcost.cc'
58--- src/logic/map_objects/buildcost.cc 2018-04-07 16:59:00 +0000
59+++ src/logic/map_objects/buildcost.cc 2018-11-23 09:22:27 +0000
60@@ -23,8 +23,6 @@
61 #include <memory>
62
63 #include "base/wexception.h"
64-#include "io/fileread.h"
65-#include "io/filewrite.h"
66 #include "logic/game_data_error.h"
67 #include "logic/map_objects/tribes/tribe_descr.h"
68 #include "logic/map_objects/tribes/tribes.h"
69@@ -37,32 +35,32 @@
70 Buildcost::Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes)
71 : std::map<DescriptionIndex, uint8_t>() {
72 for (const std::string& warename : table->keys<std::string>()) {
73- int32_t value = INVALID_INDEX;
74- try {
75- DescriptionIndex const idx = tribes.safe_ware_index(warename);
76- if (count(idx)) {
77- throw GameDataError(
78- "A buildcost item of this ware type has already been defined: %s", warename.c_str());
79- }
80- value = table->get_int(warename);
81- const uint8_t ware_count = value;
82- if (ware_count != value) {
83- throw GameDataError("Ware count is out of range 1 .. 255");
84- }
85- insert(std::pair<DescriptionIndex, uint8_t>(idx, ware_count));
86- } catch (const WException& e) {
87- throw GameDataError("[buildcost] \"%s=%d\": %s", warename.c_str(), value, e.what());
88- }
89+ // Check ware name
90+ if (!tribes.ware_exists(warename)) {
91+ throw GameDataError("Buildcost: Unknown ware: %s", warename.c_str());
92+ }
93+
94+ // Read value
95+ const int32_t value = table->get_int(warename);
96+ if (value < 1) {
97+ 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);
98+ } else if (value > 255) {
99+ throw GameDataError("Buildcost: Ware count needs to be <= 255 in \"%s=%d\".", warename.c_str(), value);
100+ }
101+
102+ // Add
103+ insert(std::pair<DescriptionIndex, uint8_t>(tribes.safe_ware_index(warename), value));
104 }
105 }
106
107 /**
108 * Compute the total buildcost.
109 */
110-uint32_t Buildcost::total() const {
111- uint32_t sum = 0;
112- for (const_iterator it = begin(); it != end(); ++it)
113+Widelands::Quantity Buildcost::total() const {
114+ Widelands::Quantity sum = 0;
115+ for (const_iterator it = begin(); it != end(); ++it) {
116 sum += it->second;
117+ }
118 return sum;
119 }
120
121@@ -79,8 +77,9 @@
122
123 for (;;) {
124 std::string name = fr.c_string();
125- if (name.empty())
126+ if (name.empty()) {
127 break;
128+ }
129
130 DescriptionIndex index = tribe.ware_index(name);
131 if (!tribe.has_ware(index)) {
132
133=== modified file 'src/logic/map_objects/buildcost.h'
134--- src/logic/map_objects/buildcost.h 2018-04-07 16:59:00 +0000
135+++ src/logic/map_objects/buildcost.h 2018-11-23 09:22:27 +0000
136@@ -23,12 +23,11 @@
137 #include <map>
138 #include <memory>
139
140+#include "io/fileread.h"
141+#include "io/filewrite.h"
142 #include "logic/widelands.h"
143 #include "scripting/lua_table.h"
144
145-class FileRead;
146-class FileWrite;
147-
148 namespace Widelands {
149
150 class TribeDescr;
151@@ -39,7 +38,7 @@
152 Buildcost();
153 Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes);
154
155- uint32_t total() const;
156+ Quantity total() const;
157
158 void save(FileWrite& fw, const TribeDescr& tribe) const;
159 void load(FileRead& fw, const TribeDescr& tribe);
160
161=== modified file 'src/logic/map_objects/tribes/building.cc'
162--- src/logic/map_objects/tribes/building.cc 2018-11-19 21:38:03 +0000
163+++ src/logic/map_objects/tribes/building.cc 2018-11-23 09:22:27 +0000
164@@ -59,7 +59,9 @@
165 const EditorGameBase& egbase)
166 : MapObjectDescr(init_type, table.get_string("name"), init_descname, table),
167 egbase_(egbase),
168- buildable_(false),
169+ buildable_(table.has_key("buildcost")),
170+ can_be_dismantled_(table.has_key("return_on_dismantle") || table.has_key("return_on_dismantle_on_enhanced")),
171+ destructible_(table.has_key("destructible") ? table.get_bool("destructible") : true),
172 size_(BaseImmovable::SMALL),
173 mine_(false),
174 port_(false),
175@@ -106,8 +108,6 @@
176 }
177
178 // Parse build options
179- destructible_ = table.has_key("destructible") ? table.get_bool("destructible") : true;
180-
181 if (table.has_key("enhancement")) {
182 const std::string enh = table.get_string("enhancement");
183
184@@ -141,7 +141,6 @@
185 return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
186 }
187 if (table.has_key("buildcost")) {
188- buildable_ = true;
189 if (!table.has_key("return_on_dismantle")) {
190 throw wexception(
191 "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"", name().c_str());
192@@ -151,15 +150,13 @@
193
194 if (table.has_key("enhancement_cost")) {
195 enhanced_building_ = true;
196- try {
197- enhance_cost_ = Buildcost(table.get_table("enhancement_cost"), egbase_.tribes());
198- return_enhanced_ =
199- Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
200- } catch (const WException& e) {
201- throw wexception("The enhanced building '%s' must define \"enhancement_cost\""
202- "and \"return_on_dismantle_on_enhanced\": %s",
203- name().c_str(), e.what());
204+ if (!table.has_key("return_on_dismantle_on_enhanced")) {
205+ throw wexception("The enhanced building '%s' has an \"enhancement_cost\" but no \"return_on_dismantle_on_enhanced\"",
206+ name().c_str());
207 }
208+ enhance_cost_ = Buildcost(table.get_table("enhancement_cost"), egbase_.tribes());
209+ return_enhanced_ =
210+ Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
211 }
212
213 needs_seafaring_ = table.has_key("needs_seafaring") ? table.get_bool("needs_seafaring") : false;
214@@ -315,11 +312,13 @@
215 const BuildingDescr& tmp_descr = descr();
216 if (tmp_descr.is_destructible()) {
217 caps |= PCap_Bulldoze;
218- if (tmp_descr.is_buildable() || tmp_descr.is_enhanced())
219+ if (tmp_descr.can_be_dismantled()) {
220 caps |= PCap_Dismantle;
221+ }
222 }
223- if (tmp_descr.enhancement() != INVALID_INDEX)
224+ if (tmp_descr.enhancement() != INVALID_INDEX) {
225 caps |= PCap_Enhancable;
226+ }
227 return caps;
228 }
229
230
231=== modified file 'src/logic/map_objects/tribes/building.h'
232--- src/logic/map_objects/tribes/building.h 2018-09-25 06:32:35 +0000
233+++ src/logic/map_objects/tribes/building.h 2018-11-23 09:22:27 +0000
234@@ -73,6 +73,9 @@
235 bool is_buildable() const {
236 return buildable_;
237 }
238+ bool can_be_dismantled() const {
239+ return can_be_dismantled_;
240+ }
241 bool is_destructible() const {
242 return destructible_;
243 }
244@@ -172,8 +175,9 @@
245 const EditorGameBase& egbase_;
246
247 private:
248- bool buildable_; // the player can build this himself
249- bool destructible_; // the player can destruct this himself
250+ const bool buildable_; // the player can build this himself
251+ const bool can_be_dismantled_; // the player can dismantle this building
252+ const bool destructible_; // the player can destruct this himself
253 Buildcost buildcost_;
254 Buildcost return_dismantle_; // Returned wares on dismantle
255 Buildcost enhance_cost_; // cost for enhancing
256
257=== modified file 'src/logic/map_objects/tribes/worker_descr.cc'
258--- src/logic/map_objects/tribes/worker_descr.cc 2018-09-03 17:53:31 +0000
259+++ src/logic/map_objects/tribes/worker_descr.cc 2018-11-23 09:22:27 +0000
260@@ -68,21 +68,20 @@
261 items_table = table.get_table("buildcost");
262 for (const std::string& key : items_table->keys<std::string>()) {
263 try {
264- if (buildcost_.count(key)) {
265- throw GameDataError(
266- "a buildcost item of this ware type has already been defined: %s", key.c_str());
267- }
268 if (!tribes.ware_exists(tribes.ware_index(key)) &&
269 !tribes.worker_exists(tribes.worker_index(key))) {
270 throw GameDataError("\"%s\" has not been defined as a ware/worker type (wrong "
271 "declaration order?)",
272 key.c_str());
273 }
274- int32_t value = items_table->get_int(key);
275- uint8_t const count = value;
276- if (count != value)
277- throw GameDataError("count is out of range 1 .. 255");
278- buildcost_.insert(std::pair<std::string, uint8_t>(key, count));
279+ const int32_t value = items_table->get_int(key);
280+ if (value < 1) {
281+ 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);
282+ } else if (value > 255) {
283+ throw GameDataError("Buildcost: Ware/Worker count needs to be <= 255 in \"%s=%d\".", key.c_str(), value);
284+ }
285+
286+ buildcost_.insert(std::pair<std::string, uint8_t>(key, value));
287 } catch (const WException& e) {
288 throw GameDataError("[buildcost] \"%s\": %s", key.c_str(), e.what());
289 }
290
291=== modified file 'src/wlapplication.cc'
292--- src/wlapplication.cc 2018-11-11 20:42:54 +0000
293+++ src/wlapplication.cc 2018-11-23 09:22:27 +0000
294@@ -1064,7 +1064,7 @@
295 log("\n%s\n%s\n", messagetitle.c_str(), message.c_str());
296
297 UI::WLMessageBox mmb(
298- &mm, messagetitle, message, UI::WLMessageBox::MBoxType::kOk, UI::Align::kLeft);
299+ &mm, messagetitle, richtext_escape(message), UI::WLMessageBox::MBoxType::kOk, UI::Align::kLeft);
300 mmb.run<UI::Panel::Returncodes>();
301
302 message.clear();
303
304=== modified file 'src/wui/buildingwindow.cc'
305--- src/wui/buildingwindow.cc 2018-09-04 15:48:47 +0000
306+++ src/wui/buildingwindow.cc 2018-11-23 09:22:27 +0000
307@@ -220,7 +220,7 @@
308 if (canceled.bootstrap == pd->expedition_bootstrap()) {
309 update_expedition_button(true);
310 }
311- });
312+ });
313 }
314 } else if (upcast(const Widelands::ProductionSite, productionsite, building)) {
315 if (!is_a(Widelands::MilitarySite, productionsite)) {
316@@ -282,14 +282,17 @@
317 }
318
319 if (capscache_ & Widelands::Building::PCap_Dismantle) {
320- const Widelands::Buildcost wares =
321- Widelands::DismantleSite::count_returned_wares(building);
322- if (!wares.empty()) {
323+ if (building->descr().can_be_dismantled()) {
324+ const Widelands::Buildcost wares =
325+ Widelands::DismantleSite::count_returned_wares(building);
326+
327+ const std::string dismantle_text =
328+ (wares.empty() ? _("Dismantle") : std::string(_("Dismantle")) + "<br><font size=11>" + _("Returns:") + "</font><br>" +
329+ waremap_to_richtext(owner.tribe(), wares));
330+
331 UI::Button* dismantlebtn =
332 new UI::Button(capsbuttons, "dismantle", 0, 0, 34, 34, UI::ButtonStyle::kWuiMenu,
333- g_gr->images().get(pic_dismantle),
334- std::string(_("Dismantle")) + "<br><font size=11>" + _("Returns:") +
335- "</font><br>" + waremap_to_richtext(owner.tribe(), wares));
336+ g_gr->images().get(pic_dismantle), dismantle_text);
337 dismantlebtn->sigclicked.connect(
338 boost::bind(&BuildingWindow::act_dismantle, boost::ref(*this)));
339 capsbuttons->add(dismantlebtn);

Subscribers

People subscribed via source and target branches

to status/vote changes: