widelands-dev team mailing list archive

[Tibor Bamhor <tiborb95@xxxxxxxxx>]

in regard to 'tests':

You for sure noticed there is a lot of k*Debug variables (like
kMilitaryDebug, kMilDismDebug, kMilitScoreDebug, kQuarryDismDebug,
kProductionDebug and many more) and these (if set to true) prints out same
important information on command line. This is my testing. I am using it a
lot...

Tibor




2014-07-02 6:58 GMT+02:00 Holger Rapp <SirVer@xxxxxx>:

> 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.
>
>
> No, it is not. Some compilers will warn you about that too, but most will
> not. ‘not’ for integers/enums means ‘i != 0 ? true : false’.
>
> If you mean "test" like during compilation and without GUI - I can not
> imagine no such tests. AI is too complex for this….
>
>
> Ah, I heard this argument so often, but it is always wrong. Make your AI
> configurable (i.e. only enable searching for tree places), and you can
> easily observe one small thing of it in a test. Not finding a way to test
> your stuff will mean that it breaks in the future when somebody else
> touches the code.
>
> 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.
>
>
> Go ahead. People who start out with bzr are the intended audience and the
> best people who can contribute what is actually useful.
>
>
>
> 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
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>