Workers with wares inside ships can crash the game

This looks like good candidate for our regression tests suite - we should be able to script the scenario...

Notabilis, would you enjoy working on a test? We could meet on IRC sometime next week.

I should have seen this coming...
But why not. :-)

After looking at the existing tests, I think most of the new test shouldn't be a problem. I am missing two functions, though.
- There does not seem a way to get the ware currently carried by a worker. But this can be circumvented by a simple delay until the woodcutter felled his tree.
- It seems to be impossible to set a warehouse to "remove from here" by script. Am I missing something or do I have to add this functionality?

Oh, and am I seeing this right? I create a test which crashes the game in trunk but should run fine in the branch?

Interface lua - C++ is very incomplete, if you add some functionality there you will help a lot.

Thank you for your williness to work on it. I myself does not have tasks where one one small task leads to another bigger one, but it happens.

re your last sentence - yes, this is exactly how it would work

We could work on a branch together, where I supply you with the missing functions in the Lua interface. This way, you can concentrate on what you're writing, and then look at my (hopefully small) code changes to see how the Lua interface works.

Thanks, but I already started working on Lua function to get/set the ware policies. I think that is all what is needed for this test. But it would be great if you could review them when I am done.

Lua methods for setting/getting the ware/worker policies are in the linked feature branch. So far, they are working fine.

Mostly, that is. They are using the same ware/worker-parsing functions as the get_wares/workers() methods. Consequently, they allow the special ware/worker "all" to select all wares/workers for a policy. This part works a bit too good, since carriers are selected and (if requested) evicted from the warehouse, too.

Well, with an unlimited supply of carriers this might take some time. How should this be fixed? Add a comment to the documentation and ignore it? Remove the carriers from "all"? Modify the Warehouse::set_policy() c++ code to ignore "evict" on carriers?

The regression test is done now, too. It is part of the feature-* branch and should crash the game when executed. When executed with a merge of both branches it runs successfully.

I tested the new regression test branch - works (at least it failed) and code looks good. Also the branch with fix looks OK.

Regarding #7 - I would exclude/limit carriers in Lua only. Carriers don't cost anything currently and it shouldn't matter, but somebody might want to make a mod where carriers do cost a ware. So, we can't just ignore them in the engine part.

I do have another branch open for a general cap though - maybe that will be sufficient for your purpose, in case it hits the same part of the code?

https://code.launchpad.net/~widelands-dev/widelands/bug-1624216-horsepocalypse

What do you mean with "Lua only"? Leaving the "all" command in a way that carriers are evicted as well? That would mean that in a lua script the script-coder has to write something like:

  wh:set_worker_policies("all", "SP_Remove")
  wh:set_worker_policies("barbarians_carrier", "SP_Normal")

This should evict all workers except the carriers.

In case you mean the Lua/C++ interface: What do you think about ignoring carriers when given "all" but allowing to set their policy when explicitly named? Not quite sure if that is possible, though, and it most likely is far from efficient. But the code is called seldom anyway.

I think your branch deals with another part of the code than the part I would need to modify. I guess I could introduce a similar cap for evicting workers but it would most likely be more coding than just ignoring carriers.

I just had a look at my branch - you're right, different part of the code. I'm back home now, so I can have a real look at this - maybe I was talking nonsense.

I adapted the set_worker_policies() method which is called by Lua. When setting policies no-cost workers will be ignored, which matches the controls possible in the GUI.

As far as I am concerned, both branches are ready for review and merge now.

Fixed in build20-rc1