Build the Pixely.Ui tree in the update phase instead of during rendering - #481
Merged
Conversation
stanoddly
force-pushed
the
ui-update-phase
branch
from
September 10, 2026 06:07
514f784 to
ce2ab87
Compare
stanoddly
enabled auto-merge (squash)
September 10, 2026 06:07
|
✅ Development package |
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.
UiRenderer.Renderbuilt the element tree. Two lines of it —_root.SetViewportSize(...)and_root.Update()— and both reach application code: pointer enter and leave as layout moves under a stationary pointer, focus lost when a focused element leaves the tree,FocusChangedthrough toITextInputService.Start/Stop(an SDL call), the viewport event, and every customMeasureContent,ArrangeContentandDrawable.Paintin the tree.RenderCoordinator.Executeruns every renderer for a window inOrdersequence over one shared render context and command buffer. So that application code ran between other renderers'Rendercalls, with a liveCommandBuffer. A renderer that reads domain data is entitled to assume it holds still for the frame, and this broke that for every other renderer in the window. What masked it isUseUi's defaultorder: 10_000, which happens to put the UI last — a default argument, not an invariant.Verified before fixing: three temporary tests against
UiRoot, making the exact callsRendermade, showed a stationary pointer receivingOnPointerLeavefrom a sibling's height changing,SetViewportSizenotifying synchronously, and hiding a focused element raisingFocusChanged(null).Pixely.Pencuilnever had this — its build isPencilSystem.Update, andPencuilRendereronly reads completed instruction buffers.The build moves to the update phase
UiUpdateSystem : IUpdatable, IOrderableis whatUseUinow registers to drive the root. It takesFunc<Vector2Int>andFunc<bool>rather than aWindow, becauseRenderSizeInPixelsandIsVisibleare non-virtual SDL calls and a system holding a window cannot be constructed in a test — the same objection that ruled out folding the build intoUiInputSystem, which is untouched.It skips hidden windows before reading the size, and skips a zero-or-negative viewport. Both are deliberate: today a hidden window never builds because the only caller of
Updateis a rendererRenderCoordinatoralready skipped, and moving the build out of render removes that protection.The renderer can no longer start a build
UiRendererholdsIUiPaintSourceinstead ofUiRoot— the completed instructions and batches, the viewport they were built for, the current viewport, and a build counter. NoUpdate, noSetViewportSize. This is what enforces the boundary: not a convention and not a test, but the renderer's dependency having nothing on it to call. Same-assembly code can still cast back toUiRooton purpose; the point is that no ordinary edit reaches a build by accident.All five members are forwarded explicitly from
UiRoot. Four of them are internal, an internal member cannot implicitly implement an interface one, and widening is unavailable becausePaintInstructionandPaintBatchare internal types.UiRoot.BuildVersionis zero-based and rises with every completed build; each renderer tracks the version it last painted, so a renderer that missed a build still repaints rather than depending on having been the caller that triggered it.A pre-existing bug, fixed first
Rebuildderived its viewport from_viewportSizeat entry, ran callbacks, then recordedPaintedViewportSize = _viewportSize— so a callback callingSetViewportSizein between made the root claim the geometry it had just laid out was built for a viewport it never saw. It now snapshots at entry and records the snapshot. Independent of the move, but the new renderer guard would have exposed it.API
UseUinow takes one order per phase, grouped, withclearTargetafter them:updateOrderis new.orderis renamedrenderOrder, because with three of them the bare name no longer says which phase it governs. This is a break for positional callers: the old order wasorder, inputOrder, clearTarget, soUseUi(scope, 100, -100, true)no longer compiles —-100lands onupdateOrderandtrueoninputOrder. Named arguments are unaffected apart fromorder→renderOrder. All three tutorials callUseUi()with no arguments.The break: a custom
IRenderContextwhose colour target is the same format as the window but a different size works today, becauseRenderreadColorTarget.Sizeand laid out against it. The update phase has no render context, so the build now lays out against the window and the renderer refuses to draw it into a differently sized target — that configuration goes blank.An earlier revision of this PR added a
viewportSourcedelegate toUseUiso the caller could restate the target size by hand. That is dropped: it is public API added for a configuration nothing uses, it can be got wrong as silently as the break it patches, and the size is something the framework already knows and should supply itself. A follow-up proposal covers making it work by default. All three tutorials use parameterlessUseUi()overBasicRenderContext, whose target is the swapchain, so the default path is unaffected.No generic no-scope overload was added: it would make
UseUi<MyContext>(default)ambiguous betweenViewScopeandint.Behaviour to accept
Render, but the update phase has already run.UiUpdateSystemthat dirties the UI is shown one frame later. That is whatupdateOrdermeans.updateOrdervalues are unspecified, not registration order:ServiceRegistrysorts withList.Sort, which is unstable.Multi-window
Each
UseUi(viewScope, ...)registers its own root, input system, update system and renderer;ScopedUiRoot.GetRequiredthrows if a scope is configured twice;RenderCoordinatorfilters renderers by scope.PixelyApp.Updateiterates everyIUpdatablewith no scope filter, which is correct — scope is bound at registration and never consulted per frame.ResolveUpdateTargetsexists so that is testable. The system itself holds only closures, and nothing outside can tell which window they captured without calling them (SDL) or reflecting over compiler-generated fields; returning the resolved(UiRoot, Window)pair makes the lookup observable headlessly. The hazard it guards is real and silent:GetWindow'sviewScopeparameter is defaulted, so dropping it compiles and binds every window's UI to the first one.Tests
New: the
Rebuildviewport snapshot and its recovery;BuildVersionsemantics; the staleness and repaint predicates including the missed-build case; the update system building, and not building for a hidden window or an empty viewport; ordering; per-scope root and window resolution.Reviewed by codex across four rounds — the plan through three, the implementation through one, which found the ordering test asserting something that held regardless of order.
Fixes #479