widelands-dev team mailing list archive

[Holger Rapp <SirVer@xxxxxx>]

On 01.07.2014, at 12:46, Tibor Bamhor <tiborb95@xxxxxxxxx> wrote:

> "the OPtr code is already implemented and it is very simple to use" - well I am not sure about this. Your use of OPtr in AI is broken. Fix it and I will have no problem with this. But I am not that good programmer to figure it out by myself…

You can do it :). I also fixed a bunch of if (not blah == militarysite) which never did what you intended (the compiler will read that as if ((not blah) == militarysite) which is always wrong. Maybe the leak comes from there too?

> BTW: I today merged trunk and my AI, but without object pointer, now I am testing it… 

Cool. Consider writing some tests!


> 
> Tibor
> 
> 
> 
> 
> 2014-07-01 6:23 GMT+02:00 Holger Rapp <SirVer@xxxxxx>:
> Hi,
> On 30.06.2014, at 10:00, Tibor Bamhor <tiborb95@xxxxxxxxx> wrote:
> 
>> Well, AI never ever before checked if a bulding listed in productionsites really exists. There is another part of code (functions lose_building and gain_buildings) that are called by core and are updating productionsites list. Widelands has been working this way for years and nobody ever felt a need to change that (or I as a newcomer do not know about this).
> 
> the AI was never really ‘working’. It was always very fragile and it contains a bunch of bugs. I do not really understand your resistance - the OPtr code is already implemented and it is very simple to use. If you merge trunk and it still compiles you are golden. 
> 
>> We are talking abut something else, see this line:
>> 
>> ProductionSiteObserver& site = productionsites.front();
>> 
>> Our question is "Is iterator 'site' valid all the time check_productionsites() function is being executed?
> 
> The code you posted is always valid in c++ as long as !productionsite.empty(). No change to productionsites can invalidate this code, only a delete &site; will. This never happens until the site is actually destroyed in Widelands. So this is not the bug that valgrind shows you.
> 
>> I googled quite a lot and it seems there is not a good way to verify validity of such iterator, only programmer has to be carefull with manipulating std::list. Manipulation (push_back, pop_back) can make such iterator invalid.
> 
> There is no iterator in this code. We peek at the content of productionsites() and remove the beginning. No iterator is hold.
> 
>> Also note that during running AI (DefaultAI::think() and all called functions) no building is finished or destroyed. AI calls "send_player_dismantle(*site.site)" and building is dismantled (in fact in two stages) later, and IA is notified about change via lose_building() and productionsites list is updated (this is done out of DefaultAI::think() function). At the end the game is single-threaded, is not it?
> 
> It is. But the AI in particular is the first thing that would be made multithreaded - the logic never will. But AIs should be able to act in parallel. 
> 
>> Also note that there is update_productionsite_stats() functions that f.e. queries statistics of buildings in productionsites and if it queried nonexisting building it would lead to segmentation fault or other problems.
>> 
>> So brief summary at the end :):
>> we are talking about different issues: I talk about validity of std:list iterator, you about possibility that productionsites contains nonexistant building.
> 
> Both are realistic issues. I offered a solution for the second problem and I think you should stick with it. 
> 
> Holger
> 
> 
>> 
>> Regards
>> 
>> Tibor
>> 
>> 
>> 
>> 2014-06-30 7:11 GMT+02:00 Holger Rapp <SirVer@xxxxxx>:
>>> (should I send the replay to mailing list?)
>> 
>> yes :). Contacting one dev individually is nearly always the wrong decision. 
>> 
>> Also, you should make a merge request if you want to have your branch reviewed. And it should be ready for inclusion into trunk from your point of view, otherwise reviewing is a waste of time. 
>> 
>>> But the summary of situation is here:
>>> 
>>> My tiborb-ai branch (merged to trunk in revision 6978) contained a bug that did not show on linux only on other platforms.
>>> 
>>> You tried to fix it in revison 6989, but the fix is wrong, valgrind reports memory issues and the std::list productionsites is treated incorrectly, every time a building is to be checked (all production buildings are checked periodically), the buiding is deleted from list and this leads to absurd behaviour. AI just doesnt know about own buildings. Visible effect is that no mines are built because mines are built only if there are 8 production buildings. And AI just believes actual number of buildings is too low (decreasing by one by every regular productionsites check - done every +- 20 seconds).
>> 
>> Most buildings should be repushed at the end of the list before returning the function. But my changes might easily contain errors of course - I find the AI code hard to understand and there are no tests, so refactorings break things. Note that no dev has the right to complain about his/her code to be broken by others if there are no tests :).
>> 
>>> In tibor-ai2 I added further changes to the AI logic and hopefully fixed the crashing. Tested under valgrind, gdb - tenths of hours of gameplay and other hundereds without debug tool. But I cannot test it under other platforms.
>> 
>> While that sounds like a lot of testing, reality shows that just letting the game run is usually poor testing - it will not test corner cases often enough. And hundreds of hours is is just 1 hour for a hundred people - i.e. they will hit corner cases much quicker. Adding tests for your new features (AND doing the run the game for a while tests) is usually much better. 
>> 
>>> So options are:
>>> a) we go object pointers way (your design) but somebody (you?) should fix it in trunk first, I am not able to
>>> b) we omit object pointers and stay with old design - this is what is done in my branch.
>> 
>> Why not do both and use object ptrs? They are the correct choice in the sense of defense programming. I am fine with your changes as long as they do not crash the game. A bad AI is preferable to a crashing one for sure though. 
>> 
>>> What I suggest: somebody should test under windows/mac and find out it it still crashes and if not - I will merge trunk into my branch - option b.
>>> 
>>> And this is also reason why I did not do any merging of trunk to my branch.
>>> 
>>> Personally - I spend some time working on this, I feel it is in reasonable state and would like to have it merged finally.
>> 
>> Okay, then propose it for reviewing and request windows and mac feedback. You will likely not get much until it is merged and hits the nightlies though - then is when people are really starting to play with it. 
>> 
>> 
>>> 
>>> 
>>> Tibor
>>> 
>>> 
>>> 2014-06-28 8:12 GMT+02:00 Holger Rapp <SirVer@xxxxxx>:
>>> Please merge Trunk Now. Make it a habit to keep your branch up to date with trunk by always merging. It will make reviewing easier by making the diff as realistic and small as possible.
>>> 
>>> Also, explaining why trunk ai is broken and how you fixed it will make it easier to comment on this part directly.
>>> 
>>> > Am 27.06.2014 um 23:22 schrieb Tibor Bamhor <tiborb95@xxxxxxxxx>:
>>> >
>>> > Hi all,
>>> >
>>> > Please review latest changes I did to AI in branch tibor-ai2
>>> >
>>> > bzr branch lp:~widelands-dev/widelands/tibor-ai2
>>> >
>>> > AI in trunk is still badly broken and I think it is time to fix it.
>>> >
>>> > I am interested especially in stability on Windows and Mac platform, as this was a problem with previous iteration of AI.
>>> >
>>> > If everything is OK, I will merge trunk there and propose merging into trunk
>>> >
>>> > Thanks!
>>> >
>>> > Tibor
>>> > _______________________________________________
>>> > Mailing list: https://launchpad.net/~widelands-dev
>>> > Post to     : widelands-dev@xxxxxxxxxxxxxxxxxxx
>>> > Unsubscribe : https://launchpad.net/~widelands-dev
>>> > More help   : https://help.launchpad.net/ListHelp
>>> 
>> 
>> 
> 
>