This article begins a trilogy about the Nau Engine game engine. In the first part, we focus on its functionality and explore three categories of common errors: copy-paste errors, logic flaws, and memory issues.
Nau Engine is a game engine designed to streamline the development and maintenance of games. Its developers say that their tool should be suitable for any task, any developer, and any platform.
1. For any task: Nau Engine has tools for every stage of game development, from initial creation to post-release support. This includes systems for analytics, error handling, and content updates.
2. For any developer: The engine is good for developers with different levels of experience. It provides intuitive tools and templates to help beginners easily get started, along with deeper customization options for advanced users.
3. For any platform: The engine allows seamless adaptation across various platforms, including PC, mobile, and web, making development more flexible and convenient.
A standout feature of Nau Engine is its adapted ECS (Entity Component System) library from the Dagor engine. This library not only delivers high performance but also brings back memories of classic games.
Overall, Nau Engine aims to help developers at all stages of game development, equipping them with the necessary tools and facilities to bring their ideas to life.
The following analysis is based on the repository code from the 020cbfe commit.
Fragment N1
NAU_ASSERT(irRequest.usage.access == req.usage.access
&& irRequest.usage.type == irRequest.usage.type);The PVS-Studio warning: V501 There are identical sub-expressions 'irRequest.usage.type' to the left and to the right of the '==' operator. irGraphBuilder.cpp 719
Let's look at the NAU_ASSERT macro, which checks conditions during code execution. Specifically, take a closer peek at the second comparison within this macro, irRequest.usage.type == irRequest.usage.type. If we write them separately, we notice that developers use the identical expression on the left and right sides of the comparison. As a result, the condition will always be true.
Most likely, developers have intended to compare irRequest.usage.type with req.usage.type to check if the types in two different objects are similar:
NAU_ASSERT(irRequest.usage.access == req.usage.access
&& irRequest.usage.type == req.usage.type);Fragment N2
bool VFXModFXInstance::deserialize(const nau::DataBlock* blk)
{
....
m_life.part_life_min = blk->getReal("lifeMin", 5.0f);
m_life.part_life_max = blk->getReal("lifeMin", 5.0f);
....
}The PVS-Studio warning: V656 Variables 'm_life.part_life_min', 'm_life.part_life_max' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'blk->getReal("lifeMin", 5.0f)' expression. Check lines: 94, 95. vfx_mod_fx_instance.cpp 95
In the VFXModFXInstance::deserialize function, there is an issue with the initialization of the m_life.part_life_min and m_life.part_life_max variables. Here we can clearly see the impact of copy-paste. Both variables receive their values by calling the same function using the same "lifeMin". Logically, the minimum and maximum instance lifetimes should differ. Therefore, we can assume that in the second case, developers should use"lifeMin" instead of "lifeMax":
m_life.part_life_min = blk->getReal("lifeMin", 5.0f);
m_life.part_life_max = blk->getReal("lifeMax", 5.0f);Fragment N3
Material* Material::clone() const
{
auto material = new (std::nothrow) Material();
if (material)
{
....
material->_textureSlots = material->_textureSlots;
material->_textureSlotIndex = material->_textureSlotIndex;
....
}
....
}The PVS-Studio warnings:
V570 The 'material->_textureSlots' variable is assigned to itself. CCMaterial.cpp 527
V570 The 'material->_textureSlotIndex' variable is assigned to itself. CCMaterial.cpp 528
Developers assign the _textureSlots and _textureSlotIndex variables to themselves, which makes no sense and may be an error. Judging by the name of the class member function (clone), developers should have copied the object states into a new one.
The fix could be like this:
material->_textureSlots = _textureSlots;
material->_textureSlotIndex = _textureSlotIndex;Fragment N4
template <typename T>
requires(std::is_enum_v<T>)
class TypedFlag
{
public:
using EnumType = T;
using ValueType = std::underlying_type_t<T>;
....
private:
ValueType m_value = 0;
....
friend TypedFlag<T> operator|(TypedFlag<T> value, TypedFlag<T> flags)
{
}
....
};The PVS-Studio warning: V591 Non-void function should return a value. typed_flag.h 145
The | operator overload for the TypedFlag class template doesn't have a return value, which can cause errors in the program logic. What makes this situation particularly interesting is the logic of the |= operator:
friend TypedFlag<T>& operator|=(TypedFlag<T>& value, T flag)
{
return value.set(flag);
}There are several ways to fix this issue.
Option N1. Implement operator functionality. They should return a result that aligns with the flag union logic.
For example:
return TypedFlag<T> { value }.set(flags);Option N2. Delete the operator. If the | operator is not needed, we can delete it to avoid confusion. Alternatively, we can declare the operator as deleted:
friend TypedFlag<T> operator|(TypedFlag<T> value, TypedFlag<T> flags) = delete;By implementing one of these fixes, the code becomes more predictable and easier to understand for other developers.
And here are more instances where warnings have been issued:
Fragment N5
EntityId EntityManager::createEntitySync(....)
{
....
if (EASTL_UNLIKELY(result != RequestResources::Loaded))
{
#if NAU_DEBUG
if (result == RequestResources::Loaded)
{
....
}
#endif
if (result == RequestResources::Error)
{
....
}
}
....
}The PVS-Studio warning: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 711, 714. entityManager2.cpp 711
There is a logic error in the code that may be misleading. Let's examine the details:
if (EASTL_UNLIKELY(result != RequestResources::Loaded)) condition: it checks that the result is not equal to RequestResources::Loaded. If this condition is true, the resources are not loaded.#if NAU_DEBUG block: the if (result == RequestResources::Loaded) check is always false, since we've already checked that result isn't equal to RequestResources::Loaded. This creates a contradiction and can be misleading.Fragment N6
IGenSave* create_async_writer(....)
{
AsyncWriterCB* ret = new AsyncWriterCB(buf_size);
if (!ret->open(fname, mode))
{
if (ret)
{
delete ret;
ret = nullptr;
}
}
return ret;
}The PVS-Studio warning: V668 There is no sense in testing the 'ret' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. asyncWrite.cpp 363
This code checks the ret pointer for nullptr after a call to new. This can be a source of errors since the ret pointer will always point to the allocated memory if the allocation was successful. If memory can't be allocated, an exception will be thrown and the code below won't be executed.
In addition, the ret pointer is dereferenced before the check, which makes the if (ret) check less meaningful. If ret was really equal to nullptr, the program would have already crashed while trying to dereference.
Here are some other cases:
Fragment N7
void Properties::skipWhiteSpace()
{
signed char c;
do
{
c = readChar();
} while (isspace(c) && c != EOF);
....
if (c != EOF)
{
....
}
}The PVS-Studio warnings:
V739 EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. CCProperties.cpp 464
V739 EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. CCProperties.cpp 468
The Properties::skipWhiteSpace member function uses a variable of the signed char type to store the character read by Properties::readChar. Based on the comment, the last one simulates the behavior of std::getchar:
//
// Stream simulation
//
signed char Properties::readChar()
{
if (eof())
return EOF;
return _data->_bytes[(*_dataIdx)++];
}This leads to two key issues:
Error N1. The std::getchar function returns the int values for a reason—it returns a character (1 byte) or EOF to indicate an error. EOF is a negative constant, usually -1. Before converting the result of std::getchar to a character type, developers need to handle the case with EOF:
if (int res = std::getchar(); res != EOF)
{
char ch = res;
// your logic with character
}Why is it important? Developers who use Extended ASCII Codes sometimes encounter an error when one of the alphabet characters is incorrectly handled by programs.
The correct implementation of the Properties::readChar function should look like this:
//
// Stream simulation
//
int Properties::readChar()
{
if (eof())
return EOF;
return _data->_bytes[(*_dataIdx)++];
}Error N2. The Properties::skipWhiteSpace function after fixing Properties::readChar should also work with the int type, ensuring proper EOF handling:
void Properties::skipWhiteSpace()
{
int c;
do
{
c = readChar();
}
while (c != EOF && isspace(c));
....
if (c != EOF)
{
// Now we can cast 'c' to 'signed char'
signed char ch = c;
....
}
}Fragment N8
void Sweep::EdgeEvent(....)
{
....
if (o1 == COLLINEAR) {
if( triangle->Contains(&eq, p1)) {
....
} else {
std::runtime_error("EdgeEvent - collinear points not supported");
assert(0);
}
return;
}
....
if (o2 == COLLINEAR) {
if (triangle->Contains(&eq, p2)){
....
} else {
std::runtime_error("EdgeEvent - collinear points not supported");
assert(0);
}
return;
}
....
}The PVS-Studio warnings:
V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); sweep.cc 123
V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); sweep.cc 140
The Sweep::EdgeEvent member function creates std::runtime_error exception objects when collinear points are detected. However, exceptions aren't thrown. As a result, the program continues execution without handling the error, which can cause incorrect behavior.
Here is the fixed code:
throw std::runtime_error("EdgeEvent - collinear points not supported");Fragment N9
std::size_t UniformLocation::operator()(const UniformLocation &uniform) const
{
return (((size_t) shaderStage) & 0xF)
|((size_t)(location[0] << 4))
|((size_t)(location[1] << 8));
}The PVS-Studio warnings:
V1028 Possible overflow. Consider casting operands of the 'location[0] << 4' operator to the 'size_t' type, not the result. Types.cpp 40
V1028 Possible overflow. Consider casting operands of the 'location[1] << 8' operator to the 'size_t' type, not the result. Types.cpp 40
The UniformLocation::operator() operator performs bitwise operations on values obtained from the shaderStage and location variables to return a unique identifier.
The problem is that shift operations (<<) may cause an overflow if the values of location[0] or location[1] are too large. To avoid this, developers could cast location[0] and location[1] to the size_t type before the shift operation:
std::size_t UniformLocation::operator()(const UniformLocation &uniform) const
{
return (((size_t) shaderStage) & 0xF)
|((size_t)(location[0]) << 4)
|((size_t)(location[1]) << 8);
}Fragment N10
char** m_memBlock = nullptr;
template<class Type>
class ThreadLocalValue final
{
....
private:
void resizeLines(size_t sizeReq)
{
....
auto newSize = sizeReq + 1;
....
m_memBlock = static_cast<char**>(realloc(m_memBlock,
sizeof(char*) * newSize));
....
}
....
};The PVS-Studio warning: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_memBlock' is lost. Consider assigning realloc() to a temporary pointer. thread_local_value.h 235
In this code, m_memBlock is an array of char pointers, and the resizeLines function resizes it using realloc.
If realloc successfully allocates memory, it returns either the same pointer or a new one having moved the data. However, if the memory allocation fails, realloc returns nullptr and the previous value of m_memBlock is lost.
To avoid memory leaks, developers should first assign the result of realloc to a temporary variable, check it for nullptr, and only then update m_memBlock.
Here are some other cases:
Fragment N11
IAssetContainerLoader* loader = nullptr;
const auto importSettingsProviders = ....;
RuntimeReadonlyDictionary::Ptr importSettings;
for (const auto& importSettingsProvider : importSettingsProviders)
{
if (importSettings =
importSettingsProvider->getAssetImportSettings(containerPath,
*loader);
importSettings)
{
break;
}
}The PVS-Studio warning: V522 Dereferencing of the null pointer 'loader' might take place. asset_file_content_provider.cpp 34
At the beginning of the code, the loader pointer is initialized with nullptr. Then, within a loop, the getAssetImportSettings member function is called, passing the dereferenced loader pointer as an argument. Strangely, it's not modified in any way between these lines. As a result, we catch guaranteed undefined behavior.
Here's another case:
Fragment N12
struct DelayedEntityCreationChunk
{
....
DelayedEntityCreationChunk(uint16_t cap) : capacity(cap)
{
queue = (DelayedEntityCreation*)
malloc(capacity * sizeof(DelayedEntityCreation));
}
....
};The PVS-Studio warning: V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. entityManager.h 1391
In this fragment, the analyzer warns that malloc is used to allocate memory for an array of class objects that have constructors and destructors. Generally, it'd be better to use other ways of creating objects on the heap. However, it's a false positive because object is created later via the emplace_back function—roughly the same thing happens in the vector.
The curious reader may question, "Why even mention this code fragment in the article?" We've planned to leave it out—until something else in this class caught our attention:
struct DelayedEntityCreationChunk
{
....
DelayedEntityCreation* queue = nullptr;
uint16_t readFrom = 0, writeTo = 0, capacity;
DelayedEntityCreationChunk(uint16_t cap) :
capacity(cap)
{
queue = (DelayedEntityCreation*)
malloc(capacity * sizeof(DelayedEntityCreation));
}
~DelayedEntityCreationChunk()
{
for(auto i = begin(), e = end(); i != e; ++i)
i->~DelayedEntityCreation();
free(queue);
}
DelayedEntityCreationChunk(const DelayedEntityCreationChunk&) = delete;
DelayedEntityCreationChunk&
operator=(const DelayedEntityCreationChunk&) = delete;
DelayedEntityCreationChunk(DelayedEntityCreationChunk&& a)
{
memcpy(this, &a, sizeof(DelayedEntityCreationChunk));
memset(&a, 0, sizeof(DelayedEntityCreationChunk));
}
DelayedEntityCreationChunk& operator=(DelayedEntityCreationChunk&& a)
{
alignas(DelayedEntityCreationChunk)
char buf[sizeof(DelayedEntityCreationChunk)];
memcpy(buf, this, sizeof(DelayedEntityCreationChunk));
memcpy(this, &a, sizeof(DelayedEntityCreationChunk));
memcpy(&a, buf, sizeof(DelayedEntityCreationChunk));
return *this;
}
....
template <typename... Args>
bool emplace_back(EntityId eid, Args &&...args)
{
DAECS_EXT_ASSERT(!full());
new (queue + (writeTo++)) DelayedEntityCreation(eid,
eastl::forward<Args>(args)...);
return full();
}
....
};Here are class properties:
noexcept, which is not good in some scenarios.Notice that the objects of this class—memcpy and memset—are actively used in the constructor and move operator. The standard clearly regulates their behavior only for trivially copyable types, otherwise it may be undefined.
Developers could fix code like this:
struct DelayedEntityCreationChunk
{
....
DelayedEntityCreation* queue = nullptr;
uint16_t readFrom = 0, writeTo = 0, capacity;
....
public:
DelayedEntityCreationChunk(DelayedEntityCreationChunk &&a) noexcept
: queue { std::exchange(a.queue, {}) }
, readFrom { std::exchange(a.readFrom, {}) }
, writeTo { std::exchange(a.writeTo, {}) }
, capacity { std::exchange(a.capacity, {}) }
{
}
void swap(DelayedEntityCreationChunk &other) noexcept
{
auto lhs = std::tie(queue, readFrom, writeTo, capacity);
auto rhs = std::tie(other.queue, other.readFrom,
other.writeTo, other.capacity);
std::swap(lhs, rhs);
}
DelayedEntityCreationChunk&
operator=(DelayedEntityCreationChunk &&a) noexcept
{
if (this == std::addressof(a)) return *this;
auto tmp = std::move(a);
swap(tmp);
return *this;
}
....
};In the first part of our deep dive into Nau Engine codebase, we've analyzed its key features, focusing on copy-paste errors, logic flaws, and memory issues. These issues can become serious roadblocks for developers aiming to create engaging games. While Nau Engine offers many promising features, the detected flaws could significantly impact performance and the overall gaming experience.
In the next part of the trilogy, we'll be looking for ways to optimize and enhance the engine's performance.
Stay tuned, and thanks for reading!