To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Free Heroes of Might and Magic II: Open…

Free Heroes of Might and Magic II: Open-Source Project that You Want to Be Part of

Feb. 24, 2021

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.

0804_heroes2/image1.png

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:

0804_heroes2/image2.png

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.

0804_heroes2/image3.png

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, ....)
{
  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.push_back( str.substr( pos1, pos2 - pos1 ) );
    pos1 = pos2 + sep.size();
  }
  ....
}

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<fheroes2::Rect> 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<std::vector<uint8_t> > 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<uint8_t> PALPAlette()
{
  std::vector<uint8_t> 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.

Popular related articles
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.21.2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
PVS-Studio ROI

Date: 01.30.2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
The Last Line Effect

Date: 05.31.2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.14.2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
PVS-Studio for Java

Date: 01.17.2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The way static analyzers fight against false positives, and why they do it

Date: 03.20.2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: 10.22.2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept