Widelands supports multiple enhancements per building, but we only use one for all buildings

[SirVer]

The engine supports that one building can be enhanced to different kind of buildings. This is not used in the datafiles though and is therefore probably broken.

Remove support for multiple enhancements from the engine.

[GunChleoc (gunchleoc)]

I have made an attempt at this, but there are still some issues that somebody else will have to fix, because I lack the skills.

http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1307844/revision/6959

Specifically, there are TODOs in these files - I'm having trouble here with the switch from Building_Index to string as a datatype:

src/ai/defaultai.cc
src/logic/player.cc

And I can't start a game - there is an error message.

[SirVer (sirver)]

I looked at the code in the branch and I do not agree with the design choice of making BuildingDescr::get_enhancement() a string. It should be a Building_Index which defaults to INVALID_INDEX when there is no enhancement. This has a bunch of reasons:, e.g converting strings to a Building_Descr is more costly, comparing strings is slower than comparing ints, strings need more space and the rest of the code identifies buildings already by its Building_Index.

I think your TODOs resolve if you change the function to return a Building_Index.

A bunch of more comments:

1) Use name() for getters instead of get_name() - the later usually means that the function does more work than simply returning a read-only reference to a member variable.
2) std::string::compare is usually overkill. Use the more intuitive a == b syntax when possible.
3) Code like this can be converted now that we have c++11:

enhancebtn->sigclicked.connect
     (boost::bind
      (&Building_Window::act_enhance,
       boost::ref(*this),
       boost::ref(enhancement)));
to

enhancebtn->sigclicked.connect([this, &enhancement] {
   this->act_enhance(enhancement);
});

which reads easier. Recommended reading: http://www.cprogramming.com/c++11/c++11-lambda-closures.html

Are you unblocked on this again?

[Ferdinand T. (f-thiessen)]

It seems that GunChleoc is not working on this anymore.
So I take advantage of it and fixed it.
It compiles without errors or warnings and I have tested it, it seems to run without problems.
Here is my code:
https://code.launchpad.net/~f-thiessen/widelands/bug-1307844

Should I propose to merge?

[SirVer (sirver)]

Yes. Please propose for merge.

> Am 12.07.2014 um 21:51 schrieb "Ferdinand T." <email address hidden>:
>
> It seems that GunChleoc is not working on this anymore.
> So I take advantage of it and fixed it.
> It compiles without errors or warnings and I have tested it, it seems to run without problems.
> Here is my code:
> https://code.launchpad.net/~f-thiessen/widelands/bug-1307844
>
> Should I propose to merge?
>
> ** Changed in: widelands
> Assignee: (unassigned) => Ferdinand T. (f-thiessen)
>
> ** Changed in: widelands
> Status: Confirmed => In Progress
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1307844
>
> Title:
> Widelands supports multiple enhancements per building, but we only use
> one for all buildings
>
> Status in Widelands:
> In Progress
>
> Bug description:
> The engine supports that one building can be enhanced to different
> kind of buildings. This is not used in the datafiles though and is
> therefore probably broken.
>
> Remove support for multiple enhancements from the engine.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/widelands/+bug/1307844/+subscriptions

[Hans Joachim Desserud (hjd)]

Hi Ferdinand and thanks for your merge proposal. (Someone will review that shortly)

Could you link up your branch to this bug report? (Can be done on the branch page, under related bugs) That way, we'll get direct links to the branch and merge proposal just below the bug description. :)

[GunChleoc (gunchleoc)]

Thanks for taking this one off my todo-list and welcome to the team :)

[GunChleoc (gunchleoc)]

Fixed in build19-rc1.