On 01.07.2014, at 12:46, Tibor Bamhor <[email protected]> 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 <[email protected]>: > Hi, > On 30.06.2014, at 10:00, Tibor Bamhor <[email protected]> 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 <[email protected]>: >>> (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 <[email protected]>: >>> 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 <[email protected]>: >>> > >>> > 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 : [email protected] >>> > Unsubscribe : https://launchpad.net/~widelands-dev >>> > More help : https://help.launchpad.net/ListHelp >>> >> >> > >
_______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
