[Draft] Rework text vertex calculation #320
Conversation
WalkthroughThe changes introduce a new static function in the 2D canvas renderer to generate vertex and index data for text rendering, updating the text drawing method to use this function. Pointer members in Changes
Sequence Diagram(s)sequenceDiagram
participant Widget as WidgetBase
participant Button as Button (UI)
participant Text as Text (UI)
participant Renderer as CanvasRenderer
Widget->>Widget: setActive()/setInactive()
Widget->>Widget: isActive()
Button->>Text: setLabel(text)
Button->>Text: getLabel()
Renderer->>Renderer: drawText()
Renderer->>Renderer: generateTextBoxVerticesAndIndices()
Renderer->>Renderer: Fill vertex/index buffers
Text->>Renderer: onRender()
Renderer->>Renderer: drawText()
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/Engine/UI/WidgetBase.h (3)
46-49: Expose the new active-state API asconstexprgetters / setters or keep symmetry with existing “dirty” API
setDirty()/setClean()communicate intent clearly.
For symmetry (and easier grepability) consider either:
setActive()/setInactive()➔ rename tosetActive()/setInactive()is fine, but then also offer the opposite queryisInactive()to match the pair, or- keep the existing naming convention and add
activate()/deactivate().Not blocking, but consistency makes the UI framework easier to reason about.
61-63: Prefer explicit boolean literals over magic numbers
mActive(true)is correct but opaque when scanning quickly.
Use an explicit literal for readability:- mParent(parent), mDirty(true), mActive(true), mRect(rect) { + mParent(parent), mDirty(true), mActive(/*active*/true), mRect(rect) {
103-113: Guard rendering / updates with the new active flagYou added
isActive()butupdate()/render()currently ignore it:inline void WidgetBase::update() { onUpdate(); }A dormant widget will still burn CPU each frame.
Consider:inline void WidgetBase::update() { - onUpdate(); + if (mActive) { + onUpdate(); + } } inline void WidgetBase::render(RenderBackend::CanvasRenderer *renderer) { - onRender(renderer); + if (mActive) { + onRender(renderer); + } }src/Engine/RenderBackend/2D/CanvasRenderer.cpp (1)
521-528: Remove redundant colour/position pre-passesSince
positionsandcolorswere already produced you could bulk-copy:memcpy(drawCmd->Vertices, positions.data(), positions.size()*sizeof(glm::vec3));or iterate once and assign both colour and tex coords, avoiding two separate loops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
contrib/vcpkg(1 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(3 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.h(1 hunks)src/Engine/UI/WidgetBase.h(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (cpp)
- GitHub Check: build
🔇 Additional comments (1)
src/Engine/RenderBackend/2D/CanvasRenderer.h (1)
166-169: 👍 Explicit nullptr initialisation prevents UBInitialising the raw pointers to
nullptris a safe default and removes any indeterminate-value UB.
| bool mActive; | ||
| Rect2i mRect; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing default-member-initialiser & thread-safety remark
mActive is only initialised via the constructor initialiser list.
To avoid future “uninitialised member” regressions (e.g. if another ctor overload is added) add an in-class initialiser:
-bool mActive;
+bool mActive{true};This also avoids a potential data-race if a widget is accessed from another thread before construction finishes.
📝 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.
| bool mActive; | |
| Rect2i mRect; | |
| bool mActive{true}; | |
| Rect2i mRect; |
🤖 Prompt for AI Agents
In src/Engine/UI/WidgetBase.h at lines 57-58, the member variable mActive lacks
a default in-class initializer, which risks uninitialized usage if new
constructors are added and may cause data races if accessed concurrently during
construction. Add a default member initializer to mActive (e.g., bool mActive =
false;) directly in the class definition to ensure it is always initialized
safely and prevent potential threading issues.
| static void generateTextBoxVerticesAndIndices(const Rect2i &resolution, i32 x, i32 y, i32 textSize, i32 layer, const String &text, | ||
| Vec3Array &positions, Vec3Array &colors, Vec2Array &tex0, cppcore::TArray<ui16> &textIndices) { | ||
| using namespace ::OSRE::Common; | ||
|
|
||
| const size_t NumTextVerts = MeshUtilities::getNumTextVerts(text); | ||
| positions.resize(NumTextVerts); | ||
| colors.resize(NumTextVerts); | ||
| tex0.resize(NumTextVerts); | ||
| textIndices.resize(MeshUtilities::getNumTextIndices(text)); | ||
|
|
||
| const f32 invCol = 1.f / 16.f; | ||
| const f32 invRow = 1.f / 16.f; | ||
| ui32 textCol(0), textRow(0); | ||
| for (size_t i = 0; i < text.size(); i++) { | ||
| const i32 colOffset = textCol * textSize; | ||
| 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 i32 rowHeight = -1 * textRow * textSize; | ||
|
|
||
| mapCoordinates(resolution, x + colOffset, y + rowHeight, positions[VertexOffset + 0].x, positions[VertexOffset + 0].y); | ||
| positions[VertexOffset + 0].z = static_cast<f32>(-layer); | ||
|
|
||
| mapCoordinates(resolution, x + colOffset, y + textSize + rowHeight, positions[VertexOffset + 1].x, positions[VertexOffset + 1].y); | ||
| positions[VertexOffset + 1].z = static_cast<f32>(-layer); | ||
|
|
||
| mapCoordinates(resolution, x + textSize + colOffset, y + rowHeight, positions[VertexOffset + 2].x, positions[VertexOffset + 2].y); | ||
| positions[VertexOffset + 2].z = static_cast<f32>(-layer); | ||
|
|
||
| mapCoordinates(resolution, x + textSize + colOffset, y + textSize + rowHeight, positions[VertexOffset + 3].x, positions[VertexOffset + 3].y); | ||
| positions[VertexOffset + 3].z = static_cast<f32>(-layer); | ||
|
|
||
| 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; | ||
|
|
||
| colors[VertexOffset + 0] = glm::vec3(0, 0, 0); | ||
| colors[VertexOffset + 1] = glm::vec3(0, 0, 0); | ||
| colors[VertexOffset + 2] = glm::vec3(0, 0, 0); | ||
| colors[VertexOffset + 3] = glm::vec3(0, 0, 0); | ||
| 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
generateTextBoxVerticesAndIndices – eliminate unused arrays & potential overflow
colorsis completely filled but never used later. Remove it or use it to drivecolor0.- Vertex/index offsets are cast to
ui16. For very long strings (> 6553 chars) this overflows. Either:
• enforceOSRE_ASSERT(text.size() < 6553)or
• switch the public interface toui32. - Consider
reserve()instead ofresize()+ manual assignment to avoid default-construction overhead.
- positions.resize(NumTextVerts);
- colors.resize(NumTextVerts);
- tex0.resize(NumTextVerts);
- textIndices.resize(MeshUtilities::getNumTextIndices(text));
+ positions.reserve(NumTextVerts);
+ tex0.reserve(NumTextVerts);
+ textIndices.reserve(MeshUtilities::getNumTextIndices(text));This keeps RAM the same but skips pointless writes.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Engine/RenderBackend/2D/CanvasRenderer.cpp lines 50 to 118, remove the
unused colors array or integrate it properly to drive color0 since it is
currently filled but not used. Replace the ui16 casts for vertex and index
offsets with a wider type like ui32 or add an assertion to ensure text.size() is
less than 6553 to prevent overflow on long strings. Change positions, colors,
tex0, and textIndices from resize() to reserve() to avoid unnecessary default
construction and improve performance.
![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
🧹 Nitpick comments (1)
src/Engine/UI/Text.h (1)
30-34: Incomplete class documentation.The class documentation contains a "Todo!" placeholder instead of describing the purpose and functionality of the Text class.
Consider updating the class documentation to describe the Text widget's purpose and usage:
//------------------------------------------------------------------------------------------------- /// @ingroup Engine /// -/// @brief Todo! +/// @brief Text widget for displaying text in UI components. +/// Renders a text string within a rectangular area. //-------------------------------------------------------------------------------------------------
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
contrib/vcpkg(1 hunks)samples/06_Widgets/Widgets.cpp(2 hunks)src/Engine/CMakeLists.txt(1 hunks)src/Engine/RenderBackend/2D/CanvasRenderer.cpp(6 hunks)src/Engine/UI/Button.cpp(1 hunks)src/Engine/UI/Button.h(2 hunks)src/Engine/UI/Text.cpp(1 hunks)src/Engine/UI/Text.h(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- contrib/vcpkg
- src/Engine/CMakeLists.txt
- src/Engine/UI/Text.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Engine/RenderBackend/2D/CanvasRenderer.cpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Engine/UI/Button.h (2)
src/Engine/UI/Text.cpp (4)
setLabel(36-42)setLabel(36-36)getLabel(44-46)getLabel(44-44)src/Engine/UI/WidgetBase.h (3)
getRect(45-45)getRect(99-101)getRect(99-99)
src/Engine/UI/Text.h (1)
src/Engine/UI/Text.cpp (2)
Text(27-30)Text(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (cpp)
- GitHub Check: build
🔇 Additional comments (15)
samples/06_Widgets/Widgets.cpp (4)
36-36: Include added for the new Text widget class.The inclusion of "UI/Text.h" is required to use the new Text widget class that has been introduced as part of this PR.
44-44: Panel size increased to provide more space for UI elements.The panel rectangle has been expanded from the previous (10,10,200,100) to (100,100,400,400), which provides more display area for the UI elements.
Also applies to: 46-47
48-49: Button replaced with Text widget to demonstrate the new widget.The UI creation now uses the new Text widget class to display "Hello World" instead of a Button widget with a "Quit" label. This change demonstrates how to use the newly introduced Text widget.
67-67: Added explicit empty comment in constructor.Adding the "// empty" comment makes it clear that the constructor intentionally has no implementation rather than accidentally being left empty.
src/Engine/UI/Button.cpp (4)
30-31: Initialize mText pointer to nullptr in constructor.The Button constructor now initializes the mText pointer to nullptr as it has been changed from a String to a Text* pointer, following best practices for pointer initialization.
33-35: Added conditional label initialization.The constructor now creates a Text widget for the label if a non-empty label string is provided. This is a good pattern that avoids creating unnecessary Text objects for buttons without labels.
38-40: Added proper cleanup in destructor.The destructor now properly deletes the mText object to prevent memory leaks. This is essential since the Button class now owns a dynamically allocated Text object.
46-56:Details
❓ Verification inconclusive
Text rendering logic removed from Button.
The Button's onRender method no longer handles text rendering directly. It only renders the button background. Text rendering responsibility has been delegated to the Text widget. However, there's no explicit call to render the text widget.
Verify that the Text widget is correctly rendered through the widget hierarchy. The parent widget (Button) should ensure its child widgets (Text) are rendered. Run this script to check:
🏁 Script executed:
#!/bin/bash # Check if Text widgets are rendered through parent-child relationship # Look for render calls in WidgetBase implementation rg -A 5 "void\s+WidgetBase::render" --include "*.cpp" # Look for child widget rendering in parent widgets rg "children|child" --include "UI/*.cpp" -A 3 -B 3Length of output: 992
I’ll re-run with corrected ripgrep flags and directly inspect the base widget’s render implementation to see if it walks its child list:
This will confirm whether WidgetBase::render automatically draws its children (including the Text widget).
🏁 Script executed:
#!/bin/bash set -e # Locate WidgetBase.cpp FILE=$(find . -type f -name 'WidgetBase.cpp' | head -n1) echo "---- Inspecting $FILE ----" sed -n '1,200p' "$FILE" echo echo "---- Searching for child-render loops ----" rg -n "mChildren|for.*render" "$FILE" -A5 -B2 || trueLength of output: 183
Verify child widget rendering in the base UI class
We couldn’t locate the implementation of WidgetBase::render to confirm it walks its child list and invokes each child’s onRender (including the Text widget). Without an explicit loop over children in the base class, the Text child won’t be drawn.
Please ensure that in your widget hierarchy:
- WidgetBase::render(canvasRenderer*) first calls its own onRender, then iterates over mChildren and calls render() on each child.
- Button adds its Text widget to mChildren so it will be picked up by the base-class loop.
If WidgetBase doesn’t already do this, either add the child‐render loop there or call the Text widget’s render method explicitly in Button::onRender.
src/Engine/UI/Button.h (3)
24-24: Added include for Text class.The Button class now depends on the Text class, so including "UI/Text.h" is necessary.
45-45: Changed label storage from String to Text pointer.The Button class now uses a Text widget pointer instead of directly storing the label string. This is a good design improvement that delegates text rendering to a specialized widget.
48-55: Updated setLabel to use Text widget.The setLabel method now creates a new Text widget if one doesn't exist, or updates the existing one. This implementation correctly handles the Text object lifecycle.
src/Engine/UI/Text.h (4)
23-29: New Text widget header with proper structure.The new Text.h file follows the project's code organization standards with appropriate header guards, includes, and namespace definitions.
35-41: Clean public interface for Text widget.The Text class has a clear public interface with constructor, destructor, and methods to get and set the label text.
42-45: Standard widget override methods.The Text class properly overrides the required methods from WidgetBase (onUpdate and onRender) to provide widget-specific behavior.
46-48: Simple private data member for the label.The Text widget uses a straightforward String member to store its label text. This is appropriate for this use case.
| inline const String &Button::getLabel() const { | ||
| return mLabel; | ||
| if (mText == nullptr) { | ||
| return String(); | ||
| } | ||
|
|
||
| return mText->getLabel(); | ||
| } |
There was a problem hiding this comment.
Potential issue with returning reference to temporary object.
The getLabel method returns a reference to a temporary String object when mText is nullptr, which is dangerous. The temporary object will be destroyed after the function returns, leaving the reference pointing to invalid memory.
Fix this by creating a static empty string or returning by value:
inline const String &Button::getLabel() const {
if (mText == nullptr) {
- return String();
+ static const String empty;
+ return empty;
}
return mText->getLabel();
}📝 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.
| inline const String &Button::getLabel() const { | |
| return mLabel; | |
| if (mText == nullptr) { | |
| return String(); | |
| } | |
| return mText->getLabel(); | |
| } | |
| inline const String &Button::getLabel() const { | |
| if (mText == nullptr) { | |
| static const String empty; | |
| return empty; | |
| } | |
| return mText->getLabel(); | |
| } |
🤖 Prompt for AI Agents
In src/Engine/UI/Button.h around lines 57 to 63, the getLabel method returns a
reference to a temporary String object when mText is nullptr, causing a dangling
reference. Fix this by either returning a static empty String object reference
or changing the return type to return by value instead of by reference to ensure
the returned object remains valid after the function returns.
|
| f32 xModel{ 0.f }, yModel{ 0.f }; | ||
| mapCoordinates(mResolution, x, y, xModel, yModel); | ||
|
|
||
| //MeshUtilities::generateTextBoxVerticesAndIndices(xModel, yModel, usedSize, text, positions, colors, tex0, indices); |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
The best way to fix this issue is to either remove the commented-out code entirely or reinstate it if it is still relevant and functional. In this case, the commented-out line appears to be an alternative implementation of the generateTextBoxVerticesAndIndices function. Since the active implementation is already present on the next line (line 512), the commented-out code is redundant and should be removed to improve code clarity.
| @@ -510,3 +510,2 @@ | ||
|
|
||
| //MeshUtilities::generateTextBoxVerticesAndIndices(xModel, yModel, usedSize, text, positions, colors, tex0, indices); | ||
| generateTextBoxVerticesAndIndices(mResolution, x, y, usedSize, mActiveLayer-2, text, positions, colors, tex0, indices); |
Summary by CodeRabbit
New Features
Improvements