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

Proposed by GunChleoc
Status: Merged
Merged at revision: 8927
Proposed branch: lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui
Merge into: lp:widelands
Diff against target: 45 lines (+15/-10)
1 file modified
src/logic/map_objects/tribes/building.cc (+15/-10)
To merge this branch: bzr merge lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui
Reviewer Review Type Date Requested Status
hessenfarmer test review Approve
Review via email: mp+358305@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

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

Ok I will try to find some time next week.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

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

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

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

@bunnybot merge

Revision history for this message
Arty (artydent) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
133 }133 }
134 }134 }
135135
136 // We define a building as buildable if it has a "buildcost" table.
137 // A buildable building must also define "return_on_dismantle".
138 // However, we support "return_on_dismantle" without "buildable", because this is used by custom scenario buildings.
139 if (table.has_key("return_on_dismantle")) {
140 return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
141 }
136 if (table.has_key("buildcost")) {142 if (table.has_key("buildcost")) {
137 buildable_ = true;143 buildable_ = true;
138 try {144 if (!table.has_key("return_on_dismantle")) {
139 buildcost_ = Buildcost(table.get_table("buildcost"), egbase_.tribes());145 throw wexception(
140 return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());146 "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"",
141 } catch (const WException& e) {147 name().c_str());
142 throw wexception(148 }
143 "A buildable building must define \"buildcost\" and \"return_on_dismantle\": %s",149 buildcost_ = Buildcost(table.get_table("buildcost"), egbase_.tribes());
144 e.what());
145 }
146 }150 }
151
147 if (table.has_key("enhancement_cost")) {152 if (table.has_key("enhancement_cost")) {
148 enhanced_building_ = true;153 enhanced_building_ = true;
149 try {154 try {
@@ -151,9 +156,9 @@
151 return_enhanced_ =156 return_enhanced_ =
152 Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());157 Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
153 } catch (const WException& e) {158 } catch (const WException& e) {
154 throw wexception("An enhanced building must define \"enhancement_cost\""159 throw wexception("The enhanced building '%s' must define \"enhancement_cost\""
155 "and \"return_on_dismantle_on_enhanced\": %s",160 "and \"return_on_dismantle_on_enhanced\": %s",
156 e.what());161 name().c_str(), e.what());
157 }162 }
158 }163 }
159164

Subscribers

People subscribed via source and target branches

to status/vote changes: