Merge lp:~widelands-dev/widelands/bug-1724145-corrupt-zip-when-saving into lp:widelands

[GunChleoc]

Do not save LuaEconomy objects in Lua scrips for use later

- Due to economy merging, the economy object can become unavailable and cause crashes.
- Fixed Lua scripts and added warnings to documentation.

The underlying economy object can disappear from eco:ware_target_quantity calls due to economy merging. I found no way to check for that in the ware_target_quantity function itself, so I have added warnings to the documentation and fixed the Lua scripts.

To reproduce the Bug:

1. Play Empire 3: Neptune's Revenge until you trigger the "Lower Marble Columns" objective. Do not take any action on the objective.

2. Delete a road so that you end up with 2 economies

3. Connect the outer economy to the Headquarters Shipwreck economy: Pick a flag in the outer economy to start road building, then a flag in the original economy.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3635. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/401193955.
Appveyor build 3434. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1724145_corrupt_zip_when_saving-3434.

[Notabilis (notabilis27)]

Diff and testing are fine.

Maybe move the warning to the economy class instead of warning at every method? But that would probably increase the risk of overlooking it.

From a brief look at the code I guess one could replace the raw pointer to the economy with a weak_ptr and check it before dereferencing it in the methods. But this would mean quite a lot of refactoring for making all economy references shared_ptr.
Another possibility would be a "economy registration" and, e.g., referencing the economies by id. A list of economies already exists in the class Player but I haven't checked whether a pointer to that class will stay valid if the player is defeated.

review: Approve

[GunChleoc (gunchleoc)]

The problem is that the method is called on a null object, so I can't do any checks in the method, because it's already too late. I tried checking whether get() is not null, but that doesn't work in the current implementation of the Lua interface. No idea if weak_ptr could help here.

The only way to avoid problems would be to completely remove LuaEconomy and not use it, but using numbers is probably fragile in its own way.

Thanks for the review!

@bunnybot merge

=== modified file 'data/campaigns/emp03.wmf/scripting/mission_thread.lua'
--- data/campaigns/emp03.wmf/scripting/mission_thread.lua	2018-01-25 19:28:03 +0000
+++ data/campaigns/emp03.wmf/scripting/mission_thread.lua	2018-07-07 11:04:51 +0000
@@ -99,8 +99,7 @@
    local objective = add_campaign_objective(obj_lower_marble_column_demand)
 
    --- Check the headquarters' flag's economy
-   local eco = sf.brn.immovable.economy
-   while eco:ware_target_quantity("marble_column") ~= 4 do
+   while sf.brn.immovable.economy:ware_target_quantity("marble_column") ~= 4 do
       sleep(2434)
    end
    sleep(4000)

=== modified file 'data/campaigns/emp04.wmf/scripting/starting_conditions.lua'
--- data/campaigns/emp04.wmf/scripting/starting_conditions.lua	2018-05-29 20:14:16 +0000
+++ data/campaigns/emp04.wmf/scripting/starting_conditions.lua	2018-07-07 11:04:51 +0000
@@ -20,8 +20,7 @@
    r4 = p3:place_road(field_mill.immovable.flag, "br", "r", true)
 
 p3:forbid_buildings("all")
-local eco = field_warehouse.brn.immovable.economy
-eco:set_ware_target_quantity("beer", 180)
+field_warehouse.brn.immovable.economy:set_ware_target_quantity("beer", 180)
 
 -- =======================================================================
 --                                 Player 1

=== modified file 'data/campaigns/tutorial04_economy.wmf/scripting/mission_thread.lua'
--- data/campaigns/tutorial04_economy.wmf/scripting/mission_thread.lua	2018-05-03 06:01:14 +0000
+++ data/campaigns/tutorial04_economy.wmf/scripting/mission_thread.lua	2018-07-07 11:04:51 +0000
@@ -152,8 +152,7 @@
    message_box_objective(plr, economy_settings2)
    o = message_box_objective(plr, economy_settings3)
 
-   local eco = sf.brn.immovable.economy
-   while eco:ware_target_quantity("marble_column") ~= 20 do
+   while sf.brn.immovable.economy:ware_target_quantity("marble_column") ~= 20 do
       sleep(200)
    end
    -- wait that the player has really changed the target quantity

=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc	2018-04-27 06:11:05 +0000
+++ src/scripting/lua_map.cc	2018-07-07 11:04:51 +0000
@@ -3585,6 +3585,10 @@
 
       Returns the amount of the given ware that should be kept in stock for this economy.
 
+      **Warning**: Since economies can disappear when a player merges them
+      through placing/deleting roads and flags, you must get a fresh economy
+      object every time you use this function.
+
       :arg warename: the name of the ware.
       :type warename: :class:`string`
 */
@@ -3605,6 +3609,10 @@
 
       Returns the amount of the given worker that should be kept in stock for this economy.
 
+      **Warning**: Since economies can disappear when a player merges them
+      through placing/deleting roads and flags, you must get a fresh economy
+      object every time you use this function.
+
       :arg workername: the name of the worker.
       :type workername: :class:`string`
 */
@@ -3625,6 +3633,10 @@
 
       Sets the amount of the given ware type that should be kept in stock for this economy.
 
+      **Warning**: Since economies can disappear when a player merges them
+      through placing/deleting roads and flags, you must get a fresh economy
+      object every time you use this function.
+
       :arg warename: the name of the ware type.
       :type warename: :class:`string`
 
@@ -3651,6 +3663,10 @@
 
       Sets the amount of the given worker type that should be kept in stock for this economy.
 
+      **Warning**: Since economies can disappear when a player merges them
+      through placing/deleting roads and flags, you must get a fresh economy
+      object every time you use this function.
+
       :arg workername: the name of the worker type.
       :type workername: :class:`string`
 
@@ -4043,6 +4059,10 @@
 
       (RO) Returns the economy that this flag belongs to.
 
+      **Warning**: Since economies can disappear when a player merges them
+      through placing/deleting roads and flags, you must get a fresh economy
+      object every time you call another function on the resulting economy object.
+
       :returns: The :class:`Economy` associated with the flag.
 */
 int LuaFlag::get_economy(lua_State* L) {