widelands-dev team mailing list archive

[Tibor Bamhor <tiborb95@xxxxxxxxx>]

Hi

I will look at those NOTs, but I am 99 % sure that it works as supposed. I
am doing a lot of tests. But I will modify the code to be "safe". Though I
can not imagine what effect has NOT to non-trivial data types, with bool it
is obvious... Perhaps the compiler is smart enough to understand what it
should mean.

The memory leaks comes from your implementation of object pointer.
Valgrinds reports also line numbers. I tested trunk, i tested mine branch.

I just created tibor-ai3 branch, it is trunk + AI from tibor-ai2 + required
changes in conf files and couple of other cc and h files out of src/si
directory. I hope it will work (will be easy to merge into trunk)

If you mean "test" like during compilation and without GUI - I can not
imagine no such tests. AI is too complex for this....

BTW, I would eventually modify slightly the BZRprimer wiki, for me as a
beginner it is a pain to follow some steps - specifically when branching
trunk and creating own branch.

Tibor


2014-07-01 20:08 GMT+02:00 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
>>>>
>>>
>>>
>>>
>>
>>
>
>