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.

8908. By GunChleoc

Merged trunk.

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
1=== modified file 'src/logic/map_objects/tribes/building.cc'
2--- src/logic/map_objects/tribes/building.cc 2018-09-23 11:10:56 +0000
3+++ src/logic/map_objects/tribes/building.cc 2018-11-16 06:07:58 +0000
4@@ -133,17 +133,22 @@
5 }
6 }
7
8+ // We define a building as buildable if it has a "buildcost" table.
9+ // A buildable building must also define "return_on_dismantle".
10+ // However, we support "return_on_dismantle" without "buildable", because this is used by custom scenario buildings.
11+ if (table.has_key("return_on_dismantle")) {
12+ return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
13+ }
14 if (table.has_key("buildcost")) {
15 buildable_ = true;
16- try {
17- buildcost_ = Buildcost(table.get_table("buildcost"), egbase_.tribes());
18- return_dismantle_ = Buildcost(table.get_table("return_on_dismantle"), egbase_.tribes());
19- } catch (const WException& e) {
20- throw wexception(
21- "A buildable building must define \"buildcost\" and \"return_on_dismantle\": %s",
22- e.what());
23- }
24+ if (!table.has_key("return_on_dismantle")) {
25+ throw wexception(
26+ "The building '%s' has a \"buildcost\" but no \"return_on_dismantle\"",
27+ name().c_str());
28+ }
29+ buildcost_ = Buildcost(table.get_table("buildcost"), egbase_.tribes());
30 }
31+
32 if (table.has_key("enhancement_cost")) {
33 enhanced_building_ = true;
34 try {
35@@ -151,9 +156,9 @@
36 return_enhanced_ =
37 Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
38 } catch (const WException& e) {
39- throw wexception("An enhanced building must define \"enhancement_cost\""
40+ throw wexception("The enhanced building '%s' must define \"enhancement_cost\""
41 "and \"return_on_dismantle_on_enhanced\": %s",
42- e.what());
43+ name().c_str(), e.what());
44 }
45 }
46

Subscribers

People subscribed via source and target branches

to status/vote changes: