feat(render): per-viewport render targets with a real lifecycle + the viewport pass (M9 e11b) - #471
Merged
Merged
Conversation
… viewport pass (M9 e11b)
Gives a viewport something to render into and a pass to render with, driven by the e11a
Camera/View. Two pieces, both in context_render and both GPU-free of any concrete backend:
* viewport_target.h -- per-viewport render targets owning a colour (RGBA8Unorm) and a depth
(Depth32Float) attachment, with create / resize-in-place / release, plus the per-frame
acquire_for(viewport_id, size) a viewport panel calls and release_for for a closed one.
A SIBLING of context_render_ui's DynamicTextureRegistry, not an extension of it. Three
independent reasons, any one settling it: (1) layering -- that registry lives in
context_render_ui, which links context_render, so a context_render pass reaching it would make
context_render depend on a library that links against it (the same constraint that promoted
lit_math.h into context/render/math.h for e11a); (2) its documented contract is PERSISTENCE and
M7 a9/a10 store its handles in the L-39 snapshot, so adding release+reuse there would let a
stale UiPanel::texture resolve to another panel's texture; (3) a viewport target is a colour
AND depth pair, allocated, resized and released together.
Handles are generation-tagged ({generation:16 | slot+1:16}): releasing bumps the generation, so
a slot is reused while a handle minted before the release is refused rather than aliasing onto
the recycled entry. That is what makes reuse safe -- the property the append-only sibling bought
by never reusing, and whose absence made a per-viewport RT leak a handle per resize.
* viewport_pass.h -- render_viewport_view(): one pass drawing the ground grid, then one box PROXY
per renderable at its authored transform tinted with Renderable::color. Each draw uploads a
{mvp, color, shade} block to its own 256-byte-aligned slice of one uniform buffer, because a
queue write is ordered before the pass it feeds and several draws sharing offset 0 would all
read the last matrix written.
PROXY GEOMETRY IS THE DATA, not a shortcut: the ctx:scene schema (kSceneSchemaJson) declares
transform as additionalProperties:false / required:["position"] -- position only, no rotation, no
scale -- and Renderable::mesh_id is opaque with no registry anywhere (the sole mapping in the tree
is the hardcoded lit-golden 0=ground/1=blocker in lit_scene.cpp). There is no mesh pipeline. The
pass still honours the full render-side Transform, since that is the snapshot's own contract, but
for anything ctx:scene can express that degrades to a translation. A grid line is likewise a thin
box: PrimitiveTopology has exactly one value, TriangleList.
The mandated signature took one extra parameter it cannot work without: ITextureView exposes no
extent, and view.h pins the rule that a View never stores its aspect ratio, so target_size is
required to build a projection at all. The four specified parameters keep their order and meaning.
Tests (R-QA-013): two ctests in the plain render-* family, auto-run by --preset dev on all three
build legs with no ci.yml change. They assert the uploaded matrices against e11a's own project()
rather than pixels, since the fake backend rasterizes only its reference triangle. 29 plants, 29
RED, each attributed to its own assertion, with distinct failure signatures.
Three defects the plant round found that reading did not: a pointer-inequality "it was
reallocated" assertion is an ABA trap that reddens on CORRECT code (freed addresses get reused);
deleting destroy_attachments() from the allocator scored GREEN because the unique_ptr assignments
below already free, so that call is load-bearing for ORDERING only and now says so; and a fixture
scale.y of 1.0 made the "+Y isolates scale" assertion unable to discriminate scale at all.
Suite 1 460/460 and Suite 2 (ASan+UBSan, run locally on the macOS executor -- the preset compiles
this diff) 460/460, 0 build errors under CONTEXT_WARNINGS_AS_ERRORS=ON, pre-push audit exit 0 with
no findings. No sanitizer wall-clock widen is added: nothing here asserts a time budget.
Closes #470
One /code-review --fix pass and one /simplify pass over origin/main...HEAD, run as nine
report-only reviewers. Four code defects, each found independently by two or three of them and
two confirmed by a reviewer that compiled and ran a probe.
* A failed resize left a live entry with NO attachments. allocate_attachments destroyed the old
pair before asking the device for the new one, so a device refusal left the entry live == true
with four null pointers and size == {0,0} -- contains(id) true while color_view(id) returned
nullptr, a state the accessors' contract does not describe and which the sanctioned call shape
render_viewport_view(..., *registry.color_view(h), ...) dereferences. Now built into locals and
committed only on full success, so a refusal leaves the target byte-untouched. That is the same
recovery posture resize already took for a degenerate extent, and it is why the
destroy_attachments() call moved below the allocation rather than being deleted -- it is still
load-bearing for ORDERING, so its comment now says which half is which.
* Generation overflow silently killed a slot forever. Entry::generation is 32-bit but a handle
carries 16, and resolve() compared the full counter against the truncated field, so after 65536
release cycles on one slot create() returned a non-zero handle its own contains() called false,
release() refused it, the slot and both textures leaked permanently, and live_targets()
over-counted -- with no diagnostic anywhere. The stored generation is now bounded to 16 bits,
degrading this to ordinary wraparound.
* textures_created() over-reported on a partially failed allocation: the += 2 ran before the
view-creation null check. Moved after it.
* A WIDTH-only resize had no coverage, and every other resize in these suites moves the height --
so dropping the width term from the early-out's comparison left the whole suite green while the
target kept its old width forever, which is what dragging a vertical splitter produces.
FakeDevice::set_texture_creation_fails() makes the device-refusal branches reachable at all; that
whole family was untestable by construction, the same gap set_import_always_fails() already exists
to close for the external-texture seam. It defaults off, so no existing fixture changes behaviour.
Six pipeline-internal references in comments -- a plant id, and nouns naming artifacts that live
outside this repository -- rewritten in the code's own terms. A note at render_viewport_view now
records that it builds its pipeline, uniform buffer and per-draw bind groups on EVERY call, so
whoever adds the first frame loop owns caching them; a stateless free function has nowhere to.
Suite 1 460/460 and Suite 2 (ASan+UBSan, macOS) 460/460, 0 build errors under
CONTEXT_WARNINGS_AS_ERRORS=ON, pre-push audit exit 0 with no findings across all ten checks. The
plant round was re-run against this tree and EXTENDED to 33: 33/33 matched their expect, 33 RED,
0 GREEN, restore-failures 0. One plant (N2) came back GREEN first time and was repaired rather
than deleted -- this pass had added the 16-bit bound at two sites, so neutering either alone left
the claim true; it is re-anchored to the site that actually bounds the counter and the redundancy
is recorded in-code so a later reader does not silently re-vacuate it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ruZPEQtVrhBkDin91GADa
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
Gives a viewport something real to show. Two pieces, both in
context_render, both GPU-free of anyconcrete backend:
context/render/viewport_target.h— per-viewport render targets with a real lifecycle. Oneentry owns a colour (
RGBA8Unorm,render_attachment+copy_src+texture_binding) and adepth (
Depth32Float) attachment.create/resize(in place, keeping the handle) /release(frees the slot and destroys the textures), plus the per-frame
acquire_for(viewport_id, size)a viewport panel actually calls, and
release_forfor a closed viewport.context/render/viewport_pass.h—render_viewport_view(...). One render pass driven by ane11a
View: the ground grid first, then one box proxy per renderable at its authored transform,tinted with
Renderable::color. Each draw uploads a{mvp, color, shade}block to its own256-byte-aligned slice of one uniform buffer.
Part of the decomposed e11 (design
m9-editorD5). Nothing binds this into the Shell yet — thatis e11e.
⚠ "A real project scene" means PROXY GEOMETRY, and that is a property of the DATA
Stated here so the rest of the e11 chain inherits it instead of rediscovering it. Both facts were
re-verified against the tree in this run:
ctx:sceneschema carries POSITION ONLY.kSceneSchemaJson(
src/editor/schema/src/kind_schema.cpp) declares exactly two components:transform, which isadditionalProperties:falsewithrequired:["position"]and haspositionas its only realproperty — no rotation, no scale — and
camera(fov/near/far).Renderable::mesh_idis opaque with NO registry anywhere. The only mapping in the tree is thehardcoded lit-golden
0 = ground / 1 = blockerinsrc/render/lit/src/lit_scene.cpp.There is no mesh pipeline. Do not go looking for one. So the pass draws a box PROXY per
renderable. It honours the full render-side
Transform(position + rotation quaternion + scale),because that is the L-39 snapshot's own contract and a package populating it may fill all three — but
for anything
ctx:scenecan express today that degrades exactly to a translation.A grid line is likewise a thin BOX rather than a line primitive:
PrimitiveTopologyinrhi.hhasexactly one value,
TriangleList.Why a SIBLING registry rather than extending
DynamicTextureRegistryThe task asked for this choice to be justified. Three independent reasons, any one of which settles
it:
DynamicTextureRegistrylives in
context_render_ui, which linkscontext_render. A viewport pass iscontext_rendercode, so reaching that registry would make
context_renderdepend on a library that linksagainst it. Exactly the constraint that promoted
lit_math.hintocontext/render/math.hfore11a.
allocated once and never reallocates mid-run, and its handles are stored in the L-39 snapshot
(
render_world.hUiPanel::texture). Adding release + reuse there would let a handle capturedlast frame silently resolve to a different panel's texture.
colour plus a
Depth32Floatattachment, because it draws 3D scene geometry that mustdepth-sort. They allocate, resize and release together.
What the new registry adds is precisely what the append-only vector cannot do: release that frees the
slot and the textures, resize in place keeping the handle (the append-only API's only way to
change size is another
create_panel_target(), which leaks a handle and a texture per resize —one per frame of an edge drag), and generation-tagged handles (
{generation:16 | slot+1:16}) soslot reuse is safe: releasing bumps the generation, and a handle minted before the release is refused
rather than aliasing onto the recycled entry. That refusal is the property the sibling bought by
never reusing at all.
⚠ Signature note
The task specified
render_viewport_view(IDevice&, const View&, const RenderSnapshot&, ITextureView& target). Those four parameters are here in that order with those meanings, but a fifth ismandatory:
ITextureViewexposes no extent, andview.hstates the rule — "The framed ASPECTRATIO is never stored on a View. It belongs to the target the view is rendered into, so every entry
point below takes that target's extent and derives it." Every e11a entry point (
projection_matrix,view_proj,project,pick_ray) takes anExtent2Dfor that reason. Without it this functioncannot build a projection at all. Everything else is a defaulted
ViewportPassConfig.Testing — 29 plants, 29 RED, each attributed to its own assertion
Two new ctests in the plain
render-*family (render-test_viewport_target,render-test_viewport_pass), auto-run by--preset devon all threebuildlegs — noci.ymlchange (pre-push audit check 8 confirms).
The fake backend rasterizes nothing but its reference triangle, so pixels prove nothing here. What
rendertest::FakeDevicedoes record is every uniform payload in draw order — which is where thegeometry lives — so the tests decode each block and assert the model origin lands where e11a's own
project()puts the authored position. Independent computation, exact value, and one that a passdrawing everything at the world origin cannot produce. Fixtures use distinct, non-origin positions on
all three axes for the same reason.
Every plant went RED through an assertion (never a build break), with 29 distinct failure
signatures, 5-6 compile lines each and uniform 4.5-5.4s cycles — no stale-binary tells. Run through
the shared harness with
--pre-verify-cmddeleting the planted objects (the macOS GNU Make 3.81 row).releasenever returns the slot to the free listreleaseskipsdestroy_attachmentsreleaseskips the generation bumpreleasepushes the slot twice.release()d, not.reset()resizereturns true and changes nothingresizeloses its same-size early-outresizereallocatescreateis acceptedacquire_forcreates a fresh target per resizereleaseleaves the viewport bindingtarget_fornever returns a dead handleproxy_sizeignoredmajor_every-th line is major2 * (2 * half_lines + 1)linesi * spacingdraw_grid=falseignoredresizefrees the old attachments BEFORE allocating the new pairresize's early-out drops the width termcreatedoes not return the slot to the free listThree findings the round produced that reading did not, all now fixed or documented in place:
ptr_after != ptr_beforeis an ABA trap that fails on CORRECT code — freeing the old pair andallocating a new one routinely returns the same heap addresses. That assertion reddened on the very
first run against an implementation that was reallocating exactly as intended. Replaced with the
device-reported extent/format, which only the new object can have.
destroy_attachments(entry)from the allocator scored GREEN: the fourunique_ptrassignments below already free the old pointees. The call is load-bearing forordering (a view outliving its texture), not for freeing — no test can observe that, so the
reason is now stated at the call site rather than left to be "simplified" away on a green suite.
scale.yof 1.0 made the "+Y isolates scale" assertion unable to discriminate scaleat all; the scale plant reddened its X and Z siblings and left Y green. Fixture widened to 3.0.
Test plan
cmake --build --preset dev && ctest --preset dev: 460/460 passed, 0 builderrors under
CONTEXT_WARNINGS_AS_ERRORS=ONsanitizepreset (ASan+UBSan) run locally on the macOS executor; the preset compilesthis diff, so the claim is supportable rather than deferred
03-refineto 33/33 RED, 0 GREEN, distinct signatures, restore verified byte-exacttools/**,bench/**or dependency changeNo
CONTEXT_TSAN_BUILD/CONTEXT_ASAN_BUILDwiden is added: nothing here asserts a wall-clockbudget, so the define would be dead configuration.
Review pass (
03-refine)One
/code-review --fix+ one/simplifypass, run as nine report-only reviewers overorigin/main...HEAD(the built-incode-reviewskill is not model-invocable in this executor, sothe mandated angles ran as dispatched helpers). Findings were applied centrally. Four code
defects were found and fixed; each was found independently by two or three reviewers, and two were
confirmed by a reviewer that compiled and ran a probe.
Fixed
resizeleft a live entry with no attachments.allocate_attachmentsdestroyed theold pair before asking the device for the new one, so a device refusal left the entry
live == truewith four null pointers andsize == {0,0}—contains(id)true whilecolor_view(id)returnednullptr, which the accessors' documented contract does not describe andwhich the sanctioned call shape
render_viewport_view(..., *registry.color_view(h), ...)dereferences. Now built into locals and committed only on full success, so a refusal leaves the
target byte-untouched — the same recovery posture
resizealready took for a degenerate extent.Entry::generationis 32-bit but a handlecarries 16, and
resolve()compared the full counter against the truncated field. After 65536release cycles on one slot,
create()returned a non-zero handle its owncontains()calledfalse,
release()refused it, the slot and both textures leaked permanently, andlive_targets()over-counted — with no diagnostic. The stored generation is now bounded to 16 bits, degrading this
to ordinary wraparound.
textures_created()over-reported on a partially failed allocation — the+= 2ran before theview-creation null check. Moved after it.
dropping the width term from the early-out's comparison left the whole suite green while the target
kept its old width forever (what dragging a vertical splitter produces). Now pinned.
FakeDevice::set_texture_creation_fails()was added to make the device-refusal branches reachable atall; the whole family was previously untestable by construction, exactly the gap
set_import_always_fails()already exists to close for the external-texture seam. Six pipeline-internalreferences in comments (a plant id, and nouns naming artifacts that live outside this repository) were
rewritten in the code's own terms.
Verified and deliberately NOT fixed here — recorded so they are not lost:
render_viewport_viewbuilds its pipeline (compiling theWGSL), its uniform buffer and one bind group per draw on every call — at 60 Hz that is a shader
recompile per frame. A
⚠note now names this at the declaration: a stateless free function hasnowhere to cache, so whoever introduces the first frame loop owns adding it, along the lines
WindowCompositoralready follows. Fixing it here would restructure the public surface andinvalidate the per-draw test oracle, which is not a refine-pass change.
(the counterpart of the write offsets already asserted), the Z-parallel grid lines' extent, the
clear op and clear colour, the depth attachment's identity and clear value, the documented
grid-before-proxies draw order, and a rotation fixture whose half-turn about Y yields a diagonal
matrix and so cannot discriminate
R*SfromS*R. Each needs either a newFakePassLogobservable or a reshaped fixture.
success, and
ViewportGridConfig::half_linesis unbounded (>= 2^31is signed-negation UB).Gates re-run on the final tree: Suite 1
460/460; Suite 2 (ASan+UBSan, macOS)460/460;pre-push audit exit 0, no findings across all ten checks; plant round re-run and EXTENDED to 33 —
33/33 matched their expect | 33 RED | 0 GREEN | restore-failures 0.One plant (N2) came back GREEN on the first round and was repaired rather than deleted: this pass had
added the 16-bit bound at two sites, so neutering either alone left the claim true. Re-anchored to
the site that actually bounds the counter, and the redundancy is now recorded in-code so a later
reader does not silently re-vacuate it.
Closes #470