Feature/editor gui - #22
Merged
Merged
Conversation
…sizing Pulls together the four architecture points from the GUI review: - Hit-testing & input routing: Widget gains HitTest/GetChildCount/GetChildAt (ForEachChild is now built on these, non-virtual) and handlers return SInputReply instead of void, so Manager can bubble a press/release/move along the actual top-to-bottom hit path with mouse capture, instead of broadcasting to every widget under the cursor. EVisibility grows HitTestInvisible/SelfHitTestInvisible/Collapsed alongside Visible/Hidden. - Measure pass: ComputeDesiredSize now takes the available space and is driven through a cached, non-virtual Widget::Measure, so containers no longer re-measure clean subtrees on every arrange. TextBlock/FontManager gain wrap-aware measurement (ETextOverflow, MeasureWrapped). - Z-order runs: RenderBatch::Sort groups commands into per-type SBatchRun ranges after sorting by ZOrder, and Renderer walks them in that order (SDrawItem/AppendRange/firstInstance) instead of one pass per command type, so quads and text interleave correctly by z instead of text always drawing on top. - Typed slot sizing: LayoutSlot's m_FillRatio becomes SSizeParam (Auto/Fill/Fixed) with corrected space distribution (fixed space first, remaining space split by fill ratio), EHorizontalAlignment/ EVerticalAlignment gain Fill for the cross axis, and Panel/TPanel<TSlot> replace the untyped Ref<Slot> vector + static_pointer_cast in every container with a typed slot list (TPanel<LayoutSlot>/TPanel<CanvasSlot>, explicitly instantiated and exported so client code can derive from them across the DLL boundary). Test fixtures (WidgetTestUtils, DrawCacheTest, ForEachChildTest, InvalidationTest, DirtyTrackingTest, WidgetLifetimeTest) updated to match: new ComputeDesiredSize signature, TPanel<LayoutSlot> instead of hand-rolled Panel subclasses, GetSlotCount() instead of GetSlots(), and slot-level SetFillSize() instead of the removed per-panel SetStretching().
- Clip stack: Widget::ClipsChildren (default false) and a clipRect param threaded through CollectDrawCommands, intersected at RenderBatch::Append time rather than baked into BuildDrawCommands - so scrolling a ScrollBox doesn't have to invalidate every descendant's cached draw commands, only reassemble the frame. SRect::Intersect added to Definitions.h as the supporting primitive. - ScrollBox (new): a ContentWidget that clips its content, measures it unconstrained on the scrolling axis/axes and capped to the viewport on the other, clamps the scroll offset to content-minus-viewport, and draws an optional track+thumb scrollbar. Routes MouseScrolledEvent through the same SInputReply convention as the rest of the input handlers. - Popup layers: Manager gains a layer stack (SLayer) above the always- present root layer - PushPopup/PopPopup/ClearPopups, hit-tested and dismissed independently (click outside closes it), arranged and rendered through the same per-subtree z-banding as everything else so a popup always ends up on top without a magic z offset. A layer-stack version counter covers the case a freshly opened popup starts dirty by construction without ever calling MarkLayoutDirty.
The if-init-statement's condition was `++i`, incrementing the loop counter a second time (the for-loop already does it) and using the post-increment's truthiness as the guard instead of checking whether GetChildAt actually returned a widget. Net effect: every other child was skipped, and a null child would never have been filtered anyway. Drop the second increment and check the child itself.
- ClipStackTest: clip only propagates from a ClipsChildren() container, nested clips intersect rather than replace, an inherited clip narrows an existing ad-hoc scissor instead of overwriting it, and a direct regression test that ScrollBox actually scissors oversized content end to end via AssembleFrame (ClipsChildren() on it was silently false at one point, with nothing catching it). - ScrollBoxTest: desired size never exceeds the configured viewport but shrinks to smaller content, LayoutChildren offsets content by the scroll offset, SetScrollOffset clamps both directions, and HandleMouseScrolled moves the offset within bounds (Handled) or defers to an ancestor at the edge (Unhandled). - PopupLayerTest: push/pop/clear track the popup count correctly (including no-op pop on an empty stack), a freshly pushed popup is arranged immediately against the last known extent, its draw commands sort strictly above the root layer's after AssembleFrame - the core guarantee of the popup stack - and a clip on one layer doesn't leak into another. - WidgetLifetimeTest: RemoveChild is protected on Widget/Panel with no outside production caller; the one test exercising it now drives it through a local TPanel<LayoutSlot> test double that promotes it via `using`, the same idiom ForEachChildTest already uses, instead of relying on VerticalBox exposing it publicly.
…ren does VerticalBox/HorizontalBox::ComputeDesiredSize summed each child's raw Measure() result along the main axis regardless of its slot's sizing rule, while LayoutChildren correctly used the slot's configured pixel size for Fixed children and gave Fill children no intrinsic contribution. A Fixed child whose content measured smaller than its configured size made the container under-report its own desired size. Found by live-debugging a real symptom: a ScrollBox wrapping a VerticalBox of fixed-height rows measured the list as shorter than its actual laid-out height, concluded nothing overflowed the viewport, and silently refused to scroll or show a scrollbar - even though the rows were visibly clipped. Fix: ComputeDesiredSize now switches on GetSizeRule() the same way LayoutChildren already does. Regression tests confirm the old code was wrong (3 of 4 new tests fail against it) and pass against the fix.
… space - TPanel<TSlot>::AddChild is now virtual, so a concrete container (Canvas) can override it instead of just hiding the base via name lookup. - Canvas/ScrollBox's configured-size setter is now SetSize on both (was SetDesiredSize), matching the rest of the widget setter naming; backing fields renamed to m_Size for the same reason. Canvas's default changed from 800x600 to 100x100. - Canvas::ComputeDesiredSize now clamps to glm::min(m_Size, availableSize) instead of returning m_Size unconditionally - Canvas was the one container that never checked whether the parent actually had room, unlike ScrollBox which already followed this rule. Test fixtures and the Editor's example dropdown menu updated for the renamed setter; CanvasTest gains coverage for the default size, the setter (including the same-value no-invalidate guard), and the new available-space clamp - confirmed the clamp test fails without the fix.
…etry Outline previously expanded past the widget's own m_Geometry in the vertex shader and drew in the [0, thickness] band outside the shape, so two Fill-sized siblings with zero gap between them would have their borders bleed into each other. Flip the outline band to [-thickness, 0] (inside the shape) and draw it after the fill/inset-shadow so it sits on top like a real border, and stop expanding the quad for it in the vertex shader - only drop shadow still needs that. Widget::Measure now budgets outline thickness into the desired size it reports, so a widget that wants a 10x10 content box with a 1px outline actually occupies 12x12 when auto-sized, instead of the outline eating into its own content.
Content was always measured and arranged at the ScrollBox's full width/height, never leaving room for the scrollbar it draws over the edge - so content with a background reaching that edge (e.g. a section header) rendered on top of the scrollbar instead of under it, and z-order meant it always would, since scrolled content is a child and children draw above their parent's own commands. Add ScrollBox::CrossAxisSpace, subtracting the scrollbar's thickness from whichever axis it actually occupies whenever m_ShowScrollbar is set, and use it in both ContentMeasureConstraint (what content thinks it has) and LayoutChildren (what it's actually arranged into) - the second one was missing before, so even content that measured itself narrower was stretched back out to the full width at arrange time.
m_DPIScale was stored but never read - QuadRenderPass and TextRenderPass both scale cmd.Geometry.Position/Size by it before building vertex data, but DebugRenderPass used the raw logical-point geometry, so a debug rect (e.g. the focus ring) landed at half its intended screen position on a 2x display.
Widget gains an IsFocusable/SetFocusable flag, independent of mouse- click focus (Manager::ProcessMousePress already focuses whatever widget consumes a mouse-down regardless of this flag) - it only controls membership in the keyboard-navigable Tab order. TextField opts itself in, since it already reacts visibly to focus. Manager::HandleKeyPressed intercepts Tab/Shift+Tab and Escape before bubbling to the focused widget, not after: TextField::HandleKeyPressed unconditionally returns Handled() for every key code, so a focused field would swallow Tab silently forever if this checked the bubble result first. FocusNext/FocusPrevious walk a lazily-rebuilt, depth- first focus order cached behind the same dirty-epoch + layer-stack- version key NeedsRebuild already uses, scoped to the topmost popup layer so Tab never escapes an open popup into whatever is underneath it - and deliberately don't clear focus when that scoped order is empty, so a popup with no focusable content of its own can't silently steal focus from the layer below. The focus ring itself reuses AddDebugRect's existing DEBUG_Z_ORDER sentinel (always-on-top, immune to any ScrollBox/popup clip in effect) rather than inventing a second z-layering mechanism alongside the popup stack's. (Widget::SetFocusable's definition landed in the prior outline-fix commit by accident - a `git commit <pathspec>` pathspec commits the working tree content of that path, not what was staged via `git apply --cached`, so it pulled in more than intended. No functional impact, just a messier split than planned.) Promotes SetFocusedWidget/ProcessMousePress/HandleKeyPressed from private to protected on Manager so tests can drive them directly, matching the existing AssembleFrame/NeedsRebuild/MarkRebuilt pattern in ManagerTestUtils.h, and adds FocusTest.cpp covering Tab ordering, wrap-around both directions, Escape, popup scoping (both directions - staying inside an open popup, and not leaking out when it has nothing focusable), and the pre-existing click-outside-clears-focus behavior as a regression guard.
Focus visualization is a per-widget concern, not something the Manager should impose uniformly: a widget that wants to show its own focus state already has everything it needs (IsFocused(), already invalidated on change via HandleFocus/HandleLostFocus calling MarkRenderDirty) to render it inside its own BuildDrawCommands, the same way Button already reacts to m_Hovered - color swap, outline, background texture, whatever fits the widget. No opt-in/opt-out flag on Widget either: a widget that doesn't render anything for its own focused state simply has no focus visual, rather than the base class carrying a default every widget has to actively suppress.
Splits the background texture into m_NormalBackground/ m_FocusedBackground and adds a separate m_FocusedOutline, then BuildDrawCommands picks between them based on m_Focused - the same per-widget pattern Button already uses for m_Hovered, and the first real consumer of the "no default focus visual" principle from the prior commit: TextField opts in by drawing something specific for its own focused state, texture or outline, instead of relying on any Manager-level fallback.
…sabled) Adds StyleSet on the Widget base: four style layers (Normal < Hovered < Pressed < Disabled) composed field-by-field, so a later active layer only overrides what it explicitly declares - a Pressed override that changes just the color still shows whatever Hovered or Normal set for texture, borders, corner radius, etc. BackgroundTexture uses optional<Ref<Texture2D>> specifically so a layer can distinguish "I didn't touch this" from "clear the inherited texture" (a non-null but empty Ref). GetInteractionState()/GetResolvedStyle() live on Widget as protected, built from the widget's own IsHovered/IsPressed/IsEnabled - available to any concrete widget's BuildDrawCommands, not just Button. The mutators (SetStyle/ClearStyle/SetBackgroundColor/SetForegroundColor/ SetBackgroundTexture/ClearBackgroundTexture) are public on Widget, following the same precedent as SetOutline/SetInsetShadow/ SetDropShadow already being public and simply inert on widgets that never draw with them. Widget also gains IsEnabled/SetEnabled: disabling marks render-dirty, cancels an in-progress press, and blocks HandleMouseDown/HandleClick - without touching EVisibility or layout. Outline/InsetShadow/DropShadow stay single-value widget properties (via the pre-existing SetOutline/SetInsetShadow/SetDropShadow) rather than becoming per-layer overrides: GetResolvedStyle() overlays them from the widget's own getters after resolving, so they don't vary by interaction state. Avoids a second source of truth for the same value, at the cost of those three fields in SStyleOverride never actually taking effect if set through SetStyle directly. Migrates Button off its old m_NormalColor/m_HoverColor/ m_NormalBackground fields onto this API, keeping only its own thin wrappers (SetCornerRadius, SetBackgroundBorders, SetTextColor) on top of the inherited one; no compatibility shims kept, since the only three call sites (Application.cpp) get updated directly in the same change. Button::HandleMouseDown also gains the IsEnabled() check it was missing before. Adds StyleTest.cpp: StyleSet::Resolve in isolation (Normal-only, partial Hovered override, Pressed-over-Hovered, Disabled-over-both, Disabled falling back when it declares nothing, explicit texture clear, an inactive layer never leaking into the result) plus Widget's public surface (style/hover/press/enabled changes mark render-dirty; disabling blocks input that would otherwise be handled).
…errides GetResolvedStyle() previously overlaid Outline/InsetShadow/DropShadow from Widget's own single-value getters after resolving m_Styles, so those three SStyleOverride fields never actually took effect - setting them through SetStyle/SetOutline/SetInsetShadow/SetDropShadow was a silent no-op. Removes the single-value m_Outline/m_InsetShadow/ m_DropShadow fields and their old setters/getters entirely; every setter (SetOutline, SetOutlineColor, SetOutlineThickness, SetInsetShadow and its per-component variants, SetDropShadow and its per-component variants) now takes an EStyleLayer, reads the layer's current override, patches just the one field it owns, and writes it back - so a widget can genuinely give Hovered a different outline than Normal. Also adds SetCornerRadius/SetBackgroundBorders directly on Widget for the same reason Button no longer needs its own copies of them. Migrates the only call sites (Application.cpp, Button's own SetTextColor wrapper) to the new per-layer signatures.
Both had their own single-value background/corner-radius/text-color fields, parallel to (and now stale next to) the base Widget's StyleSet - Panel::BuildDrawCommands and TextField::BuildDrawCommands were still reading m_Outline/m_InsetShadow/m_DropShadow directly, which the previous commit emptied out entirely (those became per- layer-only), so any Outline set through the new API on a Canvas, Overlay or TextField was silently never drawn. Panel (base of Canvas/Overlay/HorizontalBox/VerticalBox) drops its own m_Background/m_CornerRadius and their accessors; BuildDrawCommands now resolves through GetResolvedStyle() like Button already does. TextField drops m_TextColor/m_BackgroundColor/m_CornerRadius/ m_BackgroundBorders/m_NormalBackground and their setters, resolving the same way for its Normal/Hovered/Pressed/Disabled state; keeps m_FocusedBackground/m_FocusedOutline as its own fields, since Focused isn't one of StyleSet's four layers (see EStyleLayer) - the resolved style still supplies background color/corner radius/borders/shadows even while focused, only the texture/outline branch differs. Keeps a thin SetTextColor(layer, color) wrapper renaming SetForegroundColor, matching Button's own precedent. Fixes the three existing tests that called the old single-argument Widget::SetBackground.
Precedence becomes Normal < Hovered < Pressed < Focused < Disabled -
Disabled still wins even over a widget that stays focused after being
disabled (nothing clears focus just because IsEnabled() went false).
Widget::GetInteractionState() now includes IsFocused().
TextField drops m_FocusedBackground/m_FocusedOutline and their setters
entirely: the constructor sets a default outline on the Focused layer
via SetStyle the same way it already does for Normal, and
BuildDrawCommands loses its whole if (m_Focused) {...} else {...}
branch - GetResolvedStyle() already picks the right layer on its own.
Background color, corner radius, borders and shadows stay whatever
Normal/Hovered/Pressed resolved to while focused, since Focused's
default only declares Outline.
Widget::SetEnabled(false) has no way to clear Manager::m_FocusedWidget - Widget has no back-reference to the Manager - so a widget disabled while focused stays focused, just Disabled-styled. That left two real gaps in its "stops accepting input events" contract: - Manager::HandleKeyPressed/HandleKeyTyped kept bubbling from m_FocusedWidget regardless of IsEnabled(), so a disabled TextField that was focused before being disabled would keep taking keystrokes. - Manager::CollectFocusOrder never checked IsEnabled(), so Tab could land inside a disabled widget even though a mouse click already can't (Widget::HandleMouseDown's own IsEnabled() check). Both now guard on IsEnabled(). FocusNext/FocusPrevious already handle the resulting case correctly for free: once a focused widget drops out of the (now enabled-only) order, they treat it as "not in the order" and jump to the first entry instead of stepping from a stale position. Verified both new FocusTest cases actually catch the bug: reverted the Manager.cpp guards, confirmed both failed, restored the fix.
…alette Give Button and TextField a real default visual identity across every StyleSet layer (Normal/Hovered/Focused/Disabled) instead of placeholder dev colors, matching chakra-ui.com's "subtle" zinc button. Disabled uses SColor alpha directly since the engine has no render-blended opacity. Panel is intentionally left untouched - it backs Canvas/Overlay/H-V Box as transparent layout scaffolding across the Editor.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis change adds an Editor application, LunaSVG-backed icons, typed GUI styling, constrained layout, layered input routing, animation, scrolling, clipping, staged rendering, UTF-8 text wrapping, and related tests. ChangesEditor and GUI platform
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This GUI/editor change can misroute input, leave dismissed controls interactive, render stale or corrupted visuals, and trigger undefined behavior or memory-safety failures in normal and release builds. The current implementation is not merge-ready until the unresolved runtime and interaction defects are fixed or explicitly accepted by the owners. Sequence Diagram(s)sequenceDiagram
participant Editor
participant EditorUI
participant GUIManager
participant WidgetTree
participant Renderer
Editor->>EditorUI: Build menus, tabs, panels, and viewport
EditorUI->>GUIManager: SetRoot and PushPopup
GUIManager->>WidgetTree: Arrange, update, and hit-test layers
WidgetTree->>GUIManager: Return input replies and draw commands
GUIManager->>Renderer: Submit typed command ranges
Renderer->>Renderer: Bind passes and render instance ranges
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 495efa6df5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fix/fixes gui
Fix/fixes gui
There was a problem hiding this comment.
🟡 Changes recommended
It currently contains compile failures and unresolved layout, scrolling, rendering, and input-state defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Elixir/Source/Engine/Core/Application.cpp:66
- These additional
EStyleLayerreferences have the same compile failure because the enum belongs toElixir::GUI. Qualify them before this target can build.
Elixir/Source/Engine/Core/Application.cpp:83 - This final group also uses the GUI enum unqualified, so
Application.cppstill will not compile after fixing the earlier calls.
Elixir/Source/Engine/GUI/ScrollBox.cpp:256 - The horizontal thumb has the same short-track failure: an oversized minimum makes the thumb extend outside the ScrollBox and gives its position a negative travel range. Clamp it to
track.Size.x.
- Files reviewed: 99/112 changed files
- Comments generated: 11
- Review effort level: Balanced
Fix/fixes gui
There was a problem hiding this comment.
🟡 Changes recommended
Build integration, constrained layout, scrollbar sizing, keyboard accessibility, and GPU resource-lifetime issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (7) — in code that hasn't changed since the last review.
Elixir/Source/Engine/GUI/Button.cpp:15
- Buttons remain non-focusable because
Widgetdefaultsm_Focusableto false and this constructor never enables it. Consequently Tab navigation skips every Button despite the new focused style, so keyboard-only users cannot reach or activate these controls. Mark buttons focusable and handle Enter/Space as click activation.
Elixir/Source/Engine/GUI/Checkbox.cpp:26 - Checkboxes remain non-focusable because
Widgetdefaultsm_Focusableto false and this constructor never enables it. Tab navigation therefore skips them, and there is no Enter/Space activation path. Mark checkboxes focusable and map the standard keyboard activation keys toHandleClick.
Elixir/Source/Engine/Icon/Icon.cpp:51 - Every distinct raster size is retained for the lifetime of the Icon. A resizable/fill icon can generate a new texture on each window-size step; those textures are also retained by the renderer's TextureSet, eventually consuming GPU memory and the finite bindless descriptor pool. Bound this cache (for example with an LRU or by retaining only currently useful DPI/size variants).
Elixir/Source/Engine/GUI/Renderer/QuadRenderPass.cpp:161 - This keeps every superseded GPU buffer alive until the render pass is destroyed. Each capacity increase permanently retains the previous allocation, so peak GUI growth leaves roughly another full buffer's worth of GPU memory resident. Retire buffers only until the relevant in-flight frames/fence have completed, then release them.
Elixir/Source/Engine/GUI/Renderer/TextRenderPass.cpp:140 - This keeps every superseded text buffer alive until the pass is destroyed. Repeated capacity growth permanently retains all prior GPU allocations, increasing resident memory after transient text spikes. Retire each old buffer only through the in-flight frame/fence window and release it afterward.
Elixir/Source/Engine/GUI/Renderer/DebugRenderPass.cpp:129 - This retains every old debug vertex buffer for the lifetime of the pass. Capacity growth therefore leaves all superseded GPU allocations resident after a temporary debug-command spike. Use the renderer's deferred-destruction/fence mechanism so retired buffers are released once no in-flight command buffer can reference them.
Elixir/Source/Engine/GUI/Definitions.h:28 - Grammar: “when the rect do not overlap” should be “when the rects do not overlap.”
- Files reviewed: 100/113 changed files
- Comments generated: 4
- Review effort level: Balanced
Owner
Author
|
@copilot Verifique se os pontos do último code review foram resolvidos |
Contributor
Todos os quatro pontos do último code review (commit
|
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.
Summary by CodeRabbit
New Features
Bug Fixes