Fix debug text rendering #323
Conversation
|
""" WalkthroughThe debug text rendering system in DbgRenderer was refactored to use a CanvasRenderer-based approach instead of mesh-based rendering. The constructor, data structures, and rendering logic were updated to manage debug texts via unique IDs and draw them directly on a canvas, simplifying and decoupling the rendering process. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DbgRenderer
participant CanvasRenderer
Client->>DbgRenderer: renderDbgText(x, y, id, text)
DbgRenderer->>DbgRenderer: Store or update DebugText by id
DbgRenderer->>CanvasRenderer: drawText(x, y, text)
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Engine/RenderBackend/DbgRenderer.cpp (1)
193-196:⚠️ Potential issueUpdate clear() to handle canvas-based resources.
The clear method still references the old mesh-based system. It should clean up the debug text array instead.
Replace with:
void DbgRenderer::clear() { - delete mDebugMesh; - mDebugMesh = nullptr; + for (size_t i = 0; i < mDebugTextMeshes.size(); ++i) { + delete mDebugTextMeshes[i]; + } + mDebugTextMeshes.clear(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Engine/RenderBackend/DbgRenderer.cpp(3 hunks)src/Engine/RenderBackend/DbgRenderer.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Analyze (cpp)
🔇 Additional comments (4)
src/Engine/RenderBackend/DbgRenderer.h (3)
28-28: LGTM! Necessary include for canvas-based rendering.The addition of the CanvasRenderer header is appropriate for the new canvas-based debug text rendering approach.
61-63: Good refactoring to use guid-based identification.Replacing the mesh pointer with a guid identifier is a cleaner approach for managing debug text entries.
65-69: Proper member initialization.Good practice to explicitly initialize all pointers to nullptr.
src/Engine/RenderBackend/DbgRenderer.cpp (1)
198-225: Verify if addLine() is still needed with the canvas-based approach.This method still uses
mDebugMeshfrom the old mesh-based system. If line rendering should also move to canvas-based rendering, this method needs refactoring. If it's intentionally kept mesh-based, ensuremDebugMeshis properly managed.Please clarify whether line rendering should remain mesh-based or if this is legacy code that should be removed/refactored.
| mCanvasRenderer = new CanvasRenderer(2, x, y, w, h); | ||
| mCanvasRenderer->create(); |
There was a problem hiding this comment.
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.
| 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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
Show autofix suggestion
Hide autofix suggestion
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:
- Add a
delete mCanvasRenderer;statement in the destructor ofDbgRenderer. - Ensure that
mCanvasRendereris set tonullptrafter deletion to avoid dangling pointers. - Review the
clear()function to confirm it does not already releasemCanvasRenderer.
| @@ -56,2 +56,6 @@ | ||
| clear(); | ||
| if (mCanvasRenderer != nullptr) { | ||
| delete mCanvasRenderer; | ||
| mCanvasRenderer = nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/Engine/App/AppBase.h (1)
197-200:⚠️ Potential issueGetter/field type mismatch:
CanvasRenderer*vsIRenderPath*
getCanvasRenderer()advertises aRenderBackend::CanvasRenderer *, yet the backing member is declared as
RenderBackend::IRenderPath *mCanvasRenderer. Unless every call site casts, this violates the contract and can
trigger UB.-RenderBackend::IRenderPath *mCanvasRenderer = nullptr; +RenderBackend::CanvasRenderer *mCanvasRenderer = nullptr;Synchronise the types (or the getter’s return type) and adjust includes as needed.
Also applies to: 264-265
src/Engine/RenderBackend/RenderCommon.h (1)
37-57:⚠️ Potential issueNamespace switch breaks all
RenderBackend::…qualified references inside the same scopeChanging to the C++-17 nested-namespace syntax is fine, but every reference that still starts with
RenderBackend::is now interpreted as
OSRE::RenderBackend::RenderBackend::…, which does not exist and will cause compilation errors (e.g. theMeshArrayalias on l.49,UiVertexCache,
UiIndexCache, etc.).-using MeshArray = cppcore::TArray<RenderBackend::Mesh*>; +using MeshArray = cppcore::TArray<Mesh*>;Please drop the redundant qualifier (or use the fully-qualified
::OSRE::RenderBackend::Mesh) everywhere inside this namespace.
Search/replace ofRenderBackend::within the file will catch several more occurrences.
🧹 Nitpick comments (2)
src/Engine/App/ServiceProvider.cpp (2)
34-40: Singleton creation is not thread-safe
create()uses a raw static pointer without synchronisation. If two threads callcreate()concurrently during engine start-up, a double allocation or interleaved write tosInstanceis possible.Consider the usual Meyers singleton:
static ServiceProvider &instance() { static ServiceProvider instance; return instance; }or protect the allocation with
std::call_once/ a mutex.
42-46: Potential double-delete ifdestroy()is raced
destroy()deletessInstancewithout synchronisation. A second thread could reachdestroy()after the first one has already setsInstancetonullptrbut before the delete, resulting in UB, or delete twice in other race patterns. Align the lifetime management with the thread-safe singleton suggestion above or guard with a mutex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/Engine/App/AppBase.h(1 hunks)src/Engine/App/ServiceProvider.cpp(1 hunks)src/Engine/App/ServiceProvider.h(2 hunks)src/Engine/Common/AbstractService.h(1 hunks)src/Engine/RenderBackend/DbgRenderer.cpp(4 hunks)src/Engine/RenderBackend/DbgRenderer.h(2 hunks)src/Engine/RenderBackend/Mesh/MeshUtilities.h(2 hunks)src/Engine/RenderBackend/RenderBackendService.h(1 hunks)src/Engine/RenderBackend/RenderCommon.h(2 hunks)test/RenderTests/src/AABBDbgRenderTest.cpp(2 hunks)test/RenderTests/src/AbstractRenderTest.cpp(2 hunks)test/RenderTests/src/AbstractRenderTest.h(2 hunks)test/RenderTests/src/AbstractTestFixure.h(2 hunks)test/RenderTests/src/BaseTriangleRenderTest.cpp(2 hunks)test/RenderTests/src/DbgFontRenderTest.cpp(3 hunks)test/RenderTests/src/GeoInstanceRenderTest.cpp(2 hunks)test/RenderTests/src/GeoModelMatrixRenderTest.cpp(2 hunks)test/RenderTests/src/PrimTypeRenderTest.cpp(2 hunks)test/RenderTests/src/RenderBufferAccessTest.cpp(2 hunks)test/RenderTests/src/RenderTargetRenderTest.cpp(2 hunks)test/RenderTests/src/RenderTestSuite.cpp(4 hunks)test/RenderTests/src/RenderTestSuite.h(3 hunks)test/RenderTests/src/StaticTextRenderTest.cpp(2 hunks)test/RenderTests/src/SwitchCmdBufferRenderTest.cpp(3 hunks)test/RenderTests/src/main.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- test/RenderTests/src/main.cpp
✅ Files skipped from review due to trivial changes (17)
- src/Engine/RenderBackend/RenderBackendService.h
- test/RenderTests/src/GeoModelMatrixRenderTest.cpp
- test/RenderTests/src/PrimTypeRenderTest.cpp
- test/RenderTests/src/AbstractRenderTest.h
- test/RenderTests/src/AbstractRenderTest.cpp
- src/Engine/Common/AbstractService.h
- test/RenderTests/src/StaticTextRenderTest.cpp
- test/RenderTests/src/AABBDbgRenderTest.cpp
- test/RenderTests/src/RenderTargetRenderTest.cpp
- test/RenderTests/src/BaseTriangleRenderTest.cpp
- src/Engine/App/ServiceProvider.h
- test/RenderTests/src/GeoInstanceRenderTest.cpp
- test/RenderTests/src/RenderTestSuite.h
- test/RenderTests/src/AbstractTestFixure.h
- test/RenderTests/src/RenderBufferAccessTest.cpp
- test/RenderTests/src/DbgFontRenderTest.cpp
- test/RenderTests/src/RenderTestSuite.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Engine/RenderBackend/DbgRenderer.h
- src/Engine/RenderBackend/DbgRenderer.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Engine/App/ServiceProvider.cpp (3)
src/Engine/Platform/PlatformInterface.cpp (1)
sInstance(44-44)src/Engine/App/ServiceProvider.h (3)
create(47-47)destroy(48-48)setService(49-49)src/Engine/App/AssetRegistry.cpp (2)
destroy(45-52)destroy(45-45)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (cpp)
- GitHub Check: build
🔇 Additional comments (7)
src/Engine/App/ServiceProvider.cpp (1)
32-32: Renaming tosInstanceimproves consistencyMatches the rest of the codebase (AssetRegistry, PlatformInterface, etc.). 👍
src/Engine/App/AppBase.h (1)
256-256: 👍 Null-initialisation ofmTimerimproves safetyExplicitly initialising the raw pointer prevents undefined behaviour when the member is accessed before assignment.
No further issues with this single-line change.test/RenderTests/src/SwitchCmdBufferRenderTest.cpp (1)
36-36: C++17 nested-namespace syntax requires C++17 compile flags
namespace OSRE::RenderTest {is only valid from C++17 onward.
Ensure your build system passes-std=c++17(or higher) for every translation unit in this target.src/Engine/RenderBackend/RenderCommon.h (1)
1078-1080: Closing brace unchanged – LGTMNo further issues with the namespace close.
src/Engine/RenderBackend/Mesh/MeshUtilities.h (3)
23-24: Good call replacing include guards with#pragma onceReduces boiler-plate and is supported by all major compilers we target.
30-32: Namespace style consistent with the rest of the refactorNo concerns here; just remember that all fully-qualified references must be adjusted in the implementation files too.
166-166: Namespace close – LGTM
| if (type == ServiceType::Invalid || type == ServiceType::Count) { | ||
| } | ||
| sInstance->mServiceArray[static_cast<size_t>(type)] = service; | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| static constexpr ui32 NumPoints = 1000; | ||
| Mesh *m_pointMesh = nullptr; | ||
| ParticleEmitter *m_particeGen = nullptr; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memory-ownership & naming clean-up
m_pointMeshis declared but never initialised or destroyed – potential leak or dead code.m_particeGenis misspelled; considerm_particleEmitterfor clarity.
Suggested fix:
-static constexpr ui32 NumPoints = 1000;
-Mesh *m_pointMesh = nullptr;
-ParticleEmitter *m_particeGen = nullptr;
+static constexpr ui32 NumPoints = 1000;
+ParticleEmitter *m_particleEmitter = nullptr;And in destructor / usages:
- delete m_particeGen;
+ delete m_particleEmitter;If m_pointMesh truly isn’t needed, drop it entirely; otherwise allocate & free it consistently.
Also applies to: 94-97
🤖 Prompt for AI Agents
In test/RenderTests/src/SwitchCmdBufferRenderTest.cpp around lines 84 to 87 and
94 to 97, the member variable m_pointMesh is declared but never initialized or
deleted, risking a memory leak or dead code; also, m_particeGen is misspelled
and should be renamed to m_particleEmitter for clarity. Fix this by either
removing m_pointMesh if unused or properly allocating it in the constructor and
deleting it in the destructor. Rename m_particeGen to m_particleEmitter
consistently throughout the code, including all declarations, initializations,
and usages.
| /// @brief Will renumber the indices. | ||
| /// @tparam T Index type | ||
| /// @param dc The draw command container | ||
| /// @param offset Offset as renumbering parameter | ||
| template <class T> | ||
| inline void renumberIndices(const DrawCmd &dc, T offset) { | ||
| if (offset > 0) { | ||
| for (size_t j = 0; j < dc.NumIndices; ++j) { | ||
| dc.Indices[j] += static_cast<ui16>(offset); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
renumberIndices mutates through a const ref and truncates the offset
- The function takes
const DrawCmd&yet mutates the pointed-to index array – that’s misleading. offsetis cast toui16; an offset ≥ 65 536 will silently overflow.- No guard against
dc.Indices == nullptrordc.NumIndices == 0.
-template <class T>
-inline void renumberIndices(const DrawCmd &dc, T offset) {
- if (offset > 0) {
- for (size_t j = 0; j < dc.NumIndices; ++j) {
- dc.Indices[j] += static_cast<ui16>(offset);
- }
- }
+template <class TOffset>
+inline void renumberIndices(DrawCmd &dc, TOffset offset) {
+ using IndexT = std::remove_pointer_t<decltype(dc.Indices)>;
+
+ if (dc.Indices == nullptr || dc.NumIndices == 0 || offset == 0) {
+ return;
+ }
+ for (size_t j = 0; j < dc.NumIndices; ++j) {
+ dc.Indices[j] =
+ static_cast<IndexT>(dc.Indices[j] + static_cast<IndexT>(offset));
+ }
}This keeps the intent intact, avoids UB and prevents truncation.
📝 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.
| /// @brief Will renumber the indices. | |
| /// @tparam T Index type | |
| /// @param dc The draw command container | |
| /// @param offset Offset as renumbering parameter | |
| template <class T> | |
| inline void renumberIndices(const DrawCmd &dc, T offset) { | |
| if (offset > 0) { | |
| for (size_t j = 0; j < dc.NumIndices; ++j) { | |
| dc.Indices[j] += static_cast<ui16>(offset); | |
| } | |
| } | |
| } | |
| /// @brief Will renumber the indices. | |
| /// @tparam T Index type | |
| /// @param dc The draw command container | |
| /// @param offset Offset as renumbering parameter | |
| template <class TOffset> | |
| inline void renumberIndices(DrawCmd &dc, TOffset offset) { | |
| using IndexT = std::remove_pointer_t<decltype(dc.Indices)>; | |
| if (dc.Indices == nullptr || dc.NumIndices == 0 || offset == 0) { | |
| return; | |
| } | |
| for (size_t j = 0; j < dc.NumIndices; ++j) { | |
| dc.Indices[j] = | |
| static_cast<IndexT>(dc.Indices[j] + static_cast<IndexT>(offset)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Engine/RenderBackend/Mesh/MeshUtilities.h around lines 153 to 164, the
renumberIndices function incorrectly takes a const reference to DrawCmd but
modifies its Indices array, which is misleading and unsafe. Change the parameter
to a non-const reference to allow mutation. Remove the cast of offset to ui16 to
prevent silent overflow for large values. Add checks to ensure dc.Indices is not
nullptr and dc.NumIndices is greater than zero before the loop to avoid
undefined behavior.
| if (type == ServiceType::Invalid || type == ServiceType::Count) { | ||
| } |
Check notice
Code scanning / CodeQL
Futile conditional Note
Show autofix suggestion
Hide autofix suggestion
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:
- Remove the
ifstatement entirely if the condition is unnecessary and does not affect the program's functionality. - 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.
| @@ -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."); | ||
| } |
| if (type == ServiceType::Invalid || type == ServiceType::Count) { | ||
| } |
Check notice
Code scanning / CodeQL
Empty branch of conditional Note
Show autofix suggestion
Hide autofix suggestion
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:
- If the condition
type == ServiceType::Invalid || type == ServiceType::Countis not supposed to trigger any action, we should add a comment explaining why the block is intentionally left empty. - 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.
| @@ -50,2 +50,3 @@ | ||
| if (type == ServiceType::Invalid || type == ServiceType::Count) { | ||
| // No action required for invalid or count service types. | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (5)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (5)
216-224:⚠️ Potential issuePotential 16-bit overflow in
renumberIndicescall
renumberIndicescasts the offset toui16. BecausenumVerticesis a running total that can easily exceed65 535, any larger value will silently wrap, yielding corrupt indices when the cumulative vertex count grows beyond the 16-bit range.-renumberIndices(dc.Indices, dc.NumIndices, numVertices); +// use a wider temporary and guard against overflow +if (numVertices > std::numeric_limits<ui16>::max()) { + osre_error(Tag, "Vertex count exceeds 16-bit index range – update Mesh/IBO strategy"); +} +renumberIndices(dc.Indices, dc.NumIndices, static_cast<ui16>(numVertices));Consider switching the index type to 32 bit or splitting large batches.
320-324:⚠️ Potential issueOff-by-one: element
Indices[1]is never written
Indices[0]is assigned twice, leavingIndices[1]uninitialised (UB) and the line will not be rendered.-dc->Indices[0] = 0; -dc->Indices[0] = 1; +dc->Indices[0] = 0; +dc->Indices[1] = 1;
365-376: 🛠️ Refactor suggestionBroken edge-line index list in unfilled triangle
The three assignments below overwrite earlier values, producing
{0,1,1,2,1,0}instead of the expected{0,1,1,2,2,0}.- dc->Indices[1] = 1; - dc->Indices[2] = 2; - - dc->Indices[2] = 1; - dc->Indices[0] = 2; + dc->Indices[0] = 0; + dc->Indices[1] = 1; + dc->Indices[2] = 1; + dc->Indices[3] = 2; + dc->Indices[4] = 2; + dc->Indices[5] = 0;This yields the correct six indices for the three triangle edges.
139-146:⚠️ Potential issueIncorrect deallocation path for objects from
TPoolAllocator
alloc()acquires memory fromsAllocator, butdealloc()callsdelete, which is undefined behaviour for pool-managed memory and leaks the nestedVertices/Indicesbuffers.- delete cmd; + // Release nested buffers first … + delete[] cmd->Vertices; + delete[] cmd->Indices; + // … then return the object to the pool + sAllocator.free(cmd);Add a matching
free/resetfunction inTPoolAllocatorif it doesn’t exist.
222-224: 🛠️ Refactor suggestion
setLastIndexuses number of indices, not highest index value
setLastIndex(lastIndex + dc.NumIndices)increments by the count of indices, not by the offset applied (numVertices). This desynchronises the mesh’s internal cursor.- mMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices)); + mMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumVertices));Apply the same fix to the font path below.
🧹 Nitpick comments (1)
src/Engine/Common/Tokenizer.h (1)
60-64: Comment wording is unclear and slightly misleading
@return true, if any else false.does not convey the intent.
Consider re-phrasing to something like:/// @return true if a token exists at the given index, false otherwise.Pure documentation change (no functional impact).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
samples/00_HelloWorld/HelloWorld.cpp(2 hunks)src/Editor/src/RenderView/MainRenderView.cpp(1 hunks)src/Engine/App/AppBase.cpp(1 hunks)src/Engine/App/CameraComponent.cpp(1 hunks)src/Engine/Common/Tokenizer.cpp(1 hunks)src/Engine/Common/Tokenizer.h(1 hunks)src/Engine/Platform/win32/Win32DbgLogStream.h(2 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(2 hunks)src/Engine/RenderBackend/Mesh/MeshUtilities.h(2 hunks)src/Engine/RenderBackend/MeshProcessor.h(2 hunks)src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cpp(1 hunks)src/Engine/RenderBackend/Pipeline.cpp(3 hunks)src/Engine/RenderBackend/Pipeline.h(3 hunks)
✅ Files skipped from review due to trivial changes (8)
- src/Engine/App/CameraComponent.cpp
- src/Engine/RenderBackend/MeshProcessor.h
- src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cpp
- src/Engine/RenderBackend/Pipeline.cpp
- samples/00_HelloWorld/HelloWorld.cpp
- src/Editor/src/RenderView/MainRenderView.cpp
- src/Engine/RenderBackend/Pipeline.h
- src/Engine/Platform/win32/Win32DbgLogStream.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/RenderBackend/Mesh/MeshUtilities.h
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Engine/Common/Tokenizer.cpp (1)
src/Engine/Common/Tokenizer.h (1)
getTokenAt(63-63)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
src/Engine/RenderBackend/Mesh/MeshUtilities.h (2)
renumberIndices(159-165)renumberIndices(159-159)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Analyze (cpp)
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| for (size_t i = 0; i < mScenes.size(); ++i) { | ||
| // delete mScenes[i]; | ||
| } | ||
| mScenes.clear(); | ||
|
|
There was a problem hiding this comment.
💡 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 || trueLength 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.
| 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.
|
| mKeyboardEvListener = nullptr; | ||
|
|
||
| for (size_t i = 0; i < mScenes.size(); ++i) { | ||
| // delete mScenes[i]; |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
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.
| @@ -399,3 +399,3 @@ | ||
| for (size_t i = 0; i < mScenes.size(); ++i) { | ||
| // delete mScenes[i]; | ||
| delete mScenes[i]; | ||
| } |
Summary by CodeRabbit
New Features
Refactor