Repository navigation
feat: text alignment improvements and UI component migration - #29
Conversation
Added two demonstration grids to the text shapes scene: - Bounding box alignment grid (3x3) showing text within boxes - Point-based alignment grid (3x3) with origin markers Fixed point-mode vertical alignment logic in shapes.cpp: - Top: no offset (position is top-left) - Middle: -ascent/2 offset (centers vertically) - Bottom: no offset (position is baseline)
- Fix font size calculation to use BASE_FONT_SIZE (16px) instead of glyph size (32px) - Unify coordinate systems: text and shapes now use same logical pixel space - Fix z-ordering: remove premature text flush in LayerManager - Move font renderer initialization to global scope in main.cpp - Update button scene to use global font renderer
Added hAlign and vAlign properties to the bounding box alignment demonstration grid. This completes the text alignment system, showing both alignment modes working correctly: - Bounding box mode: Text aligns within box boundaries - Point mode: Text aligns relative to origin point
Replaced direct FontRenderer API calls in the navigation menu with proper UI components: - Menu items now use Button components for interactive scene switching - "Scenes" header uses Text component for consistent rendering - Removed manual mouse callbacks in favor of Button's built-in input handling - All text in ui-sandbox now uses UI components (Text or Button) except for the direct rendering demo in font_test scene This ensures consistent text rendering and alignment across the application using the batched text rendering system.
The layer_scene was incorrectly initializing its own FontRenderer instance and calling SetFontRenderer(), which overwrote the global FontRenderer. When the scene exited, this caused the navigation menu text to stop rendering because the FontRenderer pointer became invalid. FontRenderer and TextBatchRenderer are now initialized once in main.cpp and shared across all scenes. Scenes should not initialize their own instances. Changes: - Removed FontRenderer initialization from OnEnter() - Removed FontRenderer cleanup from OnExit() - Removed m_fontRenderer member variable - Removed font/font_renderer.h include
There was a problem hiding this comment.
Pull request overview
This PR implements text alignment improvements with dual-mode support (point-based and bounding box), fixes text rendering coordinate system issues, migrates the navigation menu to use Button/Text components, and consolidates FontRenderer initialization to prevent menu rendering bugs during scene transitions.
- Added bounding box and point-based text alignment modes with comprehensive visual demonstrations
- Migrated navigation menu from direct FontRenderer API calls to Button/Text components with proper hover/click handling
- Consolidated FontRenderer and TextBatchRenderer to global initialization in main.cpp, removing redundant per-scene setup
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/ui/shapes/shapes.h | Added optional width/height fields to Text struct for bounding box mode |
| libs/ui/shapes/shapes.cpp | Implemented dual-mode text alignment (point vs bounding box) with separate logic paths |
| libs/ui/layer/layer_manager.cpp | Disabled TextBatchRenderer::Flush() calls to address z-order issues |
| libs/ui/font/font_renderer.cpp | Updated font size calculations to use BASE_FONT_SIZE constant for consistent rendering |
| libs/engine/application/application.cpp | Added screen clearing before rendering to ensure clean frame start |
| apps/ui-sandbox/scenes/text_shapes_scene.cpp | Added 3x3 alignment demonstration grids and removed per-scene FontRenderer initialization |
| apps/ui-sandbox/scenes/layer_scene.cpp | Removed per-scene FontRenderer initialization |
| apps/ui-sandbox/scenes/button_scene.cpp | Removed per-scene FontRenderer and TextBatchRenderer initialization |
| apps/ui-sandbox/main.cpp | Migrated menu to Button components, added TextBatchRenderer, consolidated FontRenderer initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ui::TextBatchRenderer* textBatchRenderer = Renderer::Primitives::GetTextBatchRenderer(); | ||
| if (textBatchRenderer != nullptr) { | ||
| textBatchRenderer->Flush(); | ||
| // textBatchRenderer->Flush(); // DISABLED: causes z-order issues |
There was a problem hiding this comment.
The commented-out Flush() call should be removed entirely rather than left as a comment. If there's a z-order issue that needs to be tracked, consider adding a TODO comment with a ticket reference or detailed explanation instead of leaving dead code.
| ui::TextBatchRenderer* textBatchRenderer = Renderer::Primitives::GetTextBatchRenderer(); | ||
| if (textBatchRenderer != nullptr) { | ||
| textBatchRenderer->Flush(); | ||
| // textBatchRenderer->Flush(); // DISABLED: causes z-order issues |
There was a problem hiding this comment.
The commented-out Flush() call should be removed entirely rather than left as a comment. If there's a z-order issue that needs to be tracked, consider adding a TODO comment with a ticket reference or detailed explanation instead of leaving dead code.
| break; | ||
| case Foundation::VerticalAlign::Top: | ||
| default: | ||
| // No offset - position is already the top-left |
There was a problem hiding this comment.
The comments for Bottom and Top alignment cases are contradictory. The position cannot be 'already at the bottom (baseline)' for Bottom alignment and simultaneously 'already the top-left' for Top alignment. These comments should clarify the actual coordinate system assumption. For example, if the position represents the baseline, the Top case comment is incorrect, or vice versa.
| // No offset - position is already the top-left | |
| // No offset - position is already at the baseline (bottom); if top alignment is desired, an offset by -ascent would be needed. |
| [sceneName, sceneIndex]() { | ||
| engine::SceneManager::Get().SwitchTo(sceneName); | ||
| g_menuState.selectedIndex = sceneIndex; | ||
| LOG_INFO(UI, "Switched to scene: {}", sceneName.c_str()); |
There was a problem hiding this comment.
LOG_INFO uses printf-style format strings (as seen in log.h line 67-68), not std::format-style '{}' placeholders. This should use '%s' instead of '{}' to match the logging macro's format string requirements and be consistent with other LOG_INFO calls in this file (e.g., lines 188, 407, 438, 440).
| LOG_INFO(UI, "Switched to scene: {}", sceneName.c_str()); | |
| LOG_INFO(UI, "Switched to scene: %s", sceneName.c_str()); |
- Remove commented-out textBatchRenderer->Flush() calls from layer_manager.cpp
and replace with explanatory comment about text flushing in overlay renderer
- Fix LOG_INFO format string to use %s instead of {} for printf-style formatting
- Clarify vertical alignment comments in shapes.cpp to explain coordinate system
and remove contradictory statements about position baseline
Addresses feedback from PR #29 review
Summary
Changes
Text Alignment Demonstrations
text_shapes_scene.cppwith comprehensive alignment examplesText Rendering Coordinate System Fix
+to-to match screen coordinate system (top-left origin)Navigation Menu Component Migration
FontRendererAPI calls toButtonandTextcomponentsmain.cppInitializeNavigationMenu()function to construct Button components for each sceneHandleNavigationMenuInput()to update button states each frameFontRenderer Architecture Fix
layer_scene.cppmain.cppTesting
Files Changed
apps/ui-sandbox/main.cpp- Menu converted to Button/Text componentsapps/ui-sandbox/scenes/text_shapes_scene.cpp- Added alignment demonstration gridsapps/ui-sandbox/scenes/layer_scene.cpp- Removed per-scene FontRendererlibs/ui/shapes/shapes.cpp- Fixed text coordinate system and scaling