Renderbackend: Rework Canvas 2D-Rendering #244
Conversation
WalkthroughThe pull request introduces significant enhancements across various components of the rendering backend. Key updates include the restructuring of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 5
🧹 Outside diff range and nitpick comments (16)
src/Editor/src/main.cpp (2)
57-61: Improved error handling, but consider adding more detailsThe addition of error logging when window initialization fails is a good improvement. It enhances the application's ability to communicate issues, which is crucial for debugging.
Consider the following enhancements:
- Provide more detailed error information. For example:
- osre_error(Tag, "Cannot open the window."); + osre_error(Tag, "Failed to initialize window: %s", osreApp.getLastErrorString());
- Implement proper cleanup in case of initialization failure:
if (!osreApp.initWindow(100, 100, 1024, 768, "OSRE-Ed", WindowMode::Windowed, WindowType::Child, RenderBackendType::OpenGLRenderBackend)) { osre_error(Tag, "Cannot open the window."); + osreApp.shutdown(); return AppError; }These changes would provide more context for debugging and ensure resources are properly released on failure.
Line range hint
1-85: Minor improvements for overall code qualityWhile reviewing the file, I noticed a few areas that could be improved:
- The "Main loop" comment on line 66 could be more descriptive or removed if unnecessary.
- There's an unused static counter variable
counteron line 67. Consider removing it if it's not needed.- The
World* worldpointer on line 70 is unused. If it's not needed, consider removing it.Here's a suggested diff to address these issues:
- // Main loop bool done = false; while (!done) { - static int counter = 0; done = osreApp.handleEvents(); App::Stage *stage = osreApp.getStage(); if (stage != nullptr) { - World *world = stage->getActiveWorld(0); + // TODO: Implement world-related logic here if needed } osreApp.update(); osreApp.requestNextFrame(); }These changes will help improve code clarity and remove unused variables.
src/Engine/RenderBackend/2D/CanvasRenderer.h (1)
60-61: Improve integration of text rendering capabilitiesWhile adding text rendering capabilities is a valuable feature for the
CanvasRendererclass, the current implementation ofselectFont()anddrawText()methods seems incomplete and not well-integrated with the existing class structure. Consider the following suggestions to enhance the text rendering feature:
- Add supporting methods and properties for managing fonts and text states (e.g.,
setFontSize(),setTextAlignment(),currentFontmember variable).- Ensure consistency with existing drawing methods (e.g.,
drawLine(),drawTriangle()) in terms of parameter types and naming conventions.- Update the class documentation to reflect the new text rendering capabilities.
- Consider adding a
TextStylestruct or class to encapsulate font and text rendering properties, which can be used across multiple methods.These improvements would result in a more cohesive and feature-complete text rendering system within the
CanvasRendererclass.src/Engine/RenderBackend/MaterialBuilder.h (1)
Line range hint
53-70: Summary: Consistent simplification of return typesThe changes in this file consistently simplify the return types of
createBuildinMaterialand both overloads ofcreateTexturedMaterialfromRenderBackend::Material*toMaterial*. This improves readability and maintains consistency across theMaterialBuilderclass.To ensure these changes don't introduce any issues:
- Verify that
Materialis properly defined or imported in this file.- Check for any code that might rely on the fully qualified
RenderBackend::Material*type.- Update any documentation or comments that might reference the old return types.
Consider adding a comment at the top of the file explaining the relationship between
MaterialandRenderBackend::Materialto prevent future confusion.src/Engine/App/MouseEventListener.h (2)
62-62: Consider removing the extra blank line for consistency.The added blank line at line 62 is inconsistent with the spacing between other method declarations in the class. To maintain a uniform style throughout the file, consider removing this extra line.
Line range hint
1-124: Consider improvements for consistency and organization.
If not already corrected, fix the typo in the
rightButttonPressedmethod name torightButtonPressedfor consistency with other button methods.For better code organization, consider moving the inline method implementations at the end of the file to a separate .inl file. This approach is often used to keep header files clean while still allowing for inline optimizations.
Ensure consistent naming conventions across the codebase. For example, if other parts of the codebase use "press" instead of "pressed" for similar methods, consider aligning these method names accordingly.
Would you like assistance in implementing these suggestions?
src/Engine/Animation/AnimatorBase.h (1)
Line range hint
124-136: Approve new enum with a suggestion for documentationThe addition of the
TransformCommandTypeenum is a good improvement, providing a structured way to represent different transformation commands. The use of a scoped enum (enum class) is a good modern C++ practice.However, the comment above the enum is incomplete. Consider expanding it to provide a brief description of the enum's purpose.
Consider updating the comment above the enum to something like:
/// @brief Enumeration of transformation command types for animation control.src/Engine/App/Component.h (4)
130-132: Consider enhancing constructor and destructor commentsWhile the added comments provide basic information, they could be more informative. Consider adding details about any specific initialization or cleanup tasks performed by these methods, if applicable.
For example:
/// @brief The class constructor. /// @param owner The owning entity. /// @details Initializes the RenderComponent and sets up any necessary resources. RenderComponent(Entity *owner); /// @brief The class destructor. /// @details Ensures proper cleanup of any resources managed by this component. ~RenderComponent() override = default;
151-153: LGTM: New method addStaticMeshArray with clear documentationThe addition of this method provides a convenient way to add multiple meshes at once, which can improve efficiency in certain scenarios. The comment clearly describes the method's purpose, and the use of a const reference for the parameter is appropriate for passing an array.
Minor suggestion: Consider adding information about the behavior if the input array is empty.
You might want to expand the comment like this:
/// @brief Adds an array of new meshes to the component. /// @param array The array of new meshes to add. /// @note If the input array is empty, no changes will be made. void addStaticMeshArray(const RenderBackend::MeshArray &array);
172-174: Consider enhancing the LightComponent constructor commentWhile the added comment provides basic information, it could be more informative. Consider adding details about any specific initialization tasks performed by the constructor, particularly related to light setup.
For example:
/// @brief Constructs a new LightComponent. /// @param owner The owning entity. /// @details Initializes the LightComponent and sets up default light properties if applicable. LightComponent(Entity *owner);
179-181: LGTM: Improved documentation for setLight methodThe updated comment clearly states that the method sets a new light instance, which improves understanding of the method's purpose. The parameter name 'light' is appropriate and consistent with the method's function.
Minor suggestion: Consider adding information about the ownership of the light instance.
You might want to expand the comment like this:
/// @brief Sets a new light instance for this component. /// @param light The new light instance to set. /// @note This component takes ownership of the provided light instance. void setLight(RenderBackend::Light *light);src/Engine/RenderBackend/DbgRenderer.cpp (1)
84-87: Approve change with suggestion for additional loggingThe replacement of the assertion with a null check and early return improves the robustness of the
renderDbgTextmethod. This change prevents potential crashes whenmRbSrvis null, which aligns with good error handling practices.Consider adding a debug log message when
mRbSrvis null to help identify unexpected null cases during development or debugging. For example:if (mRbSrv == nullptr) { + osre_debug(Tag, "renderDbgText called with null mRbSrv"); return; }This will help maintain the benefits of the original assertion in terms of identifying potential issues, while still allowing for graceful handling in production environments.
src/Engine/Platform/AbstractWindow.h (2)
76-78: LGTM: Improved method documentation.The updated comment for the
setRectmethod provides a clearer description of its purpose.Consider adding a brief explanation of what the rectangle represents (e.g., window dimensions) for even more clarity:
- /// @brief Will set the new rectangle geometry. + /// @brief Will set the new rectangle geometry for the window dimensions. /// @param[in] rect The new rectangle. void setRect(const Rect2ui &rect);
132-139: LGTM: New methods for parent-child window relationship.The addition of
setParentandgetParentmethods enhances theAbstractWindowclass by introducing parent-child relationship functionality. The comments clearly explain the methods' purposes.For consistency with other methods in the class, consider adding the
virtualkeyword to the method declarations in the class definition:- void setParent(AbstractWindow *parent); + virtual void setParent(AbstractWindow *parent); - AbstractWindow *getParent() const; + virtual AbstractWindow *getParent() const;src/Engine/Platform/win32/Win32Window.cpp (1)
359-359: Standardized null-checking style inonResizemethodThe change from
nullptr != propstoprops != nullptrstandardizes the null-checking style. While this doesn't affect the logic, it improves code consistency, especially if this style is used throughout the codebase.Consider applying this null-checking style consistently across the entire codebase for better readability.
src/Engine/RenderBackend/RenderCommon.h (1)
195-206: LGTM! Consider adding comments for new attributes.The additions to the
VertexAttributeenum are logical and expand the capabilities of the vertex attributes. MovingNumVertexAttrsto the end maintains its function as a count of enum entries.Consider adding brief comments for the new attributes, similar to the existing ones, to improve code readability and documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- src/Editor/src/main.cpp (1 hunks)
- src/Engine/Animation/AnimatorBase.h (2 hunks)
- src/Engine/App/AppBase.cpp (1 hunks)
- src/Engine/App/Component.cpp (1 hunks)
- src/Engine/App/Component.h (3 hunks)
- src/Engine/App/MouseEventListener.h (1 hunks)
- src/Engine/Common/Logger.cpp (1 hunks)
- src/Engine/Common/Logger.h (2 hunks)
- src/Engine/Platform/AbstractWindow.h (3 hunks)
- src/Engine/Platform/win32/Win32Window.cpp (2 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (3 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (1 hunks)
- src/Engine/RenderBackend/DbgRenderer.cpp (1 hunks)
- src/Engine/RenderBackend/LineBuilder.h (2 hunks)
- src/Engine/RenderBackend/MaterialBuilder.h (2 hunks)
- src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.h (0 hunks)
- src/Engine/RenderBackend/RenderCommon.h (2 hunks)
💤 Files with no reviewable changes (1)
- src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.h
✅ Files skipped from review due to trivial changes (4)
- src/Engine/App/AppBase.cpp
- src/Engine/App/Component.cpp
- src/Engine/Common/Logger.cpp
- src/Engine/RenderBackend/LineBuilder.h
🧰 Additional context used
🔇 Additional comments (18)
src/Engine/RenderBackend/MaterialBuilder.h (2)
60-61: LGTM! Consistent with previous change.The simplification of the return type from
RenderBackend::Material*toMaterial*is consistent with the change increateBuildinMaterial. This maintains a uniform approach across the class methods.
53-53: LGTM! Verify usage in the codebase.The simplification of the return type from
RenderBackend::Material*toMaterial*improves readability. However, ensure that this change doesn't break any existing code that might rely on the fully qualified type.Run the following script to verify the impact of this change:
✅ Verification successful
Verified! The return type change from
RenderBackend::Material*toMaterial*has been consistently applied across the codebase without any adverse impacts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of RenderBackend::Material* createBuildinMaterial # and Material* createBuildinMaterial in the codebase echo "Searching for old signature usage (RenderBackend::Material* createBuildinMaterial):" rg --type cpp "RenderBackend::Material\*\s+createBuildinMaterial" -g '!MaterialBuilder.h' echo "Searching for new signature usage (Material* createBuildinMaterial):" rg --type cpp "Material\*\s+createBuildinMaterial" -g '!MaterialBuilder.h'Length of output: 492
Script:
#!/bin/bash # Description: Search for all usages of createBuildinMaterial in the codebase to assess impact echo "Searching for any usage of createBuildinMaterial regardless of return type:" rg --type cpp "createBuildinMaterial" -g '!MaterialBuilder.h'Length of output: 1756
src/Engine/App/MouseEventListener.h (1)
Line range hint
1-124: Verify the status of method name corrections.The AI summary mentions typo corrections in the following method names:
leftButttonPressedtoleftButtonPressedmiddleButttonPressedtomiddleButtonPressedHowever, these changes are not visible in the provided code. Please verify if these corrections have been implemented or if they are still needed.
To check the current state of these method names, run the following script:
✅ Verification successful
Method name typos have been corrected.
The method names
leftButttonPressedandmiddleButttonPressedhave been updated toleftButtonPressedandmiddleButtonPressed, respectively. No instances of the incorrect spellings are found inMouseEventListener.h.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the status of method name corrections in MouseEventListener.h # Test: Search for the method names in question echo "Checking for method names in MouseEventListener.h:" rg --type cpp '(left|middle|right)Butt[o]{1,2}nPressed' src/Engine/App/MouseEventListener.h # Expected output: # If corrections are not implemented, we should see "Buttton" in the output. # If corrections are implemented, we should see "Button" instead.Length of output: 204
src/Engine/Animation/AnimatorBase.h (1)
120-120: LGTM: Improved formattingThe spacing adjustment around the arithmetic operation enhances readability and aligns with common coding style guidelines.
src/Engine/App/Component.h (3)
52-52: LGTM: Improved documentation for AnimationComponentTypeThe added comment "For animation support" provides clear and concise information about the purpose of this enum value, enhancing code readability.
144-145: LGTM: Improved documentation for getMeshArrayThe updated comment clarifies that the mesh array is managed by the component, which provides valuable information about ownership and lifecycle. This enhancement improves the overall understanding of the method's purpose and behavior.
147-149: LGTM: Improved documentation and parameter naming for addStaticMeshThe changes enhance the method's clarity in two ways:
- The added comment clearly describes the method's purpose.
- The parameter name change from 'geo' to 'mesh' improves consistency with the method's name and function.
These modifications contribute to better code readability and maintainability.
src/Engine/Platform/AbstractWindow.h (3)
48-49: LGTM: Improved formatting for better readability.The spacing changes for
m_widthandm_heightmember variables enhance code readability and maintain consistency.
Line range hint
241-248: LGTM: Correct inline implementations for new methods.The inline implementations for
setParentandgetParentare simple, correct, and consistent with other inline implementations in the file.
Line range hint
1-265: Overall assessment: Approved with minor suggestions.The changes to
src/Engine/Platform/AbstractWindow.himprove the functionality and documentation of theAbstractWindowclass. The addition of parent-child relationship methods enhances the class's capabilities. Minor suggestions for improvements in documentation and consistency have been provided, but these are not critical issues.src/Engine/Common/Logger.h (1)
116-116: LGTM: Typo correction in commentThe typo in the comment for
VerboseModehas been corrected from "verbode" to "verbose". This improves the documentation quality.src/Engine/Platform/win32/Win32Window.cpp (2)
54-54: Improved code structure inshowWindowmethodThe removal of the extra closing brace corrects the control flow of the function. This change improves the code structure without altering the logic, making the function more readable and maintainable.
Line range hint
1-369: Overall assessment of changes in Win32Window.cppThe changes in this file are minor and focus on improving code clarity and consistency. They don't introduce any new functionality or alter existing behavior. The modifications align with the AI-generated summary and contribute to better code readability and maintainability.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
43-47: Improved documentation forDrawCmdstruct membersThe update to Doxygen-style comments for the
DrawCmdstruct members enhances code documentation and readability. This change is beneficial for maintaining clear and consistent documentation throughout the codebase.
49-53: Added comments forDrawCmdconstructor and destructorThe addition of comments for the
DrawCmdconstructor and destructor improves code documentation. This change enhances the overall readability and maintainability of the code.
156-156: Improved code readability inrendermethodThe addition of an empty line improves the visual separation of code blocks within the
rendermethod, enhancing overall readability.
361-368: ImproveddrawRectmethod implementation for filled rectanglesThe changes in the
drawRectmethod offer several improvements:
- Moving
setDirty()to the beginning ensures consistent state management.- The logic for filled rectangles is now more concise and efficient.
- The early return for filled rectangles improves code clarity and reduces nesting.
These changes enhance both the performance and readability of the method.
src/Engine/RenderBackend/RenderCommon.h (1)
225-229: LGTM! Improved documentation for light types.The added comments for each light type in the
LightTypeenum improve code readability and documentation without changing the functionality.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (2)
105-111: NewmFontmember initialized in constructorThe addition of
mFontinitialized tonullptris good. Consider if initializing it with a default font would be beneficial for immediate text rendering capabilities.
375-400: RefactoreddrawRectmethodThe refactoring of
drawRectimproves clarity and handling of filled vs. unfilled rectangles. The use of a constantthicknessfor unfilled rectangles is good for consistency.Consider optimizing the unfilled rectangle case by creating a single
DrawCmdfor the entire rectangle instead of four separate commands. This could improve performance when drawing many unfilled rectangles.src/Engine/RenderBackend/2D/CanvasRenderer.h (2)
149-153: Consider adding a return value todrawText()for enhanced functionality.Currently,
drawText()does not return any value. To improve usability, consider returning information such as:
- The dimensions (width and height) of the rendered text.
- A boolean indicating success or failure.
This can help callers adjust subsequent rendering or layout decisions based on the text size.
56-67: Ensure proper handling of copy and assignment operations forCanvasRenderer.Since
CanvasRenderercontains raw pointers (mFont,mMesh), it's important to manage the copy constructor and assignment operator to prevent unintended shallow copies, which could lead to resource leaks or memory corruption.Consider explicitly deleting the copy constructor and copy assignment operator:
class CanvasRenderer : IRenderPath { public: ... // Disable copy constructor and copy assignment operator CanvasRenderer(const CanvasRenderer&) = delete; CanvasRenderer& operator=(const CanvasRenderer&) = delete; ... };Alternatively, implement them appropriately if copying is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- contrib/assimp (1 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (6 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- contrib/assimp
🧰 Additional context used
🔇 Additional comments (5)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
43-47: Improved documentation forDrawCmdstructThe update to Doxygen-style comments for the
DrawCmdstruct members enhances code documentation quality and consistency.
256-258: Newdrawlineoverload addedThe new
drawlineoverload acceptingPoint2Diparameters enhances usability and maintains consistency by delegating to the existing method.
309-311: NewdrawTriangleoverload addedThe new
drawTriangleoverload acceptingPoint2Diparameters improves usability and maintains consistency by delegating to the existing method.
402-405: NewselectFontmethod addedThe
selectFontmethod is a good addition, preparing for text rendering functionality. It correctly sets the font and marks the state as dirty.src/Engine/RenderBackend/2D/CanvasRenderer.h (1)
145-147: The past review comment regardingselectFont()is still applicable.The
selectFont()method still lacks a return value to indicate success or failure of the font selection. Consider adding a return type (e.g.,bool) and handling potential errors.
Fix doc
Use upper-case
Fix upper-cases of point2Di
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
src/Engine/RenderBackend/2D/CanvasRenderer.h (1)
116-119: Consider renamingdrawlinetodrawLinefor consistency.The new overload for
drawlineusingPoint2Diparameters is a good addition. However, to maintain consistency with C++ naming conventions:Consider renaming the method from
drawlinetodrawLine(camelCase) for all its overloads. This change should be applied consistently throughout the class for all similar method names.- void drawline(const Point2Di &p1, const Point2Di &p2); + void drawLine(const Point2Di &p1, const Point2Di &p2);src/Engine/RenderBackend/2D/CanvasRenderer.cpp (2)
105-111: New font member added toCanvasRendererThe addition of
mFontto the constructor initialization list is consistent with the new font handling capability. The initialization tonullptris a safe default.Consider adding a comment explaining the purpose of
mFontfor better code documentation.CanvasRenderer::CanvasRenderer(i32 numLayers, i32 x, i32 y, i32 w, i32 h) : mDirty(true), mPenColor(1, 1, 1, 0), mResolution(), mActiveLayer(0), mNumLayers(numLayers), - mFont(nullptr), + mFont(nullptr), // Font used for text rendering, initialized as null mMesh(nullptr) { setResolution(x, y, w, h); }
375-400: ImproveddrawRectimplementationThe refactored
drawRectmethod offers better readability, maintainability, and control over rectangle rendering. The changes effectively address the previous review comment about improving the implementation for unfilled rectangles.Consider extracting the constant
thicknessto a class member or a configuration parameter for more flexibility:+class CanvasRenderer { +private: + static constexpr ui32 RECT_OUTLINE_THICKNESS = 2; + // ... other members ... +}; void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { setDirty(); DrawCmd *drawCmd = nullptr; if (filled) { drawCmd = alloc(); createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); mDrawCmdArray.add(drawCmd); return; } - const ui32 thickness = 2; drawCmd = alloc(); - createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); + createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, RECT_OUTLINE_THICKNESS, mActiveLayer); mDrawCmdArray.add(drawCmd); drawCmd = alloc(); - createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); + createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, RECT_OUTLINE_THICKNESS, mActiveLayer); mDrawCmdArray.add(drawCmd); drawCmd = alloc(); - createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); + createRectVertices(drawCmd, mPenColor, mResolution, x, y, RECT_OUTLINE_THICKNESS, h, mActiveLayer); mDrawCmdArray.add(drawCmd); drawCmd = alloc(); - createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); + createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, RECT_OUTLINE_THICKNESS, h, mActiveLayer); mDrawCmdArray.add(drawCmd); }This change would allow easier adjustment of the outline thickness for all rectangles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (6 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
src/Engine/RenderBackend/2D/CanvasRenderer.h (2)
58-63: LGTM: Constructor and destructor documentation.The added documentation for the constructor and destructor is clear and informative. It provides a good overview of the constructor parameters and acknowledges the presence of the destructor.
Also applies to: 66-66
131-136: LGTM:drawTriangleoverload withPoint2Diparameters.The new overload for
drawTriangleusingPoint2Diparameters is a good addition. It provides a more convenient way to draw triangles and is consistent with thePoint2Distructure introduced earlier.src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
43-47: Improved documentation forDrawCmdstructThe addition of Doxygen-style comments for the
DrawCmdstruct members, constructor, and destructor enhances code documentation and readability. This change follows good coding practices and will help in generating comprehensive documentation.Also applies to: 49-49, 53-53
256-259: Convenientdrawlineoverload addedThe new
drawlineoverload that acceptsPoint2Diparameters enhances the API's usability and consistency. It correctly delegates to the existingdrawlinemethod, maintaining the existing functionality while providing a more convenient interface for users working withPoint2Diobjects.
309-311: ConsistentdrawTriangleoverload addedThe new
drawTriangleoverload that acceptsPoint2Diparameters enhances API consistency and usability. It correctly delegates to the existingdrawTrianglemethod, maintaining functionality while providing a more convenient interface for users working withPoint2Diobjects. This addition is consistent with the newdrawlineoverload, improving overall API design.
402-405: NewselectFontmethod addedThe
selectFontmethod is a good addition that allows setting the font for text rendering. It correctly updates themFontmember and marks the state as dirty, ensuring proper rendering updates. This method is consistent with the newmFontmember added to the constructor.
kimkulling
changed the title
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (14)
src/Engine/Common/glm_common.h (1)
46-47: LGTM: New type aliases enhance code readability.The new type aliases
Vec3ArrayandVec2Arrayprovide convenient shorthand for arrays of 3D and 2D vectors, which is likely to improve code readability throughout the project. They are consistent with GLM naming conventions and use the customTArraycontainer.Consider adding a brief comment explaining the purpose of these aliases, especially if they're intended to be used widely across the project. For example:
// Convenient aliases for arrays of 3D and 2D vectors using Vec3Array = cppcore::TArray<glm::vec3>; using Vec2Array = cppcore::TArray<glm::vec2>;samples/02_Demo2D/Demo2D.cpp (1)
Line range hint
41-113: Consider refactoring theonCreatemethod for better organization.The
onCreatemethod has grown to include multiple drawing operations, including shapes and now text. To improve readability and maintainability, consider refactoring this method into smaller, focused methods.Here's a suggested refactoring approach:
- Create separate methods for different types of drawing operations:
void initializeCanvas() { mCanvasRenderer = new CanvasRenderer(2, 0, 0, 1024, 768); mCanvasRenderer->selectLayer(0); } void drawShapes() { const Color4 Red(1, 0, 0, 0); Color4 green(0, 1, 0, 1); Color4 yellow(1, 1, 0, 1); // Add all the rectangle drawing operations here } void drawText() { mCanvasRenderer->setColor(Color4(0, 0, 0, 1)); mCanvasRenderer->setFont("Arial", 16); mCanvasRenderer->drawText(300, 100, "OSRE 2D Demo"); }
- Update the
onCreatemethod to use these new methods:bool onCreate() override { // ... (existing setup code) initializeCanvas(); drawShapes(); drawText(); return true; }This refactoring will make the code more modular and easier to maintain, especially as you add more features to your 2D demo.
src/Engine/RenderBackend/Mesh/MeshUtilities.h (6)
45-48: Avoid naming conflicts with local variables.In the
getNumTextVertsmethod, the local variableNumTextVertscan be confusing since it closely resembles the class memberNumQuadVert. Consider renaming the local variable to prevent potential confusion.Apply this diff to rename the variable:
static size_t getNumTextVerts( const String &text ) { - const size_t NumTextVerts = NumQuadVert * text.size(); - return NumTextVerts; + const size_t numTextVerts = NumQuadVert * text.size(); + return numTextVerts; }
51-54: Ensure consistent naming conventions.In the
getNumTextIndicesmethod, verify that the variable naming follows the project's conventions. Consistent naming enhances code clarity and maintainability.
76-78: Remove commented-out code to enhance code cleanliness.There are several lines of commented-out code that appear to be obsolete. Removing them can improve readability and reduce clutter.
Apply this diff to remove the unnecessary code:
-//*textPos = new glm::vec3[NumTextVerts]; -//*colors = new glm::vec3[NumTextVerts]; -//*tex0 = new glm::vec2[NumTextVerts];
60-65: Eliminate redundant initialization of thecolarray.The
colarray is zero-initialized upon declaration, and the subsequent assignments toglm::vec3(0, 0, 0)for each element are redundant.Apply this diff to remove unnecessary assignments:
glm::vec3 col[NumQuadVert] = {}; -col[0] = glm::vec3(0, 0, 0); -col[1] = glm::vec3(0, 0, 0); -col[2] = glm::vec3(0, 0, 0); -col[3] = glm::vec3(0, 0, 0);This simplifies the code without changing its functionality.
66-71: Eliminate redundant initialization of theposarray.Similar to the
colarray, theposarray is initialized, and all elements are immediately assigned new values. You can declare the array with the required values directly.Apply this diff to initialize
posupon declaration:-glm::vec3 pos[NumQuadVert] = {}; -pos[0] = glm::vec3(x, y, 0); -pos[1] = glm::vec3(x, y + textSize, 0); -pos[2] = glm::vec3(x + textSize, y, 0); -pos[3] = glm::vec3(x + textSize, y + textSize, 0); +glm::vec3 pos[NumQuadVert] = { + glm::vec3(x, y, 0), + glm::vec3(x, y + textSize, 0), + glm::vec3(x + textSize, y, 0), + glm::vec3(x + textSize, y + textSize, 0) +};This makes the code more concise and improves readability.
57-57: Use consistent comparison order in assertions for readability.In the assertion
osre_assert(nullptr != textIndices);, consider switching the order totextIndices != nullptrto align with common coding practices.Apply this diff:
-osre_assert(nullptr != textIndices); +osre_assert(textIndices != nullptr);src/Engine/RenderBackend/2D/CanvasRenderer.cpp (2)
54-54: Fix typographical error in destructor commentThere is a typo in the comment for the destructor; "descructor" should be "destructor".
Apply this diff to fix the typo:
- /// The class descructor. + /// The class destructor.
257-259: Ensure consistent naming convention fordrawlinemethodThe method
drawlineis overloaded to acceptPoint2Di, improving usability. To maintain consistency, consider usingdrawLineif camelCase naming is standard in your codebase.Apply this diff to adjust the method name:
- void CanvasRenderer::drawline(const Point2Di &p1, const Point2Di &p2) { + void CanvasRenderer::drawLine(const Point2Di &p1, const Point2Di &p2) {src/Engine/RenderBackend/MeshBuilder.cpp (1)
Line range hint
341-443: Consider removing commented-out code to improve code readabilityThe block of code from lines 341 to 443 is commented out. Retaining large sections of commented-out code can clutter the codebase and reduce readability. If this code is no longer needed, consider removing it. Version control systems can retrieve it if needed in the future.
src/Engine/RenderBackend/RenderCommon.h (3)
195-206: Fix missing double quote in comment forTangentOn line 196, the comment for
Tangentis missing a closing double quote.Apply this diff to correct the comment:
- Tangent, ///< "tangent + Tangent, ///< "tangent"
229-229: Complete the comment forNumLightTypesThe comment for
NumLightTypesis incomplete. Please provide a brief description.Update the comment as follows:
- NumLightTypes ///< + NumLightTypes ///< Number of light types
936-938: Clarify the purpose of the emptyRenderTargetstructThe
RenderTargetstruct is currently empty. If it's meant for future extensions, consider adding a comment to explain its intended use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- samples/02_Demo2D/Demo2D.cpp (1 hunks)
- src/Engine/CMakeLists.txt (3 hunks)
- src/Engine/Common/glm_common.h (1 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (7 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (2 hunks)
- src/Engine/RenderBackend/Mesh/MeshUtilities.h (1 hunks)
- src/Engine/RenderBackend/MeshBuilder.cpp (6 hunks)
- src/Engine/RenderBackend/RenderCommon.h (4 hunks)
🧰 Additional context used
🔇 Additional comments (19)
src/Engine/Common/glm_common.h (2)
44-44: LGTM: Include statement for TArray is correctly placed.The new include statement for
<cppcore/Container/TArray.h>is appropriately placed after the standard GLM includes. This is necessary for the new type aliases and follows good include ordering practices.
46-47: Verify usage of new type aliases in the codebase.The new
Vec3ArrayandVec2Arraytype aliases seem to be part of a larger refactoring or feature addition. To ensure they're being used effectively:Run the following script to check for usage of these new types:
This will help ensure that the new aliases are being used consistently and identify any places where they could be applied but haven't been yet.
✅ Verification successful
Verified usage of Vec3Array and Vec2Array across the codebase.
No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of Vec3Array and Vec2Array in the codebase echo "Searching for Vec3Array usage:" rg --type cpp "Vec3Array" -C 2 echo "\nSearching for Vec2Array usage:" rg --type cpp "Vec2Array" -C 2 echo "\nSearching for potential candidates for Vec3Array (TArray<glm::vec3>):" rg --type cpp "TArray<glm::vec3>" -C 2 echo "\nSearching for potential candidates for Vec2Array (TArray<glm::vec2>):" rg --type cpp "TArray<glm::vec2>" -C 2Length of output: 6138
src/Engine/RenderBackend/2D/CanvasRenderer.h (2)
47-52: 🛠️ Refactor suggestionEnhance the
Fontstructure for better consistency and usability.The
Fontstructure correctly represents font properties, but consider the following improvements:
- Use lowercase for member names to align with C++ naming conventions.
- Add a constructor for easier initialization.
- Clarify the
Stringtype if it's not a standard type (e.g.,std::string).Apply this diff to improve the
Fontstructure:struct Font { - String Name; ///< Font name - i32 Size; ///< Font size - i32 Style; ///< Font style + std::string name; ///< Font name + i32 size; ///< Font size + i32 style; ///< Font style + + Font(const std::string& name = "", i32 size = 0, i32 style = 0) + : name(name), size(size), style(style) {} };Also, ensure that
Stringis replaced withstd::stringthroughout the file if it's not a custom type.Likely invalid or redundant comment.
178-178:⚠️ Potential issueAvoid using raw pointers for
mFont; consider smart pointers or references.Using a raw pointer for
mFontcan lead to memory management issues such as leaks or dangling pointers. Consider the following alternatives:
- Use
std::unique_ptr<Font>ifCanvasRendererowns theFontexclusively.- Use
std::shared_ptr<Font>if ownership is shared.- Use a reference (
Font&) ifCanvasRendererdoes not own theFont.Apply this diff to use
std::unique_ptr:+#include <memory> + class CanvasRenderer : IRenderPath { public: ... - Font *mFont; + std::unique_ptr<Font> mFont; ... };Likely invalid or redundant comment.
src/Engine/CMakeLists.txt (3)
406-406: LGTM: New source group for mesh utilities.The addition of a new source group for mesh utilities is a good practice. It improves the project structure and maintains consistency with other source groups in the file.
448-448: LGTM: Inclusion of mesh utility sources in library compilation.The addition of
${renderbackend_mesh_src}to theADD_LIBRARYcommand ensures that the new mesh utility files are correctly included in the shared library compilation. This is a necessary step to make the new functionality available.
318-320: LGTM: New variable for mesh utility source files.The addition of the
renderbackend_mesh_srcvariable is consistent with the project's naming conventions and is appropriately placed.However, I noticed that only a header file is included. Is there a corresponding
.cppfile for the implementation, or is this intended to be a header-only utility?To check for a potential implementation file, you can run:
src/Engine/RenderBackend/Mesh/MeshUtilities.h (1)
84-89: Verify proper handling of consecutive line breaks in text processing.When processing the text, the code handles line breaks by resetting
textColand incrementingtextRow. Ensure that multiple consecutive line breaks are handled correctly and do not cause unintended behavior.To confirm, consider adding unit tests with texts containing multiple consecutive line breaks.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (6)
28-28: IncludeMeshUtilities.hfor text renderingIncluding
"RenderBackend/Mesh/MeshUtilities.h"is necessary for usingMeshUtilities::generateTextBoxVerticesAndIndicesin thedrawTextmethod.
111-111: InitializemFonttonullptrInitializing
mFonttonullptrensures that text rendering is only attempted when a font is selected.
310-313: Ensure consistent naming convention fordrawTrianglemethodThe method
drawTriangleis overloaded to acceptPoint2Di, enhancing functionality. Ensure that the method name follows the project's naming conventions (e.g.,drawTriangleordraw_triangle).
403-406: Implement font selection functionalityThe
selectFontmethod allows setting the font for text rendering and correctly marks the renderer as dirty.
Line range hint
163-170: Verify index adjustment logic inrendermethodIn the
rendermethod, when adjusting indices for each draw command, you incrementdc.Indices[j]bynumVertices. Ensure that this adjustment correctly accounts for the cumulative vertex count to prevent rendering errors when combining multiple draw commands.Run the following script to review and confirm the index adjustment logic:
#!/bin/bash # Description: Check index adjustments in draw commands within the render method. # Search for index adjustments in CanvasRenderer.cpp rg --type cpp "dc\.Indices\[\w+\]\s*\+\=" --context 5 # Verify how numVertices and lastIndex are used rg --type cpp "numVertices|lastIndex" --context 5
107-107: Verify the initial alpha value ofmPenColor
mPenColoris initialized with an alpha value of0, which means fully transparent. Is this intentional? If you intend for the default pen color to be fully opaque white, consider setting the alpha to1.Run the following script to check how
mPenColoris used and verify if an alpha of0is appropriate:src/Engine/RenderBackend/MeshBuilder.cpp (3)
24-24: [Approved]Including
"RenderBackend/Mesh/MeshUtilities.h"is appropriate for utilizing the new utility functions. This enhances modularity and code organization.
460-460: Verify thatpositions,colors, andtex0are not empty before accessingWhen calling
allocVertices, you use&positions[0],&colors[0], and&tex0[0]. Ensure that these arrays are not empty to prevent undefined behavior when accessing the first element. Sincepositions,colors, andtex0are populated based ontext, andtext.empty()is already checked earlier, this should be safe.
485-485: [Approved]Using
MeshUtilities::getNumTextVerts(text)enhances code reuse and improves maintainability by centralizing the calculation of text vertices.src/Engine/RenderBackend/RenderCommon.h (2)
930-933:RenderPassTypeenum is well-definedThe
RenderPassTypeenum introduces types for render passes and is appropriately defined.
Line range hint
941-950:IRenderPathinterface is well-structuredThe
IRenderPathinterface provides clear virtual methods for rendering phases and properly initializes its shader member.
| /// @brief Will select a font. | ||
| /// @param font The font to select. | ||
| void selectFont(Font *font); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance selectFont method for better safety and clarity.
While the selectFont method provides necessary functionality, consider the following improvements:
- Use a smart pointer or reference instead of a raw pointer to clarify ownership.
- Add error handling or validation for the input parameter.
- Consider returning a boolean to indicate success or failure of the operation.
Here's a suggested improvement:
/**
* @brief Selects a font for text rendering.
* @param font The font to select.
* @return True if the font was successfully selected, false otherwise.
*/
bool selectFont(const std::shared_ptr<Font>& font);Implement the method to handle null or invalid font objects and return appropriate boolean values.
| /// @brief The 2D point structure for int32. | ||
| struct Point2Di { | ||
| i32 X, Y; /// Coordinate components | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refining the Point2Di structure for consistency and usability.
While the Point2Di structure correctly represents a 2D point, consider the following improvements:
- Use lowercase for member names to align with C++ naming conventions.
- Add a constructor for easier initialization.
Apply this diff to improve the Point2Di structure:
struct Point2Di {
- i32 X, Y; /// Coordinate components
+ i32 x, y; /// Coordinate components
+
+ Point2Di(i32 x = 0, i32 y = 0) : x(x), y(y) {}
};📝 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 The 2D point structure for int32. | |
| struct Point2Di { | |
| i32 X, Y; /// Coordinate components | |
| }; | |
| /// @brief The 2D point structure for int32. | |
| struct Point2Di { | |
| i32 x, y; /// Coordinate components | |
| Point2Di(i32 x = 0, i32 y = 0) : x(x), y(y) {} | |
| }; |
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | ||
| if (text.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| if (mFont == nullptr) { | ||
| osre_debug(Tag, "No font selected."); | ||
| return; | ||
| } | ||
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | ||
| mapCoordinates(mResolution, x, y, x_model, y_model); | ||
| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *indices = nullptr; | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | ||
|
|
||
| DrawCmd *drawCmd = alloc(); | ||
| i32 x_clipped, y_clipped; | ||
|
|
||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } | ||
|
|
||
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | ||
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | ||
| } | ||
|
|
||
| mDrawCmdArray.add(drawCmd); |
There was a problem hiding this comment.
Prevent memory leaks by deallocating allocated resources in drawText
In the drawText method:
- The
indicesarray is dynamically allocated withinMeshUtilities::generateTextBoxVerticesAndIndices. After copying the indices intodrawCmd->Indices, theindicesarray should be deallocated to prevent a memory leak. - The
DrawCmdstruct allocates memory forVerticesandIndices, but its destructor does not deallocate them, leading to potential memory leaks.
Deallocate indices after usage:
Add the following line after copying the indices to drawCmd->Indices:
delete[] indices;Implement destructor to free allocated memory in DrawCmd:
Update the DrawCmd destructor to deallocate Vertices and Indices:
/// The class destructor.
- ~DrawCmd() = default;
+ ~DrawCmd() {
+ delete[] Vertices;
+ delete[] Indices;
+ }| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *textIndices(nullptr); | ||
| generateTextBoxVerticesAndIndices(x,y,textSize, text, &textPos, &colors, &tex0, &textIndices); | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x, y, textSize, text, positions, colors, tex0, &textIndices); |
There was a problem hiding this comment.
Ensure proper memory management for textIndices
The MeshUtilities::generateTextBoxVerticesAndIndices function allocates memory for textIndices. To prevent memory leaks, ensure that textIndices is properly deallocated after its usage.
Apply this change to deallocate textIndices after creating the index buffer:
mActiveMesh = mesh;
+ delete[] textIndices;
return *this;Committable suggestion was skipped due to low confidence.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/Engine/RenderBackend/MeshBuilder.cpp (1)
Line range hint
435-502: Consider refactoring allocVertices to use MeshUtilitiesWhile the
allocVerticesmethod remains unchanged and likely functions correctly, it might benefit from utilizingMeshUtilitiesfor consistency with other refactored methods.Consider extracting the vertex allocation logic to
MeshUtilitiesand refactoring this method to use it. This could improve code consistency and potentially reduce duplication. For example:void MeshBuilder::allocVertices(Mesh *mesh, VertexType type, size_t numVerts, glm::vec3 *pos, glm::vec3 *col1, glm::vec2 *tex0, BufferAccessType access) { MeshUtilities::allocateVertices(mesh, type, numVerts, pos, col1, tex0, access); }This suggestion assumes that
MeshUtilitieshas or could have a method for allocating vertices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/Engine/RenderBackend/Mesh/MeshUtilities.h (1 hunks)
- src/Engine/RenderBackend/MeshBuilder.cpp (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/RenderBackend/Mesh/MeshUtilities.h
🧰 Additional context used
🔇 Additional comments (3)
src/Engine/RenderBackend/MeshBuilder.cpp (3)
24-24: Improved code organization with MeshUtilitiesThe inclusion of
MeshUtilities.hindicates a positive step towards better code organization and separation of concerns. This change should improve maintainability and reusability of mesh-related utilities.
Line range hint
1-545: Overall improvement in code organization and maintainabilityThe changes in this file, primarily revolving around the integration of
MeshUtilities, represent a significant improvement in code organization and maintainability. The refactoring reduces duplication and centralizes mesh-related utilities, which should make the code easier to maintain and extend in the future.Some key points to consider:
- Ensure proper memory management, especially for dynamically allocated resources.
- Verify the accessibility and correct definition of constants and methods in
MeshUtilities.- Consider further refactoring opportunities to leverage
MeshUtilitiesconsistently across the class.These changes are a step in the right direction for improving the overall architecture of the rendering backend.
Line range hint
381-405: Improved consistency with MeshUtilities in updateTextBoxThe changes in
updateTextBoxmethod improve consistency by utilizingMeshUtilitiesconstants and methods. This should enhance maintainability and reduce the risk of inconsistencies.However, there's a potential issue to address:
Ensure that
MeshUtilities::NumQuadVertis properly defined and accessible. If it's not already, consider adding it as a public constant inMeshUtilities:If the constant is not found, you may need to add it to the
MeshUtilitiesclass.✅ Verification successful
Verified:
MeshUtilities::NumQuadVertis properly defined and accessibleThe constant
MeshUtilities::NumQuadVertis correctly defined as a public static constexpr inMeshUtilities.h. This ensures that theupdateTextBoxmethod can access it without issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the definition of NumQuadVert in MeshUtilities ast-grep --lang cpp --pattern 'class MeshUtilities { $$$ public: $$$ static constexpr $_type NumQuadVert = $_value; $$$ }'Length of output: 140
Script:
#!/bin/bash # Search for all occurrences of NumQuadVert in the codebase rg 'NumQuadVert'Length of output: 999
| /* cppcore::TArray<TextureResource *> texResArray; | ||
| TextureResource* texRes = new TextureResource("buildin_arial", IO::Uri("file://assets/Textures/Fonts/buildin_arial.bmp")); | ||
| texResArray.add(texRes); | ||
| mesh->setMaterial(MaterialBuilder::createTexturedMaterial("text_box_tex", texResArray, VertexType::RenderVertex)); | ||
| mesh->setMaterial(MaterialBuilder::createTexturedMaterial("text_box_tex", texResArray, VertexType::RenderVertex));*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
src/Engine/RenderBackend/MeshBuilder.cpp (1)
Line range hint
1-524: Overall improvement in code organization and maintainabilityThe changes in this file represent a significant refactoring effort, moving complex logic to the
MeshUtilitiesclass. This improves code organization, maintainability, and reduces duplication. The consistent use ofMeshUtilitiesthroughout the file is a positive change.However, there are a few points to address:
- Ensure proper memory management, particularly in the
allocTextBoxmethod.- Verify the accessibility of constants like
MeshUtilities::NumQuadVert.- Clarify the changes in material setup for text boxes.
Once these points are addressed, the overall quality of the code will be significantly improved.
src/Engine/RenderBackend/RenderCommon.h (2)
930-934: LGTM: New RenderPassType enum addedThe addition of the
RenderPassTypeenum is a good start for categorizing different types of render passes. The use of enum class is a good C++ practice.Consider adding comments for each type to provide more context. Also, as the rendering system evolves, you might want to add more types of render passes (e.g., DynamicRenderPass, PostProcessRenderPass) to this enum.
936-939: Consider adding implementation or documentation for RenderTargetThe
RenderTargetstruct is currently empty. If this is a placeholder for future implementation, consider adding a TODO comment to explain its intended purpose and future plans.src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
384-409: Simplify the logic for unfilled rectangles.The changes to the
drawRectmethod have improved the handling of filled rectangles. However, the logic for unfilled rectangles can be further simplified.Consider this refactored version:
void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { setDirty(); DrawCmd *drawCmd = alloc(); createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); mDrawCmdArray.add(drawCmd); if (!filled) { const ui32 thickness = 2; drawCmd = alloc(); createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); mDrawCmdArray.add(drawCmd); drawCmd = alloc(); createRectVertices(drawCmd, mPenColor, mResolution, x, y + h - thickness, w, thickness, mActiveLayer); mDrawCmdArray.add(drawCmd); drawCmd = alloc(); createRectVertices(drawCmd, mPenColor, mResolution, x, y + thickness, thickness, h - 2 * thickness, mActiveLayer); mDrawCmdArray.add(drawCmd); drawCmd = alloc(); createRectVertices(drawCmd, mPenColor, mResolution, x + w - thickness, y + thickness, thickness, h - 2 * thickness, mActiveLayer); mDrawCmdArray.add(drawCmd); } }This version always draws the filled rectangle first, then conditionally draws the four edges for unfilled rectangles. It avoids code duplication and makes the logic clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/Engine/App/TransformComponent.h (1 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (7 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (2 hunks)
- src/Engine/RenderBackend/FontService.h (1 hunks)
- src/Engine/RenderBackend/MeshBuilder.cpp (5 hunks)
- src/Engine/RenderBackend/RenderCommon.h (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Engine/App/TransformComponent.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/RenderBackend/2D/CanvasRenderer.h
🧰 Additional context used
🔇 Additional comments (18)
src/Engine/RenderBackend/FontService.h (3)
1-7: LGTM: File structure and includes are well-organized.The file structure is clean, with appropriate use of the
#pragma oncedirective for header guards. The includes are relevant to the functionality provided and follow a logical order from local to external dependencies.
8-9: LGTM: Namespace usage is appropriate.The code is correctly wrapped in the
OSRE::RenderBackendnamespace, which aligns with the project structure and helps prevent naming conflicts.Also applies to: 32-33
21-28:⚠️ Potential issueRefactor
getFont()method to improve flexibility and correct parameter usage.The
getFont()method has several issues that need to be addressed:
- The
fontparameter is unused, which is misleading.- The texture path is hardcoded, limiting flexibility.
- Memory management for the created
TextureResourceis unclear.Here are suggested improvements:
- Use the
fontparameter to determine the texture path:static Material *getFont(const Font &font) { std::string texturePath = "file://assets/Textures/Fonts/" + font.Name + ".bmp"; TextureResource *texRes = new TextureResource(font.Name, IO::Uri(texturePath)); // ... rest of the method }
- Consider using smart pointers for better memory management:
#include <memory> static std::unique_ptr<Material> getFont(const Font &font) { auto texRes = std::make_unique<TextureResource>(font.Name, IO::Uri(texturePath)); cppcore::TArray<TextureResource*> texResArray; texResArray.add(texRes.get()); return MaterialBuilder::createTexturedMaterial("text_box_tex", texResArray, VertexType::RenderVertex); }
- Add error handling for cases where the texture file doesn't exist:
if (!IO::Uri(texturePath).exists()) { // Handle error (e.g., throw an exception or return nullptr) }These changes will improve the method's flexibility, correctness, and robustness.
To ensure the texture files exist, you can run the following script:
This script will help identify any missing texture files for the fonts used in the codebase.
src/Engine/RenderBackend/MeshBuilder.cpp (3)
24-24: Improved code organization with MeshUtilitiesThe inclusion of
MeshUtilities.hsuggests a good separation of concerns, likely moving some utility functions to a dedicated class. This change should improve code organization and maintainability.
368-377: Clarify changes in material setup for text boxThe material setup for the text box has been simplified to use a default font instead of creating a new material. However, there's commented-out code that suggests a different approach was previously used or might be reintroduced.
Could you please clarify:
- Is the current approach using
FontService::getFontfinal?- Are there plans to reintroduce custom texture handling as seen in the commented-out code?
- If the commented-out code is no longer needed, consider removing it to improve code clarity.
Line range hint
389-413: Improved consistency in updateTextBox methodThe changes in
updateTextBoxmethod improve consistency by using utility methods and constants fromMeshUtilities. This makes the code more maintainable and reduces the risk of errors.However, as noted in a previous review, ensure that
MeshUtilities::NumQuadVertis properly defined and accessible. If it's not already done, consider adding a public constant inMeshUtilities:// In MeshUtilities.h public: static constexpr size_t NumQuadVert = 4;src/Engine/RenderBackend/RenderCommon.h (5)
195-206: LGTM: Comprehensive vertex attribute additionsThe new vertex attributes added to the
VertexAttributeenum are well-structured and cover a wide range of potential use cases. The naming convention is consistent, and the comments provide clarity for each attribute.
225-229: LGTM: Improved documentation for LightType enumThe updated comments for the
LightTypeenum provide better clarity for each light type. This change enhances the code's readability and maintainability.
Line range hint
941-957: LGTM: Well-structured IRenderPath interfaceThe
IRenderPathinterface provides a clear structure for implementing render paths. The use of pure virtual functions for the rendering steps (preRender, render, postRender) allows for flexible implementations in derived classes.The inline implementation of
setShaderis appropriate for a simple setter method.
981-987: LGTM: Well-structured Font structThe
Fontstruct is well-defined with clear and relevant member variables. The comments for each member provide good clarity.
Line range hint
1-987: Overall assessment: Good improvements with minor issuesThis update to
RenderCommon.hintroduces several valuable additions and improvements to the rendering backend:
- Enhanced vertex attributes in the
VertexAttributeenum.- Improved documentation for the
LightTypeenum.- New
RenderPassTypeenum for categorizing render passes.- Addition of
IRenderPathinterface for structured render path implementations.- New
Fontstruct for font-related operations.These changes generally improve the code structure and expand the rendering capabilities. However, there are a couple of minor points to address:
- The
RenderTargetstruct is currently empty and might benefit from documentation or implementation.- There's a typo in the
getGlslVersionFromeStringfunction name that should be corrected.After addressing these minor issues, the changes will significantly enhance the rendering backend's functionality and maintainability.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (7)
28-29: LGTM!The new includes are necessary for the added font and text rendering functionality.
115-122: LGTM!The constructor changes to initialize the new
mFontmember tonullptrare correct.
265-267: LGTM!The new overload of
drawlinethat acceptsPoint2Diparameters is a convenient addition and the implementation looks correct.
318-320: LGTM!The new overload of
drawTrianglethat acceptsPoint2Diparameters is a convenient addition and the implementation looks correct.
411-414: LGTM!The new
selectFontmethod for setting the font and marking the renderer as dirty is a necessary addition for the text rendering functionality.
Line range hint
171-187: Ensure indices are correctly updated for each draw command.The changes to update the indices based on the last index and number of vertices look correct. This ensures that the indices for each draw command are correctly offset based on the previously added vertices.
To verify the indices are correctly updated, run the following script:
#!/bin/bash # Description: Verify indices are correctly updated for each draw command. # Test: Search for the index update logic. Expect: Correct usage of `lastIndex` and `numVertices`. rg --type cpp $'dc\.Indices\[j\] \+= static_cast<ui16>\(numVertices\)' -A 5
452-455: Verify the default font fallback is appropriate.The code falls back to the default font if
mFontisnullptr. While this can prevent crashes, consider if this is the desired behavior or if an error should be reported instead.Run the following script to analyze the font usage:
#!/bin/bash # Description: Analyze font usage and fallback behavior. # Test: Search for font-related logic. Expect: Appropriate fallback or error handling. rg --type cpp $'mFont\b' -A 5Based on the results, assess if the current fallback to the default font is appropriate or if alternative error handling should be implemented.
| namespace OSRE { | ||
| namespace RenderBackend { | ||
|
|
||
| struct FontService { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a namespace instead of a struct for FontService.
The FontService is currently defined as a struct but only contains static methods. This suggests it's being used as a utility class. In C++, it's more idiomatic to use a namespace for such utility functions rather than a struct with only static members.
Consider refactoring FontService to a namespace:
-struct FontService {
+namespace FontService {
// ... existing methods ...
-};This change would make the intent clearer and align better with C++ best practices for utility functions.
Also applies to: 29-29
| static Font *getDefaultFont() { | ||
| static Font defaultFont; | ||
| defaultFont.Name = "buildin_arial"; | ||
| defaultFont.Size = 10; | ||
| defaultFont.Style = 0; | ||
|
|
||
| return &defaultFont; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance getDefaultFont() with constants and consider thread safety.
The getDefaultFont() method effectively uses a static local variable to ensure single initialization. However, there are a few points to consider:
- The hardcoded default values could be replaced with named constants for better maintainability.
- In a multi-threaded environment, the initialization of the static variable might not be thread-safe in all C++ versions.
Consider the following improvements:
- Use constants for default values:
static Font *getDefaultFont() {
static const char* DEFAULT_FONT_NAME = "buildin_arial";
static const int DEFAULT_FONT_SIZE = 10;
static const int DEFAULT_FONT_STYLE = 0;
static Font defaultFont{DEFAULT_FONT_NAME, DEFAULT_FONT_SIZE, DEFAULT_FONT_STYLE};
return &defaultFont;
}- For thread-safety in all C++ versions, consider using
std::call_once:
#include <mutex>
static Font *getDefaultFont() {
static Font defaultFont;
static std::once_flag flag;
std::call_once(flag, []() {
defaultFont.Name = DEFAULT_FONT_NAME;
defaultFont.Size = DEFAULT_FONT_SIZE;
defaultFont.Style = DEFAULT_FONT_STYLE;
});
return &defaultFont;
}These changes will improve maintainability and ensure thread-safety across all C++ versions.
| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *textIndices(nullptr); | ||
| generateTextBoxVerticesAndIndices(x,y,textSize, text, &textPos, &colors, &tex0, &textIndices); | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x, y, textSize, text, positions, colors, tex0, &textIndices); | ||
|
|
||
| //GeometryDiagnosticUtils::dumpIndices( textIndices, 6 * text.size() ); | ||
|
|
||
| allocVertices(mesh, mesh->getVertexType(), text.size() * NumQuadVert, textPos, colors, tex0, access); | ||
| allocVertices(mesh, mesh->getVertexType(), text.size() * MeshUtilities::NumQuadVert, &positions[0], &colors[0], &tex0[0], access); |
There was a problem hiding this comment.
Improved text box allocation with MeshUtilities
The refactoring of allocTextBox to use MeshUtilities::generateTextBoxVerticesAndIndices simplifies the method and improves code organization. The use of Vec3Array and Vec2Array may also lead to better memory management.
However, there's a potential memory leak issue:
The textIndices pointer is allocated in MeshUtilities::generateTextBoxVerticesAndIndices but never freed. Consider adding a cleanup step:
// After creating the index buffer
mesh->createIndexBuffer(textIndices, size, IndexType::UnsignedShort, BufferAccessType::ReadOnly);
delete[] textIndices;| /// @brief Will return the GLSL version from a given string. | ||
| /// @param versionString [in] The version string. | ||
| /// @return The GLSL version. |
There was a problem hiding this comment.
Fix typo in function name
There's a typo in the function name getGlslVersionFromeString. It should be getGlslVersionFromString (remove the 'e' in 'Frome').
Apply this diff to fix the typo:
-GLSLVersion getGlslVersionFromeString(const c8 *versionString);
+GLSLVersion getGlslVersionFromString(const c8 *versionString);Committable suggestion was skipped due to low confidence.
| DrawCmd *drawCmd = alloc(); | ||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } | ||
|
|
||
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | ||
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | ||
| } | ||
|
|
||
| if (mFont == nullptr) { | ||
| mFont = FontService::getDefaultFont(); | ||
| } | ||
| drawCmd->UseFont = mFont; |
There was a problem hiding this comment.
Implement destructor to free allocated memory in DrawCmd.
The DrawCmd struct allocates memory for Vertices and Indices, but its destructor does not deallocate them, leading to potential memory leaks.
Update the DrawCmd destructor to deallocate Vertices and Indices:
/// The class destructor.
- ~DrawCmd() = default;
+ ~DrawCmd() {
+ delete[] Vertices;
+ delete[] Indices;
+ }Committable suggestion was skipped due to low confidence.
| PrimitiveType PrimType; ///< The primitive type | ||
| size_t NumVertices; ///< The number of vertices | ||
| RenderVert *Vertices; ///< The vertex buffer | ||
| size_t NumIndices; ///< Number of indices | ||
| ui16 *Indices; ///< The number of indices | ||
| Font *UseFont; ///< The used font | ||
|
|
||
| /// The class constructor. | ||
| DrawCmd() : | ||
| PrimType(PrimitiveType::Invalid), NumVertices(0u), Vertices(nullptr), NumIndices(0u), Indices(nullptr) {} | ||
| PrimType(PrimitiveType::Invalid), | ||
| NumVertices(0u), | ||
| Vertices(nullptr), | ||
| NumIndices(0u), | ||
| Indices(nullptr), | ||
| UseFont(nullptr) { | ||
| // empty | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
UseFont is Assigned but Not Utilized
- The
UseFontmember is assigned a value (drawCmd->UseFont = mFont;). - No read accesses or usages of
UseFontwere found in methods likedrawText.
🔗 Analysis chain
Verify the new UseFont member is properly utilized.
The new UseFont member variable in the DrawCmd struct is a good addition to support font selection for text rendering. However, ensure that it is properly utilized in the drawText method and any other relevant places.
Run the following script to verify the usage of UseFont:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify `UseFont` is properly utilized.
# Test: Search for read accesses to `UseFont`. Expect: Usage in `drawText` or similar methods.
rg --type cpp $'UseFont\b' -A 5
Length of output: 641
Script:
#!/bin/bash
# Description: Correctly verify `UseFont` is properly utilized.
# Test: Search for read accesses to `UseFont`. Expect: Usage in `drawText` or similar methods.
rg --type cpp 'UseFont\b' -A 5
Length of output: 1256
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (4)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
45-61: Approve changes toDrawCmdstruct with a minor correctionThe addition of
Font *UseFontand its initialization in the constructor are appropriate for the new text rendering functionality. However, there's a minor issue with the comment forIndices.Please update the comment for
Indicesto accurately describe its purpose:- ui16 *Indices; ///< The number of indices + ui16 *Indices; ///< The indices array
Line range hint
175-197: Incomplete implementation of font mesh creationThe new code for handling font meshes is a good addition, but the implementation is incomplete. The loop iterates through the draw commands and creates a material for each font, but it doesn't create or store the actual mesh.
Consider completing the implementation by creating and storing the font mesh:
if (mFont2MeshMap.isEmpty()) { if (hasTexts(mDrawCmdArray)) { for (size_t i = 0; i < mDrawCmdArray.size(); ++i) { const auto &dc = mDrawCmdArray[i]; const String &keyName = dc->UseFont->Name; const String meshName = "text." + keyName; Material *matFont = MaterialBuilder::createTextMaterial(keyName); + Mesh *fontMesh = new Mesh(meshName, VertexType::RenderVertex, IndexType::UnsignedShort); + fontMesh->setMaterial(matFont); + mFont2MeshMap[keyName] = fontMesh; } } }This change will create a new mesh for each font, set its material, and store it in the
mFont2MeshMapfor later use.
425-450: ApprovedrawRectrefactoring with optimization suggestionThe refactoring of the
drawRectmethod improves the control and clarity for drawing unfilled rectangles. The use of a constantthicknessenhances maintainability.Consider optimizing the unfilled rectangle drawing by using a single draw command with a line strip primitive type. This could improve performance, especially when drawing many unfilled rectangles. Here's a potential approach:
if (!filled) { DrawCmd *drawCmd = alloc(); drawCmd->PrimType = PrimitiveType::LineStrip; drawCmd->NumVertices = 5; // 4 corners + 1 to close the loop drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; // ... set up vertices for all four corners and the closing point ... drawCmd->NumIndices = 5; drawCmd->Indices = new ui16[drawCmd->NumIndices]; for (ui16 i = 0; i < 5; ++i) { drawCmd->Indices[i] = i; } mDrawCmdArray.add(drawCmd); }This approach would reduce memory allocations and draw calls, potentially improving performance.
Line range hint
1-505: Overall improvements with some areas needing attentionThe changes to the
CanvasRendererclass significantly enhance its capabilities, particularly in text rendering and 2D shape drawing. The new methods and data structures for handling fonts and text rendering are well-integrated into the existing codebase.While the overall implementation is good, there are a few areas that need attention:
- Memory management: Ensure all dynamically allocated memory is properly deallocated, particularly in the
drawTextmethod.- Complete the implementation of font mesh creation and usage in the
rendermethod.- Consider optimizing the
drawRectmethod for unfilled rectangles to use a single draw command.- Review the font fallback behavior in the
drawTextmethod to ensure it aligns with the intended functionality.These improvements will enhance the robustness and efficiency of the
CanvasRendererclass. Great work on expanding the 2D rendering capabilities!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- contrib/assimp (1 hunks)
- contrib/cppcore (1 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (9 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (3 hunks)
- src/Engine/RenderBackend/MaterialBuilder.cpp (2 hunks)
- src/Engine/RenderBackend/MaterialBuilder.h (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- contrib/cppcore
🚧 Files skipped from review as they are similar to previous changes (2)
- contrib/assimp
- src/Engine/RenderBackend/2D/CanvasRenderer.h
🧰 Additional context used
🔇 Additional comments (7)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
28-29: LGTM: New includes for text rendering supportThe addition of
FontService.handMeshUtilities.his appropriate for the new text rendering functionality implemented in this file.
306-308: LGTM: ConvenientdrawlineoverloadThe new
drawlineoverload that takesPoint2Diparameters is a good addition. It provides a more convenient way to draw lines when working withPoint2Diobjects and correctly delegates to the existing implementation.
359-361: LGTM: ConvenientdrawTriangleoverloadThe new
drawTriangleoverload that takesPoint2Diparameters is a good addition. It provides a more convenient way to draw triangles when working withPoint2Diobjects and correctly delegates to the existing implementation.
452-455: LGTM: NewselectFontmethodThe new
selectFontmethod is a good addition. It correctly sets themFontmember variable and marks the renderer as dirty, which is appropriate when changing the font.src/Engine/RenderBackend/MaterialBuilder.h (3)
59-59: Consistent use ofMaterialreturn type; ensure it is properly definedAs with the previous method,
createTexturedMaterialreturnsMaterial*. Please ensure thatMaterialis properly defined or aliased in the current scope to prevent compilation issues.
68-68: Consistent use ofMaterialreturn type; ensure it is properly definedThe method
createTexturedMaterialwith shader sources also returnsMaterial*. Ensure thatMaterialis properly defined or aliased.
76-76: EnsureMaterialis properly defined or aliased forcreateTextMaterialThe new method
createTextMaterialreturnsMaterial*. To avoid compilation errors, make sure thatMaterialis defined or aliased in this scope.
|
|
||
| if (dc.UseFont != nullptr) { | ||
| const String &fontKey = dc.UseFont->Name; | ||
| Mesh *text = nullptr; | ||
| if (mFont2MeshMap.hasKey(fontKey)) { | ||
| mFont2MeshMap.getValue(fontKey, text); | ||
| } | ||
| } else { | ||
| } | ||
|
|
||
| Debugging::MeshDiagnostic::dumpVertices(dc.Vertices, dc.NumVertices); | ||
| const ui32 lastIndex = mMesh->getLastIndex(); | ||
| renumberIndices(dc, numVertices); | ||
|
|
||
| //Debugging::MeshDiagnostic::dumpVertices(dc.Vertices, dc.NumVertices); |
There was a problem hiding this comment.
Incomplete text rendering implementation in the main rendering loop
The addition of text rendering logic in the main rendering loop is a good start, but the implementation is incomplete and may lead to inconsistent behavior.
Consider completing the implementation as follows:
- Handle text rendering:
if (dc.UseFont != nullptr) {
const String &fontKey = dc.UseFont->Name;
Mesh *text = nullptr;
if (mFont2MeshMap.hasKey(fontKey)) {
mFont2MeshMap.getValue(fontKey, text);
+ if (text != nullptr) {
+ // Add vertices and indices to the text mesh
+ text->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert));
+ text->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16));
+ text->addPrimitiveGroup(dc.NumIndices, dc.PrimType, 0);
+ }
}
} else {
+ // Handle non-text rendering (existing code for mMesh)
+ const ui32 lastIndex = mMesh->getLastIndex();
+ renumberIndices(dc, numVertices);
+ mMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert));
+ mMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16));
+ prim = dc.PrimType;
+ mMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices));
+ numVertices += dc.NumVertices;
+ numIndices += dc.NumIndices;
}- After the loop, add the text meshes to the render backend service:
for (const auto &pair : mFont2MeshMap) {
rbSrv->addMesh(pair.second, 0);
}These changes will ensure that both text and non-text rendering are handled correctly.
| static void renumberIndices(const DrawCmd &dc, ui16 offset) { | ||
| if (offset > 0) { | ||
| for (size_t j = 0; j < dc.NumIndices; ++j) { | ||
| dc.Indices[j] += static_cast<ui16>(offset); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static bool hasTexts(const DrawCmdArray &drawCmdArray) { | ||
| if (drawCmdArray.isEmpty()) { | ||
| return true; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < drawCmdArray.size(); ++i) { | ||
| if (drawCmdArray[i]->UseFont != nullptr) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Approve new utility functions with a suggestion
The new utility functions renumberIndices and hasTexts are well-implemented and support the enhanced rendering capabilities. However, there's a potential logic issue in hasTexts.
In the hasTexts function, it returns true if the drawCmdArray is empty. This might be counterintuitive as an empty array doesn't contain any text. Consider modifying the function as follows:
static bool hasTexts(const DrawCmdArray &drawCmdArray) {
if (drawCmdArray.isEmpty()) {
- return true;
+ return false;
}
for (size_t i = 0; i < drawCmdArray.size(); ++i) {
if (drawCmdArray[i]->UseFont != nullptr) {
return true;
}
}
return false;
}📝 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.
| static void renumberIndices(const DrawCmd &dc, ui16 offset) { | |
| if (offset > 0) { | |
| for (size_t j = 0; j < dc.NumIndices; ++j) { | |
| dc.Indices[j] += static_cast<ui16>(offset); | |
| } | |
| } | |
| } | |
| static bool hasTexts(const DrawCmdArray &drawCmdArray) { | |
| if (drawCmdArray.isEmpty()) { | |
| return true; | |
| } | |
| for (size_t i = 0; i < drawCmdArray.size(); ++i) { | |
| if (drawCmdArray[i]->UseFont != nullptr) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| static void renumberIndices(const DrawCmd &dc, ui16 offset) { | |
| if (offset > 0) { | |
| for (size_t j = 0; j < dc.NumIndices; ++j) { | |
| dc.Indices[j] += static_cast<ui16>(offset); | |
| } | |
| } | |
| } | |
| static bool hasTexts(const DrawCmdArray &drawCmdArray) { | |
| if (drawCmdArray.isEmpty()) { | |
| return false; | |
| } | |
| for (size_t i = 0; i < drawCmdArray.size(); ++i) { | |
| if (drawCmdArray[i]->UseFont != nullptr) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | ||
| if (text.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| if (mFont == nullptr) { | ||
| osre_debug(Tag, "No font selected."); | ||
| return; | ||
| } | ||
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | ||
| mapCoordinates(mResolution, x, y, x_model, y_model); | ||
| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *indices = nullptr; | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | ||
|
|
||
| DrawCmd *drawCmd = alloc(); | ||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } | ||
|
|
||
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | ||
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | ||
| } | ||
|
|
||
| if (mFont == nullptr) { | ||
| mFont = FontService::getDefaultFont(); | ||
| } | ||
| drawCmd->UseFont = mFont; | ||
|
|
||
| mDrawCmdArray.add(drawCmd); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Multiple Potential Memory Leaks Identified in CanvasRenderer.cpp
The recent review revealed not only a potential memory leak in the drawText method but also identified several other instances where memory is allocated without corresponding deallocations. This suggests a broader issue with memory management in the CanvasRenderer.cpp file.
Areas of Concern:
- Multiple
newallocations withoutdeletein methods such asdrawlineanddrawRect. - Potential for memory leaks due to unmanaged dynamically allocated memory.
Please address these memory management issues to ensure efficient resource utilization and prevent potential leaks.
🔗 Analysis chain
Approve new drawText method with suggestions
The new drawText method is a good addition for text rendering capabilities. The implementation is generally correct and follows the expected flow for text rendering.
However, there's a potential memory leak in the current implementation. The indices array allocated by MeshUtilities::generateTextBoxVerticesAndIndices is not deallocated. Add the following line after copying the indices to drawCmd->Indices:
delete[] indices;Additionally, the method falls back to the default font if mFont is null. Please verify if this is the intended behavior. If not, consider throwing an exception or returning early when no font is selected:
if (mFont == nullptr) {
osre_debug(Tag, "No font selected.");
return;
}Run the following script to check for other potential memory leaks in the drawText method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential memory leaks in drawText method
# Test: Search for new allocations and their corresponding deallocations
rg --type cpp 'new\s+\w+\[' src/Engine/RenderBackend/2D/CanvasRenderer.cpp -A 10
Length of output: 3352
| /// @return The build-in material instance will be returned. | ||
| static RenderBackend::Material *createBuildinMaterial( RenderBackend::VertexType type ); | ||
| /// @return The built-in material instance will be returned. | ||
| static Material *createBuildinMaterial( RenderBackend::VertexType type ); |
There was a problem hiding this comment.
Correct the spelling in method name createBuildinMaterial
The method name createBuildinMaterial contains a spelling error. It should be createBuiltinMaterial to correctly represent "built-in".
Apply the following diff to correct the method name:
-static Material *createBuildinMaterial( RenderBackend::VertexType type );
+static Material *createBuiltinMaterial( RenderBackend::VertexType type );📝 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.
| static Material *createBuildinMaterial( RenderBackend::VertexType type ); | |
| static Material *createBuiltinMaterial( RenderBackend::VertexType type ); |
Ensure Material is properly defined or aliased in this scope
The return type has been changed to Material*, but there is no definition or alias for Material without the RenderBackend:: namespace in this header file. This could lead to compilation errors if Material is not properly brought into scope.
Consider adding a using declaration at the top of the class:
+using RenderBackend::Material;Alternatively, fully qualify the return type:
-static Material *createBuiltinMaterial( RenderBackend::VertexType type );
+static RenderBackend::Material *createBuiltinMaterial( RenderBackend::VertexType type );Committable suggestion was skipped due to low confidence.
| return mat; | ||
| } | ||
|
|
||
| mat = materialCache->create(Render2DMat); |
There was a problem hiding this comment.
Use fontName When Creating the Material
In the createTextMaterial method, the material is being created with Render2DMat as its name (line 115):
mat = materialCache->create(Render2DMat);This can cause conflicts in the material cache since Render2DMat might already be used for other materials. Using the fontName as the material name ensures uniqueness and consistency.
Consider changing the material creation to use fontName:
- mat = materialCache->create(Render2DMat);
+ mat = materialCache->create(fontName);This change ensures that each font has its own material, preventing potential conflicts and incorrect material retrieval.
| const String vertex_2d = | ||
| getGLSLVersionString_400() + | ||
| getGLSLRenderVertexLayout() + | ||
| "out vec3 v_color0;\n" | ||
| "out vec2 v_texindex;\n" | ||
| "uniform mat4 Model;\n" | ||
| "uniform mat4 View;\n" | ||
| "uniform mat4 Projection;\n" | ||
| "void main() {\n" | ||
| " v_color0 = color0;\n" | ||
| " mat4 u_mvp = Projection * View * Model;\n" | ||
| " gl_Position = u_mvp * vec4(position, 1.0);\n" | ||
| " v_texindex = texcoord0;\n" | ||
| "}\n"; | ||
| const String fragment_2d = "#version 330 core\n" | ||
| "in vec2 v_texindex;\n" | ||
| "in vec3 v_color0;\n" | ||
| "out vec4 f_color;\n" | ||
| "uniform sampler2D u_texture;\n" | ||
| "void main() {\n" | ||
| " f_color = texture(u_texture, v_texindex);\n" | ||
| " if (f_color.r==0.0 || f_color.g ==0.0 || f_color.b==0.0)\n" | ||
| " f_color = vec4(v_color0,1);\n" | ||
| "}\n"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Duplication of Shader Source Code
The GLSL shader source strings vertex_2d and fragment_2d are defined identically in both the create2DMaterial (lines 65-89) and createTextMaterial (lines 116-142) methods. This duplication violates the DRY (Don't Repeat Yourself) principle and can lead to maintenance issues if the shader code needs to be updated.
Consider extracting the shader source code into shared constants or functions. Here's how you might refactor the code:
+// Define shader source strings once at the top or in a shared header/source file
+static const String vertex_2d_shader =
+ getGLSLVersionString_400() +
+ getGLSLRenderVertexLayout() +
+ "out vec3 v_color0;\n"
+ "out vec2 v_texindex;\n"
+ "uniform mat4 Model;\n"
+ "uniform mat4 View;\n"
+ "uniform mat4 Projection;\n"
+ "void main() {\n"
+ " v_color0 = color0;\n"
+ " mat4 u_mvp = Projection * View * Model;\n"
+ " gl_Position = u_mvp * vec4(position, 1.0);\n"
+ " v_texindex = texcoord0;\n"
+ "}\n";
+static const String fragment_2d_shader =
+ "#version 330 core\n"
+ "in vec2 v_texindex;\n"
+ "in vec3 v_color0;\n"
+ "out vec4 f_color;\n"
+ "uniform sampler2D u_texture;\n"
+ "void main() {\n"
+ " f_color = texture(u_texture, v_texindex);\n"
+ " if (f_color.r == 0.0 || f_color.g == 0.0 || f_color.b == 0.0)\n"
+ " f_color = vec4(v_color0, 1);\n"
+ "}\n";
Material *MaterialBuilder::create2DMaterial() {
// ...
- const String vertex_2d = /* shader code */;
- const String fragment_2d = /* shader code */;
+ const String &vertex_2d = vertex_2d_shader;
+ const String &fragment_2d = fragment_2d_shader;
// ...
}
Material *MaterialBuilder::createTextMaterial(const String &fontName) {
// ...
- const String vertex_2d = /* shader code */;
- const String fragment_2d = /* shader code */;
+ const String &vertex_2d = vertex_2d_shader;
+ const String &fragment_2d = fragment_2d_shader;
// ...
}This refactoring reduces code duplication, making it easier to manage and maintain the shader source code.
Also applies to: 116-142
| " if (f_color.r==0.0 || f_color.g ==0.0 || f_color.b==0.0)\n" | ||
| " f_color = vec4(v_color0,1);\n" | ||
| "}\n"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inefficient Conditional Checks in Fragment Shader
In the fragment shader code (lines 86-88 and 139-141), there is a conditional check for the color components:
if (f_color.r == 0.0 || f_color.g == 0.0 || f_color.b == 0.0)
f_color = vec4(v_color0, 1);Performing per-pixel conditional checks can negatively impact shader performance, especially if the condition is met frequently.
Consider optimizing the shader by pre-processing the texture or by rethinking the need for this condition. If the goal is to handle transparent pixels, you might use the alpha component or adjust the texture to avoid the need for runtime checks.
Alternatively, if the condition is necessary, consider using a threshold for floating-point comparisons:
const float epsilon = 0.001;
if (f_color.r < epsilon && f_color.g < epsilon && f_color.b < epsilon)
f_color = vec4(v_color0, 1);This accounts for floating-point precision errors and makes the comparison more reliable.
Also applies to: 139-141
Floating-Point Equality Comparison in Shader
Comparing floating-point values using the equality operator == can be unreliable due to floating-point precision limitations. In the fragment shader (lines 86-88 and 139-141):
if (f_color.r == 0.0 || f_color.g == 0.0 || f_color.b == 0.0)
f_color = vec4(v_color0, 1);To improve reliability, use a small epsilon value for comparison:
const float epsilon = 0.001;
if (f_color.r < epsilon && f_color.g < epsilon && f_color.b < epsilon)
f_color = vec4(v_color0, 1);This approach accounts for minor inaccuracies in floating-point calculations.
Also applies to: 139-141
| Material *MaterialBuilder::createTextMaterial(const String &fontName) { | ||
| if (fontName.empty()) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| MaterialBuilder::MaterialCache *materialCache = sData->mMaterialCache; | ||
| Material *mat = materialCache->find(fontName); | ||
| if (nullptr != mat) { | ||
| return mat; | ||
| } | ||
|
|
||
| mat = materialCache->create(Render2DMat); | ||
| const String vertex_2d = | ||
| getGLSLVersionString_400() + | ||
| getGLSLRenderVertexLayout() + | ||
| "out vec3 v_color0;\n" | ||
| "out vec2 v_texindex;\n" | ||
| "uniform mat4 Model;\n" | ||
| "uniform mat4 View;\n" | ||
| "uniform mat4 Projection;\n" | ||
| "void main() {\n" | ||
| " v_color0 = color0;\n" | ||
| " mat4 u_mvp = Projection * View * Model;\n" | ||
| " gl_Position = u_mvp * vec4(position, 1.0);\n" | ||
| " v_texindex = texcoord0;\n" | ||
| "}\n"; | ||
|
|
||
| const String fragment_2d = | ||
| "#version 330 core\n" | ||
| "in vec2 v_texindex;\n" | ||
| "in vec3 v_color0;\n" | ||
| "out vec4 f_color;\n" | ||
| "uniform sampler2D u_texture;\n" | ||
| "void main() {\n" | ||
| " f_color = texture(u_texture, v_texindex);\n" | ||
| " if (f_color.r==0.0 || f_color.g ==0.0 || f_color.b==0.0)\n" | ||
| " f_color = vec4(v_color0,1);\n" | ||
| "}\n"; | ||
|
|
||
| return nullptr; |
There was a problem hiding this comment.
Incomplete Implementation of createTextMaterial Method
The createTextMaterial method currently returns nullptr (line 143) without creating or setting up the material and shader properly. This indicates that the implementation is incomplete and will not function as intended when called.
To complete the implementation, you should create and configure the material and shader similarly to how it's done in the create2DMaterial method. Here's a suggested fix:
Material *MaterialBuilder::createTextMaterial(const String &fontName) {
if (fontName.empty()) {
return nullptr;
}
MaterialBuilder::MaterialCache *materialCache = sData->mMaterialCache;
Material *mat = materialCache->find(fontName);
if (nullptr != mat) {
return mat;
}
- mat = materialCache->create(Render2DMat);
+ mat = materialCache->create(fontName);
+ ShaderSourceArray shArray;
+ shArray[static_cast<ui32>(ShaderType::SH_VertexShaderType)] = vertex_2d;
+ shArray[static_cast<ui32>(ShaderType::SH_FragmentShaderType)] = fragment_2d;
+ mat->createShader(shArray);
+ if (nullptr != mat->m_shader) {
+ mat->m_shader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes());
+ addMaterialParameter(mat);
+ }
return mat;
}This ensures that the material is properly created, the shader is compiled and attached, and the necessary parameters are set up.
📝 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.
| Material *MaterialBuilder::createTextMaterial(const String &fontName) { | |
| if (fontName.empty()) { | |
| return nullptr; | |
| } | |
| MaterialBuilder::MaterialCache *materialCache = sData->mMaterialCache; | |
| Material *mat = materialCache->find(fontName); | |
| if (nullptr != mat) { | |
| return mat; | |
| } | |
| mat = materialCache->create(Render2DMat); | |
| const String vertex_2d = | |
| getGLSLVersionString_400() + | |
| getGLSLRenderVertexLayout() + | |
| "out vec3 v_color0;\n" | |
| "out vec2 v_texindex;\n" | |
| "uniform mat4 Model;\n" | |
| "uniform mat4 View;\n" | |
| "uniform mat4 Projection;\n" | |
| "void main() {\n" | |
| " v_color0 = color0;\n" | |
| " mat4 u_mvp = Projection * View * Model;\n" | |
| " gl_Position = u_mvp * vec4(position, 1.0);\n" | |
| " v_texindex = texcoord0;\n" | |
| "}\n"; | |
| const String fragment_2d = | |
| "#version 330 core\n" | |
| "in vec2 v_texindex;\n" | |
| "in vec3 v_color0;\n" | |
| "out vec4 f_color;\n" | |
| "uniform sampler2D u_texture;\n" | |
| "void main() {\n" | |
| " f_color = texture(u_texture, v_texindex);\n" | |
| " if (f_color.r==0.0 || f_color.g ==0.0 || f_color.b==0.0)\n" | |
| " f_color = vec4(v_color0,1);\n" | |
| "}\n"; | |
| return nullptr; | |
| Material *MaterialBuilder::createTextMaterial(const String &fontName) { | |
| if (fontName.empty()) { | |
| return nullptr; | |
| } | |
| MaterialBuilder::MaterialCache *materialCache = sData->mMaterialCache; | |
| Material *mat = materialCache->find(fontName); | |
| if (nullptr != mat) { | |
| return mat; | |
| } | |
| mat = materialCache->create(fontName); | |
| const String vertex_2d = | |
| getGLSLVersionString_400() + | |
| getGLSLRenderVertexLayout() + | |
| "out vec3 v_color0;\n" | |
| "out vec2 v_texindex;\n" | |
| "uniform mat4 Model;\n" | |
| "uniform mat4 View;\n" | |
| "uniform mat4 Projection;\n" | |
| "void main() {\n" | |
| " v_color0 = color0;\n" | |
| " mat4 u_mvp = Projection * View * Model;\n" | |
| " gl_Position = u_mvp * vec4(position, 1.0);\n" | |
| " v_texindex = texcoord0;\n" | |
| "}\n"; | |
| const String fragment_2d = | |
| "#version 330 core\n" | |
| "in vec2 v_texindex;\n" | |
| "in vec3 v_color0;\n" | |
| "out vec4 f_color;\n" | |
| "uniform sampler2D u_texture;\n" | |
| "void main() {\n" | |
| " f_color = texture(u_texture, v_texindex);\n" | |
| " if (f_color.r==0.0 || f_color.g ==0.0 || f_color.b==0.0)\n" | |
| " f_color = vec4(v_color0,1);\n" | |
| "}\n"; | |
| ShaderSourceArray shArray; | |
| shArray[static_cast<ui32>(ShaderType::SH_VertexShaderType)] = vertex_2d; | |
| shArray[static_cast<ui32>(ShaderType::SH_FragmentShaderType)] = fragment_2d; | |
| mat->createShader(shArray); | |
| if (nullptr != mat->m_shader) { | |
| mat->m_shader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | |
| addMaterialParameter(mat); | |
| } | |
| return mat; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
src/Engine/RenderBackend/2D/CanvasRenderer.h (1)
39-41: Add constructors toPoint2DiandPoint2DfConsider adding constructors to the
Point2DiandPoint2Dfstructs for easier initialization and better usability.Example implementation:
struct Point2Di { i32 X, Y; /// Coordinate components Point2Di(i32 x = 0, i32 y = 0) : X(x), Y(y) {} }; struct Point2Df { f32 X, Y; /// Coordinate components Point2Df(f32 x = 0.0f, f32 y = 0.0f) : X(x), Y(y) {} };Also applies to: 45-46
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
Line range hint
282-285: Correct indices assignment indrawlinemethodThere is a duplication in assigning
dc->Indices[0], anddc->Indices[1]is not assigned.Apply this diff to fix the indices assignment:
dc->Indices[0] = 0; - dc->Indices[0] = 1; + dc->Indices[1] = 1;
Line range hint
325-332: Correct indices assignment indrawTrianglemethodIn the unfilled case, indices are incorrectly assigned, leading to potential rendering issues.
Apply this diff to correct the indices:
dc->Indices[0] = 0; - dc->Indices[1] = 1; + dc->Indices[1] = 1; - dc->Indices[2] = 1; - dc->Indices[0] = 2; + dc->Indices[2] = 2; + dc->Indices[3] = 2; + dc->Indices[4] = 0; + dc->Indices[5] = 0;Alternatively, ensure that indices correctly define the edges of the triangle.
463-465: Remove redundant font checkThe check for
mFont == nullptris redundant, as it's already performed earlier in the method.You can safely remove this block to streamline the code.
494-496: Remove unnecessary font assignmentAssigning
mFontto the default font is unnecessary here, as the font should already be set or the method should have exited earlier.Consider removing this block to avoid unintended side effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (9 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.h (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/Engine/RenderBackend/2D/CanvasRenderer.h (4)
39-41: Standardize member variable naming inPoint2DiThe member variables
XandYinPoint2Diare capitalized. It's recommended to use lowercasexandyto follow C++ naming conventions and maintain consistency across the codebase.
147-147: Use smart pointers forselectFontto manage ownershipUsing a raw pointer for the
fontparameter inselectFont(Font *font)can lead to memory management issues. Consider usingstd::shared_ptr<Font>orstd::unique_ptr<Font>to manage the font object safely and clarify ownership semantics.
153-153: Clarify theStringtype indrawTextEnsure that
Stringis a defined and appropriate type in your codebase. If it's intended to bestd::string, consider usingstd::stringdirectly or aliasingStringappropriately to avoid confusion.
172-172: Use smart pointers formFontmember variableThe member variable
mFontis a raw pointer, which may lead to memory leaks or dangling pointers. Consider usingstd::unique_ptr<Font>orstd::shared_ptr<Font>to manage the font resource safely.src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
63-63:⚠️ Potential issueImplement destructor to deallocate allocated memory
The
DrawCmddestructor does not deallocate the dynamically allocatedVerticesandIndices, leading to potential memory leaks.Apply this diff to implement proper deallocation:
- ~DrawCmd() = default; + ~DrawCmd() { + delete[] Vertices; + delete[] Indices; + }Likely invalid or redundant comment.
| f32 X, Y; /// Coordinate components | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize member variable naming in Point2Df
The member variables X and Y in Point2Df are capitalized. Consider renaming them to lowercase x and y to adhere to C++ naming conventions and ensure consistency.
Apply this diff to rename the member variables:
struct Point2Df {
- f32 X, Y; /// Coordinate components
+ f32 x, y; /// Coordinate components
};📝 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.
| f32 X, Y; /// Coordinate components | |
| }; | |
| struct Point2Df { | |
| f32 x, y; /// Coordinate components | |
| }; |
| using Font2MeshMap = cppcore::THashMap<HashId, Mesh*>; | ||
| Font2MeshMap mFont2MeshMap; |
There was a problem hiding this comment.
Manage resource ownership in Font2MeshMap
The Font2MeshMap uses raw pointers to Mesh objects. To prevent potential memory leaks and ensure proper resource management, consider using smart pointers like std::shared_ptr<Mesh> within the map.
Example adjustment:
- using Font2MeshMap = cppcore::THashMap<HashId, Mesh*>;
+ using Font2MeshMap = cppcore::THashMap<HashId, std::shared_ptr<Mesh>>;📝 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.
| using Font2MeshMap = cppcore::THashMap<HashId, Mesh*>; | |
| Font2MeshMap mFont2MeshMap; | |
| using Font2MeshMap = cppcore::THashMap<HashId, std::shared_ptr<Mesh>>; | |
| Font2MeshMap mFont2MeshMap; |
| const HashId id = Hash::toHash(fontKey.c_str(), 10); | ||
| if (mFont2MeshMap.hasKey(id)) { | ||
| mFont2MeshMap.getValue(id, text); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Ensure text mesh is properly utilized
After retrieving the text mesh from mFont2MeshMap, it is not used. This could lead to incomplete rendering of text.
Consider adding code to utilize the text mesh:
if (text != nullptr) {
// Use the text mesh for rendering
mMesh->attachVertices(text->getVertexData(), text->getVertexSize());
mMesh->attachIndices(text->getIndexData(), text->getIndexSize());
// Additional necessary steps...
}| } | ||
| } else { |
There was a problem hiding this comment.
Handle the else case properly in rendering
The else block is empty, which may lead to missing rendering logic for non-text draw commands.
Ensure that rendering logic for non-text commands is correctly implemented within the else block.
| const ui32 thickness = 2; | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize unfilled rectangle drawing
Drawing unfilled rectangles by creating four separate DrawCmd instances can be inefficient.
Consider combining the edges into a single DrawCmd to reduce overhead:
DrawCmd *drawCmd = alloc();
// Set up vertices for all four edges
// Set up indices to draw the rectangle outline
mDrawCmdArray.add(drawCmd);| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | ||
|
|
||
| DrawCmd *drawCmd = alloc(); | ||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } | ||
|
|
||
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | ||
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Prevent memory leak by deallocating indices array
The indices array allocated by MeshUtilities::generateTextBoxVerticesAndIndices is not deallocated after use.
Add the following line after copying data to prevent a memory leak:
delete[] indices;| size_t NumVertices; ///< The number of vertices | ||
| RenderVert *Vertices; ///< The vertex buffer | ||
| size_t NumIndices; ///< Number of indices | ||
| ui16 *Indices; ///< The number of indices |
There was a problem hiding this comment.
Correct the comment for Indices member
The comment for Indices incorrectly states "The number of indices" when it should describe the pointer to the indices array.
Apply this diff to correct the comment:
- ui16 *Indices; ///< The number of indices
+ ui16 *Indices; ///< The indices array📝 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.
| ui16 *Indices; ///< The number of indices | |
| ui16 *Indices; ///< The indices array |
|
|
||
| static bool hasTexts(const DrawCmdArray &drawCmdArray) { | ||
| if (drawCmdArray.isEmpty()) { | ||
| return true; |
There was a problem hiding this comment.
Fix logic error in hasTexts function
Returning true when drawCmdArray is empty is counterintuitive, as an empty array cannot contain text commands.
Apply this diff to correct the logic:
if (drawCmdArray.isEmpty()) {
- return true;
+ return false;
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (2)
Line range hint
154-165: Correct deallocation method forDrawCmdobjectsThe
allocfunction usessAllocator.alloc()to allocateDrawCmdobjects from a pool allocator, but thedeallocfunction usesdeleteto deallocate them. This inconsistency can lead to undefined behavior and potential memory corruption. Objects allocated withsAllocator.alloc()should be deallocated usingsAllocator.free().Apply this diff to correct the deallocation:
void dealloc(DrawCmd *cmd) { if (cmd == nullptr) { osre_debug(Tag, "Invalid command to release"); return; } - delete cmd; + sAllocator.free(cmd); }
463-466: Avoid multiple checks formFontbeing nullThere's a redundant null check for
mFontat lines 463-466. SincemFontis already checked earlier, consider streamlining the error handling.You can remove the redundant check or handle it consistently throughout the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/Engine/Common/EventBus.cpp (2 hunks)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp (9 hunks)
- src/Engine/RenderBackend/2D/RenderPass2D.h (1 hunks)
- src/Engine/RenderBackend/Mesh/MeshUtilities.h (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Engine/Common/EventBus.cpp
- src/Engine/RenderBackend/2D/RenderPass2D.h
🧰 Additional context used
🔇 Additional comments (6)
src/Engine/RenderBackend/Mesh/MeshUtilities.h (3)
129-140: Use defined constants instead of magic numbers for consistencyIn the texture coordinate calculations, hard-coded values like
1.0f / 16.0fare used. SinceinvColandinvRoware already defined, use them for clarity and maintainability.Apply this diff:
tex0[VertexOffset + 1].x = s; -tex0[VertexOffset + 1].y = 1.0f - t + 1.0f / 16.0f; +tex0[VertexOffset + 1].y = 1.0f - t + invRow; -tex0[VertexOffset + 2].x = s + 1.0f / 16.0f; +tex0[VertexOffset + 2].x = s + invCol; tex0[VertexOffset + 2].y = 1.0f - t; -tex0[VertexOffset + 3].x = s + 1.0f / 16.0f; -tex0[VertexOffset + 3].y = 1.0f - t + 1.0f / 16.0f; +tex0[VertexOffset + 3].x = s + invCol; +tex0[VertexOffset + 3].y = 1.0f - t + invRow;
122-122: Remove commented-out code for cleaner codebaseThere are commented-out lines at 122 and 141:
//GeometryDiagnosticUtils::dumpTextBox( i, textPos, VertexOffset ); //GeometryDiagnosticUtils::dumpTextTex0Box(i, tex0, VertexOffset);Removing unused code improves readability and maintainability.
Also applies to: 141-141
54-59:⚠️ Potential issueCorrect spelling errors in function documentation
Similarly, the documentation for
getNumTextIndiceshas spelling errors:
- "glyphes" should be "glyphs"
Apply this diff to correct the typos:
-/// @brief Will calculate the number of indices needed for the given text glyphes. +/// @brief Calculates the number of indices needed for the given text glyphs. /// @param text The text for checking. /// @return The number of indicesLikely invalid or redundant comment.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (3)
368-370: Overloadeddrawlinemethod looks goodThe overloaded
drawlinemethod usingPoint2Dienhances usability by allowing points to be passed directly.
421-423: OverloadeddrawTrianglemethod looks goodThe addition of the
drawTrianglemethod that acceptsPoint2Diparameters improves the API consistency and usability.
494-497: 🛠️ Refactor suggestionRemove redundant null check for
mFontThe condition
if (mFont == nullptr)at line 494 is redundant becausemFontcannot benullptrat this point. Earlier in the method (lines 463-466), there is a check that returns from the function ifmFontisnullptr. Therefore, this check and the assignment of the default font can be removed to simplify the code.Apply this diff to remove the redundant code:
// Set the font used in the draw command - if (mFont == nullptr) { - mFont = FontService::getDefaultFont(); - } drawCmd->UseFont = mFont;Likely invalid or redundant comment.
| for (ui32 i = 0; i < text.size(); i++) { | ||
| const c8 ch = text[i]; | ||
| if (Tokenizer::isLineBreak(ch)) { | ||
| textCol = 0; | ||
| ++textRow; | ||
| continue; | ||
| } | ||
|
|
||
| const ui16 VertexOffset(static_cast<ui16>(i) * static_cast<ui16>(NumQuadVert)); | ||
| const f32 rowHeight(-1.0f * textRow * textSize); | ||
| positions[VertexOffset + 0].x = pos[0].x + (textCol * textSize); | ||
| positions[VertexOffset + 0].y = pos[0].y + rowHeight; | ||
| positions[VertexOffset + 0].z = 0; | ||
|
|
||
| positions[VertexOffset + 1].x = pos[1].x + (textCol * textSize); | ||
| positions[VertexOffset + 1].y = pos[1].y + rowHeight; | ||
| positions[VertexOffset + 1].z = 0; | ||
|
|
||
| positions[VertexOffset + 2].x = pos[2].x + (textCol * textSize); | ||
| positions[VertexOffset + 2].y = pos[2].y + rowHeight; | ||
| positions[VertexOffset + 2].z = 0; | ||
|
|
||
| positions[VertexOffset + 3].x = pos[3].x + (textCol * textSize); | ||
| positions[VertexOffset + 3].y = pos[3].y + rowHeight; | ||
| positions[VertexOffset + 3].z = 0; | ||
|
|
||
| //GeometryDiagnosticUtils::dumpTextBox( i, textPos, VertexOffset ); | ||
|
|
||
| const i32 column = (ch) % 16; | ||
| const i32 row = (ch) / 16; | ||
| const f32 s = column * invCol; | ||
| const f32 t = (row + 1) * invRow; | ||
|
|
||
| tex0[VertexOffset + 0].x = s; | ||
| tex0[VertexOffset + 0].y = 1.0f - t; | ||
|
|
||
| tex0[VertexOffset + 1].x = s; | ||
| tex0[VertexOffset + 1].y = 1.0f - t + 1.0f / 16.0f; | ||
|
|
||
| tex0[VertexOffset + 2].x = s + 1.0f / 16.0f; | ||
| tex0[VertexOffset + 2].y = 1.0f - t; | ||
|
|
||
| tex0[VertexOffset + 3].x = s + 1.0f / 16.0f; | ||
| tex0[VertexOffset + 3].y = 1.0f - t + 1.0f / 16.0f; | ||
|
|
||
| //GeometryDiagnosticUtils::dumpTextTex0Box(i, tex0, VertexOffset); | ||
| colors[VertexOffset + 0] = col[0]; | ||
| colors[VertexOffset + 1] = col[1]; | ||
| colors[VertexOffset + 2] = col[2]; | ||
| colors[VertexOffset + 3] = col[3]; | ||
| const ui32 IndexOffset(i * NumQuadIndices); | ||
| (*textIndices)[0 + IndexOffset] = 0 + VertexOffset; | ||
| (*textIndices)[1 + IndexOffset] = 2 + VertexOffset; | ||
| (*textIndices)[2 + IndexOffset] = 1 + VertexOffset; | ||
|
|
||
| (*textIndices)[3 + IndexOffset] = 1 + VertexOffset; | ||
| (*textIndices)[4 + IndexOffset] = 2 + VertexOffset; | ||
| (*textIndices)[5 + IndexOffset] = 3 + VertexOffset; | ||
| ++textCol; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the loop for better readability and maintainability
The loop within generateTextBoxVerticesAndIndices is substantial and can be broken down into smaller helper functions to enhance readability. Consider extracting operations like position calculation, texture coordinate assignment, and color assignment into separate functions.
For example:
void calculatePositions(size_t VertexOffset, /* other parameters */) {
// Position calculation logic
}
void assignTextureCoordinates(size_t VertexOffset, /* other parameters */) {
// Texture coordinate logic
}
void assignColors(size_t VertexOffset, /* other parameters */) {
// Color assignment logic
}Then, within the loop, call these helper functions accordingly.
| /// @brief Will calculate the numbr of vertices needed for the given string glyphes. | ||
| /// @param text The text for checking. | ||
| /// @return The number of vertives for the glyphes. | ||
| static size_t getNumTextVerts( const String &text ) { | ||
| const size_t NumTextVerts = NumQuadVert * text.size(); | ||
| return NumTextVerts; | ||
| } |
There was a problem hiding this comment.
Correct spelling errors in function documentation
The documentation for getNumTextVerts contains spelling mistakes:
- "numbr" should be "number"
- "glyphes" should be "glyphs"
- "vertives" should be "vertices"
Apply this diff to fix the typos:
-/// @brief Will calculate the numbr of vertices needed for the given string glyphes.
+/// @brief Calculates the number of vertices needed for the given string glyphs.
/// @param text The text for checking.
-/// @return The number of vertives for the glyphes.
+/// @return The number of vertices for the glyphs.📝 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 calculate the numbr of vertices needed for the given string glyphes. | |
| /// @param text The text for checking. | |
| /// @return The number of vertives for the glyphes. | |
| static size_t getNumTextVerts( const String &text ) { | |
| const size_t NumTextVerts = NumQuadVert * text.size(); | |
| return NumTextVerts; | |
| } | |
| /// @brief Calculates the number of vertices needed for the given string glyphs. | |
| /// @param text The text for checking. | |
| /// @return The number of vertices for the glyphs. | |
| static size_t getNumTextVerts( const String &text ) { | |
| const size_t NumTextVerts = NumQuadVert * text.size(); | |
| return NumTextVerts; | |
| } |
| /// @brief This class provides common utilities to generate different | ||
| /// kind of mesh topology and spatial data. | ||
| ///----------------------------------------------------------------- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve class documentation for clarity
The class documentation is incomplete and could be enhanced for better understanding. It currently reads:
/// @brief This class provides common utilities to generate different
/// kind of mesh topology and spatial data.
Consider rephrasing it to provide a clearer and more complete description.
Apply this diff to update the documentation:
-/// @brief This class provides common utilities to generate different
-/// kind of mesh topology and spatial data.
+/// @brief Provides utility functions for generating various mesh topologies and spatial data, particularly for text rendering.📝 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 This class provides common utilities to generate different | |
| /// kind of mesh topology and spatial data. | |
| ///----------------------------------------------------------------- | |
| /// @brief Provides utility functions for generating various mesh topologies and spatial data, particularly for text rendering. | |
| ///----------------------------------------------------------------- |
| const f32 invCol = 1.f / 16.f; | ||
| const f32 invRow = 1.f / 16.f; | ||
| ui32 textCol(0), textRow(0); | ||
| for (ui32 i = 0; i < text.size(); i++) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Match loop index type with container size type
The loop variable i is of type ui32, but it's compared with text.size(), which returns a size_t. To avoid potential issues on platforms where size_t and ui32 differ in size, use size_t for the loop index.
Apply this change:
-for (ui32 i = 0; i < text.size(); i++) {
+for (size_t i = 0; i < text.size(); i++) {📝 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 (ui32 i = 0; i < text.size(); i++) { | |
| for (size_t i = 0; i < text.size(); i++) { |
| osre_assert(nullptr != textIndices); | ||
|
|
||
| using namespace ::OSRE::Common; | ||
| glm::vec3 col[NumQuadVert] = {}; | ||
| col[0] = glm::vec3(0, 0, 0); | ||
| col[1] = glm::vec3(0, 0, 0); | ||
| col[2] = glm::vec3(0, 0, 0); | ||
| col[3] = glm::vec3(0, 0, 0); | ||
|
|
||
| glm::vec3 pos[NumQuadVert] = {}; | ||
| pos[0] = glm::vec3(x, y, 0); | ||
| pos[1] = glm::vec3(x, y + textSize, 0); | ||
| pos[2] = glm::vec3(x + textSize, y, 0); | ||
| pos[3] = glm::vec3(x + textSize, y + textSize, 0); | ||
|
|
||
| const size_t NumTextVerts = getNumTextVerts(text); | ||
| positions.resize(NumTextVerts); | ||
| colors.resize(NumTextVerts); | ||
| tex0.resize(NumTextVerts); | ||
| *textIndices = new ui16[getNumTextIndices(text)]; | ||
|
|
There was a problem hiding this comment.
Avoid raw pointer usage for safer memory management
The method allocates memory for *textIndices using new without a corresponding delete, leading to potential memory leaks. It also requires the caller to manage the allocated memory.
Consider using std::vector<ui16> to manage memory automatically. Update the function signature and implementation:
-static void generateTextBoxVerticesAndIndices(f32 x, f32 y, f32 textSize, const String &text,
- Vec3Array &positions, Vec3Array &colors, Vec2Array &tex0, ui16 **textIndices) {
- osre_assert(nullptr != textIndices);
+static void generateTextBoxVerticesAndIndices(f32 x, f32 y, f32 textSize, const String &text,
+ Vec3Array &positions, Vec3Array &colors, Vec2Array &tex0, std::vector<ui16> &textIndices) {
+ // No need for assertion since we're using a referenceModify the allocation:
- *textIndices = new ui16[getNumTextIndices(text)];
+ textIndices.resize(getNumTextIndices(text));Update index accesses:
- (*textIndices)[0 + IndexOffset] = 0 + VertexOffset;
+ textIndices[0 + IndexOffset] = 0 + VertexOffset;This change ensures automatic memory management and reduces the risk of leaks.
Committable suggestion was skipped due to low confidence.
| for (ui32 i = 0; i < text.size(); i++) { | ||
| const c8 ch = text[i]; | ||
| if (Tokenizer::isLineBreak(ch)) { | ||
| textCol = 0; | ||
| ++textRow; | ||
| continue; | ||
| } | ||
|
|
||
| const ui16 VertexOffset(static_cast<ui16>(i) * static_cast<ui16>(NumQuadVert)); | ||
| const f32 rowHeight(-1.0f * textRow * textSize); | ||
| positions[VertexOffset + 0].x = pos[0].x + (textCol * textSize); | ||
| positions[VertexOffset + 0].y = pos[0].y + rowHeight; | ||
| positions[VertexOffset + 0].z = 0; | ||
|
|
||
| positions[VertexOffset + 1].x = pos[1].x + (textCol * textSize); | ||
| positions[VertexOffset + 1].y = pos[1].y + rowHeight; | ||
| positions[VertexOffset + 1].z = 0; | ||
|
|
||
| positions[VertexOffset + 2].x = pos[2].x + (textCol * textSize); | ||
| positions[VertexOffset + 2].y = pos[2].y + rowHeight; | ||
| positions[VertexOffset + 2].z = 0; | ||
|
|
||
| positions[VertexOffset + 3].x = pos[3].x + (textCol * textSize); | ||
| positions[VertexOffset + 3].y = pos[3].y + rowHeight; | ||
| positions[VertexOffset + 3].z = 0; | ||
|
|
||
| //GeometryDiagnosticUtils::dumpTextBox( i, textPos, VertexOffset ); | ||
|
|
||
| const i32 column = (ch) % 16; | ||
| const i32 row = (ch) / 16; |
There was a problem hiding this comment.
Ensure correct handling of character types for extended character sets
The character ch is declared as c8, which may be a signed type. Using signed characters can cause issues when dealing with characters beyond the standard ASCII range (e.g., extended ASCII or Unicode).
Consider changing ch to an unsigned type:
-const c8 ch = text[i];
+const unsigned char ch = static_cast<unsigned char>(text[i]);This prevents negative values and ensures correct calculations for column and row.
📝 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 (ui32 i = 0; i < text.size(); i++) { | |
| const c8 ch = text[i]; | |
| if (Tokenizer::isLineBreak(ch)) { | |
| textCol = 0; | |
| ++textRow; | |
| continue; | |
| } | |
| const ui16 VertexOffset(static_cast<ui16>(i) * static_cast<ui16>(NumQuadVert)); | |
| const f32 rowHeight(-1.0f * textRow * textSize); | |
| positions[VertexOffset + 0].x = pos[0].x + (textCol * textSize); | |
| positions[VertexOffset + 0].y = pos[0].y + rowHeight; | |
| positions[VertexOffset + 0].z = 0; | |
| positions[VertexOffset + 1].x = pos[1].x + (textCol * textSize); | |
| positions[VertexOffset + 1].y = pos[1].y + rowHeight; | |
| positions[VertexOffset + 1].z = 0; | |
| positions[VertexOffset + 2].x = pos[2].x + (textCol * textSize); | |
| positions[VertexOffset + 2].y = pos[2].y + rowHeight; | |
| positions[VertexOffset + 2].z = 0; | |
| positions[VertexOffset + 3].x = pos[3].x + (textCol * textSize); | |
| positions[VertexOffset + 3].y = pos[3].y + rowHeight; | |
| positions[VertexOffset + 3].z = 0; | |
| //GeometryDiagnosticUtils::dumpTextBox( i, textPos, VertexOffset ); | |
| const i32 column = (ch) % 16; | |
| const i32 row = (ch) / 16; | |
| for (ui32 i = 0; i < text.size(); i++) { | |
| const unsigned char ch = static_cast<unsigned char>(text[i]); | |
| if (Tokenizer::isLineBreak(ch)) { | |
| textCol = 0; | |
| ++textRow; | |
| continue; | |
| } | |
| const ui16 VertexOffset(static_cast<ui16>(i) * static_cast<ui16>(NumQuadVert)); | |
| const f32 rowHeight(-1.0f * textRow * textSize); | |
| positions[VertexOffset + 0].x = pos[0].x + (textCol * textSize); | |
| positions[VertexOffset + 0].y = pos[0].y + rowHeight; | |
| positions[VertexOffset + 0].z = 0; | |
| positions[VertexOffset + 1].x = pos[1].x + (textCol * textSize); | |
| positions[VertexOffset + 1].y = pos[1].y + rowHeight; | |
| positions[VertexOffset + 1].z = 0; | |
| positions[VertexOffset + 2].x = pos[2].x + (textCol * textSize); | |
| positions[VertexOffset + 2].y = pos[2].y + rowHeight; | |
| positions[VertexOffset + 2].z = 0; | |
| positions[VertexOffset + 3].x = pos[3].x + (textCol * textSize); | |
| positions[VertexOffset + 3].y = pos[3].y + rowHeight; | |
| positions[VertexOffset + 3].z = 0; | |
| //GeometryDiagnosticUtils::dumpTextBox( i, textPos, VertexOffset ); | |
| const i32 column = (ch) % 16; | |
| const i32 row = (ch) / 16; |
| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *indices = nullptr; | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | ||
|
|
||
| DrawCmd *drawCmd = alloc(); | ||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | ||
| DrawCmd *drawCmd = nullptr; | ||
| if (filled) { | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| } else { | ||
| const ui32 thickness = 2; | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } |
There was a problem hiding this comment.
Include texture coordinates for text rendering
In the drawText method, texture coordinates (tex0) are generated by MeshUtilities::generateTextBoxVerticesAndIndices, but they are not assigned to drawCmd->Vertices. This may result in text not rendering correctly because texture coordinates are essential for mapping font textures onto the text geometry.
Apply this diff to include the texture coordinates in the vertices:
for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) {
drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4();
drawCmd->Vertices[posIndex].position.x = positions[posIndex].x;
drawCmd->Vertices[posIndex].position.y = positions[posIndex].y;
drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer);
+ drawCmd->Vertices[posIndex].tex0.x = tex0[posIndex].x;
+ drawCmd->Vertices[posIndex].tex0.y = tex0[posIndex].y;
}📝 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.
| Vec3Array positions; | |
| Vec3Array colors; | |
| Vec2Array tex0; | |
| ui16 *indices = nullptr; | |
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | |
| DrawCmd *drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = positions.size(); | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | |
| drawCmd->NumIndices = numIndices; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | |
| DrawCmd *drawCmd = nullptr; | |
| if (filled) { | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| } else { | |
| const ui32 thickness = 2; | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | |
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | |
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | |
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | |
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | |
| } | |
| Vec3Array positions; | |
| Vec3Array colors; | |
| Vec2Array tex0; | |
| ui16 *indices = nullptr; | |
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | |
| DrawCmd *drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = positions.size(); | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | |
| drawCmd->NumIndices = numIndices; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | |
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | |
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | |
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | |
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | |
| drawCmd->Vertices[posIndex].tex0.x = tex0[posIndex].x; | |
| drawCmd->Vertices[posIndex].tex0.y = tex0[posIndex].y; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
src/Engine/App/AppBase.h (2)
67-71: Enhance class documentation.While the documentation follows doxygen style, consider adding more details about:
- The purpose of the keyboard event listener
- Its role in the event handling system
- Key responsibilities and usage examples
Example enhancement:
-/// @brief This class implements the keyboard event listener. +/// @brief This class implements the keyboard event listener for handling and tracking keyboard input events. +/// +/// The KeyboardEventListener maintains the state of keyboard inputs, tracking which keys +/// are currently pressed and the last key that was pressed. It integrates with the +/// platform's event system to process keyboard events and provide a clean interface +/// for querying keyboard state.Also applies to: 78-79
264-266: Add missing documentation for getCanvasRenderer method.The method lacks documentation explaining its purpose and return value semantics.
Add documentation:
- /// @brief + /// @brief Returns the canvas renderer instance used for 2D rendering operations. + /// @return Pointer to the canvas renderer, or nullptr if no renderer is available. virtual RenderBackend::CanvasRenderer *getCanvasRenderer() const;src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
47-48: Fix incorrect comment forIndicesmember.The comment incorrectly describes the member as "The number of indices" when it's actually a pointer to the indices array.
- ui16 *Indices; ///< The number of indices + ui16 *Indices; ///< Pointer to the indices array
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Engine/App/AppBase.h(5 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(10 hunks)src/Engine/RenderBackend/MaterialBuilder.cpp(6 hunks)
🧰 Additional context used
🪛 cppcheck
src/Engine/RenderBackend/2D/CanvasRenderer.cpp
[warning] 282-282: Possible null pointer dereference
(nullPointer)
[warning] 285-285: Possible null pointer dereference
(nullPointer)
[warning] 286-286: Possible null pointer dereference
(nullPointer)
[warning] 288-288: Possible null pointer dereference
(nullPointer)
🔇 Additional comments (7)
src/Engine/App/AppBase.h (1)
52-54: LGTM!
Forward declarations are properly structured and follow naming conventions.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (2)
44-60:
Add proper memory management to prevent resource leaks.
The DrawCmd struct allocates memory for Vertices and Indices but lacks a destructor to free these resources. This will lead to memory leaks.
Add a destructor to properly clean up allocated memory:
+ /// The class destructor.
+ ~DrawCmd() {
+ delete[] Vertices;
+ delete[] Indices;
+ }Likely invalid or redundant comment.
219-231:
Fix logic error in hasTexts function.
The function incorrectly returns true when the array is empty, which is counterintuitive as an empty array cannot contain text.
static bool hasTexts(const CanvasRenderer::DrawCmdArray &drawCmdArray) {
if (drawCmdArray.isEmpty()) {
- return true;
+ return false;
}
// ... rest of the function
}Likely invalid or redundant comment.
src/Engine/RenderBackend/MaterialBuilder.cpp (4)
122-129: Ensure Proper Memory Management for mat->mTextures
The memory allocated for mat->mTextures needs to be properly managed to prevent memory leaks. Ensure that mat->mTextures is deleted appropriately when it's no longer needed, possibly by implementing a destructor or using smart pointers.
130-156: Avoid Duplication of Shader Source Code
The vertex and fragment shader source code in createTextMaterial are identical to those in create2DMaterial. To follow the DRY (Don't Repeat Yourself) principle, consider extracting the shader code into shared constants or functions.
153-154: Avoid Floating-Point Equality Comparison in Shader
Comparing floating-point values using == can be unreliable due to precision limitations. Use a small epsilon value for comparisons to improve accuracy.
107-159: Incomplete Implementation of createTextMaterial Method
The createTextMaterial method returns the material without setting up the shader. Ensure that you create and assign the shader to mat, similar to how it's done in create2DMaterial.
| void renderFontMesh(const DrawCmd &dc, CanvasRenderer::Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) { | ||
| MeshInfo info; | ||
| const String &fontKey = dc.UseFont->Name; | ||
| Mesh *textMesh = nullptr; | ||
| CanvasRenderer::Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey); | ||
| if (it != font2MeshMap.end()) { | ||
| textMesh = it->second; | ||
| if (textMesh == nullptr) { | ||
| osre_debug(Tag, "Invalid font mesh detected."); | ||
| return; | ||
| } else { | ||
| getMeshInfo(textMesh, meshInfoArray, info); | ||
| } | ||
| } | ||
|
|
||
| const ui32 lastIndex = textMesh->getLastIndex(); | ||
| renumberIndices(dc, info.mNumVertices); | ||
|
|
||
| textMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert)); | ||
| textMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16)); | ||
| info.mPrim = dc.PrimType; | ||
| textMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices)); | ||
| info.mNumVertices += dc.NumVertices; | ||
| info.mNumIndices += dc.NumIndices; | ||
| } |
There was a problem hiding this comment.
Add null pointer checks in renderFontMesh.
The function has multiple potential null pointer dereferences that could crash the application.
Add necessary checks:
void renderFontMesh(const DrawCmd &dc, CanvasRenderer::Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) {
+ if (dc.UseFont == nullptr) {
+ osre_debug(Tag, "Invalid font detected.");
+ return;
+ }
MeshInfo info;
const String &fontKey = dc.UseFont->Name;
Mesh *textMesh = nullptr;
CanvasRenderer::Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey);
if (it != font2MeshMap.end()) {
textMesh = it->second;
+ if (textMesh == nullptr) {
+ osre_debug(Tag, "Invalid font mesh detected.");
+ return;
+ }
getMeshInfo(textMesh, meshInfoArray, info);
}
+ if (textMesh == nullptr) {
+ osre_debug(Tag, "Font mesh not found.");
+ return;
+ }
// ... rest of the function
}📝 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.
| void renderFontMesh(const DrawCmd &dc, CanvasRenderer::Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) { | |
| MeshInfo info; | |
| const String &fontKey = dc.UseFont->Name; | |
| Mesh *textMesh = nullptr; | |
| CanvasRenderer::Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey); | |
| if (it != font2MeshMap.end()) { | |
| textMesh = it->second; | |
| if (textMesh == nullptr) { | |
| osre_debug(Tag, "Invalid font mesh detected."); | |
| return; | |
| } else { | |
| getMeshInfo(textMesh, meshInfoArray, info); | |
| } | |
| } | |
| const ui32 lastIndex = textMesh->getLastIndex(); | |
| renumberIndices(dc, info.mNumVertices); | |
| textMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert)); | |
| textMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16)); | |
| info.mPrim = dc.PrimType; | |
| textMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices)); | |
| info.mNumVertices += dc.NumVertices; | |
| info.mNumIndices += dc.NumIndices; | |
| } | |
| void renderFontMesh(const DrawCmd &dc, CanvasRenderer::Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) { | |
| if (dc.UseFont == nullptr) { | |
| osre_debug(Tag, "Invalid font detected."); | |
| return; | |
| } | |
| MeshInfo info; | |
| const String &fontKey = dc.UseFont->Name; | |
| Mesh *textMesh = nullptr; | |
| CanvasRenderer::Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey); | |
| if (it != font2MeshMap.end()) { | |
| textMesh = it->second; | |
| if (textMesh == nullptr) { | |
| osre_debug(Tag, "Invalid font mesh detected."); | |
| return; | |
| } | |
| getMeshInfo(textMesh, meshInfoArray, info); | |
| } | |
| if (textMesh == nullptr) { | |
| osre_debug(Tag, "Font mesh not found."); | |
| return; | |
| } | |
| const ui32 lastIndex = textMesh->getLastIndex(); | |
| renumberIndices(dc, info.mNumVertices); | |
| textMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert)); | |
| textMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16)); | |
| info.mPrim = dc.PrimType; | |
| textMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices)); | |
| info.mNumVertices += dc.NumVertices; | |
| info.mNumIndices += dc.NumIndices; | |
| } |
🧰 Tools
🪛 cppcheck
[warning] 282-282: Possible null pointer dereference
(nullPointer)
[warning] 285-285: Possible null pointer dereference
(nullPointer)
[warning] 286-286: Possible null pointer dereference
(nullPointer)
[warning] 288-288: Possible null pointer dereference
(nullPointer)
| if (nullptr != mat->mShader) { | ||
| mat->mShader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | ||
| addMaterialParameter(mat); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use getShader() Instead of Direct Member Access
For consistency and better encapsulation, use mat->getShader() instead of accessing mat->mShader directly.
Apply this diff:
- if (nullptr != mat->mShader) {
- mat->mShader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes());
+ if (nullptr != mat->getShader()) {
+ mat->getShader()->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes());
addMaterialParameter(mat);
}📝 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 (nullptr != mat->mShader) { | |
| mat->mShader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | |
| addMaterialParameter(mat); | |
| } | |
| if (nullptr != mat->getShader()) { | |
| mat->getShader()->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | |
| addMaterialParameter(mat); | |
| } |
| if (nullptr != mat->mShader) { | ||
| if (type == VertexType::ColorVertex) { | ||
| mat->m_shader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); | ||
| mat->mShader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); | ||
| } else if (type == VertexType::RenderVertex) { | ||
| mat->m_shader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | ||
| mat->mShader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use getShader() Instead of Direct Member Access
Consistently use the accessor method getShader() instead of direct member access to mat->mShader.
Apply this diff:
- if (nullptr != mat->mShader) {
+ if (nullptr != mat->getShader()) {
if (type == VertexType::ColorVertex) {
- mat->mShader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes());
+ mat->getShader()->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes());
} else if (type == VertexType::RenderVertex) {
- mat->mShader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes());
+ mat->getShader()->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes());
}
addMaterialParameter(mat);
}📝 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 (nullptr != mat->mShader) { | |
| if (type == VertexType::ColorVertex) { | |
| mat->m_shader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); | |
| mat->mShader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); | |
| } else if (type == VertexType::RenderVertex) { | |
| mat->m_shader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | |
| mat->mShader->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); | |
| if (nullptr != mat->getShader()) { | |
| if (type == VertexType::ColorVertex) { | |
| mat->getShader()->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); | |
| } else if (type == VertexType::RenderVertex) { | |
| mat->getShader()->addVertexAttributes(RenderVert::getAttributes(), RenderVert::getNumAttributes()); |
| Shader *shader = mat->mShader; | ||
| if (shader == nullptr) { | ||
| osre_assert(false); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use getShader() Instead of Direct Member Access
Accessing mat->mShader directly bypasses encapsulation. Since the Material class provides a getShader() method, consider using it to maintain encapsulation and consistency.
Apply this diff:
- Shader *shader = mat->mShader;
+ Shader *shader = mat->getShader();
if (shader == nullptr) {
osre_assert(false);
return;
}📝 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.
| Shader *shader = mat->mShader; | |
| if (shader == nullptr) { | |
| osre_assert(false); | |
| return; | |
| Shader *shader = mat->getShader(); | |
| if (shader == nullptr) { | |
| osre_assert(false); | |
| return; |
| Shader *shader = mat->mShader; | ||
| if (shader != nullptr) { | ||
| if (type == VertexType::ColorVertex) { | ||
| mat->m_shader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); | ||
| mat->mShader->addVertexAttributes(ColorVert::getAttributes(), ColorVert::getNumAttributes()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use getShader() Instead of Direct Member Access
Accessing mat->mShader directly bypasses encapsulation. Use mat->getShader() for better encapsulation and consistency.
Apply this diff:
- Shader *shader = mat->mShader;
+ Shader *shader = mat->getShader();
if (shader != nullptr) {
if (type == VertexType::ColorVertex) {Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
src/Engine/RenderBackend/Text/TextRenderer.cpp (3)
9-26: Remove commented-out code.Commented-out code should not be committed to the repository. If this code is needed for future reference, consider:
- Moving it to a separate development branch
- Creating a GitHub issue to track the implementation
- Adding it to the documentation if it serves as an example
108-112: Add error logging toonCreate.While the method correctly initializes the default font, it should log an error message when the font initialization fails.
Consider this improvement:
bool TextRenderer::onCreate() { mDefaultFont = FontService::getDefaultFont(); - + if (mDefaultFont == nullptr) { + osre_debug(Tag, "Failed to initialize default font."); + } return (mDefaultFont != nullptr); }
1-116: Consider architectural improvements.While the overall architecture is sound, consider these improvements:
- Use
std::unique_ptrfor mesh ownership inFont2MeshMapto prevent memory leaks- Add API documentation for public methods
- Consider implementing a font cache with an LRU policy to manage memory usage
🧰 Tools
🪛 cppcheck
[warning] 58-58: Possible null pointer dereference
(nullPointer)
[warning] 61-61: Possible null pointer dereference
(nullPointer)
[warning] 62-62: Possible null pointer dereference
(nullPointer)
[warning] 64-64: Possible null pointer dereference
(nullPointer)
src/Engine/RenderBackend/2D/CanvasRenderer.h (1)
44-45: Enhance class documentation with more details.Consider adding:
- Thread safety guarantees
- Typical usage examples
- Relationship with other rendering components
Add this documentation above the class declaration:
/// @brief This class implements a canvas renderer. +/// @details The CanvasRenderer provides 2D rendering capabilities including: +/// - Basic shapes (lines, triangles, rectangles) +/// - Text rendering with font support +/// - Layer-based rendering +/// @note Thread Safety: This class is not thread-safe and should only be accessed from the rendering threadsrc/Engine/RenderBackend/RenderCommon.h (2)
933-942: Align RenderPassType with established enum pattern.For consistency with other enums in the file, consider adding Invalid and Count values:
enum class RenderPassType : ui32 { + Invalid = -1, StaticRenderPass, - UiRenderPass + UiRenderPass, + Count };
1041-1046: Enhance Font struct with style enumeration.Consider using an enum for font styles to improve type safety and clarity:
+/// @brief Enumeration for font styles +enum class FontStyle { + Normal, + Bold, + Italic, + BoldItalic +}; struct Font { String Name; ///< Font name i32 Size; ///< Font size - i32 Style; ///< Font style + FontStyle Style;///< Font style };src/Engine/RenderBackend/Text/TextRenderer.h (5)
25-29: Provide meaningful documentation for constructor and destructorThe
/// @briefcomments for the constructorTextRenderer()and the destructor~TextRenderer()are empty. Adding meaningful descriptions will enhance code readability and maintainability.
31-31: Add documentation for theaddTextmethodThe
addTextmethod lacks documentation. Providing a brief description and parameter details will improve code understanding.
33-33: Document thepreRendermethodThe
preRendermethod is missing documentation. Consistently documenting all methods enhances code readability.
43-44: Add documentation for theonCreatemethodThe
onCreatemethod lacks documentation. Including a description will help clarify its purpose and usage.
14-19: Provide comments for theMeshInfostruct and its membersThe
MeshInfostruct and its member variables are undocumented. Adding brief comments for the struct and each member will improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/Engine/CMakeLists.txt(3 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(9 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.h(2 hunks)src/Engine/RenderBackend/RenderCommon.h(13 hunks)src/Engine/RenderBackend/Text/TextRenderer.cpp(1 hunks)src/Engine/RenderBackend/Text/TextRenderer.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/CMakeLists.txt
🧰 Additional context used
🪛 cppcheck
src/Engine/RenderBackend/Text/TextRenderer.cpp
[warning] 58-58: Possible null pointer dereference
(nullPointer)
[warning] 61-61: Possible null pointer dereference
(nullPointer)
[warning] 62-62: Possible null pointer dereference
(nullPointer)
[warning] 64-64: Possible null pointer dereference
(nullPointer)
🔇 Additional comments (9)
src/Engine/RenderBackend/Text/TextRenderer.cpp (1)
27-40: LGTM!
The getMeshInfo function is well-implemented with proper null checks and a clear return value indicating success or failure.
src/Engine/RenderBackend/2D/CanvasRenderer.h (2)
46-46: LGTM: Class structure and inheritance are well-defined.
The public inheritance from IRenderPath is appropriate for this interface implementation.
167-169: Consider ownership semantics for member variables.
The ownership and lifecycle of mFont and mMesh need to be clearly defined.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
183-195:
Fix logic error in hasTexts function.
The function incorrectly returns true when the array is empty, which is counterintuitive as an empty array cannot contain text commands.
Apply this fix before uncommenting:
static bool hasTexts(const CanvasRenderer::DrawCmdArray &drawCmdArray) {
if (drawCmdArray.isEmpty()) {
- return true;
+ return false;
}
for (size_t i = 0; i < drawCmdArray.size(); ++i) {
if (drawCmdArray[i]->UseFont != nullptr) {
return true;
}
}
return false;
}Likely invalid or redundant comment.
src/Engine/RenderBackend/RenderCommon.h (4)
62-68: LGTM! Consistent enum structure improvements.
The standardization of enum classes with Invalid = -1 and Count values improves type safety and enables better range checking.
Also applies to: 73-77, 82-85, 90-93, 98-102, 110-115, 120-125, 130-136, 141-145, 162-167, 172-184, 189-206, 211-220
1026-1028: Function name typo already reported.
The typo in getGlslVersionFromeString has been previously reported and needs to be addressed.
991-991:
Fix typo in LifetimeState enum value.
There's a typo in the state name "Detroyed".
- mLifetimeState = LifetimeState::Detroyed;
+ mLifetimeState = LifetimeState::Destroyed;Likely invalid or redundant comment.
984-987:
Fix state check in destroy() method.
The destroy() method checks for Running state, but the create() method transitions to Created state. This mismatch could prevent proper cleanup.
- if (mLifetimeState != LifetimeState::Running) {
+ if (mLifetimeState != LifetimeState::Created) {
return false;
}Likely invalid or redundant comment.
src/Engine/RenderBackend/Text/TextRenderer.h (1)
12-12: Ensure consistent definition and usage of String type
The type String is used but not defined in this context. Verify that String is appropriately defined and consistently used, or consider using std::string if suitable.
To verify the definition and usage of String, you can run:
✅ Verification successful
Based on the search results, I can generate the final response since I found the definition of String type in src/Engine/Common/osre_common.h:
String type is properly defined and consistently used
The String type is defined as an alias for std::string in src/Engine/Common/osre_common.h and is consistently used throughout the codebase as a common string type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the definition and usage of `String` in the codebase.
# Search for the definition of `String`. Expect to find a typedef or using declaration.
rg -A 2 'using\s+String'
# Search for all usages of `String` to ensure consistency.
rg '\bString\b'
Length of output: 98125
| void TextRenderer::addText(i32 x, i32 y, Font *font, const String &text) { | ||
| Font *usedFont = font; | ||
| if (usedFont == nullptr) { | ||
| usedFont = FontService::getDefaultFont(); | ||
| } | ||
|
|
||
| Mesh *textMesh = nullptr; | ||
| Font2MeshMap::const_iterator it = mFont2MeshMap.find(usedFont->Name); | ||
| if (it == mFont2MeshMap.end()) { | ||
| textMesh = new Mesh(usedFont->Name + ".mesh", VertexType::RenderVertex, IndexType::UnsignedShort); | ||
| mFont2MeshMap[usedFont->Name] = textMesh; | ||
| } else { | ||
| textMesh = it->second; | ||
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Complete the implementation of addText method.
The method currently creates/retrieves the mesh but doesn't implement the actual text rendering logic. Consider:
- Adding vertex generation for text glyphs
- Updating mesh indices
- Handling text positioning (x, y coordinates)
Would you like me to help implement the missing text rendering functionality?
| void TextRenderer::preRender(RenderBackendService* rbSrv) { | ||
|
|
||
| } | ||
|
|
||
| void TextRenderer::render(RenderBackendService* rbSrv) { | ||
|
|
||
| } | ||
|
|
||
| void TextRenderer::postRender(RenderBackendService* rbSrv) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement the render pipeline methods.
The preRender, render, and postRender methods are empty. These are crucial parts of the rendering pipeline and should be implemented to:
- Set up rendering state in
preRender - Perform actual text rendering in
render - Clean up resources in
postRender
Would you like assistance in implementing these methods?
| void renderFontMesh(const DrawCmd &dc, Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) { | ||
| MeshInfo info; | ||
| const String &fontKey = dc.UseFont->Name; | ||
| Mesh *textMesh = nullptr; | ||
| Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey); | ||
| if (it != font2MeshMap.end()) { | ||
| textMesh = it->second; | ||
| if (textMesh == nullptr) { | ||
| osre_debug(Tag, "Invalid font mesh detected."); | ||
| return; | ||
| } else { | ||
| getMeshInfo(textMesh, meshInfoArray, info); | ||
| } | ||
| } | ||
|
|
||
| const ui32 lastIndex = textMesh->getLastIndex(); | ||
| renumberIndices(dc, info.mNumVertices); | ||
|
|
||
| textMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert)); | ||
| textMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16)); | ||
| info.mPrim = dc.PrimType; | ||
| textMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices)); | ||
| info.mNumVertices += dc.NumVertices; | ||
| info.mNumIndices += dc.NumIndices; | ||
| } |
There was a problem hiding this comment.
Add null checks to prevent potential crashes.
The renderFontMesh function has several potential null pointer issues:
- No validation of
dc.UseFontbefore accessing its Name - Using
textMeshafter the null check without re-validating - No validation of
dc.Verticesanddc.Indicesbefore use
Consider applying this fix:
void renderFontMesh(const DrawCmd &dc, Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) {
+ if (dc.UseFont == nullptr) {
+ osre_debug(Tag, "Invalid font detected.");
+ return;
+ }
+ if (dc.Vertices == nullptr || dc.Indices == nullptr) {
+ osre_debug(Tag, "Invalid vertex or index data detected.");
+ return;
+ }
MeshInfo info;
const String &fontKey = dc.UseFont->Name;
Mesh *textMesh = nullptr;
Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey);
if (it != font2MeshMap.end()) {
textMesh = it->second;
if (textMesh == nullptr) {
osre_debug(Tag, "Invalid font mesh detected.");
return;
- } else {
- getMeshInfo(textMesh, meshInfoArray, info);
}
+ if (!getMeshInfo(textMesh, meshInfoArray, info)) {
+ osre_debug(Tag, "Failed to get mesh info.");
+ return;
+ }
+ } else {
+ osre_debug(Tag, "Font mesh not found in map.");
+ return;
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 cppcheck
[warning] 58-58: Possible null pointer dereference
(nullPointer)
[warning] 61-61: Possible null pointer dereference
(nullPointer)
[warning] 62-62: Possible null pointer dereference
(nullPointer)
[warning] 64-64: Possible null pointer dereference
(nullPointer)
| /// @brief Will select a font. | ||
| /// @param font The font to select. | ||
| void selectFont(Font *font); | ||
|
|
||
| /// @brief Will draw a text using the current font. | ||
| /// @param x The x position. | ||
| /// @param y The y position. | ||
| /// @param text The text to draw. | ||
| void drawText(i32 x, i32 y, const String &text); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error handling across methods.
Consider:
- Using return values consistently to indicate success/failure
- Adding error states for common failure cases (e.g., invalid font, text too long)
- Documenting error conditions in method comments
Example improvement for drawText:
- void drawText(i32 x, i32 y, const String &text);
+ /// @return true if text was drawn successfully, false otherwise
+ /// @throws std::runtime_error if no font is selected
+ bool drawText(i32 x, i32 y, const String &text);Committable suggestion skipped: line range outside the PR's diff.
| /* if (dc.UseFont != nullptr) { | ||
| renderFontMesh(dc, mFont2MeshMap, meshInfoArray); | ||
| continue; | ||
| }*/ |
There was a problem hiding this comment.
Add null checks before accessing font-related members.
When uncommenting the font rendering code, ensure proper null checks are in place to prevent crashes.
Add these checks:
if (dc.UseFont != nullptr) {
+ if (dc.UseFont->Name.empty()) {
+ osre_debug(Tag, "Invalid font name detected.");
+ continue;
+ }
renderFontMesh(dc, mFont2MeshMap, meshInfoArray);
continue;
}Committable suggestion skipped: line range outside the PR's diff.
| static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) { | ||
| i32 x_clipped, y_clipped; | ||
| f32 x_model, y_model; | ||
|
|
||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = 6; | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
|
|
||
| clip(resolution, x, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[0].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[0].position.x = x_model; | ||
| drawCmd->Vertices[0].position.y = y_model; | ||
| drawCmd->Vertices[0].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x+w, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[1].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[1].position.x = x_model; | ||
| drawCmd->Vertices[1].position.y = y_model; | ||
| drawCmd->Vertices[1].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[2].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[2].position.x = x_model; | ||
| drawCmd->Vertices[2].position.y = y_model; | ||
| drawCmd->Vertices[2].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[3].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[3].position.x = x_model; | ||
| drawCmd->Vertices[3].position.y = y_model; | ||
| drawCmd->Vertices[3].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[4].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[4].position.x = x_model; | ||
| drawCmd->Vertices[4].position.y = y_model; | ||
| drawCmd->Vertices[4].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[5].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[5].position.x = x_model; | ||
| drawCmd->Vertices[5].position.y = y_model; | ||
| drawCmd->Vertices[5].position.z = static_cast<f32>(-layer); | ||
|
|
||
| drawCmd->NumIndices = 6; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
| drawCmd->Indices[0] = 0; | ||
| drawCmd->Indices[1] = 2; | ||
| drawCmd->Indices[2] = 1; | ||
|
|
||
| drawCmd->Indices[3] = 3; | ||
| drawCmd->Indices[4] = 5; | ||
| drawCmd->Indices[5] = 4; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize rectangle vertex creation for better efficiency and maintainability.
The current implementation has several inefficiencies:
- Uses 6 vertices when 4 would suffice (vertices can be shared between triangles)
- Contains repetitive vertex setup code
- Could be more maintainable with a loop-based approach
Consider this optimized implementation:
static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) {
drawCmd->PrimType = PrimitiveType::TriangleList;
- drawCmd->NumVertices = 6;
+ drawCmd->NumVertices = 4;
drawCmd->Vertices = new RenderVert[drawCmd->NumVertices];
+ const Vec4 color = penColor.toVec4();
+ const f32 z = static_cast<f32>(-layer);
+
+ // Define corners: top-left, top-right, bottom-right, bottom-left
+ const i32 coords[4][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}};
+
+ for (ui32 i = 0; i < 4; ++i) {
+ i32 x_clipped, y_clipped;
+ f32 x_model, y_model;
+
+ clip(resolution, coords[i][0], coords[i][1], x_clipped, y_clipped);
+ mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model);
+
+ drawCmd->Vertices[i].color0 = color;
+ drawCmd->Vertices[i].position = Vec3(x_model, y_model, z);
+ }
drawCmd->NumIndices = 6;
drawCmd->Indices = new ui16[drawCmd->NumIndices];
- drawCmd->Indices[0] = 0;
- drawCmd->Indices[1] = 2;
- drawCmd->Indices[2] = 1;
- drawCmd->Indices[3] = 3;
- drawCmd->Indices[4] = 5;
- drawCmd->Indices[5] = 4;
+ // Define indices for two triangles (0,1,2) and (2,3,0)
+ const ui16 indices[6] = {0, 1, 2, 2, 3, 0};
+ memcpy(drawCmd->Indices, indices, sizeof(ui16) * 6);
}📝 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.
| static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) { | |
| i32 x_clipped, y_clipped; | |
| f32 x_model, y_model; | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = 6; | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| clip(resolution, x, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[0].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[0].position.x = x_model; | |
| drawCmd->Vertices[0].position.y = y_model; | |
| drawCmd->Vertices[0].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x+w, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[1].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[1].position.x = x_model; | |
| drawCmd->Vertices[1].position.y = y_model; | |
| drawCmd->Vertices[1].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[2].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[2].position.x = x_model; | |
| drawCmd->Vertices[2].position.y = y_model; | |
| drawCmd->Vertices[2].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[3].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[3].position.x = x_model; | |
| drawCmd->Vertices[3].position.y = y_model; | |
| drawCmd->Vertices[3].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[4].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[4].position.x = x_model; | |
| drawCmd->Vertices[4].position.y = y_model; | |
| drawCmd->Vertices[4].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[5].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[5].position.x = x_model; | |
| drawCmd->Vertices[5].position.y = y_model; | |
| drawCmd->Vertices[5].position.z = static_cast<f32>(-layer); | |
| drawCmd->NumIndices = 6; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| drawCmd->Indices[0] = 0; | |
| drawCmd->Indices[1] = 2; | |
| drawCmd->Indices[2] = 1; | |
| drawCmd->Indices[3] = 3; | |
| drawCmd->Indices[4] = 5; | |
| drawCmd->Indices[5] = 4; | |
| } | |
| static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) { | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = 4; | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const Vec4 color = penColor.toVec4(); | |
| const f32 z = static_cast<f32>(-layer); | |
| // Define corners: top-left, top-right, bottom-right, bottom-left | |
| const i32 coords[4][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}}; | |
| for (ui32 i = 0; i < 4; ++i) { | |
| i32 x_clipped, y_clipped; | |
| f32 x_model, y_model; | |
| clip(resolution, coords[i][0], coords[i][1], x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[i].color0 = color; | |
| drawCmd->Vertices[i].position = Vec3(x_model, y_model, z); | |
| } | |
| drawCmd->NumIndices = 6; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| // Define indices for two triangles (0,1,2) and (2,3,0) | |
| const ui16 indices[6] = {0, 1, 2, 2, 3, 0}; | |
| memcpy(drawCmd->Indices, indices, sizeof(ui16) * 6); | |
| } |
| Font2MeshMap mFont2MeshMap; | ||
| Font *mDefaultFont; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify ownership of raw pointers to prevent memory leaks
The class uses raw pointers (Mesh *, Font *) without clear ownership semantics. Consider using smart pointers like std::shared_ptr or std::unique_ptr, or document the ownership expectations to manage resources safely.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
src/Engine/RenderBackend/Text/TextRenderer.h (3)
14-19: Add documentation for MeshInfo struct membersThe struct lacks documentation explaining the purpose and constraints of each member. This is especially important for
mPrim,mNumVertices, andmNumIndicesto clarify their relationships and valid ranges.Add documentation like this:
struct MeshInfo { + /// Pointer to the mesh object Mesh *mMesh; + /// Type of primitives used in this mesh PrimitiveType mPrim; + /// Number of vertices in the mesh size_t mNumVertices; + /// Number of indices in the mesh size_t mNumIndices; };
25-26: Complete the empty documentation blocksThe constructor and destructor lack proper documentation explaining their purpose, behavior, and any resource management responsibilities.
Add documentation like this:
- /// @brief + /// @brief Constructs a TextRenderer instance + /// @details Initializes the text rendering system with default settings TextRenderer(); - /// @brief + /// @brief Destructor + /// @details Cleans up all allocated resources including fonts and meshes ~TextRenderer() override;Also applies to: 28-29
23-50: Add virtual keyword to overridden methodsWhile using
overrideis good, addingvirtualto overridden methods improves code readability by making the virtual nature of the function immediately apparent without having to check the base class.Apply this to methods like
preRender,render,postRender, andonCreate:- void preRender(RenderBackendService *rbSrv) override; + virtual void preRender(RenderBackendService *rbSrv) override;src/Engine/RenderBackend/Text/TextRenderer.cpp (1)
104-108: Add error logging for font initialization failure.While the implementation is correct, it would be helpful to log an error message when the default font initialization fails.
bool TextRenderer::onCreate() { mDefaultFont = FontService::getDefaultFont(); - + if (mDefaultFont == nullptr) { + osre_debug(Tag, "Failed to initialize default font."); + } return (mDefaultFont != nullptr); }src/Engine/RenderBackend/2D/CanvasRenderer.h (2)
46-55: Enhance destructor documentation with cleanup details.While the constructor documentation is thorough, the destructor documentation should specify:
- Resource cleanup behavior (e.g., Font, Mesh)
- Any requirements for proper cleanup
Add the following to the destructor documentation:
/// @brief The class destructor. +/// @note Releases all allocated resources including Font and Mesh objects. +/// @warning Ensure all draw commands are completed before destruction. ~CanvasRenderer() override;
144-152: Consider thread safety for dirty flag operations.The dirty flag management methods might be accessed from multiple threads (e.g., render thread and main thread). Consider adding thread safety mechanisms.
Consider using
std::atomic<bool>for the dirty flag:- bool mDirty; + std::atomic<bool> mDirty; - void setDirty() { - mDirty = true; + void setDirty() noexcept { + mDirty.store(true, std::memory_order_release); } - void setClean() { - mDirty = false; + void setClean() noexcept { + mDirty.store(false, std::memory_order_release); } - bool isDirty() const { - return mDirty; + bool isDirty() const noexcept { + return mDirty.load(std::memory_order_acquire); }src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
Line range hint
190-224: Add error handling for memory allocation failures.The mesh and material creation code should handle potential memory allocation failures.
Consider adding these checks:
if (mMesh == nullptr) { mMesh = new Mesh("2d", VertexType::RenderVertex, IndexType::UnsignedShort); + if (mMesh == nullptr) { + osre_debug(Tag, "Failed to allocate mesh."); + return; + } Material *mat2D = MaterialBuilder::create2DMaterial(); if (mat2D == nullptr) { osre_debug(Tag, "Invalid material instance detected."); + delete mMesh; + mMesh = nullptr; return; } mMesh->setMaterial(mat2D); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp(8 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.h(1 hunks)src/Engine/RenderBackend/Text/TextRenderer.cpp(1 hunks)src/Engine/RenderBackend/Text/TextRenderer.h(1 hunks)
🧰 Additional context used
🪛 cppcheck
src/Engine/RenderBackend/Text/TextRenderer.cpp
[error] 95-95: syntax error
(syntaxError)
🔇 Additional comments (10)
src/Engine/RenderBackend/Text/TextRenderer.h (1)
12-12: Consider using smart pointers in Font2MeshMap
The map uses raw pointers which could lead to memory management issues. Consider using std::shared_ptr<Mesh> to clarify ownership and prevent memory leaks.
src/Engine/RenderBackend/Text/TextRenderer.cpp (3)
10-23: LGTM! Well-structured helper function.
The implementation includes proper null checks and clear search logic.
61-75: Implementation is incomplete.
The method creates/retrieves the mesh but doesn't implement the actual text rendering logic. Required functionality:
- Vertex generation for text glyphs
- Index buffer updates
- Position handling
77-86: LGTM! Proper matrix initialization.
The method correctly sets up the rendering matrices with appropriate null checks.
src/Engine/RenderBackend/2D/CanvasRenderer.h (2)
43-43: LGTM! Explicit public inheritance improves clarity.
The explicit public inheritance modifier clarifies the intended inheritance relationship with IRenderPath.
164-165: Previous review comment about raw pointers is still applicable.
The use of raw pointers for mFont and mMesh could lead to memory management issues.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
28-29: LGTM: Required includes for text rendering.
The new includes support the text rendering functionality added to the canvas renderer.
150-160: LGTM: Proper initialization of font member.
The constructor correctly initializes the new mFont member to nullptr.
436-440: LGTM: Proper font initialization in onCreate.
The onCreate method correctly initializes the default font and checks for success.
69-128: 🛠️ Refactor suggestion
Optimize rectangle vertex creation for better efficiency.
The current implementation has several inefficiencies:
- Uses 6 vertices when 4 would suffice (vertices can be shared between triangles).
- Contains repetitive vertex setup code.
- Could be more maintainable with a loop-based approach.
Consider this optimized implementation:
static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) {
drawCmd->PrimType = PrimitiveType::TriangleList;
- drawCmd->NumVertices = 6;
+ drawCmd->NumVertices = 4;
drawCmd->Vertices = new RenderVert[drawCmd->NumVertices];
+ const Vec4 color = penColor.toVec4();
+ const f32 z = static_cast<f32>(-layer);
+
+ // Define corners: top-left, top-right, bottom-right, bottom-left
+ const i32 coords[4][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}};
+
+ for (ui32 i = 0; i < 4; ++i) {
+ i32 x_clipped, y_clipped;
+ f32 x_model, y_model;
+
+ clip(resolution, coords[i][0], coords[i][1], x_clipped, y_clipped);
+ mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model);
+
+ drawCmd->Vertices[i].color0 = color;
+ drawCmd->Vertices[i].position = Vec3(x_model, y_model, z);
+ }
drawCmd->NumIndices = 6;
drawCmd->Indices = new ui16[drawCmd->NumIndices];
- drawCmd->Indices[0] = 0;
- drawCmd->Indices[1] = 2;
- drawCmd->Indices[2] = 1;
- drawCmd->Indices[3] = 3;
- drawCmd->Indices[4] = 5;
- drawCmd->Indices[5] = 4;
+ // Define indices for two triangles (0,1,2) and (2,3,0)
+ const ui16 indices[6] = {0, 1, 2, 2, 3, 0};
+ memcpy(drawCmd->Indices, indices, sizeof(ui16) * 6);
}Likely invalid or redundant comment.
| Font2MeshMap mFont2MeshMap; | ||
| Font *mDefaultFont; | ||
| MeshInfoArray mMeshInfoArray; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve resource management for member variables
The class uses raw pointers for font and mesh management which could lead to memory leaks or dangling pointers.
Consider these improvements:
- Use
std::unique_ptr<Font>formDefaultFontsince it appears to be exclusively owned - Document ownership transfer expectations for
Fontpointers passed toaddText - Add RAII-compliant cleanup in the destructor
private:
Font2MeshMap mFont2MeshMap;
- Font *mDefaultFont;
+ std::unique_ptr<Font> mDefaultFont;
MeshInfoArray mMeshInfoArray;Committable suggestion skipped: line range outside the PR's diff.
| /// @brief | ||
| ~TextRenderer() override; | ||
|
|
||
| void addText(i32 x, i32 y, Font *font, const String &text); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis, I can now provide a definitive response about the thread safety implications:
Thread safety is required for TextRenderer::addText
The RenderBackendService runs in a dedicated render thread (see mRenderTaskPtr creation and usage), while the TextRenderer is used to submit text rendering commands that can be called from other threads. The shared state includes:
mFont2MeshMap- Modified inaddText()methodmMeshInfoArray- Used to track mesh state
The current implementation lacks synchronization which could lead to race conditions when:
- Multiple threads call
addText()concurrently - The render thread processes the mesh data while another thread is modifying it
Recommended fixes:
- Add a mutex to protect access to the shared state in TextRenderer
- Use the mutex in addText() and other methods that modify the state
- Consider using a command queue pattern to defer actual mesh updates to the render thread
🔗 Analysis chain
Consider thread safety implications of addText method
The addText method likely modifies shared state (mFont2MeshMap and mMeshInfoArray). Consider documenting thread safety guarantees or adding synchronization if the renderer can be accessed from multiple threads.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any thread-related code or synchronization mechanisms
rg -l "std::mutex|std::lock_guard|std::unique_lock" --type cpp
Length of output: 64
Script:
#!/bin/bash
# Let's check for any thread-related code or synchronization in a broader context
rg -i "thread|mutex|lock|atomic|concurrent" --type cpp
# Also check if there are any comments or documentation about threading
rg -i "thread|concurrent|synchroniz|parallel" -g "*.h" -g "*.cpp"
# Look for the actual implementation of TextRenderer to understand the shared state
ast-grep --pattern 'class TextRenderer {
$$$
}'
Length of output: 366953
Script:
#!/bin/bash
# Let's look at the TextRenderer class implementation to understand if it's used in a multi-threaded context
rg -l "TextRenderer" --type cpp
# Then examine the implementation and usage
ast-grep --pattern 'class TextRenderer {
$$$
void addText($$$);
$$$
}'
# Also check if RenderBackend is used in a multi-threaded way
rg "RenderBackend.*thread|thread.*RenderBackend" --type cpp
Length of output: 264
Script:
#!/bin/bash
# Let's examine the TextRenderer implementation and RenderBackend service
cat src/Engine/RenderBackend/Text/TextRenderer.h
cat src/Engine/RenderBackend/Text/TextRenderer.cpp
# Check if RenderBackend runs in a separate thread
cat src/Engine/RenderBackend/RenderBackendService.h
cat src/Engine/RenderBackend/RenderBackendService.cpp
Length of output: 32329
| void TextRenderer::postRender(RenderBackendService* rbSrv) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement cleanup logic in postRender.
The method should handle cleanup of any rendering state or temporary resources.
Consider implementing:
- Shader unbinding
- Texture unbinding
- Buffer cleanup
- State reset
Would you like assistance in implementing the cleanup logic?
| void TextRenderer::render(RenderBackendService* rbSrv) { | ||
| if (rbSrv == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| for (auto &it : mFont2MeshMap) { | ||
| if (it.second != nullptr) { | ||
| rbSrv- | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix syntax error and complete the rendering implementation.
There are two issues:
- Syntax error on line 95 with the dangling
-operator - Incomplete rendering implementation for font meshes
Complete the rendering logic:
void TextRenderer::render(RenderBackendService* rbSrv) {
if (rbSrv == nullptr) {
return;
}
for (auto &it : mFont2MeshMap) {
if (it.second != nullptr) {
- rbSrv-
+ rbSrv->renderMesh(it.second);
}
}
}📝 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.
| void TextRenderer::render(RenderBackendService* rbSrv) { | |
| if (rbSrv == nullptr) { | |
| return; | |
| } | |
| for (auto &it : mFont2MeshMap) { | |
| if (it.second != nullptr) { | |
| rbSrv- | |
| } | |
| } | |
| } | |
| void TextRenderer::render(RenderBackendService* rbSrv) { | |
| if (rbSrv == nullptr) { | |
| return; | |
| } | |
| for (auto &it : mFont2MeshMap) { | |
| if (it.second != nullptr) { | |
| rbSrv->renderMesh(it.second); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 cppcheck
[error] 95-95: syntax error
(syntaxError)
| /// @brief Will draw a text using the current font. | ||
| /// @param x The x position. | ||
| /// @param y The y position. | ||
| /// @param text The text to draw. | ||
| void drawText(i32 x, i32 y, const String &text); |
There was a problem hiding this comment.
Enhance error handling in drawText method.
The method should handle cases where:
- No font is selected (
mFontis nullptr) - Text is empty or invalid
Consider this improvement:
- void drawText(i32 x, i32 y, const String &text);
+ /// @throws std::runtime_error if no font is selected
+ /// @return true if text was successfully drawn, false if text is empty
+ bool drawText(i32 x, i32 y, const String &text);Committable suggestion skipped: line range outside the PR's diff.
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | ||
| setDirty(); | ||
| DrawCmd *drawCmd = nullptr; | ||
| if (filled) { | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| return; | ||
| } | ||
|
|
||
| clip(resolution, x+w, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[1].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[1].position.x = x_model; | ||
| drawCmd->Vertices[1].position.y = y_model; | ||
| drawCmd->Vertices[1].position.z = static_cast<f32>(-layer); | ||
| const ui32 thickness = 2; | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[2].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[2].position.x = x_model; | ||
| drawCmd->Vertices[2].position.y = y_model; | ||
| drawCmd->Vertices[2].position.z = static_cast<f32>(-layer); | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[3].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[3].position.x = x_model; | ||
| drawCmd->Vertices[3].position.y = y_model; | ||
| drawCmd->Vertices[3].position.z = static_cast<f32>(-layer); | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| clip(resolution, x, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[4].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[4].position.x = x_model; | ||
| drawCmd->Vertices[4].position.y = y_model; | ||
| drawCmd->Vertices[4].position.z = static_cast<f32>(-layer); | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize unfilled rectangle rendering.
Creating four separate draw commands for an unfilled rectangle is inefficient. This approach increases memory usage and draw calls.
Consider using a single draw command with line strip primitive:
void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) {
setDirty();
DrawCmd *drawCmd = alloc();
if (filled) {
createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer);
} else {
- // Remove multiple draw commands
- drawCmd = alloc();
- createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer);
- mDrawCmdArray.add(drawCmd);
- // ... more draw commands ...
+ drawCmd->PrimType = PrimitiveType::LineStrip;
+ drawCmd->NumVertices = 5; // 4 corners + closing point
+ drawCmd->Vertices = new RenderVert[drawCmd->NumVertices];
+
+ // Setup vertices for all corners
+ const Vec4 color = mPenColor.toVec4();
+ const f32 z = static_cast<f32>(-mActiveLayer);
+
+ // Define corners: top-left, top-right, bottom-right, bottom-left, top-left
+ i32 coords[5][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}, {x,y}};
+
+ for (ui32 i = 0; i < 5; ++i) {
+ i32 x_clipped, y_clipped;
+ f32 x_model, y_model;
+ clip(mResolution, coords[i][0], coords[i][1], x_clipped, y_clipped);
+ mapCoordinates(mResolution, x_clipped, y_clipped, x_model, y_model);
+
+ drawCmd->Vertices[i].color0 = color;
+ drawCmd->Vertices[i].position = Vec3(x_model, y_model, z);
+ }
+
+ // Setup indices for line strip
+ drawCmd->NumIndices = 5;
+ drawCmd->Indices = new ui16[drawCmd->NumIndices];
+ for (ui16 i = 0; i < 5; ++i) {
+ drawCmd->Indices[i] = i;
+ }
}
mDrawCmdArray.add(drawCmd);
}📝 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.
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | |
| setDirty(); | |
| DrawCmd *drawCmd = nullptr; | |
| if (filled) { | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| return; | |
| } | |
| clip(resolution, x+w, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[1].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[1].position.x = x_model; | |
| drawCmd->Vertices[1].position.y = y_model; | |
| drawCmd->Vertices[1].position.z = static_cast<f32>(-layer); | |
| const ui32 thickness = 2; | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[2].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[2].position.x = x_model; | |
| drawCmd->Vertices[2].position.y = y_model; | |
| drawCmd->Vertices[2].position.z = static_cast<f32>(-layer); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[3].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[3].position.x = x_model; | |
| drawCmd->Vertices[3].position.y = y_model; | |
| drawCmd->Vertices[3].position.z = static_cast<f32>(-layer); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| clip(resolution, x, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[4].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[4].position.x = x_model; | |
| drawCmd->Vertices[4].position.y = y_model; | |
| drawCmd->Vertices[4].position.z = static_cast<f32>(-layer); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| } | |
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | |
| setDirty(); | |
| DrawCmd *drawCmd = alloc(); | |
| if (filled) { | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | |
| } else { | |
| drawCmd->PrimType = PrimitiveType::LineStrip; | |
| drawCmd->NumVertices = 5; // 4 corners + closing point | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| // Setup vertices for all corners | |
| const Vec4 color = mPenColor.toVec4(); | |
| const f32 z = static_cast<f32>(-mActiveLayer); | |
| // Define corners: top-left, top-right, bottom-right, bottom-left, top-left | |
| i32 coords[5][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}, {x,y}}; | |
| for (ui32 i = 0; i < 5; ++i) { | |
| i32 x_clipped, y_clipped; | |
| f32 x_model, y_model; | |
| clip(mResolution, coords[i][0], coords[i][1], x_clipped, y_clipped); | |
| mapCoordinates(mResolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[i].color0 = color; | |
| drawCmd->Vertices[i].position = Vec3(x_model, y_model, z); | |
| } | |
| // Setup indices for line strip | |
| drawCmd->NumIndices = 5; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| for (ui16 i = 0; i < 5; ++i) { | |
| drawCmd->Indices[i] = i; | |
| } | |
| } | |
| mDrawCmdArray.add(drawCmd); | |
| } |
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | ||
| if (text.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| drawCmd->Indices[3] = 3; | ||
| drawCmd->Indices[4] = 5; | ||
| drawCmd->Indices[5] = 4; | ||
| } | ||
| if (mFont == nullptr) { | ||
| osre_debug(Tag, "No font selected."); | ||
| return; | ||
| } | ||
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | ||
| mapCoordinates(mResolution, x, y, x_model, y_model); | ||
| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *indices = nullptr; | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | ||
|
|
||
| DrawCmd *drawCmd = alloc(); | ||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | ||
| DrawCmd *drawCmd = nullptr; | ||
| if (filled) { | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| } else { | ||
| const ui32 thickness = 2; | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec3(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | ||
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | ||
| } | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| if (mFont == nullptr) { | ||
| mFont = FontService::getDefaultFont(); | ||
| } | ||
| drawCmd->UseFont = mFont; | ||
|
|
||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| setDirty(); |
There was a problem hiding this comment.
Fix critical issues in text rendering implementation.
The drawText method has several issues:
- Memory leak:
indicesarray is not deallocated - Inconsistent color vector usage: Using
toVec3()instead oftoVec4() - Missing texture coordinates:
tex0is generated but not assigned to vertices - Redundant font check after font usage
Apply these fixes:
void CanvasRenderer::drawText(i32 x, i32 y, const String &text) {
// ... existing code ...
for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) {
- drawCmd->Vertices[posIndex].color0 = mPenColor.toVec3();
+ drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4();
drawCmd->Vertices[posIndex].position.x = positions[posIndex].x;
drawCmd->Vertices[posIndex].position.y = positions[posIndex].y;
drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer);
+ drawCmd->Vertices[posIndex].tex0 = tex0[posIndex];
}
// ... copy indices ...
- if (mFont == nullptr) {
- mFont = FontService::getDefaultFont();
- }
drawCmd->UseFont = mFont;
mDrawCmdArray.add(drawCmd);
+ delete[] indices; // Free allocated memory
setDirty();
}📝 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.
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | |
| if (text.empty()) { | |
| return; | |
| } | |
| drawCmd->Indices[3] = 3; | |
| drawCmd->Indices[4] = 5; | |
| drawCmd->Indices[5] = 4; | |
| } | |
| if (mFont == nullptr) { | |
| osre_debug(Tag, "No font selected."); | |
| return; | |
| } | |
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | |
| mapCoordinates(mResolution, x, y, x_model, y_model); | |
| Vec3Array positions; | |
| Vec3Array colors; | |
| Vec2Array tex0; | |
| ui16 *indices = nullptr; | |
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | |
| DrawCmd *drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = positions.size(); | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | |
| drawCmd->NumIndices = numIndices; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) { | |
| DrawCmd *drawCmd = nullptr; | |
| if (filled) { | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| } else { | |
| const ui32 thickness = 2; | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | |
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec3(); | |
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | |
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | |
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | |
| } | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | |
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | |
| } | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| if (mFont == nullptr) { | |
| mFont = FontService::getDefaultFont(); | |
| } | |
| drawCmd->UseFont = mFont; | |
| mDrawCmdArray.add(drawCmd); | |
| setDirty(); | |
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | |
| if (text.empty()) { | |
| return; | |
| } | |
| if (mFont == nullptr) { | |
| osre_debug(Tag, "No font selected."); | |
| return; | |
| } | |
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | |
| mapCoordinates(mResolution, x, y, x_model, y_model); | |
| Vec3Array positions; | |
| Vec3Array colors; | |
| Vec2Array tex0; | |
| ui16 *indices = nullptr; | |
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | |
| DrawCmd *drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = positions.size(); | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | |
| drawCmd->NumIndices = numIndices; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | |
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | |
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | |
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | |
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | |
| drawCmd->Vertices[posIndex].tex0 = tex0[posIndex]; | |
| } | |
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | |
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | |
| } | |
| drawCmd->UseFont = mFont; | |
| mDrawCmdArray.add(drawCmd); | |
| delete[] indices; // Free allocated memory | |
| setDirty(); |
…ing/osre into kimkulling/fix_2d_coords
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/Engine/RenderBackend/RenderBackendService.h (1)
199-233: Consider enhancing pipeline state management.The new methods suggest a manual pipeline state management approach that could be prone to errors:
beginPass/endPassandbeginRenderBatch/endRenderBatchpairs could be forgotten or mismatched- The relationship between 2D and 3D pipelines isn't clearly defined
Consider these improvements:
- Use RAII patterns to manage pipeline states:
class ScopedPass { public: ScopedPass(RenderBackendService& service, const c8* id) : mService(service) { mPass = mService.beginPass(id); } ~ScopedPass() { mService.endPass(); } private: RenderBackendService& mService; PassData* mPass; };
- Document the relationship between 2D and 3D pipelines:
- Can they be active simultaneously?
- What happens when switching between them?
- Are there any shared resources?
src/Engine/RenderBackend/MeshBuilder.cpp (3)
Line range hint
187-237: Consider removing commented-out codeThe entire
allocUiQuadmethod is commented out. Keeping large blocks of commented code can clutter the codebase and reduce readability. If this code is no longer needed, consider removing it from the file. If it's needed for future reference, consider documenting the reason for its deprecation.
373-376: Remove or clarify commented-out code blockThe code block setting up
TextureResourceandMaterialBuilder::createTexturedMaterialis commented out. If this code is deprecated or replaced, consider removing it to clean up the codebase. If it's kept for a specific reason, add a comment explaining why it's retained.
389-399: Refactor vertex initialization to utilizeMeshUtilitiesYou are manually initializing the
colandposarrays forMeshUtilities::NumQuadVertvertices. To maintain consistency and reduce code duplication, consider using utility functions or constants fromMeshUtilitiesfor initializing these arrays.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
contrib/assimp(1 hunks)contrib/cppcore(1 hunks)src/Engine/App/AppBase.cpp(7 hunks)src/Engine/RenderBackend/MeshBuilder.cpp(7 hunks)src/Engine/RenderBackend/MeshBuilder.h(1 hunks)src/Engine/RenderBackend/OGLRenderer/OGLRenderEventHandler.cpp(2 hunks)src/Engine/RenderBackend/RenderBackendService.h(7 hunks)
✅ Files skipped from review due to trivial changes (2)
- contrib/assimp
- src/Engine/RenderBackend/MeshBuilder.h
🚧 Files skipped from review as they are similar to previous changes (2)
- contrib/cppcore
- src/Engine/App/AppBase.cpp
🔇 Additional comments (5)
src/Engine/RenderBackend/RenderBackendService.h (1)
179-179: LGTM: Proper use of override specifier.
The destructor correctly uses the override specifier, which helps catch errors at compile-time if the base class destructor signature changes.
src/Engine/RenderBackend/OGLRenderer/OGLRenderEventHandler.cpp (1)
Line range hint 1-500: Overall code structure and error handling looks good
The changes maintain the existing error handling patterns and separation of concerns. The code follows consistent patterns for:
- Null pointer checks
- Event handling structure
- Resource management
- Debug logging
The modifications align well with the Canvas 2D-Rendering rework objectives while preserving the robustness of the rendering pipeline.
src/Engine/RenderBackend/MeshBuilder.cpp (3)
24-26: Appropriate inclusion of new dependencies
The addition of MeshUtilities.h and FontService.h ensures that the necessary utilities and font services are accessible in this file.
350-354:
Potential memory leak with textIndices
The MeshUtilities::generateTextBoxVerticesAndIndices function allocates memory for textIndices. To prevent a memory leak, ensure that textIndices is properly deallocated after its use.
Apply this diff to deallocate textIndices after creating the index buffer:
mesh->createIndexBuffer(textIndices, size, IndexType::UnsignedShort, BufferAccessType::ReadOnly);
+ delete[] textIndices;
// setup primitives
mesh->addPrimitiveGroup(6 * text.size(), PrimitiveType::TriangleList, 0);Likely invalid or redundant comment.
413-413: Verify accessibility of MeshUtilities::NumQuadVert
Ensure that MeshUtilities::NumQuadVert is defined and accessible in this context to prevent potential compilation issues.
Run the following script to confirm the definition and accessibility:
✅ Verification successful
MeshUtilities::NumQuadVert is properly defined and accessible
The constant is defined as a public static constexpr member in src/Engine/RenderBackend/Mesh/MeshUtilities.h:
static constexpr size_t NumQuadVert = 4;The usage in MeshBuilder.cpp is correct and the constant is accessible since it's:
- Defined as public
- Used consistently throughout the codebase for quad vertex operations
- Properly included (evidenced by multiple successful usages in
MeshBuilder.cpp)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definition of `NumQuadVert` in MeshUtilities.h
# Expected: A public constant definition of `NumQuadVert` should be found.
ast-grep --pattern $'class MeshUtilities {
$$$
public:
static const $$ NumQuadVert = $_;
$$$
}'
-- 'RenderBackend/Mesh/MeshUtilities.h'
Length of output: 219
Script:
#!/bin/bash
# Let's first find the MeshUtilities header file
fd "MeshUtilities.h"
# Then search for NumQuadVert definition in any header files
rg "NumQuadVert" -A 2 -B 2
Length of output: 3744
| Filename(filename), Width(w), Height(h) {} | ||
|
|
||
| String Filename; | ||
| ui32 Width, Height; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent naming conventions across member variables.
The changes introduce inconsistent naming patterns within the event data structures:
- Some members use PascalCase (e.g.,
Filename,Width,Height) - Others retain Hungarian notation with m-prefix (e.g.,
m_activeSurface)
This inconsistency could lead to maintenance issues and confusion for developers.
Consider standardizing all member variables to follow the same naming convention. For example:
struct CreateRendererEventData : public Common::EventData {
CreateRendererEventData(Platform::AbstractWindow *pSurface) :
- EventData(OnCreateRendererEvent, nullptr), m_activeSurface(pSurface), DefaultFont(""), Pipeline(nullptr)
+ EventData(OnCreateRendererEvent, nullptr), ActiveSurface(pSurface), DefaultFont(""), Pipeline(nullptr)
}
- Platform::AbstractWindow *m_activeSurface;
+ Platform::AbstractWindow *ActiveSurface;
String DefaultFont;
Pipeline *Pipeline;
};Also applies to: 95-95, 100-101, 124-124, 128-128, 139-139, 148-148, 158-158, 161-161
| @@ -487,7 +487,7 @@ bool OGLRenderEventHandler::onScreenshot(const EventData *eventData) { | |||
| bool result = false; | |||
| ScreenshotEventData *data = (ScreenshotEventData*) eventData; | |||
| if (data != nullptr){ | |||
| result = makeScreenShot(data->mFilename.c_str(), data->mWidth, data->mHeight); | |||
| result = makeScreenShot(data->Filename.c_str(), data->Width, data->Height); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation for screenshot data
While the naming convention update looks good, consider adding parameter validation before using the screenshot data:
bool OGLRenderEventHandler::onScreenshot(const EventData *eventData) {
bool result = false;
ScreenshotEventData *data = (ScreenshotEventData*) eventData;
- if (data != nullptr){
+ if (data != nullptr && !data->Filename.empty() && data->Width > 0 && data->Height > 0) {
result = makeScreenShot(data->Filename.c_str(), data->Width, data->Height);
+ } else {
+ osre_debug(Tag, "Invalid screenshot parameters");
}
return result;
}Committable suggestion skipped: line range outside the PR's diff.
| Font defaultFont; | ||
| defaultFont.Name = "buildin_arial"; | ||
| defaultFont.Size = 10; | ||
| defaultFont.Style = 0; | ||
| Material *mat = FontService::getFont(defaultFont); |
There was a problem hiding this comment.
Ensure robust error handling for font retrieval
When retrieving the font using FontService::getFont(defaultFont), there's a possibility that the font may not be found, and mat could be nullptr. Ensure that there's a check after retrieving the font to handle such cases gracefully to prevent null pointer dereferences.
…ing/osre into kimkulling/fix_2d_coords
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Engine/Common/Logger.h (2)
116-116: Improve documentation consistencyThe documentation style in the
PrintModeenum is inconsistent:
- Comments should be properly aligned with their respective enum values
- All comments should consistently end with periods
Apply this diff to fix the documentation:
enum class PrintMode { WithDateTime, ///< A dateTime string will put be in front of the entry. - WhithoutDateTime ///< No DateTime will be there. + WithoutDateTime ///< No DateTime will be there. };Also applies to: 139-139
139-139: Consider making logging behavior configurableInstead of hardcoding the default logging behavior, consider making it configurable:
- Add a configuration option to set default
PrintMode- Allow per-stream configuration of
PrintMode- Consider adding a configuration file for logging preferences
This would provide more flexibility and better separation of concerns.
src/Engine/App/AppBase.h (2)
72-72: Enhance class and method documentation.While the added documentation is good, consider enhancing it with:
- Class-level documentation explaining the purpose and lifecycle of keyboard event handling
- Constructor documentation mentioning the initialization of key states
- Destructor documentation explaining any cleanup if needed
Example enhancement for class documentation:
-/// @brief This class implements the keyboard event listener. +/// @brief This class implements the keyboard event listener for tracking key states. +/// +/// The KeyboardEventListener maintains the state of keyboard inputs across frames, +/// allowing the application to query key states and track the most recently pressed key. +/// It automatically initializes all key states to unpressed on construction.Also applies to: 79-80
265-267: Add documentation for canvas renderer getter.The
getCanvasRenderermethod lacks documentation. Add a detailed description explaining:
- The purpose of the canvas renderer
- Return value semantics (ownership, lifetime)
- When this method should be called in the application lifecycle
Example documentation:
- /// @brief + /// @brief Returns the canvas renderer for 2D rendering operations. + /// @return A pointer to the canvas renderer instance, or nullptr if 2D rendering is not initialized. + /// @note The returned pointer is owned by the AppBase instance and remains valid until destroy() is called. virtual RenderBackend::CanvasRenderer *getCanvasRenderer() const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Engine/App/AppBase.cpp(8 hunks)src/Engine/App/AppBase.h(5 hunks)src/Engine/Common/Logger.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/App/AppBase.cpp
🔇 Additional comments (2)
src/Engine/Common/Logger.h (1)
139-139: Verify impact on Canvas 2D-Rendering
While the typo and behavioral change have been noted in a previous review, let's verify how this logging change impacts the Canvas 2D-Rendering functionality.
#!/bin/bash
# Description: Check usage of Logger::print in Canvas 2D-Rendering related files
# Search for Logger::print usage in Canvas 2D-Rendering files
rg -l "Logger.*print" | rg -i "canvas|2d|render"
# Search for direct print calls
rg "print\(" | rg -i "canvas|2d|render"src/Engine/App/AppBase.h (1)
53-55: LGTM: Forward declarations are well-organized.
The forward declarations for IRenderPath and CanvasRenderer are properly placed in the RenderBackend namespace, following good practices with interface declarations before implementation classes.
| @@ -26,7 +26,7 @@ jobs: | |||
| run: | | |||
| sudo add-apt-repository -y "deb http://archive.ubuntu.com/ubuntu `lsb_release -sc` main universe restricted multiverse" | |||
| sudo apt-get update -y -qq | |||
| sudo apt-get install libsdl2-dev libglm-dev libglew-dev libgtest-dev | |||
| sudo apt-get install libsdl2-dev libglm-dev libglew-dev libgtest-dev googletest | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Consider removing redundant Google Test package.
The project uses GTest with modern CMake configuration (find_package(GTest CONFIG REQUIRED)) and links against the namespaced targets (GTest::gtest and GTest::gtest_main). In Ubuntu, libgtest-dev is sufficient for this setup, making the additional googletest package redundant.
- sudo apt-get install libsdl2-dev libglm-dev libglew-dev libgtest-dev googletest
+ sudo apt-get install libsdl2-dev libglm-dev libglew-dev libgtest-dev🔗 Analysis chain
Consider removing redundant Google Test package.
The line installs both libgtest-dev and googletest packages, which appear to be redundant as they both provide Google Test framework. This could potentially lead to version conflicts.
Consider using only one package:
- sudo apt-get install libsdl2-dev libglm-dev libglew-dev libgtest-dev googletest
+ sudo apt-get install libsdl2-dev libglm-dev libglew-dev libgtest-devLet's verify the Google Test setup and package information:
Let me verify the CMake configuration to understand how GTest is being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package information for Google Test packages
# Expected results: Show that these packages provide the same framework
# Check package details
apt-cache show libgtest-dev
apt-cache show googletest
# Verify if tests are properly configured in CMake
rg -l "find_package\(.*GTest.*\)" -g "CMakeLists.txt"
Length of output: 295
Script:
#!/bin/bash
# Check the CMake configuration for GTest
cat test/UnitTests/CMakeLists.txt
# Look for any GTest-related includes or dependencies
rg -g "CMakeLists.txt" "GTest|gtest" -A 3 -B 3
Length of output: 4686
…ing/osre into kimkulling/fix_2d_coords
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Engine/RenderBackend/RenderBackendService.h (2)
101-101: Avoid using underscore suffix in member variable names.The member variable
Pipeline_uses an underscore suffix which is inconsistent with the codebase's naming conventions. Consider renaming it to justPipelineto match other member variables.- Pipeline *Pipeline_; + Pipeline *Pipeline;
212-222: Consider thread safety in pipeline state management.The begin/end pattern with state stored in member variables (
mCurrentPass,mCurrentBatch) could lead to race conditions in multi-threaded scenarios. Consider:
- Adding thread safety mechanisms
- Documenting thread safety requirements
- Using RAII pattern to manage pass/batch lifecycle
Example RAII approach:
class PassScope { RenderBackendService &service; public: PassScope(RenderBackendService &s, const c8 *id) : service(s) { service.beginPass(id); } ~PassScope() { service.endPass(); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
contrib/vcpkg(1 hunks)src/Engine/RenderBackend/RenderBackendService.h(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- contrib/vcpkg
🔇 Additional comments (1)
src/Engine/RenderBackend/RenderBackendService.h (1)
179-179: LGTM! Good use of override.
The explicit override specifier improves code clarity and helps catch errors at compile-time if the base class interface changes.
| /// @brief | ||
| /// @param | ||
| void setActivePipeline(Pipeline *pipeline); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @return | ||
| PassData *getPassById(const c8 *id) const; | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @return | ||
| PassData *beginPass(const c8 *id); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @return | ||
| RenderBatchData *beginRenderBatch(const c8 *id); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| void setRenderTarget(FrameBuffer *fb); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @param | ||
| void setMatrix(MatrixType type, const glm::mat4 &m); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @param |
There was a problem hiding this comment.
Add parameter validation in pipeline management methods.
The new pipeline management methods lack parameter validation. This could lead to crashes or undefined behavior if null pointers are passed.
Consider adding validation for:
setActivePipeline: Check if pipeline is nullgetPassById: Check if id is nullbeginPass: Check if id is nullbeginRenderBatch: Check if id is nullsetRenderTarget: Check if fb is nullsetMatrix: Check if matrix parameters are valid
Example implementation:
void setActivePipeline(Pipeline *pipeline) {
if (pipeline == nullptr) {
// Consider throwing an exception or logging an error
return;
}
// ... rest of the implementation
}
PassData *getPassById(const c8 *id) const {
if (id == nullptr) {
// Consider throwing an exception or logging an error
return nullptr;
}
// ... rest of the implementation
}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (5)
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
Line range hint
190-224: Add error handling and memory management.Issues to address:
- Missing null check for mesh creation
- No cleanup of mesh in destructor
Add these safety checks:
if (mMesh == nullptr) { + try { mMesh = new Mesh("2d", VertexType::RenderVertex, IndexType::UnsignedShort); + } catch (const std::bad_alloc&) { + osre_debug(Tag, "Failed to allocate mesh."); + return; + } Material *mat2D = MaterialBuilder::create2DMaterial(); if (mat2D == nullptr) { osre_debug(Tag, "Invalid material instance detected."); + delete mMesh; + mMesh = nullptr; return; } mMesh->setMaterial(mat2D); }Also, add mesh cleanup in the destructor:
~CanvasRenderer() { delete mMesh; // Add this line for (size_t i = 0; i < mDrawCmdArray.size(); ++i) { auto &dc = *mDrawCmdArray[i]; dealloc(&dc); } }src/Engine/RenderBackend/RenderBackendService.cpp (3)
Line range hint
219-294: Consider extracting frame submission logicThe frame submission logic in
commitNextFrame()is quite complex with multiple nested loops and conditions. Consider extracting the batch update logic into separate methods for better maintainability.Example refactoring:
void RenderBackendService::commitNextFrame() { if (!mRenderTaskPtr.isValid()) { return; } CommitFrameEventData *data = CommitFrameEventData::create(); data->NextFrame = mSubmitFrame; + updateBatches(data); + std::swap(mSubmitFrame, mRenderFrame); + osre_assert(mSubmitFrame != mRenderFrame); + mRenderTaskPtr->sendEvent(&OnCommitFrameEvent, data); } +void RenderBackendService::updateBatches(CommitFrameEventData* data) { + for (ui32 i = 0; i < mPasses.size(); ++i) { + PassData *currentPass = mPasses[i]; + if (currentPass == nullptr) { + continue; + } + updatePassBatches(currentPass, data); + } } +void RenderBackendService::updatePassBatches(PassData* currentPass, CommitFrameEventData* data) { // Extract the batch update logic here }
Line range hint
245-266: Optimize uniform buffer updatesThe uniform buffer update logic involves multiple memory allocations and copies. Consider using a pre-allocated buffer pool to reduce memory fragmentation and improve performance.
- cmd->m_data = new c8[cmd->m_size]; + // Use a buffer pool instead of raw allocation + cmd->m_data = mBufferPool.allocate(cmd->m_size);
293-296: Add frame transition logging for debuggingConsider adding debug logging around frame transitions to help track down potential synchronization issues.
data->NextFrame = mSubmitFrame; + osre_debug(Tag, "Transitioning frames: Submit=%p Render=%p", mSubmitFrame, mRenderFrame); std::swap(mSubmitFrame, mRenderFrame); osre_assert(mSubmitFrame != mRenderFrame); + osre_debug(Tag, "Frame transition complete: Submit=%p Render=%p", mSubmitFrame, mRenderFrame);src/Engine/RenderBackend/RenderCommon.h (1)
1031-1039: Enhance Point2D structs with common operationsConsider adding useful operators and methods to improve usability.
struct Point2Di { i32 X, Y; + + Point2Di operator+(const Point2Di& other) const { + return {X + other.X, Y + other.Y}; + } + + Point2Di operator-(const Point2Di& other) const { + return {X - other.X, Y - other.Y}; + } + + bool operator==(const Point2Di& other) const { + return X == other.X && Y == other.Y; + } }; struct Point2Df { f32 X, Y; + + Point2Df operator+(const Point2Df& other) const { + return {X + other.X, Y + other.Y}; + } + + Point2Df operator-(const Point2Df& other) const { + return {X - other.X, Y - other.Y}; + } + + bool operator==(const Point2Df& other) const { + return X == other.X && Y == other.Y; + } + + float length() const { + return std::sqrt(X * X + Y * Y); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
contrib/cppcore(1 hunks)samples/02_Demo2D/Demo2D.cpp(5 hunks)src/Engine/App/AppBase.cpp(10 hunks)src/Engine/CMakeLists.txt(4 hunks)src/Engine/Common/DateTime.cpp(2 hunks)src/Engine/Common/EventBus.cpp(3 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(8 hunks)src/Engine/RenderBackend/OGLRenderer/OGLRenderEventHandler.cpp(7 hunks)src/Engine/RenderBackend/RenderBackendService.cpp(4 hunks)src/Engine/RenderBackend/RenderBackendService.h(7 hunks)src/Engine/RenderBackend/RenderCommon.h(13 hunks)src/Engine/RenderBackend/Text/TextRenderer.cpp(1 hunks)test/RenderTests/src/RenderTestSuite.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Engine/Common/DateTime.cpp
- test/RenderTests/src/RenderTestSuite.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- contrib/cppcore
- samples/02_Demo2D/Demo2D.cpp
- src/Engine/App/AppBase.cpp
- src/Engine/CMakeLists.txt
- src/Engine/Common/EventBus.cpp
🧰 Additional context used
🪛 cppcheck
src/Engine/RenderBackend/Text/TextRenderer.cpp
[warning] 41-41: Possible null pointer dereference
(nullPointer)
[warning] 44-44: Possible null pointer dereference
(nullPointer)
[warning] 45-45: Possible null pointer dereference
(nullPointer)
[warning] 47-47: Possible null pointer dereference
(nullPointer)
🔇 Additional comments (14)
src/Engine/RenderBackend/Text/TextRenderer.cpp (2)
10-23: LGTM: Well-implemented helper function
The implementation is clean with proper null checks and clear success/failure indicators.
106-108: 🛠️ Refactor suggestion
Implement state cleanup in postRender
The empty postRender method should handle cleanup of rendering state.
Consider implementing:
- Shader unbinding
- Texture unbinding
- Buffer cleanup
- State reset
void TextRenderer::postRender(RenderBackendService* rbSrv) {
+ if (rbSrv == nullptr) {
+ return;
+ }
+ // Reset render states
+ rbSrv->resetRenderStates();
+ // Clear mesh info array for next frame
+ mMeshInfoArray.clear();
}Likely invalid or redundant comment.
src/Engine/RenderBackend/RenderBackendService.h (1)
179-179: LGTM: Good use of override specifier
Adding the override specifier improves type safety by catching potential inheritance issues at compile-time.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (3)
28-29: LGTM: Required includes for text rendering.
The new includes support the text rendering functionality.
69-128: Optimize vertex creation for better memory usage and maintainability.
The current implementation:
- Uses 6 vertices when 4 would suffice (vertices can be shared between triangles)
- Contains duplicated vertex setup code
- Could be more maintainable with a loop-based approach
150-160: LGTM: Proper initialization of new font member.
The font member is correctly initialized to nullptr in the constructor's initialization list.
src/Engine/RenderBackend/OGLRenderer/OGLRenderEventHandler.cpp (2)
490-490: Previous review comment about parameter validation is still applicable
The suggestion to add parameter validation and error logging for the screenshot data remains valid.
Referring to the previous review comment about adding parameter validation for screenshot data.
314-314: 🛠️ Refactor suggestion
Consider maintaining consistent naming conventions and reducing method complexity
- The change from
m_frametoNextFramecontinues the inconsistent naming pattern. - The method is quite complex with multiple nested loops and null checks.
Consider:
- Maintaining the
m_prefix:
- Frame *frame = frameToCommitData->NextFrame;
+ Frame *frame = frameToCommitData->m_frame;- Breaking down the method into smaller, focused functions to improve readability and maintainability. For example:
processMeshEntry(MeshEntry*, TArray<size_t>&)processBatchData(RenderBatchData*)processPass(PassData*)
Likely invalid or redundant comment.
src/Engine/RenderBackend/RenderBackendService.cpp (1)
208-208: Verify frame synchronization with the new naming
The renaming from m_frame to NextFrame improves readability by making the purpose clearer. However, we should verify that this change doesn't affect frame synchronization, especially in the complex frame submission logic.
Also applies to: 219-219, 245-245, 293-293
✅ Verification successful
Frame synchronization verified with no remaining m_frame references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining m_frame references that might have been missed
rg "m_frame" -A 3 -B 3
# Look for frame-related synchronization patterns
ast-grep --pattern 'NextFrame = $expr;
$$$
std::swap($_, $_);'
Length of output: 969
src/Engine/RenderBackend/RenderCommon.h (5)
Line range hint 62-206: LGTM! Well-structured enum improvements.
The standardization of enum classes with Invalid and Count values improves error handling and iteration capabilities.
1026-1028: Function name typo already reported
The typo in getGlslVersionFromeString has been previously reported and needs to be fixed.
991-991:
Fix typo in state enum value
There's a typo in the state name "Detroyed".
- mLifetimeState = LifetimeState::Detroyed;
+ mLifetimeState = LifetimeState::Destroyed;Likely invalid or redundant comment.
984-987:
Fix incorrect state check in destroy()
The destroy() method checks for Running state, but the create() method transitions to Created state. This state mismatch could prevent proper cleanup.
- if (mLifetimeState != LifetimeState::Running) {
+ if (mLifetimeState != LifetimeState::Created) {
return false;
}Likely invalid or redundant comment.
969-982:
Add missing Running state transition
The create() method transitions to Created state but never to Running state. This could cause issues with the destroy() method's state check.
if (result) {
mLifetimeState = LifetimeState::Created;
+ mLifetimeState = LifetimeState::Running;
} else {
mLifetimeState = LifetimeState::Error;
}Likely invalid or redundant comment.
| void TextRenderer::addText(i32 x, i32 y, Font *font, const String &text) { | ||
| Font *usedFont = font; | ||
| if (usedFont == nullptr) { | ||
| usedFont = FontService::getDefaultFont(); | ||
| } | ||
|
|
||
| Mesh *textMesh = nullptr; | ||
| Font2MeshMap::const_iterator it = mFont2MeshMap.find(usedFont->Name); | ||
| if (it == mFont2MeshMap.end()) { | ||
| textMesh = new Mesh(usedFont->Name + ".mesh", VertexType::RenderVertex, IndexType::UnsignedShort); | ||
| mFont2MeshMap[usedFont->Name] = textMesh; | ||
| } else { | ||
| textMesh = it->second; | ||
| } | ||
| } |
There was a problem hiding this comment.
Complete the text rendering implementation
The addText method only creates/retrieves the mesh but doesn't implement the actual text rendering logic.
The implementation should:
- Generate vertices for text glyphs
- Update mesh indices
- Handle text positioning (x, y coordinates)
Would you like me to help implement the text rendering functionality?
| TextRenderer::~TextRenderer() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement proper cleanup in destructor
The empty destructor could lead to memory leaks. The class owns Mesh objects stored in mFont2MeshMap that need to be properly deleted.
Consider implementing:
TextRenderer::~TextRenderer() {
+ for (auto& pair : mFont2MeshMap) {
+ delete pair.second;
+ }
+ mFont2MeshMap.clear();
}📝 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.
| TextRenderer::~TextRenderer() { | |
| } | |
| TextRenderer::~TextRenderer() { | |
| for (auto& pair : mFont2MeshMap) { | |
| delete pair.second; | |
| } | |
| mFont2MeshMap.clear(); | |
| } |
| void renderFontMesh(const DrawCmd &dc, Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) { | ||
| MeshInfo info; | ||
| const String &fontKey = dc.UseFont->Name; | ||
| Mesh *textMesh = nullptr; | ||
| Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey); | ||
| if (it != font2MeshMap.end()) { | ||
| textMesh = it->second; | ||
| if (textMesh == nullptr) { | ||
| osre_debug(Tag, "Invalid font mesh detected."); | ||
| return; | ||
| } else { | ||
| getMeshInfo(textMesh, meshInfoArray, info); | ||
| } | ||
| } | ||
|
|
||
| const ui32 lastIndex = textMesh->getLastIndex(); | ||
| renumberIndices(dc, info.mNumVertices); | ||
|
|
||
| textMesh->attachVertices(dc.Vertices, dc.NumVertices * sizeof(RenderVert)); | ||
| textMesh->attachIndices(dc.Indices, dc.NumIndices * sizeof(ui16)); | ||
| info.mPrim = dc.PrimType; | ||
| textMesh->setLastIndex(lastIndex + static_cast<ui16>(dc.NumIndices)); | ||
| info.mNumVertices += dc.NumVertices; | ||
| info.mNumIndices += dc.NumIndices; | ||
| } |
There was a problem hiding this comment.
Add critical null checks to prevent crashes
The function has several potential null pointer dereference issues that need to be addressed:
- No validation of
dc.UseFontbefore accessing its Name - Using
textMeshafter the if block without re-validation - No validation of
dc.Verticesanddc.Indicesbefore use
Apply this fix:
void renderFontMesh(const DrawCmd &dc, Font2MeshMap &font2MeshMap, MeshInfoArray &meshInfoArray) {
+ if (dc.UseFont == nullptr) {
+ osre_debug(Tag, "Invalid font detected.");
+ return;
+ }
+ if (dc.Vertices == nullptr || dc.Indices == nullptr) {
+ osre_debug(Tag, "Invalid vertex or index data detected.");
+ return;
+ }
MeshInfo info;
const String &fontKey = dc.UseFont->Name;
Mesh *textMesh = nullptr;
Font2MeshMap::const_iterator it = font2MeshMap.find(fontKey);
if (it != font2MeshMap.end()) {
textMesh = it->second;
if (textMesh == nullptr) {
osre_debug(Tag, "Invalid font mesh detected.");
return;
- } else {
- getMeshInfo(textMesh, meshInfoArray, info);
}
+ if (!getMeshInfo(textMesh, meshInfoArray, info)) {
+ osre_debug(Tag, "Failed to get mesh info.");
+ return;
+ }
+ } else {
+ osre_debug(Tag, "Font mesh not found in map.");
+ return;
}
+ if (textMesh == nullptr) {
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 cppcheck
[warning] 41-41: Possible null pointer dereference
(nullPointer)
[warning] 44-44: Possible null pointer dereference
(nullPointer)
[warning] 45-45: Possible null pointer dereference
(nullPointer)
[warning] 47-47: Possible null pointer dereference
(nullPointer)
| /// @brief | ||
| /// @return | ||
| Pipeline *createDefault2DPipeline(); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| void setActivePipeline(Pipeline *pipeline); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @return | ||
| PassData *getPassById(const c8 *id) const; | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @return | ||
| PassData *beginPass(const c8 *id); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @return | ||
| RenderBatchData *beginRenderBatch(const c8 *id); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| void setRenderTarget(FrameBuffer *fb); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @param | ||
| void setMatrix(MatrixType type, const glm::mat4 &m); | ||
|
|
||
| /// @brief | ||
| /// @param | ||
| /// @param |
There was a problem hiding this comment.
💡 Codebase verification
Documentation for 2D Rendering Architecture is Missing
There is no existing documentation covering the 2D rendering architecture. It is recommended to add the suggested architectural overview and related documentation.
🔗 Analysis chain
Consider documenting the 2D rendering architecture
The introduction of createDefault2DPipeline and related methods suggests a significant architectural change in how 2D rendering is handled. To ensure proper usage and maintainability:
- Consider adding a high-level architectural overview in the class documentation
- Document the relationship and differences between 2D and 3D pipelines
- Clarify the expected rendering state management flow using these new methods
Let's verify if there's existing documentation about the rendering architecture:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for architecture documentation
echo "Searching for architecture documentation..."
fd -e md -e txt . | xargs rg -l "architecture|rendering|pipeline"
# Search for related test files
echo "Searching for related test files..."
fd -e cpp -e h | xargs rg -l "Default2DPipeline|RenderBackendService.*Test"
Length of output: 560
| bool CanvasRenderer::onCreate() { | ||
| mFont = FontService::getDefaultFont(); | ||
|
|
||
| return (mFont != nullptr); | ||
| } |
There was a problem hiding this comment.
Add error recovery in onCreate.
The method returns false if font initialization fails but doesn't clean up any resources that might have been allocated.
Consider adding cleanup and logging:
bool CanvasRenderer::onCreate() {
mFont = FontService::getDefaultFont();
+ if (mFont == nullptr) {
+ osre_debug(Tag, "Failed to initialize default font.");
+ // Clean up any resources allocated before font initialization
+ return false;
+ }
return true;
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -453,7 +453,7 @@ | |||
| return false; | |||
| } | |||
|
|
|||
| for (FrameSubmitCmd *cmd : data->m_frame->m_submitCmds) { | |||
| for (FrameSubmitCmd *cmd : data->NextFrame->m_submitCmds) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix method name typo and maintain naming consistency
- The method name contains a typo:
onCommitNexFrameshould beonCommitNextFrame - The changes from
m_frametoNextFramecontinue the inconsistent naming pattern
Fix the method name typo:
-bool OGLRenderEventHandler::onCommitNexFrame(const EventData *eventData) {
+bool OGLRenderEventHandler::onCommitNextFrame(const EventData *eventData) {Maintain consistent naming:
- for (FrameSubmitCmd *cmd : data->NextFrame->m_submitCmds) {
+ for (FrameSubmitCmd *cmd : data->m_frame->m_submitCmds) {
- data->NextFrame->m_submitCmds.resize(0);
- data->NextFrame->m_submitCmdAllocator.release();
+ data->m_frame->m_submitCmds.resize(0);
+ data->m_frame->m_submitCmdAllocator.release();Also applies to: 465-466
| @@ -477,7 +477,7 @@ | |||
| bool OGLRenderEventHandler::onResizeRenderTarget(const EventData *eventData) { | |||
| ResizeEventData *data = (ResizeEventData*) eventData; | |||
| if (data != nullptr) { | |||
| m_oglBackend->setViewport(data->m_x, data->m_y, data->m_w, data->m_h); | |||
| m_oglBackend->setViewport(data->X, data->Y, data->W, data->H); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve variable naming and maintain consistency
- Single-letter variable names (X, Y, W, H) could be more descriptive
- The naming change continues the inconsistent pattern
Consider using more descriptive names and maintaining the m_ prefix:
- m_oglBackend->setViewport(data->X, data->Y, data->W, data->H);
+ m_oglBackend->setViewport(data->m_posX, data->m_posY, data->m_width, data->m_height);Committable suggestion skipped: line range outside the PR's diff.
| /// @brief The font structure. | ||
| struct Font { | ||
| String Name; ///< Font name | ||
| i32 Size; ///< Font size | ||
| i32 Style; ///< Font style | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Font struct with style enumeration and initialization
Consider adding an enum class for font styles and a constructor for proper initialization.
+/// @brief Enumeration for font styles
+enum class FontStyle {
+ Normal,
+ Bold,
+ Italic,
+ BoldItalic
+};
struct Font {
String Name; ///< Font name
i32 Size; ///< Font size
- i32 Style; ///< Font style
+ FontStyle Style; ///< Font style
+
+ Font() :
+ Name(),
+ Size(12),
+ Style(FontStyle::Normal) {}
};📝 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 The font structure. | |
| struct Font { | |
| String Name; ///< Font name | |
| i32 Size; ///< Font size | |
| i32 Style; ///< Font style | |
| }; | |
| /// @brief Enumeration for font styles | |
| enum class FontStyle { | |
| Normal, | |
| Bold, | |
| Italic, | |
| BoldItalic | |
| }; | |
| /// @brief The font structure. | |
| struct Font { | |
| String Name; ///< Font name | |
| i32 Size; ///< Font size | |
| FontStyle Style; ///< Font style | |
| Font() : | |
| Name(), | |
| Size(12), | |
| Style(FontStyle::Normal) {} | |
| }; |
| 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<T>(offset); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks to renumberIndices template function
The function lacks bounds checking and type constraints.
+#include <type_traits>
+
template<class T>
-inline void renumberIndices(const DrawCmd &dc, T offset) {
+inline void renumberIndices(const DrawCmd &dc, T offset) {
+ static_assert(std::is_integral<T>::value, "Offset must be an integral type");
+
+ if (dc.Indices == nullptr) {
+ return;
+ }
+
if (offset > 0) {
+ const T maxValue = std::numeric_limits<T>::max();
for (size_t j = 0; j < dc.NumIndices; ++j) {
+ if (dc.Indices[j] > maxValue - offset) {
+ throw std::overflow_error("Index offset would cause overflow");
+ }
dc.Indices[j] += static_cast<T>(offset);
}
}
}📝 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.
| 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<T>(offset); | |
| } | |
| } | |
| } | |
| #include <type_traits> | |
| template<class T> | |
| inline void renumberIndices(const DrawCmd &dc, T offset) { | |
| static_assert(std::is_integral<T>::value, "Offset must be an integral type"); | |
| if (dc.Indices == nullptr) { | |
| return; | |
| } | |
| if (offset > 0) { | |
| const T maxValue = std::numeric_limits<T>::max(); | |
| for (size_t j = 0; j < dc.NumIndices; ++j) { | |
| if (dc.Indices[j] > maxValue - offset) { | |
| throw std::overflow_error("Index offset would cause overflow"); | |
| } | |
| dc.Indices[j] += static_cast<T>(offset); | |
| } | |
| } | |
| } |
| /// This struct is used to store all 2d draw commands. | ||
| struct DrawCmd { | ||
| PrimitiveType PrimType; ///< The primitive type | ||
| size_t NumVertices; ///< The number of vertices | ||
| RenderVert *Vertices; ///< The vertex buffer | ||
| size_t NumIndices; ///< Number of indices | ||
| ui16 *Indices; ///< The number of indices | ||
| Font *UseFont; ///< The used font | ||
|
|
||
| /// The class constructor. | ||
| DrawCmd() : | ||
| PrimType(PrimitiveType::Invalid), | ||
| NumVertices(0u), | ||
| Vertices(nullptr), | ||
| NumIndices(0u), | ||
| Indices(nullptr), | ||
| UseFont(nullptr) { | ||
| // empty | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve memory safety in DrawCmd struct
The struct uses raw pointers without clear ownership semantics, which could lead to memory leaks.
Consider using smart pointers or containers:
struct DrawCmd {
PrimitiveType PrimType;
size_t NumVertices;
- RenderVert *Vertices;
+ std::unique_ptr<RenderVert[]> Vertices;
size_t NumIndices;
- ui16 *Indices;
+ std::unique_ptr<ui16[]> Indices;
- Font *UseFont;
+ std::shared_ptr<Font> UseFont;
DrawCmd() :
PrimType(PrimitiveType::Invalid),
NumVertices(0u),
Vertices(nullptr),
NumIndices(0u),
Indices(nullptr),
UseFont(nullptr) {}
+
+ // Prevent copying, allow moving
+ DrawCmd(const DrawCmd&) = delete;
+ DrawCmd& operator=(const DrawCmd&) = delete;
+ DrawCmd(DrawCmd&&) = default;
+ DrawCmd& operator=(DrawCmd&&) = default;
};📝 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.
| /// This struct is used to store all 2d draw commands. | |
| struct DrawCmd { | |
| PrimitiveType PrimType; ///< The primitive type | |
| size_t NumVertices; ///< The number of vertices | |
| RenderVert *Vertices; ///< The vertex buffer | |
| size_t NumIndices; ///< Number of indices | |
| ui16 *Indices; ///< The number of indices | |
| Font *UseFont; ///< The used font | |
| /// The class constructor. | |
| DrawCmd() : | |
| PrimType(PrimitiveType::Invalid), | |
| NumVertices(0u), | |
| Vertices(nullptr), | |
| NumIndices(0u), | |
| Indices(nullptr), | |
| UseFont(nullptr) { | |
| // empty | |
| } | |
| }; | |
| /// This struct is used to store all 2d draw commands. | |
| struct DrawCmd { | |
| PrimitiveType PrimType; ///< The primitive type | |
| size_t NumVertices; ///< The number of vertices | |
| std::unique_ptr<RenderVert[]> Vertices; ///< The vertex buffer | |
| size_t NumIndices; ///< Number of indices | |
| std::unique_ptr<ui16[]> Indices; ///< The number of indices | |
| std::shared_ptr<Font> UseFont; ///< The used font | |
| /// The class constructor. | |
| DrawCmd() : | |
| PrimType(PrimitiveType::Invalid), | |
| NumVertices(0u), | |
| Vertices(nullptr), | |
| NumIndices(0u), | |
| Indices(nullptr), | |
| UseFont(nullptr) { | |
| // empty | |
| } | |
| // Prevent copying, allow moving | |
| DrawCmd(const DrawCmd&) = delete; | |
| DrawCmd& operator=(const DrawCmd&) = delete; | |
| DrawCmd(DrawCmd&&) = default; | |
| DrawCmd& operator=(DrawCmd&&) = default; | |
| }; |
| @@ -323,6 +330,14 @@ | |||
|
|
|||
| App::AssetRegistry::registerAssetPathInBinFolder("assets", "assets"); | |||
|
|
|||
| Rect2ui rect; | |||
| mPlatformInterface->getRootWindow()->getWindowsRect(rect); | |||
| mCanvasRenderer = new CanvasRenderer(2, (i32)rect.getX1(), (i32)rect.getY1(), (i32)rect.getWidth(), (i32)rect.getHeight()); | |||
Check warning
Code scanning / CodeQL
Resource not released in destructor Warning
| NumMaterialColorTypes, ///< Number of enums. | ||
|
|
||
| InvalidMaterialColorType ///< Enum for invalid enum. | ||
| enum class MaterialColorType : i32 { |
Check notice
Code scanning / CodeQL
Irregular enum initialization Note
| /*MeshBuilder &MeshBuilder::allocUiQuad( const Rect2ui &dim, UiVertexCache &vc, RenderBackend::UiIndexCache &ic ) { | ||
| const f32 x = static_cast<f32>(dim.getX1()); | ||
| const f32 y = static_cast<f32>(dim.getY1()); | ||
| const f32 w = static_cast<f32>(dim.getWidth()); | ||
| const f32 h = static_cast<f32>(dim.getHeight()); | ||
|
|
||
| // setup triangle vertices | ||
| static const ui32 NumVert = 4; | ||
| glm::vec3 col[NumVert] = {}; | ||
| col[ 0 ] = glm::vec3( 1, 0, 0 ); | ||
| col[ 1 ] = glm::vec3( 0, 1, 0 ); | ||
| col[ 2 ] = glm::vec3( 0, 0, 1 ); | ||
| col[ 3 ] = glm::vec3( 1, 0, 0 ); | ||
|
|
||
| glm::vec3 pos[NumVert] = {}; | ||
| pos[ 0 ] = glm::vec3( x, y, 0 ); | ||
| pos[ 1 ] = glm::vec3( x, y+h, 0 ); | ||
| pos[ 2 ] = glm::vec3( x+w, y, 0 ); | ||
| pos[ 3 ] = glm::vec3( x+h, y+h, 0 ); | ||
|
|
||
| glm::vec2 tex0[NumVert] = {}; | ||
| tex0[ 0 ] = glm::vec2( 0, 0 ); | ||
| tex0[ 1 ] = glm::vec2( 0, 1 ); | ||
| tex0[ 2 ] = glm::vec2( 1, 0 ); | ||
| tex0[ 3 ] = glm::vec2( 1, 1 ); | ||
|
|
||
| for (ui32 i = 0; i < 4; ++i) { | ||
| RenderVert v; | ||
| v.position = pos[ i ]; | ||
| v.color0 = col[ i ]; | ||
| v.tex0 = tex0[ i ]; | ||
| vc.add( v ); | ||
| } | ||
|
|
||
| // setup triangle indices | ||
| static const ui32 NumIndices = 6; | ||
| ui16 indices[NumIndices] = {}; | ||
| indices[ 0 ] = 0; | ||
| indices[ 1 ] = 2; | ||
| indices[ 2 ] = 1; | ||
|
|
||
| indices[ 3 ] = 1; | ||
| indices[ 4 ] = 2; | ||
| indices[ 5 ] = 3; | ||
|
|
||
| for (ui32 i = 0; i < 4; ++i) { | ||
| ic.add( indices[ i ] ); | ||
| } | ||
|
|
||
| return *this; | ||
| } | ||
| }*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
src/Engine/App/Component.h (1)
130-158: Maintain consistent naming between parameters and member variables.While the documentation improvements are excellent, there's an inconsistency in terminology. The parameter name was changed from 'geo' to 'mesh', but the member variable
m_newGeostill uses the old terminology.Consider renaming the member variable for consistency:
- cppcore::TArray<RenderBackend::Mesh*> m_newGeo; + cppcore::TArray<RenderBackend::Mesh*> m_newMeshes;src/Engine/App/AppBase.h (1)
265-267: Add documentation for getCanvasRenderer method.The method lacks documentation explaining its purpose, return value semantics (ownership, lifetime), and possible null return cases.
- /// @brief + /// @brief Returns the canvas renderer instance. + /// @return Pointer to the canvas renderer, or nullptr if not initialized. virtual RenderBackend::CanvasRenderer *getCanvasRenderer() const;src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
Line range hint
189-223: Add error handling and null checks in render method.The render method needs additional error handling and null checks:
- No null check for
dc.Indices- No validation of
numVerticesandnumIndicesbefore usageAdd necessary checks:
for (size_t i=0; i<mDrawCmdArray.size(); ++i) { const auto &dc = *mDrawCmdArray[i]; if (dc.Vertices == nullptr) { osre_debug(Tag, "Invalid draw command detected."); continue; } + if (dc.Indices == nullptr) { + osre_debug(Tag, "Invalid indices detected."); + continue; + } + if (dc.NumVertices == 0 || dc.NumIndices == 0) { + osre_debug(Tag, "Empty draw command detected."); + continue; + } const ui32 lastIndex = mMesh->getLastIndex(); renumberIndices(dc, numVertices);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
contrib/cppcore(1 hunks)src/Engine/App/AppBase.cpp(11 hunks)src/Engine/App/AppBase.h(5 hunks)src/Engine/App/Component.cpp(1 hunks)src/Engine/App/Component.h(3 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(8 hunks)src/Engine/RenderBackend/RenderCommon.h(13 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Engine/App/Component.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- contrib/cppcore
- src/Engine/App/AppBase.cpp
🔇 Additional comments (14)
src/Engine/App/Component.h (2)
52-52: LGTM: Clear and consistent documentation.
The added description for AnimationComponentType follows the established documentation pattern and provides clear context.
168-188: LGTM: Good documentation and proper pointer initialization.
The changes demonstrate good practices:
- Clear and consistent documentation style
- Proper initialization of the pointer member variable
src/Engine/App/AppBase.h (2)
53-55: LGTM: Forward declarations are properly structured.
The new forward declarations for IRenderPath and CanvasRenderer are correctly placed and follow the codebase's naming conventions.
308-310: LGTM: Modern C++ member initialization.
The in-class initializers for mStageMode, mShutdownRequested, and mCanvasRenderer follow modern C++ best practices.
src/Engine/RenderBackend/2D/CanvasRenderer.cpp (4)
28-29: LGTM: Required includes for font and mesh utilities.
The new includes are necessary for the added text rendering functionality.
299-301: LGTM: Convenient Point2Di overloads.
The Point2Di overloads provide a cleaner interface for the drawing methods.
Also applies to: 352-353
384-387: LGTM: Font selection method.
The selectFont method correctly updates the font and marks the renderer as dirty.
430-434:
Add error recovery in onCreate.
The method returns false if font initialization fails but doesn't clean up any resources that might have been allocated.
Add cleanup and logging:
bool CanvasRenderer::onCreate() {
mFont = FontService::getDefaultFont();
+ if (mFont == nullptr) {
+ osre_debug(Tag, "Failed to initialize default font.");
+ // Clean up any resources allocated before font initialization
+ return false;
+ }
return true;
}Likely invalid or redundant comment.
src/Engine/RenderBackend/RenderCommon.h (6)
62-68: LGTM: Consistent enum standardization
The standardization of enum values with Invalid = -1 and Count is a good practice for error handling and iteration. This change improves type safety and makes the code more maintainable.
Also applies to: 73-77, 82-85, 90-93, 98-102, 110-115, 120-125, 130-136, 141-145, 162-167, 172-184, 189-206, 211-220, 225-229
991-991:
Fix typo in state enumeration value
There's a typo in the state name "Detroyed".
1041-1046: 🛠️ Refactor suggestion
Enhance Font structure with type-safe style enumeration
The Font structure uses a raw integer for style which could lead to invalid values.
1048-1067: 🛠️ Refactor suggestion
Improve memory safety in DrawCmd struct
The struct uses raw pointers without clear ownership semantics.
1026-1028:
Fix function name typo
The function name contains a typo: "Frome" should be "From".
984-997:
Fix state transition in destroy() method
The destroy() method checks for LifetimeState::Running but the create() method transitions to LifetimeState::Created. This mismatch could prevent proper cleanup.
Apply this diff to fix the state check:
inline bool IRenderPath::destroy() {
- if (mLifetimeState != LifetimeState::Running) {
+ if (mLifetimeState != LifetimeState::Created) {
return false;
}Likely invalid or redundant comment.
| /// @brief Returns true, when the key is pressed | ||
| /// @param key The key to look for | ||
| /// @return true for is pressed. | ||
| bool isKeyPressed(Platform::Key key) const { | ||
| return mKeymap[key] == 1; | ||
| } | ||
|
|
||
| /// @brief Will return the latest pressed key. | ||
| /// @return The latest pressed key. | ||
| Platform::Key getLastKey() const { | ||
| return mLast; | ||
| } | ||
|
|
||
| /// @brief Clearn the map. | ||
| void clearKeyMap() { | ||
| ::memset(mKeymap, 0, sizeof(char) * Platform::KEY_LAST); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety in keyboard state management.
Consider these improvements for better type safety and maintainability:
- Use
std::array<bool, Platform::KEY_LAST>instead of raw array - Use
std::fillinstead ofmemset - Add explicit initialization in the constructor
class OSRE_EXPORT KeyboardEventListener : public Platform::OSEventListener {
private:
Platform::Key mLast;
- char mKeymap[Platform::KEY_LAST];
+ std::array<bool, Platform::KEY_LAST> mKeymap;
public:
KeyboardEventListener() :
OSEventListener("App/KeyboardEventListener"),
- mLast(Platform::KEY_UNKNOWN) {
- clearKeyMap();
+ mLast(Platform::KEY_UNKNOWN),
+ mKeymap{} {
}
void clearKeyMap() {
- ::memset(mKeymap, 0, sizeof(char) * Platform::KEY_LAST);
+ mKeymap.fill(false);
}
bool isKeyPressed(Platform::Key key) const {
return mKeymap[key];
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| CanvasRenderer::CanvasRenderer(i32 numLayers, i32 x, i32 y, i32 w, i32 h) : | ||
| mDirty(true), mPenColor(1, 1, 1, 0), mResolution(), mActiveLayer(0), mNumLayers(numLayers), mMesh(nullptr) { | ||
| IRenderPath(), | ||
| mDirty(true), | ||
| mPenColor(1, 1, 1, 0), | ||
| mResolution(), | ||
| mActiveLayer(0), | ||
| mNumLayers(numLayers), | ||
| mFont(nullptr), | ||
| mMesh(nullptr) { |
There was a problem hiding this comment.
Ensure proper cleanup in destructor.
The constructor correctly initializes the new members, but the destructor needs to handle cleanup of font resources.
Add font cleanup to the destructor:
CanvasRenderer::~CanvasRenderer() {
for (size_t i = 0; i < mDrawCmdArray.size(); ++i) {
auto &dc = *mDrawCmdArray[i];
dealloc(&dc);
}
+ // Font is managed by FontService, no need to delete
+ mFont = nullptr;
+ delete mMesh;
+ mMesh = nullptr;
}Committable suggestion skipped: line range outside the PR's diff.
| static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) { | ||
| i32 x_clipped{0}, y_clipped{0}; | ||
| f32 x_model{0.f}, y_model{0.f}; | ||
|
|
||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = 6; | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
|
|
||
| clip(resolution, x, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[0].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[0].position.x = x_model; | ||
| drawCmd->Vertices[0].position.y = y_model; | ||
| drawCmd->Vertices[0].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x+w, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[1].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[1].position.x = x_model; | ||
| drawCmd->Vertices[1].position.y = y_model; | ||
| drawCmd->Vertices[1].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[2].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[2].position.x = x_model; | ||
| drawCmd->Vertices[2].position.y = y_model; | ||
| drawCmd->Vertices[2].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[3].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[3].position.x = x_model; | ||
| drawCmd->Vertices[3].position.y = y_model; | ||
| drawCmd->Vertices[3].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x, y+h, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[4].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[4].position.x = x_model; | ||
| drawCmd->Vertices[4].position.y = y_model; | ||
| drawCmd->Vertices[4].position.z = static_cast<f32>(-layer); | ||
|
|
||
| clip(resolution, x, y, x_clipped, y_clipped); | ||
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | ||
| drawCmd->Vertices[5].color0 = penColor.toVec4(); | ||
| drawCmd->Vertices[5].position.x = x_model; | ||
| drawCmd->Vertices[5].position.y = y_model; | ||
| drawCmd->Vertices[5].position.z = static_cast<f32>(-layer); | ||
|
|
||
| drawCmd->NumIndices = 6; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
| drawCmd->Indices[0] = 0; | ||
| drawCmd->Indices[1] = 2; | ||
| drawCmd->Indices[2] = 1; | ||
|
|
||
| drawCmd->Indices[3] = 3; | ||
| drawCmd->Indices[4] = 5; | ||
| drawCmd->Indices[5] = 4; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize rectangle vertex creation for better efficiency.
The current implementation has several inefficiencies:
- Uses 6 vertices when 4 would suffice (vertices can be shared between triangles)
- Contains repetitive vertex setup code
- Could be more maintainable with a loop-based approach
Consider this optimized implementation:
static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) {
drawCmd->PrimType = PrimitiveType::TriangleList;
- drawCmd->NumVertices = 6;
+ drawCmd->NumVertices = 4;
drawCmd->Vertices = new RenderVert[drawCmd->NumVertices];
+ const Vec4 color = penColor.toVec4();
+ const f32 z = static_cast<f32>(-layer);
+
+ // Define corners: top-left, top-right, bottom-right, bottom-left
+ const i32 coords[4][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}};
+
+ for (ui32 i = 0; i < 4; ++i) {
+ i32 x_clipped, y_clipped;
+ f32 x_model, y_model;
+
+ clip(resolution, coords[i][0], coords[i][1], x_clipped, y_clipped);
+ mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model);
+
+ drawCmd->Vertices[i].color0 = color;
+ drawCmd->Vertices[i].position = Vec3(x_model, y_model, z);
+ }
drawCmd->NumIndices = 6;
drawCmd->Indices = new ui16[drawCmd->NumIndices];
- drawCmd->Indices[0] = 0;
- drawCmd->Indices[1] = 2;
- drawCmd->Indices[2] = 1;
- drawCmd->Indices[3] = 3;
- drawCmd->Indices[4] = 5;
- drawCmd->Indices[5] = 4;
+ // Define indices for two triangles (0,1,2) and (2,3,0)
+ const ui16 indices[6] = {0, 1, 2, 2, 3, 0};
+ memcpy(drawCmd->Indices, indices, sizeof(ui16) * 6);
}📝 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.
| static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) { | |
| i32 x_clipped{0}, y_clipped{0}; | |
| f32 x_model{0.f}, y_model{0.f}; | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = 6; | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| clip(resolution, x, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[0].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[0].position.x = x_model; | |
| drawCmd->Vertices[0].position.y = y_model; | |
| drawCmd->Vertices[0].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x+w, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[1].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[1].position.x = x_model; | |
| drawCmd->Vertices[1].position.y = y_model; | |
| drawCmd->Vertices[1].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[2].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[2].position.x = x_model; | |
| drawCmd->Vertices[2].position.y = y_model; | |
| drawCmd->Vertices[2].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x+w, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[3].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[3].position.x = x_model; | |
| drawCmd->Vertices[3].position.y = y_model; | |
| drawCmd->Vertices[3].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x, y+h, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[4].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[4].position.x = x_model; | |
| drawCmd->Vertices[4].position.y = y_model; | |
| drawCmd->Vertices[4].position.z = static_cast<f32>(-layer); | |
| clip(resolution, x, y, x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[5].color0 = penColor.toVec4(); | |
| drawCmd->Vertices[5].position.x = x_model; | |
| drawCmd->Vertices[5].position.y = y_model; | |
| drawCmd->Vertices[5].position.z = static_cast<f32>(-layer); | |
| drawCmd->NumIndices = 6; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| drawCmd->Indices[0] = 0; | |
| drawCmd->Indices[1] = 2; | |
| drawCmd->Indices[2] = 1; | |
| drawCmd->Indices[3] = 3; | |
| drawCmd->Indices[4] = 5; | |
| drawCmd->Indices[5] = 4; | |
| } | |
| static void createRectVertices(DrawCmd *drawCmd, const Color4 &penColor, const Rect2i &resolution, i32 x, i32 y, i32 w, i32 h, i32 layer) { | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = 4; | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const Vec4 color = penColor.toVec4(); | |
| const f32 z = static_cast<f32>(-layer); | |
| // Define corners: top-left, top-right, bottom-right, bottom-left | |
| const i32 coords[4][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}}; | |
| for (ui32 i = 0; i < 4; ++i) { | |
| i32 x_clipped, y_clipped; | |
| f32 x_model, y_model; | |
| clip(resolution, coords[i][0], coords[i][1], x_clipped, y_clipped); | |
| mapCoordinates(resolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[i].color0 = color; | |
| drawCmd->Vertices[i].position = Vec3(x_model, y_model, z); | |
| } | |
| drawCmd->NumIndices = 6; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| // Define indices for two triangles (0,1,2) and (2,3,0) | |
| const ui16 indices[6] = {0, 1, 2, 2, 3, 0}; | |
| memcpy(drawCmd->Indices, indices, sizeof(ui16) * 6); | |
| } |
| setDirty(); | ||
| DrawCmd *drawCmd = nullptr; | ||
| if (filled) { | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| } else { | ||
| const ui32 thickness = 2; | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| return; | ||
| } | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| const ui32 thickness = 2; | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| drawCmd = alloc(); | ||
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | ||
| mDrawCmdArray.add(drawCmd); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize unfilled rectangle rendering.
Creating four separate draw commands for an unfilled rectangle is inefficient. This approach:
- Increases memory usage with multiple allocations
- Results in more draw calls
- Makes state management more complex
Consider using a single draw command with line strip primitive:
void CanvasRenderer::drawRect(i32 x, i32 y, i32 w, i32 h, bool filled) {
setDirty();
DrawCmd *drawCmd = nullptr;
if (filled) {
drawCmd = alloc();
createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer);
mDrawCmdArray.add(drawCmd);
return;
}
- const ui32 thickness = 2;
- drawCmd = alloc();
- createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer);
- mDrawCmdArray.add(drawCmd);
- // ... more draw commands ...
+ drawCmd = alloc();
+ drawCmd->PrimType = PrimitiveType::LineStrip;
+ drawCmd->NumVertices = 5; // 4 corners + closing point
+ drawCmd->Vertices = new RenderVert[drawCmd->NumVertices];
+
+ const Vec4 color = mPenColor.toVec4();
+ const f32 z = static_cast<f32>(-mActiveLayer);
+
+ // Define corners: top-left, top-right, bottom-right, bottom-left, top-left
+ i32 coords[5][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}, {x,y}};
+
+ for (ui32 i = 0; i < 5; ++i) {
+ i32 x_clipped, y_clipped;
+ f32 x_model, y_model;
+ clip(mResolution, coords[i][0], coords[i][1], x_clipped, y_clipped);
+ mapCoordinates(mResolution, x_clipped, y_clipped, x_model, y_model);
+
+ drawCmd->Vertices[i].color0 = color;
+ drawCmd->Vertices[i].position = Vec3(x_model, y_model, z);
+ }
+
+ drawCmd->NumIndices = 5;
+ drawCmd->Indices = new ui16[drawCmd->NumIndices];
+ for (ui16 i = 0; i < 5; ++i) {
+ drawCmd->Indices[i] = i;
+ }
+ mDrawCmdArray.add(drawCmd);
}📝 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.
| setDirty(); | |
| DrawCmd *drawCmd = nullptr; | |
| if (filled) { | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| } else { | |
| const ui32 thickness = 2; | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| return; | |
| } | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| const ui32 thickness = 2; | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y + h, w, thickness, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x+w, y, thickness, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| } | |
| setDirty(); | |
| DrawCmd *drawCmd = nullptr; | |
| if (filled) { | |
| drawCmd = alloc(); | |
| createRectVertices(drawCmd, mPenColor, mResolution, x, y, w, h, mActiveLayer); | |
| mDrawCmdArray.add(drawCmd); | |
| return; | |
| } | |
| drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::LineStrip; | |
| drawCmd->NumVertices = 5; // 4 corners + closing point | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const Vec4 color = mPenColor.toVec4(); | |
| const f32 z = static_cast<f32>(-mActiveLayer); | |
| // Define corners: top-left, top-right, bottom-right, bottom-left, top-left | |
| i32 coords[5][2] = {{x,y}, {x+w,y}, {x+w,y+h}, {x,y+h}, {x,y}}; | |
| for (ui32 i = 0; i < 5; ++i) { | |
| i32 x_clipped, y_clipped; | |
| f32 x_model, y_model; | |
| clip(mResolution, coords[i][0], coords[i][1], x_clipped, y_clipped); | |
| mapCoordinates(mResolution, x_clipped, y_clipped, x_model, y_model); | |
| drawCmd->Vertices[i].color0 = color; | |
| drawCmd->Vertices[i].position = Vec3(x_model, y_model, z); | |
| } | |
| drawCmd->NumIndices = 5; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| for (ui16 i = 0; i < 5; ++i) { | |
| drawCmd->Indices[i] = i; | |
| } | |
| mDrawCmdArray.add(drawCmd); | |
| } |
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | ||
| if (text.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| if (mFont == nullptr) { | ||
| osre_debug(Tag, "No font selected."); | ||
| return; | ||
| } | ||
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | ||
| mapCoordinates(mResolution, x, y, x_model, y_model); | ||
| Vec3Array positions; | ||
| Vec3Array colors; | ||
| Vec2Array tex0; | ||
| ui16 *indices = nullptr; | ||
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | ||
|
|
||
| DrawCmd *drawCmd = alloc(); | ||
| drawCmd->PrimType = PrimitiveType::TriangleList; | ||
| drawCmd->NumVertices = positions.size(); | ||
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | ||
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | ||
| drawCmd->NumIndices = numIndices; | ||
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | ||
|
|
||
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | ||
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec3(); | ||
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | ||
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | ||
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | ||
| } | ||
|
|
||
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | ||
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | ||
| } | ||
|
|
||
| mDrawCmdArray.add(drawCmd); | ||
|
|
||
| setDirty(); |
There was a problem hiding this comment.
Fix critical issues in text rendering implementation.
The drawText method has several issues:
- Memory leak: indices array is not deallocated
- Missing texture coordinates for text rendering
- Incorrect color vector usage (toVec3 vs toVec4)
Apply these fixes:
void CanvasRenderer::drawText(i32 x, i32 y, const String &text) {
// ... existing code ...
for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) {
- drawCmd->Vertices[posIndex].color0 = mPenColor.toVec3();
+ drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4();
drawCmd->Vertices[posIndex].position.x = positions[posIndex].x;
drawCmd->Vertices[posIndex].position.y = positions[posIndex].y;
drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer);
+ drawCmd->Vertices[posIndex].tex0 = tex0[posIndex];
}
// ... copy indices ...
mDrawCmdArray.add(drawCmd);
+ delete[] indices; // Free allocated memory
setDirty();
}📝 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.
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | |
| if (text.empty()) { | |
| return; | |
| } | |
| if (mFont == nullptr) { | |
| osre_debug(Tag, "No font selected."); | |
| return; | |
| } | |
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | |
| mapCoordinates(mResolution, x, y, x_model, y_model); | |
| Vec3Array positions; | |
| Vec3Array colors; | |
| Vec2Array tex0; | |
| ui16 *indices = nullptr; | |
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | |
| DrawCmd *drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = positions.size(); | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | |
| drawCmd->NumIndices = numIndices; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | |
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec3(); | |
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | |
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | |
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | |
| } | |
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | |
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | |
| } | |
| mDrawCmdArray.add(drawCmd); | |
| setDirty(); | |
| void CanvasRenderer::drawText(i32 x, i32 y, const String &text) { | |
| if (text.empty()) { | |
| return; | |
| } | |
| if (mFont == nullptr) { | |
| osre_debug(Tag, "No font selected."); | |
| return; | |
| } | |
| f32 x_model, y_model, fontSize = static_cast<f32>(mFont->Size)/static_cast<f32>(mResolution.getWidth()); | |
| mapCoordinates(mResolution, x, y, x_model, y_model); | |
| Vec3Array positions; | |
| Vec3Array colors; | |
| Vec2Array tex0; | |
| ui16 *indices = nullptr; | |
| MeshUtilities::generateTextBoxVerticesAndIndices(x_model, y_model, fontSize, text, positions, colors, tex0, &indices); | |
| DrawCmd *drawCmd = alloc(); | |
| drawCmd->PrimType = PrimitiveType::TriangleList; | |
| drawCmd->NumVertices = positions.size(); | |
| drawCmd->Vertices = new RenderVert[drawCmd->NumVertices]; | |
| const size_t numIndices = MeshUtilities::getNumTextIndices(text); | |
| drawCmd->NumIndices = numIndices; | |
| drawCmd->Indices = new ui16[drawCmd->NumIndices]; | |
| for (size_t posIndex = 0; posIndex < positions.size(); ++posIndex) { | |
| drawCmd->Vertices[posIndex].color0 = mPenColor.toVec4(); | |
| drawCmd->Vertices[posIndex].position.x = positions[posIndex].x; | |
| drawCmd->Vertices[posIndex].position.y = positions[posIndex].y; | |
| drawCmd->Vertices[posIndex].position.z = static_cast<f32>(-mActiveLayer); | |
| drawCmd->Vertices[posIndex].tex0 = tex0[posIndex]; | |
| } | |
| for (size_t idxIndex = 0; idxIndex < numIndices; ++idxIndex) { | |
| drawCmd->Indices[idxIndex] = indices[idxIndex]; | |
| } | |
| mDrawCmdArray.add(drawCmd); | |
| delete[] indices; // Free allocated memory | |
| setDirty(); |
| 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.
Add safety checks to renumberIndices template function
The function lacks bounds checking and type constraints which could lead to undefined behavior.
Apply this diff to add safety checks:
template<class T>
inline void renumberIndices(const DrawCmd &dc, T offset) {
+ static_assert(std::is_integral<T>::value, "Offset must be an integral type");
+
+ if (dc.Indices == nullptr) {
+ return;
+ }
+
if (offset > 0) {
+ const T maxValue = std::numeric_limits<T>::max();
for (size_t j = 0; j < dc.NumIndices; ++j) {
+ if (dc.Indices[j] > maxValue - offset) {
+ throw std::overflow_error("Index offset would cause overflow");
+ }
dc.Indices[j] += static_cast<ui16>(offset);
}
}
}📝 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.
| 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 T> | |
| inline void renumberIndices(const DrawCmd &dc, T offset) { | |
| static_assert(std::is_integral<T>::value, "Offset must be an integral type"); | |
| if (dc.Indices == nullptr) { | |
| return; | |
| } | |
| if (offset > 0) { | |
| const T maxValue = std::numeric_limits<T>::max(); | |
| for (size_t j = 0; j < dc.NumIndices; ++j) { | |
| if (dc.Indices[j] > maxValue - offset) { | |
| throw std::overflow_error("Index offset would cause overflow"); | |
| } | |
| dc.Indices[j] += static_cast<ui16>(offset); | |
| } | |
| } | |
| } |
Summary by CodeRabbit
New Features
TextRendererclass for improved text rendering capabilities.addText,preRender,render, andpostRender.CanvasRendererwith text rendering and triangle drawing functionalities.Improvements
UIElementsWin32class during library loading.Bug Fixes
Loggerclass to exclude date and time by default.Chores