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).
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? 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. 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? 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. 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
