DEV Community

Anastasiia Vorobeva
Anastasiia Vorobeva

Posted on

Flax Engine. Exploring game engine & analyzing its source code

#gamedev #cpp #csharp #programming

"It's like Unreal and Unity had a baby," as the GameDev community has affectionately described the engine. Not only is that a cute way to describe the engine, but it's also quite spot-on. It's designed to be a "golden mean" between Unity and Unreal Engine.

Intro

Hello, dear readers! I'd like to introduce you to another interesting find from the endless GitHub expanses. Flax Engine is a full-fledged, multiplatform commercial game engine by Polish developers.

In this article, we'll briefly review the key features of the engine. Then, we'll look at some of the most interesting issues we found in its source code using PVS-Studio static code analyzer. It's a good approach to boost our skills in spotting bugs and typos in C# and C++ code.

PVS-Studio? What is it?

PVS-Studio is a static code analyzer that assists developers in searching for potential errors and vulnerabilities in C#, C, C++, and Java code without executing the program. PVS-Studio can analyze both separate files and the whole project.

The analyzer will generate a report with warnings that can be easily handled via a special panel in your favorite IDEs:

Want to know more about the analyzer? Check out our website. Now let's get back to the engine!

About Flax Engine

Dear readers, meet Flax Engine:

What makes Flax stand out among other engines?

  • Support for scripting in both C++ and C#. Developers can also leverage visual programming.
  • Support for .NET 8 and C# 12, so the latest language features are available, resulting in performance boost through vast runtime enhancements. If you'd like to revise these .NET features, you can check out our article. By the way, at the time of my writing, Unity still doesn't support new versions.
  • If you may think that the engine is more C#-oriented than C++, but that's not the case. The engine core is written in C++, which allows C++ developers to use its API directly. This enhances the performance and lets devs use some other APIs.
  • Considering it's an open-source engine, it offers a very robust toolkit. You can find the list of tools on a separate page.
  • You can fully use the engine for free as long as the project revenue doesn't exceed $250,000 per three months. If the revenue is higher, the engine cost will be equal to 4% of the profit you make from your Flax-driven projects.

Now that we've got to learn about the engine a bit, let's see what PVS-Studio will find in its source code. We'll discuss potential issues in the C# and C++ code of the latest engine version, which is 1.8.6512.2 as of this writing. Are you ready? Let's go!

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;
  }
}
Enter fullscreen mode Exit fullscreen mode

The 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 has detected two identical expressions bounded by the && operator. Most likely, one of them was written just accidentally. Is it just redundant code, or a sign of a more serious problem? For example, instead of the second repeated inequality, the developers might have wanted to write such a check:

typeNamespaces[typeNamespaces.Length - 1].Length != 0
Enter fullscreen mode Exit fullscreen mode

Here's 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;
  }
  ....
}
Enter fullscreen mode Exit fullscreen mode

The 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();
  }
}
Enter fullscreen mode Exit fullscreen mode

The 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.

This is a typical error that occurs when developers aren't careful enough when copy-pasting. The Hide method calls the _window.Show *method instead of *_window.Hide.

Array overrun 

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

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

The PVS-Studio warning:

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

The condition shows that the values array has only 4 items. Since the item indexing starts from 0, the array index of the latest item will be 3. However, in the last assignment, the index 4 is accessed, which will certainly lead to an exception being thrown.

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);
  ....
}
Enter fullscreen mode Exit fullscreen mode

The 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 has detected two identical conditions inside bound else if's. If type.Type has the int value, the code will be executed only in the body of the first one. The code in the second else if body will be unreachable. Since conversion to UInt16 type is in the body of the second else if, it'd be better to replace its condition with the following:

type.Type == typeof(ushort)
Enter fullscreen mode Exit fullscreen mode

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;
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

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

The analyzer has detected an always false if statement, so the then branch code will never be executed.

Note that as the result of the conditional statement, _dragOverMode gets a new value other than DragItemPositioning.None. The next if will be meaningless.

Developers might have written it intentionally, and just forgotten to delete redundant code. However, if it's an error, one of the approaches to fix it is to move the first condition from the beginning of the method to its end. The fixed code 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; 
}
Enter fullscreen mode Exit fullscreen mode

I don't think this fix may break the method logic, but only the engine developers can say it for sure.

Issue with thread synchronization 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;
}
Enter fullscreen mode Exit fullscreen mode

The 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.

The code has a potential vulnerability that can't be caught in the Debug configuration. What's the secret to this code beast's stealth? Such an issue may occur only when devs build the Release version because of the code optimization by a compiler. It can also happen for other reasons, like the version of .NET we're using and the number of processors in the system.

It's all the fault of endless looping of while that may occur because of the compiler caching the _window field value. Such an issue arises because the _window field value doesn't change inside the loop, and the compiler doesn't expect changes in other threads. The thing is that field is declared without the volatile keyword. You can read more about this on MSDN.

Errors in Flax Engine C++ code

Possible 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)
  ....
}
Enter fullscreen mode Exit fullscreen mode

The 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 MemoryCopy method. A potential problem is that prev may be nullptr, as indicated by the check. But is passing nullptr to MemoryCopy that dangerous? What if it is handled inside the function? To answer these questions, let's take a look at the MemoryCopy implementation:

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

We can now see that the second parameter value is directly passed to the memcpy function without any prior check for nullptr, which can lead to undefined behavior.

One may argue that this parameter also passes size—it's the number of bytes to be copied—so, if this parameter is 0, there will be no copying.

Unfortunately, that's not quite the case. If we read the memcry documentation, we'll understand that we can't pass invalid pointers to it, even if the number of the copied data is equal to 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);
  }
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

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

The code is rather strange. It seems that the first if statement should handle the old AsAsset value before it is replaced with a new one, as indicated by the if statement.

The second if statement, which checks the asset parameter for nullptr, also indicates an error in this place. If we expect that asset can be equal to nullptr, then dereferencing inside the first if can lead to undefined behavior.

The most logical fix here is to replace asset with 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);
  }
}
Enter fullscreen mode Exit fullscreen mode

Possible bitwise shift error 

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)))));            // <=
  ....
}
Enter fullscreen mode Exit fullscreen mode

The 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 has detected a potential issue with the bitwise shift in the uni >> (6 * (count - 1)) expression, which can lead to unexpected behavior. This may happen because uni is of type int32. The right value shift to 32 bit and more may cause undefined behavior.

The analyzer has detected that the largest possible shift during bitwise shifting the uni variable is 36. How did it detect that? Note the ternary operator used to initialize the count variable:

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

We can see that the maximum value that can be assigned to a variable is 7. Now let's substitute this value into the expression representing the shift:

6 * (count - 1) = 6 * (7 – 1) = 36
Enter fullscreen mode Exit fullscreen mode

Well, the analyzer was right here. Case closed!

Conclusion

This article wraps up here, hope you enjoyed it :)

That's not the first game engine we've checked. If you're interested in learning more, I've included links to some related articles below.

Here's a list of articles analyzing the C++-based engines:

Here's a list of articles analyzing the C#-based engines:

I hope you found the tool piqued your interest—the one that helped us throughout this article to search for issues in extensive code base of the engine.

I invite you to give it a try! Just request a trial license on our website. Evaluate all the PVS-Studio features for free during the trial period.

See you in future articles!

Top comments (1)

Collapse
 
jocomvag profile image
Jocom Vag •

I appreciate engines that have editors as a first-class priority, this stands out.

Read next

motuncoded profile image

How to create Subscribe to Newsletter with Reactjs and Supabase

Adijat Motunrayo Adeneye -

arjun98k profile image

Introduction to JavaScript

arjun -

mdarifulhaque profile image

2182. Construct String With Repeat Limit

MD ARIFUL HAQUE -

cryptosandy profile image

How to Debug Code Effectively: Tools and Techniques

Crypto.Andy (DEV) -