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 <sir...@gmx.de>: > 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 <sir...@gmx.de>: > >> >> On 01.07.2014, at 12:46, Tibor Bamhor <tibor...@gmail.com> 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 <sir...@gmx.de>: >> >>> Hi, >>> On 30.06.2014, at 10:00, Tibor Bamhor <tibor...@gmail.com> 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 <sir...@gmx.de>: >>> >>>> (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 <sir...@gmx.de>: >>>> >>>>> 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 <tibor...@gmail.com>: >>>>> > >>>>> > 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@lists.launchpad.net >>>>> > Unsubscribe : https://launchpad.net/~widelands-dev >>>>> > More help : https://help.launchpad.net/ListHelp >>>>> >>>> >>>> >>>> >>> >>> >> >> > >
_______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
