RHI Phase 2 step 3: attachment consumers + the command-layer bind cache (#691) - #736
Merged
drsnuggles8 merged 6 commits intoAug 1, 2026
Merged
Conversation
…#691) Phase 2 step 3, worklist item 1. The framebuffer attachment getters were the highest-yield migration unit: the producers (GetColorAttachmentHandle / GetDepthAttachmentHandle) already shipped in #732, so consumers could move without a new producer. Migrated: all seven bakers (thumbnail, light probe, reflection probe, IBL, impostor, sky cubemap, asset preview), the straightforward bind passes (Fog, Overdraw, SelectionOutline, Cloudscape composite, Decal, Bloom), every attachment read in DDGIProbeUpdatePass including the SetAtlasTextureParams signature, and RenderGraph's attachment clear + NaN-census readback. The worklist predicted "no new facade surface needed"; that was wrong. Five virtuals were required - CopyImageSubData, CopyImageSubDataFull, ClearTextureFloat, ReadTextureImage, ReadTextureSubImage - because the survey behind that prediction counted only BIND sinks, while the attachment getters also feed a copy family (bakers staging an attachment into a persistent texture) and a readback family (probe capture). Same lesson as the previous slice, one level up: migrate one real consumer PER SINK FAMILY and let the compiler enumerate the rest. Two silent defects found and fixed: 1. RenderPipeline's DDGI cache fingerprint could not observe an atlas recreate. EnsureResources calls DestroyResources() BEFORE creating the replacements, so GL may reissue the freed texture names - under which hashing the raw id sees no change at all, BuildFrameGraph is never rebuilt, and the graph keeps an import whose Width/Height still describe the OLD resolution (what olo_render_list_targets then reports). Now hashes RHI::HashKey(handle), whose generation cannot be reissued. Pinned by RHIResourceRegistry.HashKeyDiffersAcrossADestroyRecreateThatReusesTheNativeName. Worth internalising: the opposite teardown order (allocate-then-release) hides this completely, so whether the bug is live depends on a line of teardown code nowhere near the hash. 2. measure_rendererid.py hard-coded the (now-deleted) worktree it was written in, and reported a confident "TOTAL 0 across 0 files" from anywhere else. Root is now derived from the script's own location. Eight sites are deliberately left on the native currency, each because its other operand cannot mint yet - a graph transient, a Renderer3D setter, or an externally-registered raw id. The reason for each is tabulated in docs/agent-rules/rhi-abstraction-boundary.md rather than left as an unexplained gap. DDGI's importAtlas is among them: ImportTextureHandle blinds RenderGraph::ResolveTexture, which the MCP capture endpoints read, so an import may only migrate once the diagnostics can read a handle-imported resource. Counters: sweep_renderer_id 699 -> 653 (baseline lowered here). facade_native_id_params deliberately unchanged at 68 - this slice adds handle overloads and deletes no u32 form, which is item 4's job and the documented order. Verification: 5221 passed / 0 failed / 6 environmental skips (baseline 5220/0/6, +1 for the new test); goldens run with OLOENGINE_GOLDEN_REBASE unset. Eight evidence PNGs drifted; each image's own run-to-run noise floor was measured on the identical binary and every drift sat within it, except a single pixel in OcclusionCull_Deferred_VisualEvidence (177,190) belonging to a pass that calls none of the migrated entry points. All eight reverted, not committed. Live editor renders correctly with zero errors in OloEngine.log and no ResolveNativeAs warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#691) RenderGraph::ImportTextureHandle stores the identity and leaves TextureID at 0 - textureID and identity are ALTERNATIVES on a PhysicalTexture, deliberately so, because that is what makes AllocateTextureHandle's change detection honest. The consequence was not noticed: ResolveTexture ends at PhysicalTexture::TextureID, so it answers 0 for a handle-imported resource, and every id the MCP render tools report goes through it - olo_render_list_targets' GLTextureId, olo_render_validate's identity table, and ResolveTargetTexture, which backs olo_render_capture_target. So migrating a resource's import silently DELETES it from the diagnostics. It does not fail, warn, or look any different from a resource that genuinely has no backing - the capture just reports id 0. #732 already did this to SSAO's noise texture when it migrated that chain. This matters more than one broken probe: CLAUDE.md's rendering-verification rule is enforced through these endpoints, so a slice that quietly blinds them removes the check on itself. It is also a blocker - it is the reason the attachment slice had to leave DDGI's importAtlas on the native currency. The fix is a fallback, not a second resolver: try the native id, and when it is 0 ask the identity and go through RHI::GetNativeHandleForDebug - the hatch RHIResources.h documents for exactly this, naming "the MCP capture endpoints" as a legitimate caller. It lives in OloEditor, which RHIBoundaryRatchetTest does not scan (it walks OloEngine/src only), so debug_escape_hatch stays honestly 0 rather than being waived. Renderer3D gains ResolveFrameGraphTextureHandle as the by-name sibling of ResolveFrameGraphTexture, since ResolveTargetTexture resolves by name. NOT reproduced live, and worth being precise about: the sandbox scene uses GTAO, so SSAONoise is not in its graph, and switching the AO technique over MCP is refused by the write-consent gate (which cannot be unlocked non-interactively - see docs/agent-rules/mcp-setter-based-field-registry.md). The defect is established by construction plus three green tests covering the individual legs - RenderGraph.HandleImportResolvesAsAnIdentityAndNotNatively asserts ResolveTexture returns 0 for a handle import, and RHIResourceRegistry.DebugEscapeHatchResolvesThroughTheSameRegistry covers the hatch. The three-line composition has no test, because it lives in OloEditor, which the test target does not link. Verified live that the fallback does not disturb the normal path: captures of SceneColor, SceneDepth, ShadowMapCSM, BloomMip2 and SceneColorTexture all still resolve (texture-backed, framebuffer-backed, depth, array-layer and mip views). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ble (#691) The fallback added in the previous commit was correct and in the wrong place. It lived inline in OloEditor/src/MCP/McpToolsRender.cpp, which OloEngine-Tests does not link — so the COMPOSITION had no test, only its individual legs did. That is the same configuration that let the original defect through, so fixing the bug there re-armed the identical trap one layer out: deleting the fallback would have been silently green everywhere. It was also passing the ratchet for the wrong reason. debug_escape_hatch stayed 0 because RHIBoundaryRatchetTest walks OloEngine/src only and therefore could not see the call — a counter reading clean without the property being proven, which is exactly what the baseline's own commentary warns about elsewhere. Now Debug::NativeTextureIdForDiagnostics in Renderer/Debug/RenderGraphResourceIdentity.{h,cpp}. Renderer/Debug/ is the only directory that satisfies both constraints at once: RHIResources.h already names the introspection tools and the MCP endpoints they back as legitimate callers of the hatch (so the counter stays 0 honestly), and it is inside the engine library (so the test target can reach it). Deliberately NOT a RenderGraph member — that would put the hatch inside Renderer/, where backend_resolve_hatch bans it at 0, and moving that boundary is a decision on its own merits. Three tests in RenderGraphTest pin it: the native currency still resolves (the fallback is additive), a handle-imported resource the native path structurally cannot see still reports its backing object, and 0 is returned only when there genuinely is no backing — including for a RETIRED identity, which must not report a name the driver may since have reissued. Mutation-checked rather than assumed: removing the fallback line fails exactly DiagnosticsResolveAHandleImportedResourceTheNativePathCannotSee and leaves the other two green, so the guard is targeted rather than a blanket tripwire. Suite: 5231 tests, 5227 passed, 0 failed, 4 skipped. The skip count fell from 6 because building OloEditor (which add_dependencies on OloRuntime) made both binaries exist, so AppLaunchSmoke's editor and runtime cases stopped skipping and ran green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 2 step 3, worklist item 2 — the currency itself, not one producer family. `using RendererID = u32` is deleted; the POD command structs, the redundant-bind cache (BoundTextures / BoundUBOs / CurrentBoundShader / CurrentBoundVAO and the six per-frame shadow fields), DepthPrepassShaderIDs, InstanceGroupKey and the draw sort keys all carry RHI::ResourceHandle now. sweep_renderer_id 653 -> 355. For scale, item 1 moved it 46. THREE CORRECTNESS FIXES, not just a type change: 1. InstanceGroupKey batched draws by VAO GL NAME. A delete/create pair can hand two objects the same name, so two unrelated draws could merge into one batch and render one mesh with another's geometry. Two live handles cannot collide. 2. DepthPrepassShaderIDs compared PROGRAMS by GL name to decide whether a material's shader may be swapped for the depth-only one. Across a shader hot reload a relinked program can inherit a freed name. 3. RenderPipeline's shadow and IBL fingerprints hashed raw ids. ShadowMap::SetSettings calls Shutdown() BEFORE Init() on a resolution change, so GL may reissue the freed names and the hash sees nothing — the same defect the DDGI atlases had in the previous slice. THE DOC CLAIM THIS FALSIFIED, corrected here rather than left standing. rhi-abstraction-boundary.md said keying the cache on identities makes the Invalidate* calls "a pure optimisation". That is wrong, in the direction that ships bugs. The recycled-name collision does die, but an IN-PLACE RELOAD deliberately PRESERVES the identity while replacing the storage (ScopedResourceHandle::Sync never retires — that is what makes caching a handle safe). The cache then holds the very handle being rebound, concludes "already bound", and skips a bind that must happen, leaving the unit pointing at a deleted GL name. Under native-id keying that self-corrected because the name changed. So every site that recreates a texture's storage MUST now call InvalidateTextureBinding, and the doc says so. SCOPE WAS BIGGER THAN THE WORKLIST PREDICTED, and the doc now records why: 253 errors across 16 files rather than "~180 concentrated in CommandDispatch.cpp" (that file was ~40%). Deleting the alias is a TYPE change in headers included everywhere — Renderer3D.h alone used it in 38 declarations and produced a 4551-error parse cascade — so item 3's Renderer3D.h/Scene.cpp surface lands here. Seven unlisted resource chains came too, each feeding the cache: CloudShadowMap, SnowAccumulationSystem, OceanFFTField, FoliageRenderer, DepthPrepassShaderIDs, the global IBL maps, ShadowMap's compare-off views. Nine new facade virtuals were needed against a prediction of zero. facade_native_id_params 68 -> 67, and the fall rather than a rise is the interesting part. DrawElementsIndirectRaw(vaoID, bufferID) needed a handle form; the obvious mixed (handle, u32 bufferID) overload would have pushed a never-rises ratchet to 69. Its one caller had already run BindVAOIfNeeded, so the draw was re-binding the VAO behind the redundant-bind cache's back — DrawBoundElementsIndirect(u32) replaces both u32 forms, removes the redundant bind, and nets -1. When a migration seems to require raising a ratchet, that is usually the call site's shape being wrong, not the ratchet. Dual currency is kept where a consumer genuinely needs the native id: ShadowMap and the global IBL maps expose both, because RenderPipeline imports them into the render graph and DeferredLightingPass reads them back through context.ResolveTexture, which answers 0 for a handle import. Migrating those imports would have silently dropped IBL from the whole deferred path. Verification: 5231 tests, 5227 passed, 0 failed, 4 environmental skips — matching baseline. Goldens run with OLOENGINE_GOLDEN_REBASE unset. 68 guard conversions machine-checked for inverted polarity (none) and all 10 .Index uses audited as reporting/bucketing keys, never backend calls — the compiler accepts either spelling and only one is correct. Seven evidence PNGs drifted; a third sample showed run1-vs-run3 exceeding head-vs-run3 on the fluid images, and OcclusionCull_Deferred differs by the same single pixel (177,190) as the previous slice, so all were reverted. Live editor: zero errors in OloEngine.log, no handle-resolution warnings, olo_render_validate clean (0 hazards, 0 resolve failures, 40/40 resources resolved), 8 render targets captured including the compare-off ShadowCSMRaw view that the new handle path creates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe renderer migration replaces legacy numeric resource identifiers with generation-checked ChangesRHI resource handle migration
Sequence Diagram(s)sequenceDiagram
participant Renderer3D
participant CommandDispatch
participant RendererAPI
participant ResourceRegistry
Renderer3D->>CommandDispatch: submit ResourceHandle draw resources
CommandDispatch->>CommandDispatch: validate and cache handles
CommandDispatch->>RendererAPI: issue handle-based draw or bind
RendererAPI->>ResourceRegistry: resolve native resources
ResourceRegistry-->>RendererAPI: native resource
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
9 tasks
…nt (#691) Migrates the eight sites slice 5 deferred as "blocked on the transient pool": SSAO's resolve chain and blur->AO copy, SceneRenderPass's depth/normals/velocity exports, GPUDrivenOcclusion's two re-exports, and Cloudscape's raymarch source plus its history chain. sweep_renderer_id 355 -> 347. THE BLOCKER WAS NEVER REAL, and how it came to be recorded as fact is the part worth keeping. TransientPool::AcquireTexture returns a Ref<Texture2D>, which has minted handles since slice 2; AcquiredInfo::RendererID is a diagnostics-report field with no role in resolution. What actually stood in the way was one line in the planner that never set `.Handle`, and behind it a DESIGN INVARIANT rather than a missing producer: PhysicalTexture documented TextureID and Handle as "ALTERNATIVES, exactly one is set per entry". That rule is correct for an IMPORT — an importer only ever HAS one currency, and neither is derivable from the other. It was never true of a TRANSIENT: the planner holds the pooled Ref itself, so it has both in hand and reads them off one pointer in one statement. Nothing is derived, so nothing can drift. The contract comment now says which case it was written for instead of stating the narrower rule as universal. Generalisable: when a migration says "blocked on X", check whether X is a missing CAPABILITY or an INVARIANT someone wrote down. The first is work; the second is a decision that can be revisited once you know which case it covers. Not checking cost a slice of imagined work here and put a false claim on #691 until it was corrected. A real behavioural win falls out. Every one of these sites guards its copy with `if (src != dst)`. Those comparisons are between OBJECTS now, so a recycled driver name can no longer make a source and its export look identical and skip a copy the frame needed — the same defect class as InstanceGroupKey, in four more places. Also adds GBuffer::GetColorAttachmentHandle / GetDepthAttachmentHandle, and migrates CloudscapeRenderPass::SetHistory plus its RenderPipeline caller. Verification: 5231 tests, 5226 passed, 0 failed, 4 environmental skips (the ratchet failed only by IMPROVING; baseline lowered here). Goldens with OLOENGINE_GOLDEN_REBASE unset. Six evidence PNGs drifted, all within their own measured noise floor except OcclusionCull_Deferred, which differs from HEAD by the SAME single pixel (177,190) as the two previous slices — meaningful negative evidence, since this slice migrated GPUDrivenOcclusionPass itself and the output did not move. All reverted. Live editor: zero errors in OloEngine.log, no handle-resolution warnings, olo_render_validate clean (0 hazards, 0 resolve failures, 40/40 resolved), and the five targets on the changed paths (AOBuffer, SceneDepth, SceneViewNormals, Velocity, GTAODenoisePing) all capture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
All nine review findings verified against current code; all nine were still
valid, so none were skipped. Seven were stale prose left behind by the
identity migration. Two were defects introduced by it:
* CommandPacketDebugger passed RHI::ResourceHandle (an 8-byte struct) through
varargs to ImGui::Text("%u") at three sites. It happens to print the Index
on little-endian, which is why it looked fine. Now "%s" + FormatHandle.
The earlier pass converted the Markdown export sites and missed these.
* olo_material_get emitted formatted handle strings ("#3:1") into textureIds,
whose published schema is "Bound GL texture id per slot ... 0 = none".
That breaks every existing consumer and makes the values incomparable with
olo_render_list_targets' numeric glTextureId. Restored via
Debug::NativeTextureIdForDiagnostics rather than by rewriting the schema:
the numeric contract is the one worth keeping.
Texture::operator== compared GetRendererID(), so two genuinely different
textures could compare equal once GL recycled a name -- the exact defect the
generation exists to prevent. The review offered "fix the code or weaken the
doc"; fixing the operator is one line and is the point of the phase. All three
callers are Renderer2D texture-slot dedup with both textures alive, where
handle equality is equivalent-or-stricter.
MockRendererAPI::Native() resolved untyped, so a wrong-family handle could
succeed in the mock and return 0 in the real backend -- a green mock test that
says nothing about the shipping path. Now kind-checked like the backend's
ResolveNativeAs, with all 26 adapters routed through the kind their operation
requires.
Also: three sort keys in Renderer3DSpecializedDraws still read
shader->GetRendererID() while the command stored the same shader as a handle;
a hoisted loop-invariant in SkyCubemapBake; four stale comment blocks; four
test messages.
sweep_renderer_id 347 -> 345, and the split demonstrates the counter's
documented UNDERCOUNT: Texture::operator== accounts for the whole -2, while
the three sort-key fixes net ZERO (shaderRendererID is still a `RendererID`
spelling). The sort-key fix is the more consequential of the two and the proxy
cannot see it.
Verified: full suite 5227 passed / 0 failed; OloEditor builds clean (it is the
only target that compiles the MCP change). Seven evidence PNGs regenerated and
reverted -- six sit at or below their measured run-to-run noise floor on an
identical binary. WorldOriginRebase_far_before was the one outlier worth
chasing: deterministic run-to-run (0 px) yet 1694 px vs HEAD. An isolated A/B
with only the sort key reverted proved that change pixel-identical (0/0), and
a full-suite control reproduced the same 1694/8 without it -- so the delta is
full-suite-vs-isolated test context, not this diff. Amplified 24x it is a
1-pixel horizon line and sparse dots on receding grid lines: thin-line
rasterization at 50 km, no geometry or lighting change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drsnuggles8
deleted the
feature/vulkan-rhi-phase2-attachment-handles-691
branch
August 1, 2026 20:59
drsnuggles8
added a commit
that referenced
this pull request
Aug 2, 2026
…validity All seven findings verified against current code and applied. Notes on the three that are more than they look, and the one suggestion not taken. fs::exists(Dir) inside ~Cleanup() was a std::terminate, not a warning. A destructor is implicitly noexcept, so the throwing overload cannot report a failed cleanup — it aborts the process. Switched to the error_code form. Fixing that surfaced a second defect in the same retry, introduced with it: the loop returned on `!ec || !exists`, and `!ec` short-circuits. The race being retried is a recreate landing AFTER the removal, which leaves remove_all reporting success while the tree survives — so the guard bailed out in precisely the case it existed for, and a staging root leaked again. It now decides on whether the directory is actually gone. Three consecutive runs leave nothing behind. Relational comparison of pointers that do not point into the same array is unspecified behaviour, and AppendChars' self-alias test does exactly that with an external `str`. NOT taken: the suggested fix scans the buffer comparing `str` against each valid index, which is correct but turns an O(1) range test into O(n) on a hot append path. std::less/std::greater_equal are required to impose a total order over all pointers of a type, so the test is well-defined at O(1). Mipmap filters are now gated on mips actually holding data, not on m_MipLevels. glTextureStorage2D ALLOCATES the chain, so m_MipLevels > 1 only means the levels exist; sampling one that was never written is defined but returns undefined content, and Resize() recreates storage without regenerating (there is no level-0 data to generate from). A resized texture therefore minified against garbage. m_MipsPopulated is set at the three generate sites and cleared by Resize(). The workflow's job pools are removed in favour of the project's own. Both bound CMAKE_JOB_POOL_LINK, so the workflow-local pool would have silently won and left OLO_LINK_JOBS looking effective while doing nothing — an artefact of adding the workflow cap first and the project cap second. The configure step now just passes -DOLO_LINK_JOBS=1. Also: the fog wave-reach is gated on the same condition the shader runs on (both FFT textures valid), not on m_UseFFT alone — until the field produces them the surface is Gerstner-displaced and m_FFTAmplitude describes waves that are not on screen; OLOENGINE_GOLDEN_VENDOR rejects anything fs::path considers rooted, since a drive-relative "C:vendor" contains no separator or dot yet still discards the base on `/=`; the staging loop keeps the last REAL error rather than letting a merely-already-exists candidate overwrite it with Success; and CLAUDE.md now documents CMAKE_BUILD_PARALLEL_LEVEL as a valid cap (it is what the nightly uses) and that an explicit --parallel N overrides it. Validated on the tree merged with #736 (Vulkan RHI phase 2): 5244 tests, 5237 passed, 1 failed — Atmosphere against the stale shared baselines, which passes under OLOENGINE_GOLDEN_VENDOR=amd as the nightly runs it, pending the NVIDIA rebake in #735. That count is up from 5192: the RHI work's ~52 new tests pass alongside these changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drsnuggles8
added a commit
that referenced
this pull request
Aug 2, 2026
…gate
The quality gate failed on Reliability C and Maintainability B for new
code. All three BUGs behind the C rating were cpp:S867 in String.h --
the C idiom of using <cctype>'s int return as the right operand of `&&`.
Rather than append `!= 0`, the ctype dependency is removed outright,
because it was a latent correctness problem of its own.
std::toupper/tolower/isspace are LOCALE-SENSITIVE: under a Turkish
locale std::toupper('i') is not 'I', so a case-insensitive comparison of
engine identifiers -- asset names, shader uniforms, scene keys -- would
change meaning with the user's system locale. FString does ASCII
identifier work, so explicit constexpr ASCII helpers (ToUpperChar,
ToLowerChar, IsSpaceChar, the last matching std::isspace's "C" locale
set exactly) are both correct here and free of the whole class of
problem. That also clears three CRITICAL M23_404 ("call a function from
<locale>") and the S810 signedness findings on the same lines, since
the unsigned-char cast those required is gone with them.
This is a behaviour change for non-ASCII input under a non-default
locale, which is why it is verified rather than assumed:
FStringTest.CaseAndTrimming and ComparisonRespectsCase cover exactly
these paths and pass.
Maintainability, in descending order of what it actually buys:
* S3358 x4 -- extracted SelectMinFilter(). The inline form was a nested
conditional over integer-format AND mip-validity in two places, which
was genuinely hard to read; the helper names both conditions.
* S6166 x58 -- [[nodiscard]] messages. Note the repo is 2963 bare vs 682
messaged, so bare is the DOMINANT convention and this rule fires
codebase-wide; these 44 in String.h (plus 11 in AtmosphereSky.cpp and
3 in Texture.h) only count because the gate scores new code. Rather
than a blanket "Store this!", the FString-returning methods get a
message that flags a real mistake: ToUpper() and ToUpperInline() both
exist, so discarding the pure one is a silent no-op.
* S6004 -- init-statement form, which CLAUDE.md §1 requires anyway.
Deliberately NOT fixed, so the next reader does not re-litigate them:
* S1709 x3, "add explicit to this constructor". FString is implicitly
constructible from const char*/std::string_view/std::string BY
DESIGN -- it is a port of UE's type, and making these explicit would
break `FString s = "hello"` along with every implicit std::string
conversion at call sites such as Material::Set.
* S923/S945/S5281 on Printf. The ellipsis, the array decay and the
non-literal format string ARE the UE API being ported; replacing it
with std::format changes the contract rather than fixing a defect.
* Findings in OpenGLRendererAPI.*, Renderer3DMeshSubmission.cpp and
AssimpMeshExporter.cpp. Those arrived with the #736 Vulkan RHI merge
and belong to that work, not this branch.
Full suite unchanged: 5244 tests, 5237 passed, 1 failed (Atmosphere
against the stale shared baselines, passing under
OLOENGINE_GOLDEN_VENDOR=amd, pending the NVIDIA rebake in #735).
Co-Authored-By: Claude Opus 5 (1M context) <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
1 participant
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.
Continues issue #691 Phase 2 step 3, picking up from #732. #691 stays open — this is worklist items 1 and 2 of four; Phase 2's checkbox stays unticked.
What landed
Item 1 — framebuffer attachment consumers (
74955a7c). All seven bakers, the straightforward bind passes (Fog, Overdraw, SelectionOutline, Cloudscape composite, Decal, Bloom), every attachment read inDDGIProbeUpdatePass, andRenderGraph's attachment clear + NaN census.Item 2 — the command-layer bind cache (
35a66251).using RendererID = u32is deleted. The POD command structs, the redundant-bind cache,DepthPrepassShaderIDs,InstanceGroupKeyand the draw sort keys all carryRHI::ResourceHandle.Item 3 - the leftovers (
c6960d4d). All eight sites slice 5 had deferred as "blocked on the transient pool". That blocker was never real; see below.Two MCP fixes ride along (
d2df533e,5f9fce01) as separately-reviewable commits — see "extra scope" below.Counters
sweep_renderer_idfacade_native_id_paramssweep_gl_calls/sweep_glad_includes/backend_resolve_hatch/debug_escape_hatchBoth lowered baselines are in the same commits that earned them.
facade_native_id_paramsfalling is the notable part: the obvious mixed-currencyDrawElementsIndirectRaw(handle, u32 bufferID)overload would have pushed a never-rises ratchet to 69. Its one caller had already runBindVAOIfNeeded, so the draw was re-binding the VAO behind the redundant-bind cache's back —DrawBoundElementsIndirect(u32)replaced bothu32forms, removed the redundant bind, and netted −1.The "blocked on the transient pool" claim was wrong
Flagging it because it was recorded as fact in the worklist and posted to #691 before anyone measured it.
TransientPool::AcquireTexturereturns aRef<Texture2D>, which has minted handles since slice 2;AcquiredInfo::RendererIDis a diagnostics field with no role in resolution. The real constraint was one planner line that never set.Handle, and behind it a design invariant rather than a missing producer:PhysicalTexturedocumentedTextureIDandHandleas "ALTERNATIVES... exactly one is set".That is right for an import - an importer only ever has one currency, and neither is derivable from the other. It was never true of a transient: the planner holds the pooled
Ref, so it has both in hand and reads them off one pointer in one statement. Setting both unblocked all eight sites.Generalisable: when a migration says "blocked on X", check whether X is a missing capability or an invariant someone wrote down. The first is work; the second is a decision that can be revisited once you know which case it was written for.
Five correctness fixes, not just type changes
InstanceGroupKeybatched draws by VAO GL name. A delete/create pair can hand two objects the same name, merging unrelated draws into one batch — one mesh rendered with another's geometry.DepthPrepassShaderIDscompared programs by GL name to decide whether a material's shader may be swapped for the depth-only one. A relinked program can inherit a freed name across a hot reload.EnsureResources/SetSettingsdestroy before recreating, so GL may reissue the freed names and the fingerprint sees no change — the graph then keeps an import describing the old resolution.ImportTextureHandleblindsRenderGraph::ResolveTexture, which every MCP capture endpoint reads. feat(rhi): generation-checked RHI::ResourceHandle — the mint and the first migrations (#691 Phase 2 step 3) #732 had already done this to SSAO's noise texture, soolo_render_capture_target SSAONoisecannot work on master.measure_rendererid.pyhard-coded a deleted worktree path and reported a confidentTOTAL 0 across 0 filesfrom anywhere else.A doc claim this falsified
rhi-abstraction-boundary.mdsaid keying the cache on identities makes theInvalidate*calls "a pure optimisation". That is wrong in the direction that ships bugs, and is corrected in this PR rather than left standing. The recycled-name collision does die — but an in-place reload deliberately preserves the identity while replacing the storage, so the cache holds the very handle being rebound, concludes "already bound", and skips a bind that must happen. Under native-id keying that self-corrected because the name changed. Every site recreating a texture's storage must now callInvalidateTextureBinding.Extra scope, flagged deliberately
Renderer3D.h/Scene.cppsurface landed early, not by choice: deleting the alias is a type change in headers included everywhere, andRenderer3D.halone used it in 38 declarations (a 4551-error parse cascade on the first build). Seven resource chains the worklist never mentions came with it for the same reason.Verification
OLOENGINE_GOLDEN_REBASEunset..Indexuses audited as reporting/bucketing keys rather than backend calls — the compiler accepts either spelling and only one is correct. I nearly shippedDrawIndexedPatchesRaw(vao.Index, …), which would have handed GL a registry slot number as a VAO name.run1-vs-run3exceedinghead-vs-run3on the fluid images.OcclusionCull_Deferreddiffers by the same single pixel (177,190) in both slices — pre-existing drift.OloEngine.log, no handle-resolution warnings,olo_render_validateclean (0 hazards, 0 resolve failures, 40/40 resolved), 8 render targets captured including theShadowCSMRawcompare-off view the new handle path creates, 3 posed screenshots inspected (PBR materials, planar reflection, terrain, water all correct).Still to do on #691
Items 1-3 are done. Item 4 remains, and it is the tail of the migration rather than a chunk after item 3 - a
u32form can only be deleted once its last caller is gone. The 67 remaining native-id parameters: 18 buffers (VertexBuffer/IndexBufferexpose noGetRHIHandle()at all), 16 textures, 13 framebuffers, 8 VAOs, 5 occlusion queries (no identity type exists - a new resource kind, not a migration).Suggested order: buffer producers -> framebuffer call sites -> CloudNoise -> query identities -> then item 4 falls out.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Rendering Improvements
Tests