MantisBT - VCMI
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0000764VCMIMechanics - Battlespublic2011-07-05 06:122012-02-27 12:51
ReporterWarmonger 
Assigned Tobeegee 
PriorityhighSeverityblockReproducibilityrandom
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version0.86Fixed in Version0.87 
Summary0000764: Random crashes thanks to bonus system
DescriptionI'm afraid that caching is not properly implemented - sometimes I observe my Gold Dragons regenerating or poisoned at the beginning of battle. Looks like they have corrupted bonus.
Also, often game crashes at random, especially when fighting AI. I'm trying to implement new battle abilities, so it drives me crazy.
Steps To ReproduceI attach nice map with many creature abilities avaliable. Try to run the game in Hotseat and attack enemy hero with Adela or Valeska to reproduce the issue - but it happened also in eveyr other game I tried.
Additional Information
TagsNo tags attached.
Relationships
related to 0000823closed Tow Artifact "Vial of Dragon Blood" freeze game 
Attached Files? Testy4 Power Rating.h3m (3,331) 2011-07-05 06:12
https://bugs.vcmi.eu/file_download.php?file_id=745&type=bug
Issue History
Date ModifiedUsernameFieldChange
2011-07-05 06:12WarmongerNew Issue
2011-07-05 06:12WarmongerStatusnew => assigned
2011-07-05 06:12WarmongerAssigned To => beegee
2011-07-05 06:12WarmongerFile Added: Testy4 Power Rating.h3m
2011-07-05 18:31beegeeNote Added: 0001840
2011-07-05 19:14beegeeNote Added: 0001842
2011-07-05 19:20WarmongerNote Added: 0001843
2011-07-07 10:17Tow dragonNote Added: 0001844
2011-07-13 18:54beegeeNote Added: 0001846
2011-07-13 19:14WarmongerNote Added: 0001848
2011-07-14 17:21TowNote Added: 0001849
2011-07-14 17:22TowNote Edited: 0001849bug_revision_view_page.php?bugnote_id=1849#r1354
2011-07-16 14:32WarmongerNote Added: 0001850
2011-07-16 14:49beegeeNote Added: 0001851
2012-02-27 12:26WarmongerNote Added: 0002240
2012-02-27 12:26WarmongerStatusassigned => resolved
2012-02-27 12:26WarmongerFixed in Version => 0.87
2012-02-27 12:26WarmongerResolutionopen => fixed
2012-02-27 12:51WarmongerStatusresolved => closed
2012-03-01 23:31TowRelationship addedrelated to 0000823

Notes
(0001840)
beegee   
2011-07-05 18:31   
Thanks much that you informed me about problems concerning caching of the bonus system. Sorry about that it drives you crazy, I've implemented it carefully but as you know the bonus system isn't trivial and the interactions with them too.

So I'll kill them all those evil bugs in the following weekends. =)

I've noticed these random crashes only when I've played against AI, in hotseat no problem.

In the newest revision, I've disabled bonus caching.
(0001842)
beegee   
2011-07-05 19:14   
OK, I've detected where the bug comes from. Some simple re-arrangement of the code is necessary.

Just want to know, do you noticed any speed gains through the bonus caching? Without I get 25 FPS in a non-siege battle, with caching I get 46-48 fps.
(0001843)
Warmonger   
2011-07-05 19:20   
Yes, it helps a lot. Simply no more waiting for every stack to move.
(0001844)
Tow dragon   
2011-07-07 10:17   
I think we just shouldn't cache bonuses, it causes too much trouble. And there are better ways of gaining speedup like turning off STL debugging or implementing "subset bonus retrieval". Bonus system-side caching is much more troublesome than it's worth. And you can always cache your queries in place where you need the knowledge about bonuses (e.g. make flag curStakckFlies in battle interface) --- it's much simpler and more transparent, because the battle interface is what cares about validity of cache (bonus system just can't do that without heavy assumptions).
(0001846)
beegee   
2011-07-13 18:54   
Sorry but I had to do it, the bonus system caching system is now much more robust(>99%) and more important thread-safe. It should work without any problems. Warmonger could you test it if the bugs you recognized are fixed?

I've implemented thread safety really simple, perhaps it could be done better, but I haven't much experience about it(i'll look further into it some day=)). So it's a little bit slower than the original implemantion, but anyway it's much more faster than without bonus caching.

@Tow dragon: Turning off STL debugging particurly _HAS_ITERATOR_DEBUGGING is not possible due to many complication. There are reported bugs for Visual Studio concerning that point. I don't have the link ATM, but it's true. Also every binary have to be build with that macro, also every boost lib. So changing BOOST_FOREACH to simple for loops and list STL containers to vector STL containers just for performance-critical functions should do the trick.

What do you meant with subset bonus retrieval?
(0001848)
Warmonger   
2011-07-13 19:14   
I think it's working. However, thread-unsafe problems rarely got properly solved ;) so I leave this bug for now.
(0001849)
Tow   
2011-07-14 17:21   
(edited on: 2011-07-14 17:22)
Bonus caching concept has two elemental flaws.

Firstly, the underlying issue (performance in debug) has different cause — STL implementation in MSVC. This in conjunction with flaws in GUI (costly checks whether stack is a catapult at every cursor movement, etc) was indeed causing annoying slowdowns, however it was not a problem for release builds. Introducing so much additional logic just for sake of debug builds performance looks like an overkill, especially when much simpler solutions should also do the trick. (The few actual chokepoints can be buffered on GUI/engine; replacing STL list with another list implementation should also help).

Secondly, bonus caching system is based on assumption that is not entirely valid and we rather don't want to make it valid. The assumption is that result of a bonus query is dependant only on that query and bonus nodes „tree” state. However we have limiters that can rely on other elements of gamestate (eg. creature type tier, native terrain -> that's data outside bonus system) and we generally intended on introducing more such type limiters in future. That means that result of bonus query can change because of change in other parts of game state (which changes are not „visible” for bonus system).
However other parts of engine can often assume that gamestate (including bonus „tree” state) won't change for some time (GUI obtains mutex on GS when processing input). So if any caching is to work, it should be turned on/off each time by asker (GUI tells bonus system, that between two calls to given funcs, gamestate won't change -> then no checking on the bonus system is needed at all.)

@Thread-safety
That won't work, you have a global state that is left unprotected.
Consider following scenario with two threads A and B trying to parallely access cache.
A: setCachingStr("a");
B: setCachingStr("b");
A: enters getAllBonuses, obtains a mutex, returns results meant for thread B (invalid!), clears cachingStr
B: enters getAllBonuses, makes full look-up and returns valid results

>>What do you meant with subset bonus retrieval?
You shouldn't have asked :P
(0001850)
Warmonger   
2011-07-16 14:32   
It doesn't make much sense to cache bonuses without limiters, as currently major part of bonuses (coming from stack experience) is handled by them. On the other hand, stack experience certainly won't change during battle.
(0001851)
beegee   
2011-07-16 14:49   
Sure, there are simpler solutions to archieve a faster bonus system performance but I think it's better in the long-term to create a global cache than to store often used bonus request results several times here and there. Second point is that you have to manually validate those stored information when you know that the bonus system has been changed.

With the revision 2252 the bonus caching system doesn't cache results of limiter objects. Normally everything which gets changed in the bonus system and which is visible for the caching system get's now cached.

Thread-safety was already correctly implemented in my previous commit. I completely forgot about that point in the first version of the bonus caching system. Thread-safety is programmed in the following way: It gets exclusive access in the getAllBonuses method where all cached data are read or written. So this should be correct, but I don't know if it could be done better because acquiring a exclusive lock every frame could be a bit slow on network/internet games with several players. It's runs fast on a single player game or a hotseat game. With Threading Building Blocks(TBB) or with C++0x in some years it could be done a bit faster due to atomic data types.

The latest commit to the bonus caching system increased performance when moving the cursor in battle. Have a nice day =)
(0002240)
Warmonger   
2012-02-27 12:26   
Seems already fixed. Please reopen if you discover random crashes at the beginning of stacks' turn.