Flax Engine. Introduction to the game engine and analysis of its source code

“It's like Unreal and Unity had a baby” – this is the touching description given to this engine in the GameDev community. This phrase not only sounds cute, but also accurately conveys its essence, because the engine was really conceived as something between the Unity Engine and Unreal Engine.

Introduction

Hello, dear readers! I want to introduce you to another interesting find from the vast expanses of GitHub. Flax Engine is a full-fledged multi-platform commercial game engine from Polish developers.

In this article, we will briefly review the main features of the engine, and then analyze the most interesting problems found in its source code using the PVS-Studio static code analyzer. This is a good way to slightly improve your ability to notice bugs and typos in C# and C++ code.

PVS-Studio? What is it?

PVS-Studio is a static code analyzer designed to find potential errors and vulnerabilities in C#, C, C++ and Java code without actually executing it. With PVS-Studio, you can analyze both individual files and the entire project.

The result of such analysis is a report with warnings, which can be conveniently worked with through a special panel in your favorite IDE:

If you wish, you can get more information about the analyzer by visiting the official websiteand now let's get back to the engine!

About Flax Engine

So, dear readers, meet Flax Engine:

What makes Flax stand out from other engines?

  • Support for scripting in both C++ and C#. Visual programming is also possible;

  • Support for .NET 8 and C# 12, which means you can not only use the latest language features, but also get a performance boost from significant runtime improvements. For a more detailed look at these new features, check out our article. I would also like to note that at the time of writing, Unity does not support these versions;

  • If you thought that this engine is more focused on C# development than C++, you are wrong. On the contrary, the core of the engine is implemented in C++, which allows the corresponding developers to use its API directly. This gives an advantage in performance, as well as the ability to use some additional APIs;

  • The presence of a very solid set of tools for an Open Source engine, a list of which can be found at on a separate page engine;

  • Absolutely all engine functionality can be used for free if the income from your projects does not exceed $250,000 per quarter. Once this threshold is exceeded, the cost of the engine will be equal to 4% of the profit from your projects implemented on its basis.

Now that we know a little about the engine, why not find out what the analyzer thinks PVS-Studio about its source code? Next, we will look at potential problems in the C# and C++ code of the most current version of the engine at the time of writing – 1.8.6512.2. Ready? Then let's go!

Parsing errors in Flax Engine C# code

Redundant condition checks

Case 1

public override string TypeDescription
{
  get
  {
    // Translate asset type name
    var typeName = TypeName;
    string[] typeNamespaces = typeName.Split('.');

    if (   typeNamespaces.Length != 0 
        && typeNamespaces.Length != 0)    // <=
    {
      typeName = Utilities.Utils
                          .GetPropertyNameUI(
                            typeNamespaces[typeNamespaces.Length - 1]);
    }

    return typeName;
  }
}

PVS-Studio warning:

V3001 There are identical sub-expressions 'typeNamespaces.Length != 0' to the left and to the right of the '&&' operator. AssetItem.cs: 83.

The analyzer found two identical expressions linked by the operator &&. Surely one of them was specified by mistake. But is it just redundant code or is it a sign of a more serious problem? For example, instead of the second repeated inequality, the following check could have been intended:

typeNamespaces[typeNamespaces.Length - 1].Length != 0

Here is another similar case found in the project.

Case 2

public override bool OnMouseDown(Float2 location, MouseButton button)
{
  ....
  if (_rightMouseDown || (_middleMouseDown && _middleMouseDown)) // <=
  {
    // Start navigating
    StartMouseCapture();
    Focus();
    return true;
  }
  ....
}

PVS-Studio warning:

V3001 There are identical sub-expressions '_middleMouseDown' to the left and to the right of the '&&' operator. VisjectSurface.Input.cs: 495.

Error due to careless copy-paste

partial class Window
{
  ....
  public void Show()
  ....
  public void Hide()
  ....
}
public class ContextMenuBase : ContainerControl
{ 
  private Window _window;
  ....
  public void Show()                      // <=
  {
    _window.Show();
  }

  public void Hide()                      // <=
  {
    _window.Show();
  }

  public void Minimize()
  {
    _window.Minimize();
  }
}

PVS-Studio warning:

V3013 It is odd that the body of 'Show' function is fully equivalent to the body of 'Hide' function (70, line 78). WindowRootControl.cs: 70, 78.

A typical mistake due to carelessness when using Copy-Paste. In the method Hide the method is called _window.Show instead of _window.Hide.

Array out of bounds

public Matrix2x2(float[] values)
{
  ....
  if (values.Length != 4)
    throw new ArgumentOutOfRangeException(....);

  M11 = values[0];
  M12 = values[1];
  M21 = values[3];
  M22 = values[4];                        // <=
}

PVS-Studio warning:

V3106 Possibly index is out of bound. The '4' index is pointing beyond 'values' bound. Matrix2x2.cs: 98.

From the condition it is clear that in the array values there can only be exactly four elements. Since the indexing of elements starts from 0, the index of the very last element of the array will be equal to 3. However, in the last assignment, an access is performed by index 4, which will inevitably lead to an exception.

Unreachable code

Case 1

public void SetMemberValue(object instance, object value)
{
  ....
  if (type.IsEnum)
    value = Convert.ChangeType(value, Enum.GetUnderlyingType(type.Type));
  else if (type.Type == typeof(byte))
    value = Convert.ToByte(value);
  else if (type.Type == typeof(sbyte))
    value = Convert.ToSByte(value);
  else if (type.Type == typeof(short))
    value = Convert.ToInt16(value);
  else if (type.Type == typeof(int))      // <=
    value = Convert.ToInt32(value);
  else if (type.Type == typeof(long))
    value = Convert.ToInt64(value);
  else if (type.Type == typeof(int))      // <=
    value = Convert.ToUInt16(value);
  ....
}

PVS-Studio warning:

V3003. The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 78, 82. MemberComparison.cs: 78, 82.

The analyzer found two identical conditions within related ones else ifIn case if type.Type will matter intthe code will be executed only in the body of the first of them, and the code in the body of the second will be unreachable. Since in the body of the second one under consideration else if conversion to type is in progress UInt16a logical solution would be to replace the corresponding condition with the following:

type.Type == typeof(ushort)

Case 2

private void UpdateDragPositioning(....)
{
  if (....)
    _dragOverMode = DragItemPositioning.Above;
  else if (....)
    _dragOverMode = DragItemPositioning.Below;
  else
    _dragOverMode = DragItemPositioning.At;

  // Update DraggedOverNode
  var tree = ParentTree; 
  if (_dragOverMode == DragItemPositioning.None) // <=
  {
    if (tree != null && tree.DraggedOverNode == this)
      tree.DraggedOverNode = null;
  }
  else if (tree != null)
    tree.DraggedOverNode = this;
}

PVS-Studio warning:

V3022 Expression '_dragOverMode == DragItemPositioning.None' is always false. TreeNode.cs: 566.

The analyzer always found false if– the condition that causes the code inside then-the branch will never be executed.

Note that as a result of executing the first conditional construct, the field _dragOverMode always gets a new value different from DragItemPositioning.None. Because of this, the next if becomes meaningless.

It is possible that this was done intentionally, and the extra code was simply forgotten to be removed. But if it is still an error, one of the options for fixing it could be to move the first conditional construct from the beginning of the method to its end. Thus, the corrected version could look like this:

private void UpdateDragPositioning(....)
{
  // Update DraggedOverNode
  var tree = ParentTree; 
  if (_dragOverMode == DragItemPositioning.None)
    ....

  if (....)
    _dragOverMode = DragItemPositioning.Above;
  else if (....)
    _dragOverMode = DragItemPositioning.Below;
  else
    _dragOverMode = DragItemPositioning.At; 
}

This solution should not violate the logic of the method, but only the engine developers can check this for sure.

Thread synchronization issue due to compiler optimizations

protected Window _window;
....
public DialogResult ShowDialog(Window parentWindow)
{
  // Show window
  Show(parentWindow);
  // Set wait flag
  Interlocked.Increment(ref _isWaitingForDialog);
    // Wait for the closing
  do
  {
    Thread.Sleep(1);
  } while (_window);                      // <=

  // Clear wait flag
  Interlocked.Decrement(ref _isWaitingForDialog);

  return _result;
}

PVS-Studio warning:

V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. Dialog.cs: 108.

This code contains a potential problem that cannot be caught when working with Debug configuration. What is the secret of this beast's elusiveness? The fact is that this problem can only arise when assembling a Release version due to code optimization by the compiler. In addition, its occurrence depends on other factors, such as the version of .NET used and the number of processors in the system.

The essence of the problem is the infinite loop while due to possible compiler caching of the field value _window. This may happen because the field value _window does not change in any way within the loop itself, and the change in other threads is not expected by the compiler, since the field was declared without a keyword volatile. You can read more about this in MSDN.

Parsing errors in Flax Engine C++ code

Potential nullptr dereference

Case 1

void Append(const T* data, int32 length)
{
  ....
  auto prev = Base::_data;
  ....
  Base::_data = (T*)Allocator::Allocate(Base::_length * sizeof(T));
  Platform::MemoryCopy(Base::_data, prev, prevLength * sizeof(T)); // <=
  ....
  if (_isAllocated && prev)
  ....
}

PVS-Studio warning:

V595 The 'prev' pointer was utilized before it was verified against nullptr. Check lines: 'PlatformBase.h: 178', 'DataContainer.h: 339', 'DataContainer.h: 342'. DataContainer.h 339.

The analyzer indicates the use of prev as the second argument of the method MemoryCopyThe potential problem is that prev may matter nullptr, which is indicated by the corresponding check. But is the transfer really nullptr V MemoryCopy dangerous? What if this case is handled within the method itself? To answer these questions, let's look at the implementation MemoryCopy:

FORCE_INLINE static void MemoryCopy(void* dst, const void* src, uint64 size)
{
  memcpy(dst, src, static_cast<size_t>(size));
}

Now you can see that the value of the second parameter is directly passed to the function. memcpy without any prior check for inequality nullptrwhich creates the risk of occurrence undefined behavior.

A thoughtful reader may disagree with this, objecting: “Here, too, is additionally conveyed size (the number of bytes to copy), which means that if this parameter is 0, then no copying will be performed.”

Unfortunately, this is not quite true. Function documentation memcpy makes it clear that invalid pointers cannot be passed to it. Even if the amount of data being copied is zero:

“If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero”

Case 2

void Variant::SetAsset(Asset* asset)
{
  if (Type.Type != VariantType::Asset)
    SetType(VariantType(VariantType::Asset));
  if (AsAsset)
  {
    asset->OnUnloaded.Unbind<Variant,                           // <=
     &Variant::OnAssetUnloaded>(this);
    asset->RemoveReference(); // <=
  }
  AsAsset = asset;
  if (asset)
  {
    asset->AddReference();
    asset->OnUnloaded.Bind<Variant, &Variant::OnAssetUnloaded>(this);
  }
}

PVS-Studio warning:

V595 The 'asset' pointer was utilized before it was verified against nullptr. Check lines: 2706, 2709. Variant.cpp: 2706, 2709.

Very strange code. It seems that in the body of the first operator if the old value must be processed AsAsset before replacing it with a new one, as indicated by the condition of this if.

Also, the error in this place is indicated by the condition of the second if, in which the parameter check is performed assets on inequality nullptr. If it is expected that assets can be equal nullptrthen its dereference is inside the first if may lead to undefined behavior.

The most logical fix in this case is replacement assets on *AsAsset *inside the first if:

void Variant::SetAsset(Asset* asset)
{
  ....
  if (AsAsset)
  {
    AsAsset ->OnUnloaded.Unbind<Variant,                         // <=
     &Variant::OnAssetUnloaded>(this);
    AsAsset ->RemoveReference(); // <=
  }
  AsAsset = asset;
  if (asset)
  {
    asset->AddReference();
    asset->OnUnloaded.Bind<Variant, &Variant::OnAssetUnloaded>(this);
  }
}

Potential error in bit shifting

void StringUtils::ConvertUTF162UTF8(....)
{
  Array<uint32> unicode;
  ....
  const uint32 uni = unicode[j];
  const uint32 count =   uni <= 0x7FU ? 1 
                       : uni <= 0x7FFU ? 2 
                       : uni <= 0xFFFFU ? 3 
                       : uni <= 0x1FFFFFU ? 4 
                       : uni <= 0x3FFFFFFU ? 5 
                       : uni <= 0x7FFFFFFFU ? 6 
                       : 7;                       // <=       

  to[i++] = (char)(count <=   1 ? (byte)uni 
  : ((byte(0xFFU) << (8 - count)) | 
     byte(uni >> (6 * (count - 1)))));            // <=
  ....
}

PVS-Studio warning:

V610 Undefined behavior. Check the shift operator '>>'. The right operand ('(6 * (count – 1))' = [6..36]) is greater than or equal to the length in bits of the promoted left operand. StringUtilsBase.cpp. 253.

The analyzer draws attention to the possibility of getting an unexpected result with a bit shift in the expression uni >> (6 * (count – 1))This may happen because uni has a type int32. Shifting this value 32 bits or more to the right can result in undefined behavior.

The analyzer calculated that the largest possible step when bitwise shifting a variable is uni is 36. How did he determine this? Notice the ternary operator used to initialize the variable count:

const uint32 count =   uni <= 0x7FU ? 1 
                     : uni <= 0x7FFU ? 2 
                     : uni <= 0xFFFFU ? 3 
                     : uni <= 0x1FFFFFU ? 4 
                     : uni <= 0x3FFFFFFU ? 5 
                     : uni <= 0x7FFFFFFFU ? 6 
                     : 7;

It is clear that the maximum value that can be assigned to the variable is 7. Now let's substitute this value into the expression representing the shift step:

6 * (count - 1) = 6 * (7 – 1) = 36

Well, this time the analyzer was right. Case closed!

Conclusion

This concludes the article, I hope you enjoyed it 🙂

This is far from the first game engine whose source code we have checked. In case you want to read our other similar articles, I will leave links to them below.

Articles that analyze problems in C++ code:

Articles that analyze problems in C# code:

I hope you were also interested in the tool that was used to find the issues described above in the engine's vast code base.

Taking advantage of the moment, I would like to suggest that you get to know him by requesting a trial license on our website. After that, you will be able to try out all the functionality of the PVS-Studio analyzer for free during the trial period.

See you in the next articles!

If you would like to share this article with an English-speaking audience, please use the translation link: Andrey Moskalev. Flax Engine. Exploring game engine & analyzing its source code.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *