Merge lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands
- empire04_unused_key_return_on_dismantle
- Merge into trunk
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 |
Related bugs: |
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.
Description of the change
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
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.
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 4198. State: passed. Details: https:/
Appveyor build 3994. State: failed. Details: https:/
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.
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 4251. State: passed. Details: https:/
Appveyor build 4045. State: success. Details: https:/
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.
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.
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
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.
GunChleoc (gunchleoc) wrote : | # |
Arty has convinced me, so I have changed the design. Needs retesting.
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.
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/buildingwi
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.
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.
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.
hessenfarmer (stephan-lutz) wrote : | # |
Perhaps it might be worth having it without the string . (showing nothing beneath the Headings)
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.
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
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.
hessenfarmer (stephan-lutz) wrote : | # |
Agreed. now it looks consistent with the rest of the window. Thanks for fixing.
@bunnybot merge
Preview Diff
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 | 14 | granite = 1 | 14 | granite = 1 |
6 | 15 | }, | 15 | }, |
7 | 16 | return_on_dismantle_on_enhanced = { | 16 | return_on_dismantle_on_enhanced = { |
8 | 17 | planks = 0, | ||
9 | 18 | }, | 17 | }, |
10 | 19 | 18 | ||
11 | 20 | animations = { | 19 | animations = { |
12 | 21 | 20 | ||
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 | 9 | size = "big", | 9 | size = "big", |
18 | 10 | enhancement = "empire_farm2", | 10 | enhancement = "empire_farm2", |
19 | 11 | 11 | ||
20 | 12 | return_on_dismantle = { | ||
21 | 13 | planks = 0, | ||
22 | 14 | granite = 0, | ||
23 | 15 | marble = 0, | ||
24 | 16 | marble_column = 0 | ||
25 | 17 | }, | ||
26 | 18 | |||
27 | 19 | animations = { | 12 | animations = { |
28 | 20 | idle = { | 13 | idle = { |
29 | 21 | pictures = path.list_files(dirname .. "idle_??.png"), | 14 | pictures = path.list_files(dirname .. "idle_??.png"), |
30 | 22 | 15 | ||
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 | 15 | }, | 15 | }, |
36 | 16 | 16 | ||
37 | 17 | return_on_dismantle = { | 17 | return_on_dismantle = { |
38 | 18 | planks = 0, | ||
39 | 19 | }, | 18 | }, |
40 | 20 | 19 | ||
41 | 21 | animations = { | 20 | animations = { |
42 | 22 | 21 | ||
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 | 15 | marble = 1 | 15 | marble = 1 |
48 | 16 | }, | 16 | }, |
49 | 17 | return_on_dismantle_on_enhanced = { | 17 | return_on_dismantle_on_enhanced = { |
50 | 18 | log = 0, | ||
51 | 19 | granite = 0, | ||
52 | 20 | marble = 0 | ||
53 | 21 | }, | 18 | }, |
54 | 22 | 19 | ||
55 | 23 | animations = { | 20 | animations = { |
56 | 24 | 21 | ||
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 | 23 | #include <memory> | 23 | #include <memory> |
62 | 24 | 24 | ||
63 | 25 | #include "base/wexception.h" | 25 | #include "base/wexception.h" |
64 | 26 | #include "io/fileread.h" | ||
65 | 27 | #include "io/filewrite.h" | ||
66 | 28 | #include "logic/game_data_error.h" | 26 | #include "logic/game_data_error.h" |
67 | 29 | #include "logic/map_objects/tribes/tribe_descr.h" | 27 | #include "logic/map_objects/tribes/tribe_descr.h" |
68 | 30 | #include "logic/map_objects/tribes/tribes.h" | 28 | #include "logic/map_objects/tribes/tribes.h" |
69 | @@ -37,32 +35,32 @@ | |||
70 | 37 | Buildcost::Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes) | 35 | Buildcost::Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes) |
71 | 38 | : std::map<DescriptionIndex, uint8_t>() { | 36 | : std::map<DescriptionIndex, uint8_t>() { |
72 | 39 | for (const std::string& warename : table->keys<std::string>()) { | 37 | for (const std::string& warename : table->keys<std::string>()) { |
89 | 40 | int32_t value = INVALID_INDEX; | 38 | // Check ware name |
90 | 41 | try { | 39 | if (!tribes.ware_exists(warename)) { |
91 | 42 | DescriptionIndex const idx = tribes.safe_ware_index(warename); | 40 | throw GameDataError("Buildcost: Unknown ware: %s", warename.c_str()); |
92 | 43 | if (count(idx)) { | 41 | } |
93 | 44 | throw GameDataError( | 42 | |
94 | 45 | "A buildcost item of this ware type has already been defined: %s", warename.c_str()); | 43 | // Read value |
95 | 46 | } | 44 | const int32_t value = table->get_int(warename); |
96 | 47 | value = table->get_int(warename); | 45 | if (value < 1) { |
97 | 48 | const uint8_t ware_count = value; | 46 | 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 | 49 | if (ware_count != value) { | 47 | } else if (value > 255) { |
99 | 50 | throw GameDataError("Ware count is out of range 1 .. 255"); | 48 | throw GameDataError("Buildcost: Ware count needs to be <= 255 in \"%s=%d\".", warename.c_str(), value); |
100 | 51 | } | 49 | } |
101 | 52 | insert(std::pair<DescriptionIndex, uint8_t>(idx, ware_count)); | 50 | |
102 | 53 | } catch (const WException& e) { | 51 | // Add |
103 | 54 | throw GameDataError("[buildcost] \"%s=%d\": %s", warename.c_str(), value, e.what()); | 52 | insert(std::pair<DescriptionIndex, uint8_t>(tribes.safe_ware_index(warename), value)); |
88 | 55 | } | ||
104 | 56 | } | 53 | } |
105 | 57 | } | 54 | } |
106 | 58 | 55 | ||
107 | 59 | /** | 56 | /** |
108 | 60 | * Compute the total buildcost. | 57 | * Compute the total buildcost. |
109 | 61 | */ | 58 | */ |
113 | 62 | uint32_t Buildcost::total() const { | 59 | Widelands::Quantity Buildcost::total() const { |
114 | 63 | uint32_t sum = 0; | 60 | Widelands::Quantity sum = 0; |
115 | 64 | for (const_iterator it = begin(); it != end(); ++it) | 61 | for (const_iterator it = begin(); it != end(); ++it) { |
116 | 65 | sum += it->second; | 62 | sum += it->second; |
117 | 63 | } | ||
118 | 66 | return sum; | 64 | return sum; |
119 | 67 | } | 65 | } |
120 | 68 | 66 | ||
121 | @@ -79,8 +77,9 @@ | |||
122 | 79 | 77 | ||
123 | 80 | for (;;) { | 78 | for (;;) { |
124 | 81 | std::string name = fr.c_string(); | 79 | std::string name = fr.c_string(); |
126 | 82 | if (name.empty()) | 80 | if (name.empty()) { |
127 | 83 | break; | 81 | break; |
128 | 82 | } | ||
129 | 84 | 83 | ||
130 | 85 | DescriptionIndex index = tribe.ware_index(name); | 84 | DescriptionIndex index = tribe.ware_index(name); |
131 | 86 | if (!tribe.has_ware(index)) { | 85 | if (!tribe.has_ware(index)) { |
132 | 87 | 86 | ||
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 | 23 | #include <map> | 23 | #include <map> |
138 | 24 | #include <memory> | 24 | #include <memory> |
139 | 25 | 25 | ||
140 | 26 | #include "io/fileread.h" | ||
141 | 27 | #include "io/filewrite.h" | ||
142 | 26 | #include "logic/widelands.h" | 28 | #include "logic/widelands.h" |
143 | 27 | #include "scripting/lua_table.h" | 29 | #include "scripting/lua_table.h" |
144 | 28 | 30 | ||
145 | 29 | class FileRead; | ||
146 | 30 | class FileWrite; | ||
147 | 31 | |||
148 | 32 | namespace Widelands { | 31 | namespace Widelands { |
149 | 33 | 32 | ||
150 | 34 | class TribeDescr; | 33 | class TribeDescr; |
151 | @@ -39,7 +38,7 @@ | |||
152 | 39 | Buildcost(); | 38 | Buildcost(); |
153 | 40 | Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes); | 39 | Buildcost(std::unique_ptr<LuaTable> table, const Tribes& tribes); |
154 | 41 | 40 | ||
156 | 42 | uint32_t total() const; | 41 | Quantity total() const; |
157 | 43 | 42 | ||
158 | 44 | void save(FileWrite& fw, const TribeDescr& tribe) const; | 43 | void save(FileWrite& fw, const TribeDescr& tribe) const; |
159 | 45 | void load(FileRead& fw, const TribeDescr& tribe); | 44 | void load(FileRead& fw, const TribeDescr& tribe); |
160 | 46 | 45 | ||
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 | 59 | const EditorGameBase& egbase) | 59 | const EditorGameBase& egbase) |
166 | 60 | : MapObjectDescr(init_type, table.get_string("name"), init_descname, table), | 60 | : MapObjectDescr(init_type, table.get_string("name"), init_descname, table), |
167 | 61 | egbase_(egbase), | 61 | egbase_(egbase), |
169 | 62 | buildable_(false), | 62 | buildable_(table.has_key("buildcost")), |
170 | 63 | can_be_dismantled_(table.has_key("return_on_dismantle") || table.has_key("return_on_dismantle_on_enhanced")), | ||
171 | 64 | destructible_(table.has_key("destructible") ? table.get_bool("destructible") : true), | ||
172 | 63 | size_(BaseImmovable::SMALL), | 65 | size_(BaseImmovable::SMALL), |
173 | 64 | mine_(false), | 66 | mine_(false), |
174 | 65 | port_(false), | 67 | port_(false), |
175 | @@ -106,8 +108,6 @@ | |||
176 | 106 | } | 108 | } |
177 | 107 | 109 | ||
178 | 108 | // Parse build options | 110 | // Parse build options |
179 | 109 | destructible_ = table.has_key("destructible") ? table.get_bool("destructible") : true; | ||
180 | 110 | |||
181 | 111 | if (table.has_key("enhancement")) { | 111 | if (table.has_key("enhancement")) { |
182 | 112 | const std::string enh = table.get_string("enhancement"); | 112 | const std::string enh = table.get_string("enhancement"); |
183 | 113 | 113 | ||
184 | @@ -141,7 +141,6 @@ | |||
185 | 141 | return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes()); | 141 | return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes()); |
186 | 142 | } | 142 | } |
187 | 143 | if (table.has_key("buildcost")) { | 143 | if (table.has_key("buildcost")) { |
188 | 144 | buildable_ = true; | ||
189 | 145 | if (!table.has_key("return_on_dismantle")) { | 144 | if (!table.has_key("return_on_dismantle")) { |
190 | 146 | throw wexception( | 145 | throw wexception( |
191 | 147 | "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"", name().c_str()); | 146 | "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"", name().c_str()); |
192 | @@ -151,15 +150,13 @@ | |||
193 | 151 | 150 | ||
194 | 152 | if (table.has_key("enhancement_cost")) { | 151 | if (table.has_key("enhancement_cost")) { |
195 | 153 | enhanced_building_ = true; | 152 | enhanced_building_ = true; |
204 | 154 | try { | 153 | if (!table.has_key("return_on_dismantle_on_enhanced")) { |
205 | 155 | enhance_cost_ = Buildcost(table.get_table("enhancement_cost"), egbase_.tribes()); | 154 | throw wexception("The enhanced building '%s' has an \"enhancement_cost\" but no \"return_on_dismantle_on_enhanced\"", |
206 | 156 | return_enhanced_ = | 155 | name().c_str()); |
199 | 157 | Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes()); | ||
200 | 158 | } catch (const WException& e) { | ||
201 | 159 | throw wexception("The enhanced building '%s' must define \"enhancement_cost\"" | ||
202 | 160 | "and \"return_on_dismantle_on_enhanced\": %s", | ||
203 | 161 | name().c_str(), e.what()); | ||
207 | 162 | } | 156 | } |
208 | 157 | enhance_cost_ = Buildcost(table.get_table("enhancement_cost"), egbase_.tribes()); | ||
209 | 158 | return_enhanced_ = | ||
210 | 159 | Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes()); | ||
211 | 163 | } | 160 | } |
212 | 164 | 161 | ||
213 | 165 | needs_seafaring_ = table.has_key("needs_seafaring") ? table.get_bool("needs_seafaring") : false; | 162 | needs_seafaring_ = table.has_key("needs_seafaring") ? table.get_bool("needs_seafaring") : false; |
214 | @@ -315,11 +312,13 @@ | |||
215 | 315 | const BuildingDescr& tmp_descr = descr(); | 312 | const BuildingDescr& tmp_descr = descr(); |
216 | 316 | if (tmp_descr.is_destructible()) { | 313 | if (tmp_descr.is_destructible()) { |
217 | 317 | caps |= PCap_Bulldoze; | 314 | caps |= PCap_Bulldoze; |
219 | 318 | if (tmp_descr.is_buildable() || tmp_descr.is_enhanced()) | 315 | if (tmp_descr.can_be_dismantled()) { |
220 | 319 | caps |= PCap_Dismantle; | 316 | caps |= PCap_Dismantle; |
221 | 317 | } | ||
222 | 320 | } | 318 | } |
224 | 321 | if (tmp_descr.enhancement() != INVALID_INDEX) | 319 | if (tmp_descr.enhancement() != INVALID_INDEX) { |
225 | 322 | caps |= PCap_Enhancable; | 320 | caps |= PCap_Enhancable; |
226 | 321 | } | ||
227 | 323 | return caps; | 322 | return caps; |
228 | 324 | } | 323 | } |
229 | 325 | 324 | ||
230 | 326 | 325 | ||
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 | 73 | bool is_buildable() const { | 73 | bool is_buildable() const { |
236 | 74 | return buildable_; | 74 | return buildable_; |
237 | 75 | } | 75 | } |
238 | 76 | bool can_be_dismantled() const { | ||
239 | 77 | return can_be_dismantled_; | ||
240 | 78 | } | ||
241 | 76 | bool is_destructible() const { | 79 | bool is_destructible() const { |
242 | 77 | return destructible_; | 80 | return destructible_; |
243 | 78 | } | 81 | } |
244 | @@ -172,8 +175,9 @@ | |||
245 | 172 | const EditorGameBase& egbase_; | 175 | const EditorGameBase& egbase_; |
246 | 173 | 176 | ||
247 | 174 | private: | 177 | private: |
250 | 175 | bool buildable_; // the player can build this himself | 178 | const bool buildable_; // the player can build this himself |
251 | 176 | bool destructible_; // the player can destruct this himself | 179 | const bool can_be_dismantled_; // the player can dismantle this building |
252 | 180 | const bool destructible_; // the player can destruct this himself | ||
253 | 177 | Buildcost buildcost_; | 181 | Buildcost buildcost_; |
254 | 178 | Buildcost return_dismantle_; // Returned wares on dismantle | 182 | Buildcost return_dismantle_; // Returned wares on dismantle |
255 | 179 | Buildcost enhance_cost_; // cost for enhancing | 183 | Buildcost enhance_cost_; // cost for enhancing |
256 | 180 | 184 | ||
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 | 68 | items_table = table.get_table("buildcost"); | 68 | items_table = table.get_table("buildcost"); |
262 | 69 | for (const std::string& key : items_table->keys<std::string>()) { | 69 | for (const std::string& key : items_table->keys<std::string>()) { |
263 | 70 | try { | 70 | try { |
264 | 71 | if (buildcost_.count(key)) { | ||
265 | 72 | throw GameDataError( | ||
266 | 73 | "a buildcost item of this ware type has already been defined: %s", key.c_str()); | ||
267 | 74 | } | ||
268 | 75 | if (!tribes.ware_exists(tribes.ware_index(key)) && | 71 | if (!tribes.ware_exists(tribes.ware_index(key)) && |
269 | 76 | !tribes.worker_exists(tribes.worker_index(key))) { | 72 | !tribes.worker_exists(tribes.worker_index(key))) { |
270 | 77 | throw GameDataError("\"%s\" has not been defined as a ware/worker type (wrong " | 73 | throw GameDataError("\"%s\" has not been defined as a ware/worker type (wrong " |
271 | 78 | "declaration order?)", | 74 | "declaration order?)", |
272 | 79 | key.c_str()); | 75 | key.c_str()); |
273 | 80 | } | 76 | } |
279 | 81 | int32_t value = items_table->get_int(key); | 77 | const int32_t value = items_table->get_int(key); |
280 | 82 | uint8_t const count = value; | 78 | if (value < 1) { |
281 | 83 | if (count != value) | 79 | 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 | 84 | throw GameDataError("count is out of range 1 .. 255"); | 80 | } else if (value > 255) { |
283 | 85 | buildcost_.insert(std::pair<std::string, uint8_t>(key, count)); | 81 | throw GameDataError("Buildcost: Ware/Worker count needs to be <= 255 in \"%s=%d\".", key.c_str(), value); |
284 | 82 | } | ||
285 | 83 | |||
286 | 84 | buildcost_.insert(std::pair<std::string, uint8_t>(key, value)); | ||
287 | 86 | } catch (const WException& e) { | 85 | } catch (const WException& e) { |
288 | 87 | throw GameDataError("[buildcost] \"%s\": %s", key.c_str(), e.what()); | 86 | throw GameDataError("[buildcost] \"%s\": %s", key.c_str(), e.what()); |
289 | 88 | } | 87 | } |
290 | 89 | 88 | ||
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 | 1064 | log("\n%s\n%s\n", messagetitle.c_str(), message.c_str()); | 1064 | log("\n%s\n%s\n", messagetitle.c_str(), message.c_str()); |
296 | 1065 | 1065 | ||
297 | 1066 | UI::WLMessageBox mmb( | 1066 | UI::WLMessageBox mmb( |
299 | 1067 | &mm, messagetitle, message, UI::WLMessageBox::MBoxType::kOk, UI::Align::kLeft); | 1067 | &mm, messagetitle, richtext_escape(message), UI::WLMessageBox::MBoxType::kOk, UI::Align::kLeft); |
300 | 1068 | mmb.run<UI::Panel::Returncodes>(); | 1068 | mmb.run<UI::Panel::Returncodes>(); |
301 | 1069 | 1069 | ||
302 | 1070 | message.clear(); | 1070 | message.clear(); |
303 | 1071 | 1071 | ||
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 | 220 | if (canceled.bootstrap == pd->expedition_bootstrap()) { | 220 | if (canceled.bootstrap == pd->expedition_bootstrap()) { |
309 | 221 | update_expedition_button(true); | 221 | update_expedition_button(true); |
310 | 222 | } | 222 | } |
312 | 223 | }); | 223 | }); |
313 | 224 | } | 224 | } |
314 | 225 | } else if (upcast(const Widelands::ProductionSite, productionsite, building)) { | 225 | } else if (upcast(const Widelands::ProductionSite, productionsite, building)) { |
315 | 226 | if (!is_a(Widelands::MilitarySite, productionsite)) { | 226 | if (!is_a(Widelands::MilitarySite, productionsite)) { |
316 | @@ -282,14 +282,17 @@ | |||
317 | 282 | } | 282 | } |
318 | 283 | 283 | ||
319 | 284 | if (capscache_ & Widelands::Building::PCap_Dismantle) { | 284 | if (capscache_ & Widelands::Building::PCap_Dismantle) { |
323 | 285 | const Widelands::Buildcost wares = | 285 | if (building->descr().can_be_dismantled()) { |
324 | 286 | Widelands::DismantleSite::count_returned_wares(building); | 286 | const Widelands::Buildcost wares = |
325 | 287 | if (!wares.empty()) { | 287 | Widelands::DismantleSite::count_returned_wares(building); |
326 | 288 | |||
327 | 289 | const std::string dismantle_text = | ||
328 | 290 | (wares.empty() ? _("Dismantle") : std::string(_("Dismantle")) + "<br><font size=11>" + _("Returns:") + "</font><br>" + | ||
329 | 291 | waremap_to_richtext(owner.tribe(), wares)); | ||
330 | 292 | |||
331 | 288 | UI::Button* dismantlebtn = | 293 | UI::Button* dismantlebtn = |
332 | 289 | new UI::Button(capsbuttons, "dismantle", 0, 0, 34, 34, UI::ButtonStyle::kWuiMenu, | 294 | new UI::Button(capsbuttons, "dismantle", 0, 0, 34, 34, UI::ButtonStyle::kWuiMenu, |
336 | 290 | g_gr->images().get(pic_dismantle), | 295 | g_gr->images().get(pic_dismantle), dismantle_text); |
334 | 291 | std::string(_("Dismantle")) + "<br><font size=11>" + _("Returns:") + | ||
335 | 292 | "</font><br>" + waremap_to_richtext(owner.tribe(), wares)); | ||
337 | 293 | dismantlebtn->sigclicked.connect( | 296 | dismantlebtn->sigclicked.connect( |
338 | 294 | boost::bind(&BuildingWindow::act_dismantle, boost::ref(*this))); | 297 | boost::bind(&BuildingWindow::act_dismantle, boost::ref(*this))); |
339 | 295 | capsbuttons->add(dismantlebtn); | 298 | capsbuttons->add(dismantlebtn); |
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.