Lua does not wrap Descr Classes which makes hard coding values in the help needed.

[_aD]

The following files:lines refer to a variable chance of finding a resource in an exhausted mine:
tribes/barbarians/deeper_coalmine/help.lua:17
tribes/barbarians/deeper_goldmine/help.lua:16
tribes/barbarians/deeper_oremine/help.lua:16
tribes/barbarians/granitemine/help.lua:16

These files seem to have the chance of 5% hard-coded:
tribes/barbarians/coalmine/help.lua:17
tribes/barbarians/goldmine/help.lua:16
tribes/barbarians/oremine/help.lua:16

[Hans Joachim Desserud (hjd)]

I only checked a couple of the files, but you are right. Should be fairly straightforward to move the value out of the string and into the list of variables which gets included in it.

(In a way, it would still be hardcoded though)

[SirVer (sirver)]

The problem is, that there is no way to get to this data from Lua currently - the tribe/world/worker/building Descr Classes are not wrapped. Adding those is not difficult, but tedious. But as long as they are not there, this bug can not be closed.

I still believe this is a low hanging fruit for a C++ coder - I would offer help with this in any case.

[GunChleoc (gunchleoc)]

SirVer has shown me how to write the first wrapper, so I'm taking on this bug.

[SirVer (sirver)]

Thanks for working on this! Your help is very much appreciated and I am super impressed how you found your way around the code so quickly - the Lua bindings are not easy code and a high entry level for someone starting out with c++.

I added a bunch of comments a small feature in i first commit and changed the way the help is generated in a second commit: the help generating script is now running a Lua method that gets the description passed in.

All comments are marked TODO(GunChleoc) so that you can grep for them easily - let me know if anything is unclear. Also, I only modified the barbarian lumberjacks_hut (for now) and I did not touch any tests because I assume that you will do changes on them when the implementation evolves.

[GunChleoc (gunchleoc)]

I am struggling a bit with the C++ control flow to get more wrappers in, so I fiddled on the Lua end to create some generic funtions to use in formatting the help. This gives us a bit of an interface to look at what we still need to get from C++.

If you could write one of the wares/costs lists for me, I could copy what you've done - I still don't understand how I get anything into Lua that's not an int in the Building_Descr class.

The things we still need:

- Localized names of the buildings and resources, which can be found in each tribe's main conf file

- For the ware types, we need to have singluar/plural forms with ngettext capability. This would entail thinking up a format for the conf files that would be parsed in the buildcat python script.

- Lists of things (building/upgrade/dismantle wares, workers, tools)

- Some more int stuff like vision range, conquer range etc.

[SirVer (sirver)]

I thought I did that already.... I need to look at the branch. Could you in the meantime merge master into the branch so that we do not diverge too far? I'll put it on my list to add one list property.

[SirVer (sirver)]

I tried merging trunk into this (it is a good habit to always pull && merge trunk before starting to work in a branch. That way they do not diverge so badly), but there are plenty of merge conflicts in help.lua files. Could you have a look at that?

Otherwise, I checked and I did implement build_cost already. I put a sample usage in the lumberjack help. Here it is for easier reference too:

-- TODO(GunChleoc): SirVer says: This is just an example usage. Remove again please.
      for ware, count in pairs(building_description.build_cost) do
         print("### ", ware, count)
      end
      print(("Needs %d trunks."):format(building_description.build_cost["trunk"]))

This is implemented in int L_BuildingDescription::get_build_cost(lua_State * L) in lua_map.cc. It returns a Lua table with key = warename and value = integer.

Was that the question or did I misunderstand anything?

[GunChleoc (gunchleoc)]

Yes, thanks!

I will have a look at the merging.

[GunChleoc (gunchleoc)]

I am having problems with merging again - bzr keeps flagging stuff I should resolve where there is nothing left to resolve.

Also, I can't compile - if I use the wrong version of compile.sh just once, my branches get messed up and I have to get a fresh copy, because I don't know how to clean up the cmake files.

So, could somebody please do the following for me:

1. Pull this branch

2. Merge trunk

3. Copy the attached files into the merged branch

4. Compile to make sure everything's OK and then resolve

Thanks!

[SirVer (sirver)]

I did the following:

I merged trunk and resolved the conflicts in all files, but the help.lua files. Those I copied over from your archive.

So trunk is merged and work can continue.

[GunChleoc (gunchleoc)]

Thanks!

[GunChleoc (gunchleoc)]

I'm stuck again

- I have added WorkerDescription, which works fine. I have added an identical (as far as I can see) WareDescription, which does not compile, and I have no idea why:

/home/bratzbert/sources/widelands/bug-1074353/src/scripting/lua_map.cc: In function ‘int LuaMap::upcasted_bob_to_lua(lua_State*, Widelands::Bob*)’:
/home/bratzbert/sources/widelands/bug-1074353/src/scripting/lua_map.cc:545:9: warning: switch missing default case [-Wswitch-default]
/home/bratzbert/sources/widelands/bug-1074353/src/scripting/lua_map.cc: In member function ‘int LuaMap::L_WareDescription::get_name(lua_State*)’:
/home/bratzbert/sources/widelands/bug-1074353/src/scripting/lua_map.cc:1214:30: error: invalid use of incomplete type ‘const struct Widelands::Ware_Descr’
In file included from /home/bratzbert/sources/widelands/bug-1074353/src/scripting/lua_map.cc:20:0:
/home/bratzbert/sources/widelands/bug-1074353/src/scripting/lua_map.h:44:9: error: forward declaration of ‘const struct Widelands::Ware_Descr’
make[2]: *** [src/CMakeFiles/widelands_all.dir/scripting/lua_map.cc.o] Error 1
make[1]: *** [src/CMakeFiles/widelands_all.dir/all] Error 2
make: *** [all] Error 2

- I also tried to get a list of Worker_Descr->becomes(), but the data structures are different from what we're doing with wares_map_to_lua in lua_map.cc, so I have nothing to frankencode.

[SirVer (sirver)]

> error: invalid use of incomplete type ‘const struct Widelands:

this indicates that the compiler does not know how the class looks. that means that it is conly forward declared, but the compiler did not see a definition. A #include in the .cc file will fix this.
Forward declaring is much cheaper than #including a file, that is when you are in a .h file and you only use pointers to a class, you only forward declare it:

class A
int foo(A* pointer_to_A); // this will compile fine, compiler does not need to know about the internals of A.

int foo(A* pointer_to_A) { pointer_to_A->method_blub(); } // now the compiler needs to know about A, so include it's header.

Also, there was a typo: the class is called WareDescr not, Ware_Descr.

I added wrappings for becomes in the latest revision and enabled get_ware_description too. Could you merge trunk before continuing? there are some merge conflicts again.

[GunChleoc (gunchleoc)]

Will do. I will be gone in a few minutes, so I expect there to be more changes in trunk before I get back.

Thanks for the explanation and the code :)

[GunChleoc (gunchleoc)]

Could somebody please build a lumberjacks_hut and have a look at the Lua error message in the help window? It doesn't like the global unpack Lua function anymore and I'm stumped as to why.

[SirVer (sirver)]

The Methode does not exist in lua 5.2 anymore. Use table.unpack which is the same thing

> Am 11.04.2014 um 12:06 schrieb GunChleoc <email address hidden>:
>
> Could somebody please build a lumberjacks_hut and have a look at the Lua
> error message in the help window? It doesn't like the global unpack Lua
> function anymore and I'm stumped as to why.
>
> --
> You received this bug notification because you are subscribed to
> widelands.
> https://bugs.launchpad.net/bugs/1074353
>
> Title:
> Lua does not wrap Descr Classes which makes hard coding values in the
> help needed.
>
> Status in Widelands:
> In Progress
>
> Bug description:
> The following files:lines refer to a variable chance of finding a resource in an exhausted mine:
> tribes/barbarians/deeper_coalmine/help.lua:17
> tribes/barbarians/deeper_goldmine/help.lua:16
> tribes/barbarians/deeper_oremine/help.lua:16
> tribes/barbarians/granitemine/help.lua:16
>
>
> These files seem to have the chance of 5% hard-coded:
> tribes/barbarians/coalmine/help.lua:17
> tribes/barbarians/goldmine/help.lua:16
> tribes/barbarians/oremine/help.lua:16
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/widelands/+bug/1074353/+subscriptions

[GunChleoc (gunchleoc)]

I need help again. I have commented my problems in the code, grep for TODO(SirVer). Thanks!

[SirVer (sirver)]

Right now the game does not start on my system due to lua errors. Could you change the files so that it starts again that I can investigate the TODOs? Also, for one time todos could you use a more descriptive marker, something like TODO(SirVerPleaseInvestigate) or so, because there are other TODOs with my name all over the code base :).

[GunChleoc (gunchleoc)]

> Right now the game does not start on my system due to lua errors.

Well it does on mine, so how do I find out what the difference is? I only get a Lua error message when I access a help file.

> TODO(SirVerPleaseInvestigate) Will do in the future. the files to check are:

scripting/format_help.lua
src/scripting/lua_bases.cc
src/scripting/lua_map.cc

The issue is the L_ProductionSiteDescription that I am trying to add.

[GunChleoc (gunchleoc)]

I will need the following two things in Lua next, and I tried and failed again:

struct Building_Descr : public Map_Object_Descr {
 typedef std::set<Building_Index> Enhancements;
 typedef std::vector<Building_Index> FormerBuildings;

If someone could do the FormerBuildings, I could try the Enhancements. What I really need from the containers is the MapObject_Desrc->name(). so I can then use that as a reference to grab building info.

[SirVer (sirver)]

I added support for enhancements, which now returns a list of building_descr (but see https://bugs.launchpad.net/widelands/+bug/1307844, this should just be one entry).

FormerBuildings is not part of the tribe description, i.e. we only keep the list of buildings in the upwards direction. Each building keeps track of its former buildings to calculate dismantling cost. There is no way to expose that through descriptions right now.

[GunChleoc (gunchleoc)]

15/04/2014 06:39, sgrìobh SirVer:
> I added support for enhancements, which now returns a list of
> building_descr (but see
> https://bugs.launchpad.net/widelands/+bug/1307844, this should just be
> one entry).

I thought so - have already implemented this safe for multiple entries
though, so it should work either way.

> FormerBuildings is not part of the tribe description, i.e. we only keep
> the list of buildings in the upwards direction. Each building keeps
> track of its former buildings to calculate dismantling cost. There is no
> way to expose that through descriptions right now.

We will just have to provide them manually in each help file then. Since
this isn't changed that often, I hope this will be stable enough.

[GunChleoc (gunchleoc)]

Another juicy problem - I managed to steal the correct code to implement:

int L_ProductionSiteDescription::get_working_positions(lua_State * L)

in lua_map.cc, but all I get is an empty list. Same problem with

int L_ProductionSiteDescription::get_nr_working_positions(lua_State * L)

which always pushes 1.

I tried casting to const ProductionSite_Descr *, but that didn't solve the problem.

[GunChleoc (gunchleoc)]

Here's my current wish list - whenever you find the time :)

1. My last post above

2. Same problem with int L_ProductionSiteDescription::get_inputs(lua_State * L)

3. Is there a way to get a list of all buildings that need a certain ware, e.g. coal?

4. Is there a way to get a list of all buildings that produce a certain ware, e.g. snacks?

5. I have commented TODO's in the help.lua for the barbarian coalmine, there are some more odds and ends in there that I could live with though if they remained unresolved.

[SirVer (sirver)]

> We will just have to provide them manually in each help file then. Since
> this isn't changed that often, I hope this will be stable enough.

Could you elaborate what you want to add manually? I did not understand.

1-2, 5) Acknowledge. Maybe over eastern.

3-4) Right now there is not, but nothing stops us to build such a list while we parse the tribe and add it to the ware descriptions.

[GunChleoc (gunchleoc)]

16/04/2014 07:58, sgrìobh SirVer:
>> We will just have to provide them manually in each help file then. Since
>
>> this isn't changed that often, I hope this will be stable enough.
>
> Could you elaborate what you want to add manually? I did not understand.

Have a look at the Barbarian Deep Coalmine. In:

building_help_building_section("barbarians", building_description,
"coalmine") ..

I am providing "coalmine" as the former building, since this can't be
read from C++. This will have to be done for all building that are
usually upgraded from another building.

The parameter is then used to calculate the cumulative costs and the
dismantle return. It probably will need to be an array to accomodate
multiple upgrades, e.g. for the Deeper Coalmine. I haven't finished
implementing that yet.

> 1-2, 5) Acknowledge. Maybe over eastern.

5) is actually commented in the deep_coalmine, not the coalmine. Sorry.

> 3-4) Right now there is not, but nothing stops us to build such a list
> while we parse the tribe and add it to the ware descriptions.

I had a look ath the Wares Encyclopedia, and there is a function there
that does a whole lot of iterating to get all the buildings for a ware.
So, we will have potential for reusing some code here - get it into the
tribe's ware descriptions and then refactor the encyclopedia as well.

So, 2 functions that each return a Building_Index? 3) produced_by, 4)
consumed_by?

[wl-zocker (wl-zocker)]

Just a note: Some buildings can be built directly or via upgrade (e.g. Imperial sentry, upgraded via the baracks). Will there be two categories for the dismantle yields ("Dismantle yields if upgraded" and "D. y. if built directly")?

[GunChleoc (gunchleoc)]

Thanks for giving me an example building for this. I'll put it on the todo-list; this can certainly be done.

[Hans Joachim Desserud (hjd)]

Looks like sentry has two separate entries for the wares returned by dismantling it at least. See also the discussion in bug 1170086.

[GunChleoc (gunchleoc)]

Yes, that is correct and as it needs to be - one of them is for when the building was built directly, and one for when it was built as an enhanced building, and they work the same way as for buildings that can be built only directly/built only as enhanced buildings - the naming is consistent. So, all I need to do is add a few more cases in format-help.lua to display all pertinent information :)

[SirVer (sirver)]

Okay, I did some work here now.

1,2) Seems to be working for me. At least this prints three lines with the correct count (and same for .inputs)
game = wl.Game()
b = game:get_building_description("barbarians", "deeper_coalmine")
for name, count in pairs(b.working_positions) do
   print(name, count)
end
print(b.nr_working_positions) -- outputs 3

3, 4) Did not start on this. Do you want to try it or should I give it a shot?

5) Added a bunch of comments. Getting the parameters of the building programs (or the worker programs for that matter) is complicated and I have no clear concept on how it can be done right now easily without wrapping every supported program - which is more work than keeping the hardcoding up to date.

[GunChleoc (gunchleoc)]

Thanks!

I'm separated from my Linux computer at the moment because I'm cat sitting, so if you have time to do 3, 4, it would be great. If not, I'll have another look in 2 weeks.

I also tried and failed merging master again.

[SirVer (sirver)]

I merged trunk.

For now, I want to prioritize the one_world branch so that it can be merged. I am nearly done. If I finish with it before you find time to work on this again, I can help out :). Just ping me.

[GunChleoc (gunchleoc)]

Yes, do finish one_world first :)

[GunChleoc (gunchleoc)]

I have added some comments in http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1074353/revision/6882

Summarizing the current todo-list:

1. int L_ProductionSiteDescription::get_working_positions(lua_State * L) still produces nil instead of a table. I have added a testing call into the help file for the deep coalmine.

2. Same problem with int L_ProductionSiteDescription::get_inputs(lua_State * L)

3. List of all buildings that need a certain ware, e.g. coal

4. List of all buildings that produce a certain ware, e.g. snack

[GunChleoc (gunchleoc)]

I tried to implement 4. and I've hit a catch-22: If I fill up the list in the constructor of WareDescr, the tribe's buildings haven't been parsed yet. If I fill it up later, I can't make it const, which then blows up in lua_map.cc.

Any ideas?

c.f. my NOCOM(#gunchleoc2sirver) comments in http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1074353/revision/6884/src/logic/ware_descr.cc

[SirVer (sirver)]

Why do you not do it in the parsing of the building descriptions?

[GunChleoc (gunchleoc)]

How would I get it from there into the constructor of the WareDescr, which have already been parsed and constructed by this point?

I guess the easiest way right now might be to collect the info directly in the LUA wrapper, but then I get code duplication with the encyclopedia, which is also doing the same thing.

[GunChleoc (gunchleoc)]

I have decided to move this into lua_game.cc, because than at least we won't get any weird sider effects down the road.

Does anybody know if there is an elegant way to find out if two char* are equal? At the moment, I'm doing a boost:lexical_cast to String so I can use the string.compare function. It's working, but I was wondering if there is a better way.

[GunChleoc (gunchleoc)]

1-4 are solved :)

[GunChleoc (gunchleoc)]

I think this is pretty much done, except for a few comments in src/scripting/lua_map.cc. Search the file for "gunchleoc" to get at the remaining issues - I will definitely need help with those. Basically, I will need some code for persist/unpersist to steal from, and I have a question in there concerning a char* compare.

I haven't programmed getting the image filenames from C++ yet, because things will change for the spritesheets anyway. So, I'd rather wait for those and open a new branch then. I have added a comment for this in scripting/format_help.lua. At the moment, the big help picture on the top is the first frame of the idle animation, there are special filenames for resi and soldiers, and then we have menu.png for everything else.

The big help picture crashes for the shipyard, because it's "idle_00.png" instead of "shipyard_i_00.png". Which of these two is the preferred format, and would it be worth my time to do some renaming? If the spritesheets will make these obsolete before the next release, I guess my time will be better spent on something else. If not, I will be happy to do the work.

[SirVer (sirver)]

> How would I get it from there into the constructor of the WareDescr, which have already been parsed and constructed by this point?

You don't. You get the (mutable) wares_description container from the tribe_description into the building constructor and add a member (mutable_ware_description(), either in Tribe_Descr or if there is a container with every ware in there than there). So you can get something like this in the buildingdesc constructor:

tribe.mutable_ware_description("log")->add_producer(*this);

> I have decided to move this into lua_game.cc, because than at least we won't get any weird sider effects down the road.

Not sure what you mean, but I do not think this is ideal. The Lua classes should contain as little logic as possible and mostly just pass through data.

> Does anybody know if there is an elegant way to find out if two char* are equal?

std::string(a) == std::string(b) ; or alternatively !strcmp(a, b). The first is more c++, the second C. I suggest using the first.

[SirVer (sirver)]

Great work. I'll look into this as soon as jetlag allows.

> I haven't programmed getting the image filenames from C++ yet, because things will change for the spritesheets anyway.

Spritesheets seem to have moved in a more difficult situation though - with the one world branch there are more ways of defining a sprite which are not supported by the spritesheet creator. Also the algorithm to create spritesheets is buggy. So we cannot wait for it.

> So, I'd rather wait for those and open a new branch then.

I think a new branch is fine either way. Let's get your changes in.

> The big help picture crashes for the shipyard, because it's "idle_00.png" instead of "shipyard_i_00.png". Which of these two is the preferred format, and would it be worth my time to do some renaming? If the spritesheets will make these obsolete before the next release, I guess my time will be better spent on something else. If not, I will be happy to do the work.

Relying on the filenames when they are not hardcoded in the engine is fragile either way. Let's open a bug with a repro case and discuss there how we can move forward.

[GunChleoc (gunchleoc)]

Thanks for the info.

I am adding your comments to the code. Let me get the Atlanteans done so we have a complete set of test cases, then we can merge and open a new branch for shifting stuff in the code and solving the image filename thing.

[GunChleoc (gunchleoc)]

Fixed in build19-rc1.