widelands-dev team mailing list archive

[Tibor Bamhor <tiborb95@xxxxxxxxx>]

"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...


front() returns reference, not iterator, you are right, I was wrong.


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


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
>>>
>>
>>
>>
>
>