Lift TextBox caret glyph-advance fast path into IFormsText#3547
Open
vchelaru wants to merge 4 commits into
Open
Lift TextBox caret glyph-advance fast path into IFormsText#3547vchelaru wants to merge 4 commits into
vchelaru wants to merge 4 commits into
Conversation
TextBoxBase.GetIndex (caret hit-testing) had an #if XNALIKE fast path (BitmapFont XAdvance walk) and a slower #else fallback (repeated growing-substring MeasureString, O(n^2)). Since Forms controls moved into GumCommon (which defines no XNALIKE), every backend except FRB's XNA-family builds silently lost the fast path and took the O(n^2) fallback. Add IFormsText.GetCharacterAdvance(char), a default interface method that falls back to measuring a single-character string (O(1) per call, so O(n) total - already better than the old substring-growing fallback). The XNA-family concrete Text (RenderingLibrary/Graphics/Text.cs) overrides it with the allocation-free BitmapFont.XAdvance lookup. GetIndex now calls coreTextObject.GetCharacterAdvance(...) directly with no #if: virtual dispatch on the concrete Text picks the right implementation at runtime, so MonoGame/KNI/FNA/FRB get the allocation-free walk back and Raylib/Sokol get O(n) instead of O(n^2). Closes #3542 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nitialized Directly constructing a legacy V1/V2 default-visual class (e.g. new DefaultButtonRuntime() - the 'create a control through its visual type' pattern demonstrated in GumFormsSample's FrameworkElementExampleScreen) crashed with a NullReferenceException reading Styling.ActiveStyle.Colors.Primary whenever the app only ever called InitializeDefaults with V3/Newest (or never called the V1/V2 overload at all). V3 populates its own separate DefaultVisuals.V3.Styling.ActiveStyle and never touches the legacy DefaultVisuals.Styling one, so it stayed null - found while manually testing #3542 in the sample. Made Styling.ActiveStyle lazily self-initialize (loading the embedded UISpriteSheet) when read while unset, instead of relying solely on InitializeDefaults(V1/V2) to populate it. Also extended the constructor's null-spritesheet fallback (already present for XNALIKE) to RAYLIB and SOKOL so the lazy path works on every platform. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The sample demonstrated the 'create/subclass a control through its visual type' pattern using V1 legacy classes (DefaultButtonRuntime, DefaultScrollViewerRuntime), which triggered CS0618 obsolete warnings and is what silently crashed pressing '2' in the previous commit (only initialize the pattern people would copy). Swapped to the V3 equivalents: - FrameworkElementExampleScreen: DefaultButtonRuntime -> V3.ButtonVisual, and the custom ScrollViewer background template from DefaultScrollViewerRuntime / ColoredRectangleRuntime -> V3.ScrollViewerVisual / NineSliceRuntime (V3's Background child). - StyledButtonRuntime (dead sample code, never instantiated, but still triggered the same obsolete warning via its base type): now subclasses V3.ButtonVisual and sets BackgroundColor directly - V3 computes highlighted/pushed/etc. tints from that single color instead of requiring per-state variable edits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sual Button template Pressing '2' still crashed after the earlier Styling fix, this time in SliderVisual.SetValuesForState. Two distinct bugs, both surfaced by the same screen (FrameworkElementExampleScreen overrides DefaultFormsTemplates[typeof(Button)] with a custom visual for its own demo button, which leaks into every composite control built afterward that internally constructs a Button sub-part, like Slider's thumb): 1. Gum.Forms.DefaultVisuals.V3.Styling.ActiveStyle had the exact same never-initialized-if-only-a-different-version-ran bug as the legacy DefaultVisuals.Styling fixed in the previous commit - it's a separate static class, so that fix didn't cover it. Applied the identical lazy-self-init fix. 2. SliderVisual.ThumbInstance is nullable by design (the 'as ButtonVisual' cast fails when the registered Button template isn't a ButtonVisual - exactly what FrameworkElementExampleScreen's override does), but SetValuesForState dereferenced it unguarded. Added the null check. New regression test (SliderTests.UpdateState_ShouldNotThrow_WhenButtonTemplateIsNotButtonVisual) reproduces both bugs by registering a non-ButtonVisual Button template and constructing a V3 Slider; confirmed it failed on each bug in turn before its respective fix, green after both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3542.
Problem
TextBoxBase.GetIndex(caret hit-testing) had an#if XNALIKEfast path (BitmapFontXAdvancewalk, allocation-free O(n)) and an#elsefallback (repeated growing-substringMeasureString, O(n^2), allocating a substring per character). Since Forms controls moved into GumCommon (which defines noXNALIKE), every backend except FRB's XNA-family builds silently lost the fast path and took the O(n^2) fallback — including standalone MonoGame/KNI/FNA.Change
IFormsText.GetCharacterAdvance(char), a default interface method that falls back to measuring a single-character string (O(1) per call → O(n) total, already an improvement over the old growing-substring fallback).Text(RenderingLibrary/Graphics/Text.cs) overrides it with the allocation-freeBitmapFont.XAdvancelookup — the same computation the old#if XNALIKEblock did.TextBoxBase.GetIndexnow callscoreTextObject.GetCharacterAdvance(...)directly with no#if— virtual dispatch on the concreteTextinstance picks the right implementation at runtime. Result: MonoGame/KNI/FNA/FRB get the allocation-free walk back (restoring what was silently lost), and Raylib/Sokol move from O(n^2) to O(n).Verification
MonoGameGum.Tests: full suite green (1821/1821), including the existingFontScale_ShouldScaleClickHitTestXregression test (TextBox odd/broken behavior #2687) which now exercises the newGetCharacterAdvanceoverride.RaylibGum.Tests: full suite green (455/455), including a newGetCaretIndexAtPosition_MapsFurtherRightClicksToLaterIndicestest pinning the new default-interface fallback path (previously untested — Raylib's caret hit-test had no coverage at all). Needed addingRaylibGum.TeststoGumCommon'sInternalsVisibleTolist (mirroring the existingMonoGameGum.Testsentry) to reach theinternal GetCaretIndexAtPosition.SkiaGumbuild: clean (unaffected — Skia doesn't compileTextBoxBase.csyet).FlatRedBall.Forms.DesktopGlNet6.csproj), built from the primary checkout: baseline (main) clean, branch clean, 0 new errors/warnings.Manual test: built
Samples/GumFormsSample/MonoGameGumFormsSample(clean, no sample edits needed —FrameworkElementExampleScreenalready has TextBox instances) and opened the.slnfor a real click-to-caret sanity check, given this touches a genuinely user-facing input path (clicking in a TextBox) across runtimes, even though the unit tests directly cover the click→index computation end-to-end.Steps to verify manually in the opened solution:
MonoGameGumFormsSampleproject (F5).2to switch to the Framework Element Example screen (only works while no control is focused — click empty space first if needed).🤖 Generated with Claude Code
Additional fix (found during manual testing above)
Pressing
2in theMonoGameGumFormsSampleto reachFrameworkElementExampleScreencrashed with:Root cause: the sample directly constructs a legacy V1 default-visual class (
new DefaultButtonRuntime()— the documented "create a control through its visual type" pattern).DefaultButtonRuntime's constructor readsStyling.ActiveStyle.Colors.Primary, butStyling.ActiveStyle(the V1/V2-shared style) is only ever set insideFormsUtilities.InitializeDefaults's V1/V2 switch cases. The sample only callsInitializeDefaultswithDefaultVisualsVersion.Newest(V3), which populates a separateDefaultVisuals.V3.Styling.ActiveStyleand never touches the legacy one — so it stayed null for any V3-only app that also directly instantiates a legacy visual class.Fix:
Styling.ActiveStylenow lazily self-initializes (loading the embeddedUISpriteSheet) when read while unset, instead of depending entirely onInitializeDefaults(V1/V2)having run. Extended the constructor's null-spritesheet fallback (already present for XNALIKE) to RAYLIB and SOKOL too, so the lazy path works on every platform.Verification:
ButtonTests.DefaultButtonRuntimeConstructor_ShouldNotThrow_WhenActiveStyleNeverInitialized— confirmed it reproduced the crash before the fix, green after.MonoGameGum.Tests(1822/1822),MonoGameGum.Tests.V2(120/120),MonoGameGum.Tests.V3(59/59),RaylibGum.Tests(455/455) all green.SkiaGumbuild clean.SokolGumbuild fails on pre-existing, unrelated native-binding errors (sg_image/sgp_blend_modenot found — missing submodule, not touched by this change).FlatRedBall.Forms.DesktopGlNet6.csproj): baseline clean, branch clean, 0 new errors.2again in the already-open Visual Studio solution.Sample cleanup (follow-up to the crash fix above)
The crash above only surfaced because the sample demonstrated the "create/subclass a control through its visual type" pattern using obsolete V1 classes (
DefaultButtonRuntime,DefaultScrollViewerRuntime) while the rest of the sample runs V3. Migrated the sample off V1:FrameworkElementExampleScreen:DefaultButtonRuntime→V3.ButtonVisual; the custom ScrollViewer background template moved fromDefaultScrollViewerRuntime/ColoredRectangleRuntime→V3.ScrollViewerVisual/NineSliceRuntime(V3'sBackgroundchild type).StyledButtonRuntime(dead sample code — never actually instantiated, but still triggered aCS0618warning via its base type): now subclassesV3.ButtonVisualand setsBackgroundColordirectly, since V3 computes highlighted/pushed/etc. tints from that one color instead of requiring per-state variable edits.Verified:
MonoGameGumFormsSample.csprojand the fullMonoGameGumFormsSample.slnbuild clean (0 errors), and theCS0618obsolete-API warnings from these two files are gone on a full rebuild.Second crash fix (still on screen 2)
Pressing
2still crashed after the firstStyling.ActiveStylefix, this time inSliderVisual.SetValuesForState:Two distinct bugs, both surfaced by the same root cause:
FrameworkElementExampleScreen's constructor overridesFrameworkElement.DefaultFormsTemplates[typeof(Button)]with a custom visual for its own demo button — and that override leaks into every composite control built afterward that internally constructs aButtonsub-part, likeSlider's thumb.Gum.Forms.DefaultVisuals.V3.Styling.ActiveStylehad the exact same bug as the legacyStylingfixed earlier in this PR — it's a separate static class, so that first fix didn't cover it. Applied the identical lazy-self-init fix.SliderVisual.ThumbInstanceis nullable by design (theas ButtonVisualcast fails when the registeredButtontemplate isn't aButtonVisual— exactly what the screen's override does), butSetValuesForStatedereferenced it unguarded. Added the null check. Checked every other V3 visual that builds a nestedButton(ScrollBarVisual) — it never dereferences its equivalent nullable fields unsafely, so this was the only latent instance of the bug.Verification:
SliderTests.UpdateState_ShouldNotThrow_WhenButtonTemplateIsNotButtonVisualreproduces both bugs (registers a non-ButtonVisualButtontemplate, constructs a V3Slider) — confirmed it failed on each bug in turn before its respective fix, green after both.MonoGameGum.Tests(1823/1823),.V2(120/120),.V3(59/59),RaylibGum.Tests(455/455) all green.SkiaGumbuild clean.2.