Recently we found out that the new version of the fheroes2 project was released. In our company there are many fans of Heroes of Might and Magic game series. So, we couldn't pass it up and checked the project by PVS-Studio.
Introduction to the project
Free Heroes of Might and Magic II is an open source implementation of the Heroes of Might and Magic II game engine. To play the updated version, you need the original Heroes of Might and Magic II or at least its demo version. The latter is available by the script distributed with the source code. Depending on the operating system, you need to choose the appropriate version.
After a successful project build, I decided to get a little nostalgic and run the game. For convenience, I slightly edited the fheroes2.cfg file by setting the parameters:
heroes speed = 10
ai speed = 10
battle speed = 10
I also set its resolution in the videomode parameter.
After all manipulations, I started the game and saw a familiar home screen:
If you set wrong screen resolution or don't want to tinker around with the config file, open the game in full-screen mode by pressing f4.
Next, I chose the standard game. Since I downloaded the demo version, the only available map was Broken Alliance.
It is very convenient that the windows with the map, heroes, and settings can be moved to the needed parts of the screen. Some reviews claimed the AI had problems in previous game versions. Now it masters the map quite quickly and fights well. Playing around with it was a real blast.
At the time of writing the last available project version was 0.8.4. It enhanced game performance on low-performance devices, added a large number of gameplay and cosmetic features that you can check out here. The following note got my attention: "fixed more than a hundred bugs compared to the previous version". The authors seem to carefully monitor code quality: as we can see from the project page on GitHub, they regularly use a Sonar Cxx static analyzer, occasionally perform checks by Cppcheck.
It seems to me that if astrologers announce a static analysis week and developers add PVS-Studio to their utilities list, there will be even fewer bugs. Let's make sure of this by looking at a few erroneous code snippets I found using this tool. Just in case, developers of open projects can use the PVS-Studio analyzer for free.
Micro optimizations
For a change, let's start with shallow code optimizations rather than actual errors. Deep optimizations require profilers, so here we'll be limited to low hanging fruits. Static analyzers often lack information on how particular code operates and therefore are not able to show actual bottlenecks. That's why we use "micro-optimizations" for a set of PVS-Studio warnings about increasing the speed of work.
We do not expect that tips in this article will utterly help speed up the game. I just wanted to pay attention to this set of diagnostics that is usually not covered in our regular articles about checking open projects and therefore remains in the shadow.
Warning N1
V823 Decreased performance. Object may be created in-place in the 'list' container. Consider replacing methods: 'push_back' -> 'emplace_back'. tools.cpp 231
std::list<std::string> StringSplit( const std::string & str, ....)
{
....
while (....)
{
list.push_back( str.substr( pos1, pos2 - pos1 ) );
....
}
....
}
The analyzer suggests that in this case it will be more efficient to use the emplace_back method. In general, a simple change from push_back to emplace_back won't yield performance improvement when the argument is an rvalue. However, in our case, the std::string has a constructor accepting two iterators (see constructor #6). It will allow us to avoid a redundant move constructor call when emplace_back is used:
std::list<std::string> StringSplit( const std::string & str, ....)
{
std::list<std::string> list;
size_t pos1 = 0;
size_t pos2 = std::string::npos;
while ( pos1 < str.size()
&& std::string::npos != (pos2 = str.find(sep, pos1)))
{
list.emplace_back(str.begin() + pos1, str.begin() + pos2);
pos1 = pos2 + sep.size();
}
....
}The analyzer found more than 100 of such warnings which provides insight into the importance of the issue. Here are some of them:
V823 Decreased performance. Object may be created in-place in the 'loop_sounds' container. Consider replacing methods: 'push_back' -> 'emplace_back'. agg.cpp 461
V823 Decreased performance. Object may be created in-place in the 'projectileOffset' container. Consider replacing methods: 'push_back' -> 'emplace_back'. bin_info.cpp 183
V823 Decreased performance. Object may be created in-place in the 'actions' container. Consider replacing methods: 'push_back' -> 'emplace_back'. ai_normal_battle.cpp 264
V823 Decreased performance. Object may be created in-place in the 'actions' container. Consider replacing methods: 'push_back' -> 'emplace_back'. ai_normal_battle.cpp 288
V823 Decreased performance. Object may be created in-place in the 'actions' container. Consider replacing methods: 'push_back' -> 'emplace_back'. ai_normal_battle.cpp 433
and others
Warning N2
V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. tools.cpp 216
void StringReplace( std::string & dst,
const char * pred,
const std::string & src )
{
size_t pos = std::string::npos;
while ( std::string::npos != ( pos = dst.find( pred ) ) )
{
dst.replace( pos, std::strlen( pred ), src );
}
}
In this case, the strlen function is called at each loop iteration, and the size of the pred string does not change. The most cliche'd way to make it simpler is to calculate the string length outside the loop and make it constant.
void StringReplace( std::string & dst,
const char * pred,
const std::string & src )
{
size_t pos = std::string::npos;
const size_t predSize = std::strlen( pred);
while ( std::string::npos != ( pos = dst.find( pred ) ) )
{
dst.replace( pos, predSize, src );
}
}
Warning N3
V827 Maximum size of the 'optionAreas' vector is known at compile time. Consider pre-allocating it by calling optionAreas.reserve(6) battle_dialogs.cpp 217
void Battle::DialogBattleSettings( .... )
{
std::vector optionAreas;
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 36,
pos_rt.y + 47,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 128,
pos_rt.y + 47,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 220,
pos_rt.y + 47,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 36,
pos_rt.y + 157,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 128,
pos_rt.y + 157,
panelWidth,
panelHeight ) );
optionAreas.push_back( fheroes2::Rect( pos_rt.x + 220,
pos_rt.y + 157,
panelWidth,
panelHeight ) );
}
The analyzer detected std::vector, the maximum size of which is known at compile time. Before filling the container, it would be much more efficient to call:
optionAreas.reserve(6);
In this case, push_back calls will not reallocate the internal buffer in the vector and move the elements to a new memory area. Another option is to rewrite this code using std::array.
Warnings N4. 0, 4.1...4.7
V809 Verifying that a pointer value is not NULL is not required. The 'if (armyBar)' check can be removed. kingdom_overview.cpp 62
V809 Verifying that a pointer value is not NULL is not required. The 'if (artifactsBar)' check can be removed. kingdom_overview.cpp 64
V809 Verifying that a pointer value is not NULL is not required. The 'if (secskillsBar)' check can be removed. kingdom_overview.cpp 66
V809 Verifying that a pointer value is not NULL is not required. The 'if (primskillsBar)' check can be removed. kingdom_overview.cpp 68
V809 Verifying that a pointer value is not NULL is not required. The 'if (armyBarGuard)' check can be removed. kingdom_overview.cpp 279
V809 Verifying that a pointer value is not NULL is not required. The 'if (armyBarGuest)' check can be removed. kingdom_overview.cpp 281
V809 Verifying that a pointer value is not NULL is not required. The 'if (dwellingsBar)' check can be removed. kingdom_overview.cpp 283
The analyzer found some interesting Clear functions, see the code below. What's interesting, such behavior can be found in other code parts.
void Clear( void )
{
if ( armyBar )
delete armyBar;
if ( artifactsBar )
delete artifactsBar;
if ( secskillsBar )
delete secskillsBar;
if ( primskillsBar )
delete primskillsBar;
}
void Clear( void )
{
if ( armyBarGuard )
delete armyBarGuard;
if ( armyBarGuest )
delete armyBarGuest;
if ( dwellingsBar )
delete dwellingsBar;
}
In this case, we can refactor the code by removing all checks for null pointers from the functions. The delete operator handles the code correctly anyway. This may not be a performance benefit (the compiler will remove the checks itself), but it will make the code simpler and more readable.
General Analysis
Warning N5
The analyzer has issued 2 warnings for this code fragment:
V654 The condition 'i < originalPalette.size()' of loop is always false. battle_interface.cpp 3689
V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. battle_interface.cpp 3689
void Battle::Interface::RedrawActionBloodLustSpell( Unit & target )
{
std::vector > originalPalette;
if ( target.Modes( SP_STONE ) )
{
originalPalette.push_back( PAL::GetPalette( PAL::GRAY ) );
}
else if ( target.Modes( CAP_MIRRORIMAGE ) )
{
originalPalette.push_back( PAL::GetPalette( PAL::MIRROR_IMAGE ) );
}
if ( !originalPalette.empty() )
{
for ( size_t i = 1; i < originalPalette.size(); ++i )
{
originalPalette[0] = PAL::CombinePalettes( originalPalette[0],
originalPalette[i] );
}
fheroes2::ApplyPalette( unitSprite, originalPalette[0] );
}
....
}
As we can see, the programmer made a mistake in the algorithm. As the function runs, the originalPalette vector increases in size by one or remains empty. We'll enter the if statement above only when originalPalette.size() equals one. Therefore, the i variable will never be less than the size of the vector. This is how we get a fragment of unreachable code.
Warning N6
V547 Expression 'palette.empty()' is always true. image_tool.cpp 32
const std::vector PALPAlette()
{
std::vector palette;
if (palette.empty()) //<=
{
palette.resize( 256 * 3 );
for ( size_t i = 0; i < palette.size(); ++i )
{
palette[i] = kb_pal[i] << 2;
}
}
return palette;
}
In this case, the analyzer sees that we unconditionally create an empty vector. So, this check is redundant. We can remove it and make the code simpler.
Warning N7
V668 There is no sense in testing the 'listlog' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. battle_interface.cpp 986
Battle::Interface::Interface(....)
{
....
listlog = new StatusListBox();
....
if ( listlog )
{
....
}
....
}
The analyzer detected that the pointer value, returned by the new operator is checked for null. This usually means that a program won't behave in the way the programmer expects in case it's not possible to allocate memory. Since the new operator was unable to allocate memory, according to the C++ standard, we get the std::bad_alloc() exception. This means this check is redundant.
Here are two similar warnings:
V668 There is no sense in testing the 'elem' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. battle_arena.cpp 1079
V668 There is no sense in testing the 'image' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. battle_arena.cpp 1095
Warning N8
V595 The '_currentUnit' pointer was utilized before it was verified against nullptr. Check lines: 2336, 2358. battle_interface.cpp 2336
void Battle::Interface::MouseLeftClickBoardAction( .... )
{
....
themes = GetSwordCursorDirection( Board::GetDirection( index,
_currentUnit->GetHeadIndex()));
....
if ( _currentUnit )
{
....
}
....
}
The _currentUnit pointer is first dereferenced and then checked for NULL. This can mean one of two obvious things: undefined behavior will take place if the pointer is null, or the pointer can't be null and the program will always work correctly. If the first option is implied, the check should be performed before dereferencing. In the second case, one can omit the redundant check.
Conclusion
In my opinion, the project is now very close to the original version of the game. As for the code, it is of quite high quality. Not a surprise, because the developers use several static analyzers. However, there are no limits to perfection. If used by project developers, PVS-Studio can help reduce even more bugs. Don't forget it's free for open-source projects.
In conclusion, kudos to the developers - the engine is really cool! If you are looking for a decent interesting open-source project to take part in, fheroes2 is just what you need.
