Skip to content

Fix debug text rendering #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix debug text rendering #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c7354f
Fix debug text rendering
kullingk Jun 6, 2025
2516b9d
Refactorings
kullingk Jun 11, 2025
f43f4a6
Refactorings
kullingk Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions samples/00_HelloWorld/HelloWorld.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ DECL_OSRE_LOG_MODULE(HelloWorldApp)
/// @brief This is the simple HelloWorld-App to get familiar with the base concept of the
/// OSRE engine.
//-------------------------------------------------------------------------------------------------
class HelloWorldApp : public App::AppBase {
class HelloWorldApp final : public App::AppBase {
/// The transform block, contains the model-, view- and projection-matrix.
TransformMatrixBlock mTransformMatrix;
/// The entity to render.
Expand Down Expand Up @@ -115,11 +115,12 @@ int main( int argc, char *argv[] ) {
if ( !myApp.create() ) {
return 1;
}
while ( myApp.handleEvents() ) {
while (myApp.handleEvents()) {
myApp.update();
myApp.requestNextFrame();
}
myApp.destroy();

return 0;
}

2 changes: 1 addition & 1 deletion src/Editor/src/RenderView/MainRenderView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ void MainRenderView::createEditorElements(RenderComponent *rc) {
mEditorElements.add(axis);
}

void MainRenderView::render( RenderBackendService *rbSrv, glm::mat4 model ) {
void MainRenderView::render(RenderBackendService *rbSrv, glm::mat4 model) {
rbSrv->beginPass(RenderPass::getPassNameById(RenderPassId));
rbSrv->beginRenderBatch("b1");

Expand Down
5 changes: 5 additions & 0 deletions src/Engine/App/AppBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,11 @@
delete mKeyboardEvListener;
mKeyboardEvListener = nullptr;

for (size_t i = 0; i < mScenes.size(); ++i) {
// delete mScenes[i];

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

Copilot Autofix

AI 4 months ago

To fix the problem, we need to either remove the commented-out code or reinstate it if it is still relevant. In this case, the line delete mScenes[i]; appears to be part of a cleanup process in the onDestroy method, where other dynamically allocated resources are being deleted. If this line is necessary for proper cleanup, it should be reinstated. Otherwise, it should be removed entirely to avoid confusion.

Given the context of the onDestroy method, it seems likely that the line was commented out by mistake or during debugging. Reinstating the line ensures that the mScenes objects are properly deleted, preventing potential memory leaks.

Suggested changeset 1
src/Engine/App/AppBase.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Engine/App/AppBase.cpp b/src/Engine/App/AppBase.cpp
--- a/src/Engine/App/AppBase.cpp
+++ b/src/Engine/App/AppBase.cpp
@@ -399,3 +399,3 @@
     for (size_t i = 0; i < mScenes.size(); ++i) {
-     //   delete mScenes[i];
+        delete mScenes[i];
     }
EOF
@@ -399,3 +399,3 @@
for (size_t i = 0; i < mScenes.size(); ++i) {
// delete mScenes[i];
delete mScenes[i];
}
Copilot is powered by AI and may make mistakes. Always verify output.
}
mScenes.clear();

Comment on lines +399 to +403
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Scene objects are leaked – heap-allocated Scene instances are never deleted

onCreate() allocates scenes with new Scene(...) and pushes raw pointers into mScenes.
In onDestroy() the loop merely iterates but the delete statement is commented out, so every scene (and its GPU/stateful resources) survives application shutdown.

Minimal corrective patch:

-    for (size_t i = 0; i < mScenes.size(); ++i) {
-     //   delete mScenes[i];
-    }
+    for (Scene *scene : mScenes) {
+        delete scene;
+    }

Preferable long-term fix: change mScenes to Vector<std::unique_ptr<Scene>> (or std::vector) to enforce RAII and avoid manual memory management altogether.

Please confirm that no other subsystem assumes ownership of these pointers before applying.

🏁 Script executed:

#!/bin/bash
set -e
echo "=== mScenes Declaration in AppBase.cpp ==="
rg -n "mScenes" -n src/Engine/App/AppBase.cpp || true

echo "=== mScenes Declaration in AppBase.h ==="
rg -n "mScenes" -n src/Engine/App/AppBase.h || true

echo "=== Scene Allocations in onCreate methods ==="
rg -n "new Scene" -n src/Engine/App || true

echo "=== Any delete calls for mScenes elements ==="
rg -n "delete.*mScenes" -n src || true
rg -n "delete.*Scene" -n src/Engine/App/AppBase.cpp || true

Length of output: 1227

Delete heap-allocated Scene objects in onDestroy to avoid memory leaks

In src/Engine/App/AppBase.cpp (lines 399–403), the loop that should delete each Scene* is commented out, so every Scene allocated in onCreate() survives shutdown:

-    for (size_t i = 0; i < mScenes.size(); ++i) {
-     //   delete mScenes[i];
-    }
-    mScenes.clear();
+    for (Scene* scene : mScenes) {
+        delete scene;
+    }
+    mScenes.clear();

• Long-term, change
cppcore::TArray<Scene*> mScenes; →
cppcore::TArray<std::unique_ptr<Scene>> mScenes;
(or std::vector<std::unique_ptr<Scene>>) to enforce RAII and remove manual deletes.
• Confirm no other subsystem takes ownership of these raw pointers before merging.
• Bonus: check src/Engine/App/AssimpWrapper.cpp:183 (mAssetContext.mWorld = new Scene("scene");) to ensure it’s deleted appropriately.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (size_t i = 0; i < mScenes.size(); ++i) {
// delete mScenes[i];
}
mScenes.clear();
// in AppBase.cpp, onDestroy (around lines 399–403):
- for (size_t i = 0; i < mScenes.size(); ++i) {
- // delete mScenes[i];
- }
- mScenes.clear();
+ for (Scene* scene : mScenes) {
+ delete scene;
+ }
+ mScenes.clear();
🤖 Prompt for AI Agents 
In src/Engine/App/AppBase.cpp around lines 399 to 403, the loop intended to
delete heap-allocated Scene objects is commented out, causing memory leaks. To
fix this, uncomment the delete statement inside the loop to properly delete each
Scene pointer before clearing mScenes. For a long-term fix, refactor mScenes to
hold std::unique_ptr<Scene> instead of raw pointers to automate memory
management, but first verify no other code assumes ownership of these raw
pointers.

osre_debug(Tag, "Set application state to destroyed.");
mAppState = State::Destroyed;

Expand Down
2 changes: 1 addition & 1 deletion src/Engine/App/AppBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ class OSRE_EXPORT AppBase {
Common::Environment *mEnvironment = nullptr;
Properties::Settings *mSettings = nullptr;
Platform::PlatformInterface *mPlatformInterface = nullptr;
Platform::AbstractTimer *mTimer;
Platform::AbstractTimer *mTimer = nullptr;
RenderBackend::RenderBackendService *mRbService = nullptr;
cppcore::TArray<Scene*> mScenes;
Scene *mActiveScene = nullptr;
Expand Down
14 changes: 7 additions & 7 deletions src/Engine/App/CameraComponent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ using namespace ::glm;

DECL_OSRE_LOG_MODULE(Camera)

static const String CameraModelName[3] = {
"Perspective ",
"Orthogonal",
"Invalid"
};

static constexpr f32 DefaultNear = 0.001f;
static constexpr f32 DefaultFar = 1000.0f;
static constexpr f32 DefaultAspectRatio = 1.0f;
Expand Down Expand Up @@ -121,7 +115,13 @@ void CameraComponent::setProjectionMode(f32 fov, f32 aspectRatio, f32 nearPlane,
}

void CameraComponent::setCameraModel(CameraModel cm) {
osre_trace(Tag, "Set camera model to " + CameraModelName[static_cast<size_t>(cm)]);
static constexpr c8 *CameraModelName[3] = {
"Perspective ",
"Orthogonal",
"Invalid"
};

osre_trace(Tag, "Set camera model to " + String(CameraModelName[static_cast<size_t>(cm)]));

mCameraModel = cm;
}
Expand Down
18 changes: 10 additions & 8 deletions src/Engine/App/ServiceProvider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,27 @@
using namespace ::OSRE::RenderBackend;
using namespace ::OSRE::Common;

ServiceProvider *ServiceProvider::s_instance = nullptr;
ServiceProvider *ServiceProvider::sInstance = nullptr;

ServiceProvider *ServiceProvider::create() {
if (nullptr == s_instance) {
s_instance = new ServiceProvider;
if (nullptr == sInstance) {
sInstance = new ServiceProvider;
}

return s_instance;
return sInstance;
}

void ServiceProvider::destroy() {
if (nullptr != s_instance) {
delete s_instance;
s_instance = nullptr;
if (nullptr != sInstance) {
delete sInstance;
sInstance = nullptr;
}
}

void ServiceProvider::setService(ServiceType type, Common::AbstractService *service) {
s_instance->mServiceArray[static_cast<size_t>(type)] = service;
if (type == ServiceType::Invalid || type == ServiceType::Count) {
}
Comment on lines +50 to +51

Check notice

Code scanning / CodeQL

Futile conditional Note

If-statement with an empty then-branch and no else-branch.

Copilot Autofix

AI 4 months ago

To fix the issue, we need to address the empty then-branch of the if statement. There are two possible approaches:

  1. Remove the if statement entirely if the condition is unnecessary and does not affect the program's functionality.
  2. Implement the then-branch to handle the case when the condition is true. For example, we could log an error, throw an exception, or return early to indicate that invalid service types are not allowed.

In this case, the best approach is to implement the then-branch to handle invalid service types explicitly. This ensures the code is robust and avoids potential misuse of the setService method. We will add a logging statement or throw an exception to indicate that the provided service type is invalid.

Suggested changeset 1
src/Engine/App/ServiceProvider.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Engine/App/ServiceProvider.cpp b/src/Engine/App/ServiceProvider.cpp
--- a/src/Engine/App/ServiceProvider.cpp
+++ b/src/Engine/App/ServiceProvider.cpp
@@ -23,2 +23,3 @@
 #include "App/ServiceProvider.h"
+#include <stdexcept>
 #include "RenderBackend/RenderBackendService.h"
@@ -50,2 +51,3 @@
     if (type == ServiceType::Invalid || type == ServiceType::Count) {
+        throw std::invalid_argument("Invalid service type provided.");
     }
EOF
@@ -23,2 +23,3 @@
#include "App/ServiceProvider.h"
#include <stdexcept>
#include "RenderBackend/RenderBackendService.h"
@@ -50,2 +51,3 @@
if (type == ServiceType::Invalid || type == ServiceType::Count) {
throw std::invalid_argument("Invalid service type provided.");
}
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines +50 to +51

Check notice

Code scanning / CodeQL

Empty branch of conditional Note

Empty block without comment.

Copilot Autofix

AI 4 months ago

To fix the issue, we need to address the empty block on line 51. The best approach depends on the intent of the code:

  1. If the condition type == ServiceType::Invalid || type == ServiceType::Count is not supposed to trigger any action, we should add a comment explaining why the block is intentionally left empty.
  2. If the condition is meant to handle a specific case, we should implement the appropriate logic within the block.

In this case, we will assume the first option (no action is required for the condition) and add a comment to clarify the intent. This ensures the code is more maintainable and avoids confusion for future developers.

Suggested changeset 1
src/Engine/App/ServiceProvider.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Engine/App/ServiceProvider.cpp b/src/Engine/App/ServiceProvider.cpp
--- a/src/Engine/App/ServiceProvider.cpp
+++ b/src/Engine/App/ServiceProvider.cpp
@@ -50,2 +50,3 @@
     if (type == ServiceType::Invalid || type == ServiceType::Count) {
+        // No action required for invalid or count service types.
     }
EOF
@@ -50,2 +50,3 @@
if (type == ServiceType::Invalid || type == ServiceType::Count) {
// No action required for invalid or count service types.
}
Copilot is powered by AI and may make mistakes. Always verify output.
sInstance->mServiceArray[static_cast<size_t>(type)] = service;
}
Comment on lines +50 to 53
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Guard against Invalid / Count to avoid out-of-bounds access

The check for ServiceType::Invalid and ServiceType::Count is empty, but the code immediately writes to mServiceArray with the (still) invalid index.
ServiceType::Count is one-past-the-end, so this is an off-by-one write; Invalid is typically 0 or -1, also unintended. Both will corrupt memory in release builds and trip assertions in debug builds.

-    if (type == ServiceType::Invalid || type == ServiceType::Count) {
-    }
-    sInstance->mServiceArray[static_cast<size_t>(type)] = service;
+    if (type == ServiceType::Invalid || type == ServiceType::Count) {
+        OSRE_ASSERT(false, "Attempt to register an invalid service type");
+        return;
+    }
+
+    OSRE_ASSERT(nullptr != sInstance, "ServiceProvider::create() was never called");
+    sInstance->mServiceArray[static_cast<size_t>(type)] = service;

Fail-fast (assert / early-return) prevents undefined behaviour and makes misuse obvious in tests.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (type == ServiceType::Invalid || type == ServiceType::Count) {
}
sInstance->mServiceArray[static_cast<size_t>(type)] = service;
}
if (type == ServiceType::Invalid || type == ServiceType::Count) {
OSRE_ASSERT(false, "Attempt to register an invalid service type");
return;
}
OSRE_ASSERT(nullptr != sInstance, "ServiceProvider::create() was never called");
sInstance->mServiceArray[static_cast<size_t>(type)] = service;
}
🤖 Prompt for AI Agents 
In src/Engine/App/ServiceProvider.cpp around lines 50 to 53, the code checks for
ServiceType::Invalid and ServiceType::Count but does not prevent using these
invalid indices to write to mServiceArray, causing potential out-of-bounds
memory corruption. Fix this by adding a fail-fast mechanism such as an assertion
or an early return inside the if block to prevent writing to mServiceArray when
type is Invalid or Count, ensuring no invalid index is used.


ServiceProvider::ServiceProvider() :
Expand Down
17 changes: 5 additions & 12 deletions src/Engine/App/ServiceProvider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,9 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
#pragma once

#include "App/AppCommon.h"
#include "Common/AbstractService.h"

namespace OSRE {

// Forward declarations ---------------------------------------------------------------------------
namespace Common {
class AbstractService;
}

namespace App {
namespace OSRE::App {

/// @brief
enum class ServiceType {
Expand All @@ -58,21 +52,20 @@ class OSRE_EXPORT ServiceProvider {
if (type == ServiceType::Invalid || type == ServiceType::Count) {
return nullptr;
}
return (T *) s_instance->mServiceArray[static_cast<size_t>(type)];
return (T *) sInstance->mServiceArray[static_cast<size_t>(type)];
}

private:
explicit ServiceProvider();
~ServiceProvider() = default;

private:
static ServiceProvider *s_instance;
static ServiceProvider *sInstance;

using ServiceArray = cppcore::TArray<Common::AbstractService*>;
ServiceArray mServiceArray;
};

} // Namespace App
} // Namespace OSRE
} // Namespace OSRE::App


2 changes: 1 addition & 1 deletion src/Engine/Common/AbstractService.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public: \
/// @brief Helper macro to define a singleton and implement all needed methods.
//-------------------------------------------------------------------------------------------------
#define IMPLEMENT_SINGLETON( type ) \
type * type::s_instance( nullptr );
type * type::s_instance(nullptr);

//-------------------------------------------------------------------------------------------------
/// @ingroup Engine
Expand Down
12 changes: 6 additions & 6 deletions src/Engine/Common/Tokenizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,18 @@ namespace OSRE::Common {

using namespace ::cppcore;

static const String EmptyToken = "";

size_t Tokenizer::getNumOfToken() const {
return m_Token.size();
}

const String &Tokenizer::getTokenAt( ui32 index ) const {
if ( m_Token.isEmpty() || m_Token.size() >= index ) {
return EmptyToken;
bool Tokenizer::getTokenAt(size_t index, String &token) const {
if ( m_Token.isEmpty() || m_Token.size() >= index ) {
token = "";
return false;
}

return m_Token[ index ];
token = m_Token[index];
return true;
}
Comment on lines +33 to 41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Off-by-one / reversed bound check causes the method to always report failure

m_Token.size() >= index is true for every valid index (e.g. size = 10, index = 3 ⇒ 10 ≥ 3), so the function exits early and returns false almost all the time.
The comparison has to be index >= m_Token.size().

-    if ( m_Token.isEmpty() || m_Token.size() >= index ) {        
-        token = "";
-        return false;
-    }
+    if (m_Token.isEmpty() || index >= m_Token.size()) {
+        token = "";
+        return false;
+    }

Without this fix every caller will assume the token was not found, breaking all parsing that relies on the tokenizer.
Please patch before merging.

🤖 Prompt for AI Agents 
In src/Engine/Common/Tokenizer.cpp between lines 33 and 41, the condition
checking if the index is out of bounds is reversed, causing the function to
incorrectly return false for valid indices. Change the condition from
'm_Token.size() >= index' to 'index >= m_Token.size()' to correctly detect
out-of-range indices and allow valid tokens to be returned properly.


bool Tokenizer::isEmpty() const {
Expand Down
8 changes: 4 additions & 4 deletions src/Engine/Common/Tokenizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ class OSRE_EXPORT Tokenizer {
size_t getNumOfToken() const;

/// @brief Returns the token at the given index.
/// @param idx [in] The given index, must be between ( 0 ... numToken-1 ).
/// @return The token at the given index. If the index is out of range an empty string will be
/// returned.
const String &getTokenAt( ui32 idx ) const;
/// @param index [in] The given index, must be between (0 ... numToken-1).
/// @param token [out] the token at the requested index.
/// @return true, if any else false.
bool getTokenAt(size_t index, String &token) const;

/// @brief Returns true, if the stored list of token is empty.
/// @return true, if no token are store, false if tokens are stored.
Expand Down
12 changes: 5 additions & 7 deletions src/Engine/Platform/win32/Win32DbgLogStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,15 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
#include "Common/Logger.h"
#include "Platform/Windows/MinWindows.h"

namespace OSRE {
namespace Platform {
namespace OSRE::Platform {

//-------------------------------------------------------------------------------------------------
/// @ingroup Engine
///
/// @brief This class implements a log-stream, which will be visible in the Windows-Debugger
/// output window.
//-------------------------------------------------------------------------------------------------
class Win32DbgLogStream : public Common::AbstractLogStream {
class Win32DbgLogStream final : public Common::AbstractLogStream {
public:
/// @brief The default class constructor.
Win32DbgLogStream() = default;
Expand All @@ -44,14 +43,13 @@ class Win32DbgLogStream : public Common::AbstractLogStream {

/// Writes the message into the debug output buffer.
/// @param[in] msg Message to log.
void write(const String &msg);
void write(const String &msg) override;
};

inline void Win32DbgLogStream::write(const String &msg) {
if (!msg.empty()) {
::OutputDebugString(msg.c_str());
OutputDebugString(msg.c_str());
}
}

} // Namespace Platform
} // Namespace OSRE
} // Namespace OSRE::Platform
4 changes: 2 additions & 2 deletions src/Engine/RenderBackend/2D/CanvasRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ void CanvasRenderer::render(RenderBackendService *rbSrv) {
}

const ui32 lastIndex = mMesh->getLastIndex();
renumberIndices(dc, numVertices);
renumberIndices(dc.Indices, dc.NumIndices, numVertices);

mMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert));
mMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16));
Expand All @@ -239,7 +239,7 @@ void CanvasRenderer::render(RenderBackendService *rbSrv) {
}

const ui32 lastIndex = mText->getLastIndex();
renumberIndices(dc, numVertices);
renumberIndices(dc.Indices, dc.NumIndices, numVertices);

mText->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert));
mText->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16));
Expand Down
43 changes: 21 additions & 22 deletions src/Engine/RenderBackend/DbgRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,27 @@
#include "RenderBackend/DbgRenderer.h"
#include "RenderBackend/MaterialBuilder.h"
#include "RenderBackend/MeshBuilder.h"
#include "Properties/Settings.h"

namespace OSRE::RenderBackend {

using namespace ::OSRE::Common;
using namespace ::OSRE::Properties;

DbgRenderer *DbgRenderer::sInstance = nullptr;

DbgRenderer::DbgRenderer(RenderBackendService *rbSrv) :
mRbSrv(rbSrv),
mDebugMesh(nullptr),
mLastIndex(0) {
osre_assert(nullptr != mRbSrv);
auto *settings = rbSrv->getSettings();
i32 x = settings->getInt(Settings::WinX);
i32 y = settings->getInt(Settings::WinY);
i32 w = settings->getInt(Settings::WinWidth);
i32 h = settings->getInt(Settings::WinHeight);

mCanvasRenderer = new CanvasRenderer(2, x, y, w, h);

Check warning

Code scanning / CodeQL

Resource not released in destructor Warning

Resource mCanvasRenderer is acquired by class DbgRenderer but not released anywhere in this class.

Copilot Autofix

AI 4 months ago

To fix the issue, the DbgRenderer destructor should release the mCanvasRenderer resource by calling delete on it. This ensures that the resource is properly cleaned up when the DbgRenderer instance is destroyed. Additionally, the clear() function should be reviewed to ensure it does not redundantly release mCanvasRenderer. If clear() already handles this, the destructor should not duplicate the cleanup.

Steps:

  1. Add a delete mCanvasRenderer; statement in the destructor of DbgRenderer.
  2. Ensure that mCanvasRenderer is set to nullptr after deletion to avoid dangling pointers.
  3. Review the clear() function to confirm it does not already release mCanvasRenderer.
Suggested changeset 1
src/Engine/RenderBackend/DbgRenderer.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Engine/RenderBackend/DbgRenderer.cpp b/src/Engine/RenderBackend/DbgRenderer.cpp
--- a/src/Engine/RenderBackend/DbgRenderer.cpp
+++ b/src/Engine/RenderBackend/DbgRenderer.cpp
@@ -56,2 +56,6 @@
     clear();
+    if (mCanvasRenderer != nullptr) {
+        delete mCanvasRenderer;
+        mCanvasRenderer = nullptr;
+    }
 }
EOF
@@ -56,2 +56,6 @@
clear();
if (mCanvasRenderer != nullptr) {
delete mCanvasRenderer;
mCanvasRenderer = nullptr;
}
}
Copilot is powered by AI and may make mistakes. Always verify output.
mCanvasRenderer->create();
Comment on lines +51 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null check after allocation and ensure proper cleanup.

The CanvasRenderer allocation should be checked for success, and it needs to be properly deleted in the destructor to prevent memory leaks.

Consider adding:

 mCanvasRenderer = new CanvasRenderer(2, x, y, w, h);
+if (mCanvasRenderer != nullptr) {
     mCanvasRenderer->create();
+}

Also, update the destructor:

 DbgRenderer::~DbgRenderer() {
     clear();
+    delete mCanvasRenderer;
+    mCanvasRenderer = nullptr;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mCanvasRenderer = new CanvasRenderer(2, x, y, w, h);
mCanvasRenderer->create();
// In the constructor (around lines 51–52)
mCanvasRenderer = new CanvasRenderer(2, x, y, w, h);
if (mCanvasRenderer != nullptr) {
mCanvasRenderer->create();
}
// In the destructor
DbgRenderer::~DbgRenderer() {
clear();
delete mCanvasRenderer;
mCanvasRenderer = nullptr;
}
🤖 Prompt for AI Agents 
In src/Engine/RenderBackend/DbgRenderer.cpp around lines 51 to 52, add a null
check after allocating mCanvasRenderer to ensure it was successfully created
before calling create(). Additionally, update the class destructor to properly
delete mCanvasRenderer to prevent memory leaks by releasing the allocated
memory.

}

DbgRenderer::~DbgRenderer() {
Expand Down Expand Up @@ -86,26 +95,16 @@
return;
}

DebugText *foundDebugText = getInstance()->getDebugText(id);
mRbSrv->beginPass(RenderPass::getPassNameById(DbgPassId));
mRbSrv->beginRenderBatch(DbgRenderer::getDebugRenderBatchName());
glm::mat4 projection = glm::ortho(0.0f, 800.0f, 0.0f, 600.0f);
mRbSrv->setMatrix(MatrixType::Projection, projection);
if (foundDebugText == nullptr) {
MeshBuilder meshBuilder;
meshBuilder.allocTextBox(static_cast<f32>(x), static_cast<f32>(y), 20, text, BufferAccessType::ReadWrite);
DebugText *entry = new DebugText;
entry->mesh = meshBuilder.getMesh();
entry->mesh->setId(id);
getInstance()->mDebugTextMeshes.add(entry);
mRbSrv->setMatrix(MatrixType::Projection, projection);
mRbSrv->addMesh(entry->mesh, 0);
} else if (foundDebugText->text != text) {
MeshBuilder::updateTextBox(foundDebugText->mesh, 10, text);
mRbSrv->updateMesh(foundDebugText->mesh);
DebugText *debugText = getInstance()->getDebugText(id);
if (debugText == nullptr) {
debugText = new DebugText;
debugText->id = id;
debugText->text = text;
mDebugTextMeshes.add(debugText);
} else if (debugText->text != text) {
debugText->text = text;
}
mRbSrv->endRenderBatch();
mRbSrv->endPass();
mCanvasRenderer->drawText(x, y, 10, text);
}

static constexpr size_t NumIndices = 24;
Expand Down Expand Up @@ -233,7 +232,7 @@

DebugText *found = nullptr;
for (size_t i = 0; i < mDebugTextMeshes.size(); ++i) {
if (mDebugTextMeshes[i]->mesh->getId() == id) {
if (mDebugTextMeshes[i]->id == id) {
found = mDebugTextMeshes[i];
break;
}
Expand All @@ -242,4 +241,4 @@
return found;
}

} // namespace OSRE::RenderBackend
} // namespace OSRE::RenderBackend
22 changes: 10 additions & 12 deletions src/Engine/RenderBackend/DbgRenderer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
#include <cppcore/Container/TArray.h>
#include <cppcore/Container/THashMap.h>
#include "RenderBackend/RenderCommon.h"
#include "RenderBackend/2D/CanvasRenderer.h"
#include "RenderBackend/TransformMatrixBlock.h"
#include "Common/TAABB.h"

namespace OSRE {
namespace RenderBackend {
namespace OSRE::RenderBackend {

//-------------------------------------------------------------------------------------------------
/// @ingroup Engine
///
/// @brief
/// @brief Will render debug data onto the current frame.
//-------------------------------------------------------------------------------------------------
class OSRE_EXPORT DbgRenderer {
struct DebugText;
Expand All @@ -56,20 +56,18 @@ class OSRE_EXPORT DbgRenderer {
DebugText *getDebugText(guid id) const;

private:
static DbgRenderer *sInstance;

RenderBackend::RenderBackendService *mRbSrv;
RenderBackend::TransformMatrixBlock mTransformMatrix;
RenderBackend::Mesh *mDebugMesh;

struct DebugText {
RenderBackend::Mesh *mesh;
guid id;
String text;
};

static DbgRenderer *sInstance;
RenderBackend::RenderBackendService *mRbSrv = nullptr;
RenderBackend::TransformMatrixBlock mTransformMatrix;
RenderBackend::CanvasRenderer *mCanvasRenderer = nullptr;
RenderBackend::Mesh *mDebugMesh = nullptr;
cppcore::TArray<DebugText*> mDebugTextMeshes;
ui16 mLastIndex;
};

} // Namespace RenderBackend
} // namespace OSRE
} // Namespace OSRE::RenderBackend
Loading
Loading