[kcjonson]

Summary

Optimizes SDF text rendering by implementing a fixed-size LRU cache for generated glyph quads, eliminating expensive per-frame regeneration while maintaining the immediate-mode API design.

Problem

Text rendering was regenerating glyph quads every frame:

• 11,160 glyph generations/second for just 62 characters (3 text strings at 60 FPS)
• Each generation required position calculations, UV lookups, and vector allocations
• Scenes with hundreds of text elements would see significant CPU overhead

Solution

Implemented a bounded LRU cache in FontRenderer:

• Cache key: (text string, scale) with epsilon comparison for floats
• Cache size: 4096 entries (~6-8MB max) - sized for complex UIs with ~1000 text strings × 4 scales
• Eviction policy: LRU - evicts oldest entry when cache is full
• Performance: O(1) hash lookup + vector copy instead of per-character calculations

Changes

1. FontRenderer Cache (libs/ui/font/font_renderer.{h,cpp})

• Added FontRendererConfig namespace with tuning constants
• Implemented cache structures: CacheKey, CacheKeyHash, CacheEntry
• Modified GenerateGlyphQuads() to check cache first
• Added UpdateFrame() for LRU tracking, ClearGlyphQuadCache(), GetGlyphQuadCacheSize()

2. Frame Update Callback Mechanism (libs/renderer/primitives/primitives.{h,cpp})

• Added SetFrameUpdateCallback() API (similar to existing SetTextFlushCallback())
• Invokes callback in BeginFrame() to update FontRenderer's frame counter
• Avoids circular dependency between renderer and ui libraries

3. Integration (apps/ui-sandbox/main.cpp)

• Registered FontRenderer::UpdateFrame() callback after initialization
• Ensures LRU tracking updates once per frame

4. Debug Log Cleanup (libs/ui/font/text_batch_renderer.cpp)

• Removed noisy DEBUG logs that fired 360+ times/second
• Kept ERROR logs for OpenGL failures

Performance Impact

• Before: 11,160+ glyph regenerations/second
• After: O(1) cache lookups with occasional cache miss (first render only)
• Memory: Bounded to 4096 entries with automatic LRU eviction
• API: Immediate-mode API preserved - optimization is transparent

Testing

• Verified with sdf_minimal scene: clean logs, no "First glyph" spam
• Cache hit rate approaches 100% after first frame for static text
• Text renders correctly at multiple scales

Architecture Notes

The immediate-mode API design (AddText() called every frame) is intentional per /docs/technical/ui-framework/primitive-rendering-api.md:

• API: Immediate mode (simple, no memory management)
• Implementation: Retained mode (caching, batching, optimization)

This cache completes the retained-mode implementation while maintaining the clean immediate-mode API.

[Copilot]

Pull request overview

This PR optimizes SDF text rendering by implementing a fixed-size LRU cache for generated glyph quads, reducing expensive per-frame regeneration from 11,160+ operations/second to O(1) cache lookups after the first frame. The implementation maintains the immediate-mode API while adding retained-mode optimization through caching.

Key Changes:

• Implemented LRU cache in FontRenderer with 4096-entry limit and (text, scale) cache keys
• Added frame update callback mechanism to track LRU access patterns via Primitives::BeginFrame()
• Removed noisy debug logs from TextBatchRenderer that fired 360+ times/second

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
libs/ui/font/font_renderer.h	Added cache structures (CacheKey, CacheKeyHash, CacheEntry), configuration constants, and cache management API
libs/ui/font/font_renderer.cpp	Implemented cache lookup/eviction in GenerateGlyphQuads(), cache management methods, and refactored MeasureText() for SDF support
libs/renderer/primitives/primitives.h	Added SetFrameUpdateCallback() API for LRU frame tracking
libs/renderer/primitives/primitives.cpp	Implemented frame update callback invocation in BeginFrame() and stub DrawText() function
apps/ui-sandbox/main.cpp	Registered FontRenderer::UpdateFrame() callback for global font renderer
apps/ui-sandbox/scenes/text_shapes_scene.cpp	Registered text flush callback for scene-specific renderer
apps/ui-sandbox/scenes/button_scene.cpp	Registered text flush callback for scene-specific renderer
libs/ui/font/text_batch_renderer.cpp	Removed verbose DEBUG logs from SetProjectionMatrix(), AddText(), and Flush()
libs/ui/shapes/shapes.cpp	Added alignment calculation debug log and dependency clarification comment
libs/ui/components/button/button.cpp	Refactored to use Center/Middle text alignment instead of manual positioning
docs/technical/sdf-text-rendering-debug-log.md	Removed development debug log (no longer needed)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The hash function uses std::hash<float>{} which can produce identical hashes for floating-point values that are considered equal in the epsilon comparison (line 199). This creates a potential for hash collisions where keys that are considered equal (within 0.001 epsilon) may have different hash values.

Consider quantizing the scale value in the hash function to match the epsilon comparison:

struct CacheKeyHash {
    size_t operator()(const CacheKey& key) const {
        size_t h1 = std::hash<std::string>{}(key.text);
        // Quantize scale to match epsilon comparison (0.001)
        int quantized = static_cast<int>(key.scale * 1000.0f);
        size_t h2 = std::hash<int>{}(quantized);
        return h1 ^ (h2 << 1);
    }
};

This ensures that scales within the epsilon range hash to the same bucket.

Suggested change

	size_t h2 = std::hash<float>{}(key.scale);
	// Quantize scale to match epsilon comparison (0.001)
	int quantized = static_cast<int>(key.scale * 1000.0f);
	size_t h2 = std::hash<int>{}(quantized);

[Copilot]

The LRU eviction algorithm has O(n) time complexity as it performs a linear search through all cache entries to find the oldest one. With a cache size of 4096 entries, this could cause performance spikes when the cache is full.

Consider using a more efficient LRU data structure (e.g., a doubly-linked list with hash map) or at minimum, track the oldest entry separately to avoid the linear search:

// In private members:
mutable CacheKey m_oldestCacheKey;

// During eviction:
if (!m_oldestCacheKey.text.empty()) {
    m_glyphQuadCache.erase(m_oldestCacheKey);
}
// Update m_oldestCacheKey when adding new entries

[Copilot]

The cache members are declared as mutable to allow modification from the const method GenerateGlyphQuads(). However, there's no thread safety mechanism (mutex, atomic operations) protecting concurrent access to m_glyphQuadCache and m_currentFrame.

If GenerateGlyphQuads() can be called from multiple threads (e.g., during parallel UI updates), this could lead to race conditions:

• Multiple threads reading/writing m_glyphQuadCache simultaneously
• Non-atomic reads/writes to m_currentFrame
• Cache corruption during eviction

Consider adding thread safety if concurrent access is possible, or document that FontRenderer is not thread-safe.

[Copilot]

The UpdateFrame() method increments the frame counter without overflow protection. With a uint64_t counter incrementing once per frame at 60 FPS, it would take ~9.7 billion years to overflow, which is acceptable. However, the method lacks documentation about thread safety.

Since m_currentFrame is mutable and accessed from GenerateGlyphQuads() (which can be called from rendering threads), consider adding documentation about thread safety expectations or implementing atomic operations if concurrent access is possible.

[Copilot]

This debug log statement uses LOG_INFO which will output for every text render call (potentially 60+ times per second for dynamic text). This contradicts the PR's goal of removing "noisy DEBUG logs that fired 360+ times/second" from text_batch_renderer.cpp.

Consider either:

1. Changing to LOG_DEBUG for consistency, or
2. Removing this log entirely if alignment calculations are working correctly

The comment says "DEBUG:" but uses INFO level logging.

Suggested change

	// DEBUG: Log alignment calculations
	LOG_INFO(UI, "Text '%s': pos(%.1f,%.1f) size(%.1f,%.1f) ascent=%.1f -> aligned(%.1f,%.1f)",
	text.c_str(), position.x, position.y, textSize.x, textSize.y, ascent, alignedPos.x, alignedPos.y);

[Copilot]

The DrawText function is defined but deliberately left unimplemented (just suppresses the unused parameter warning). This creates a misleading API where a function exists but does nothing.

Consider one of these approaches:

1. Remove the function entirely if it's not meant to be used
2. Make it call TextBatchRenderer::AddText() if the circular dependency can be resolved
3. Add [[deprecated]] or = delete to make it clear the function shouldn't be called
4. At minimum, add a runtime warning/assertion to alert callers that this function is non-functional

[Copilot]

The frame update callback registered in main.cpp only updates the global g_fontRenderer instance. However, the button_scene and text_shapes_scene each create their own FontRenderer instances (see button_scene.cpp:34 and text_shapes_scene.cpp:28) which won't have their frame counters updated.

This means the LRU cache in scene-specific FontRenderer instances will never evict entries because m_currentFrame will always remain 0, and all cache entries will have lastAccessFrame = 0.

Consider either:

1. Registering the frame update callback in each scene's OnEnter() to update that scene's FontRenderer
2. Using a singleton FontRenderer shared across all scenes
3. Making the frame update callback iterate through all active FontRenderer instances

[Copilot]

The kEnableGlyphQuadCache constant is declared as constexpr bool but described as a "runtime toggle". Since it's constexpr, it cannot be changed at runtime - the compiler will optimize away the disabled branch entirely.

If runtime toggling is needed (e.g., for A/B performance testing), consider making it a non-const static variable or a configuration parameter. Otherwise, update the comment to clarify it's a compile-time toggle:

// Compile-time toggle for cache (rebuild required to disable)
constexpr bool kEnableGlyphQuadCache = true;

Suggested change

	// Runtime toggle for cache (disable for testing/comparison)
	constexpr bool kEnableGlyphQuadCache = true;
	// Runtime toggle for cache (can be changed at runtime for testing/comparison)
	static bool kEnableGlyphQuadCache;