refactor(rhi): delete the u32 facade forms — Phase 2 step 3 complete, facade_native_id_params 67 → 0 (#691) - #737
Merged
Conversation
facade_native_id_params 67 -> 0; sweep_renderer_id 345 -> 168. RendererAPI.h declares no `u32 <name>ID` parameter. That is the completion criterion ADR 0011 set for Phase 2 step 3, and it is the same kind of guarantee sweep_glad_includes gives one level down: a translation unit in the sweep bucket that holds a native GL name has nothing to pass it to, so the boundary is compiler-enforced rather than measured. The counter did NOT drain per entry point as the worklist predicted. The last caller of nearly every u32 form is another form's caller (ResolveTexture feeds BindTexture, GetColorAttachmentRendererID feeds CopyImageSubData, CreateTexture2D feeds SetTextureFilter), so the facade flipped in one edit and ~250 call sites were repaired against the compiler's own (file, line) output. QUERIES BECAME IDENTITIES — the design question this item carried. RHI::ResourceKind::Query already existed in the registry, and the recycled-name hazard is concrete: OcclusionQueryPool hands a query issued in frame N to BeginConditionalRender in frame N+1, so a Shutdown()+Initialize() in between frees the names and GL may reissue one, gating a draw on an unrelated occlusion result. CreateQueries never hands its out-span to the driver (§4's .data() trap was written about this function); DeleteQueries resolves AND unregisters. THREE SILENT DEFECTS, same shape, none a type error and none caught by a test: 1. DeclareTransientTexture(name, desc, backingTextureID) set only the native id. That publishes ShadowMapCSM / ShadowMapAtlas / the compare-off raw views, and consumers read them through ResolveTextureHandle — null without a Handle — so DeferredLightingPass would have bound the CSM placeholder every frame. 2. The transient planner's alias fan-out set only the native id. Pass 1 sets both since slice 7; pass 2 (entries inheriting a sibling under aliasing) did not, so the aliased half of any plan resolved to a null identity. 3. ResolveTextureHandle bailed on texture subresource and multisample-resolve views. Under MSAA the whole G-Buffer on the blackboard is a resolve view and every CSM cascade is a layer view — they would have bound nothing. The rule these share corrects PhysicalTexture's own comment: "TextureID and Handle are ALTERNATIVES" is right for an IMPORT, and wrong everywhere the graph itself does the setting, because there it holds the resource object and reads both off one pointer in one statement. Two worklist "blockers" were phantoms. VertexBuffer/IndexBuffer's missing GetRHIHandle() is real and irrelevant — their accessor is GetBufferHandle(), backend-only, and the consumers hold Ref<StorageBuffer>, which has minted handles since slice 2. And "an import may only move to ImportTextureHandle once the diagnostics can read one" was already satisfied by #736, which unblocked six imports at once. Also gone: the IBL trio's second currency. SetGlobalIBL took four identities AND three native ids; the natives existed for three lines of RenderPipeline's graph.ImportTexture(...). ImportTextureHandle removed the parameters, three Renderer3DData fields, three accessors and the comment that kept them in step. Not done deliberately: GetRendererID() survives on the resource classes (it is what Platform/ and Renderer/Debug/ are for, and the graph's diagnostics path reads it); the creators keep their ...Handle suffix; shaderRendererID keeps its field name — it has been an RHI::ResourceHandle since slice 6 and those 114 mentions are the bulk of the remaining 168. Verification: OloEngine / OloEngine-Tests / OloEditor all build clean. Full suite 5231 tests, 5227 passed, 0 failed, 4 environmental skips — the #736 baseline exactly — with all 7 RHIBoundaryRatchet tests green against the lowered baselines. Six evidence PNGs pixel-diffed against HEAD with a per-image noise floor measured on the identical binary; every delta sits inside its own band and the churn is reverted. Live editor: olo_render_validate ok, 0 hazards, 0 resolve failures, 0 consumed-but-unbacked; ShadowMapCSM, ShadowMapCSMCascade0/3, ShadowCSMRaw, ShadowAtlasRaw, SceneDepth, SceneNormals, Velocity, WaterRefraction and IrradianceMap all capture with real content; three posed angles inspected; OloEngine.log error-free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedToo many files! This PR contains 113 files, which is 13 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (113)
You can disable this status message by setting the 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
|
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.
Closes Phase 2 step 3 of #691 (item 4 — the native-id tail). #691 itself stays open: Phases 3–8 remain.
26d2fec0facade_native_id_paramssweep_renderer_idsweep_gl_calls/sweep_glad_includesdebug_escape_hatch/backend_resolve_hatchRendererAPI.hdeclares nou32 <name>IDparameter. That zero is the completion criterion ADR 0011 set for step 3, and it is the same kind of guaranteesweep_glad_includesgives one level down: a translation unit in the sweep bucket that holds a native GL name has nothing to pass it to, so the boundary is compiler-enforced rather than measured.The design question item 4 carried: queries become identities
RHI::ResourceKind::Queryalready existed in the registry when the mint landed, so leaving queries onu32kept that enumerator dead. The concrete hazard is sharper than uniformity:OcclusionQueryPoolhands a query issued in frame N toBeginConditionalRenderin frame N+1, so aShutdown()+Initialize()in between frees the names and GL may reissue one — gating a draw on an unrelated occlusion result, which reads as a plausible frame rather than a broken one. A retired handle resolves to 0 and the conditional render is skipped instead.CreateQueriesis where §4's.data()trap applies literally (that warning was written about this function): the out-span never reaches the driver — the backend creates into its ownstd::vector<GLuint>, registers each name, and writes handles back.DeleteQueriesresolves and unregisters, with the same wrong-kind guard the otherDelete*virtuals carry.IsQueryResultAvailablereportsfalsefor a stale handle rather than reading query 0, because for occlusion a zero result reads as "fully occluded" — the honest-looking answer is the one that deletes geometry.Three silent defects, same shape
None is a type error; all three compile and none failed a test.
DeclareTransientTexture(name, desc, backingTextureID)set only the native id. That call publishesShadowMapCSM/ShadowMapAtlas/ the compare-off raw views into the frame graph. Consumers read them throughResolveTextureHandle, which answers null for an entry with noHandle, soDeferredLightingPasswould have fallen through to the CSM placeholder every frame — shadows gone, silently.WillAllocate) sets both since slice 7; pass 2 — entries that inherit a sibling's physical resource under aliasing — still setTextureIDalone. Worse than (1) because it is load-dependent: it only appears when the planner decides to alias.ResolveTextureHandlebailed on texture subresource and multisample-resolve views. Under MSAA,SceneDepth/SceneNormals/ every G-Buffer attachment on the blackboard is aCreateTextureMultisampleResolveView, and every CSM cascade is aCreateTextureArrayLayerView. With theu32bind form deleted, those would have bound nothing — a black frame under MSAA. It now mirrorsResolveTexture's branch case for case.The rule they share corrects
PhysicalTexture's own comment: "TextureIDandHandleare ALTERNATIVES, exactly one is set" is right for an import — the importer holds one currency and neither is derivable from the other. It is wrong everywhere the graph itself does the setting, because there it holds the resource object and reads both off one pointer in one statement. Slice 7 learned this for the transient acquire and fixed that one site; item 4 found three more governed by the same rule.Two worklist "blockers" were phantoms
VertexBuffer/IndexBufferexpose noGetRHIHandle()" — true and irrelevant. Their accessor isGetBufferHandle(), used only insidePlatform/OpenGL/; whatRenderer3DMeshSubmissionandVirtualMeshRegistryactually hold is aRef<StorageBuffer>, which has minted handles since slice 2. No producer needed adding. Grep for the accessor the caller uses, not the one the type family suggests.ImportTextureHandleafter the diagnostics can read one" — still true, and already satisfied. RHI Phase 2 step 3: attachment consumers + the command-layer bind cache (#691) #736'sDebug::NativeTextureIdForDiagnosticsunblocked six imports at once (DDGI's atlases, the colour-grading LUT, the fluid intermediates, the fog volumes, the TAA/clouds history, the virtual-geometry debug target). A sequencing constraint that has been met reads exactly like one still blocking.Also gone: the IBL trio's second currency.
SetGlobalIBLtook four identities and three native ids; the natives existed for exactly three lines ofRenderPipeline'sgraph.ImportTexture(...). Switching those toImportTextureHandleremoved the parameters, threeRenderer3DDatafields, three accessors, and the "both currencies clear together" comment that existed to keep them in step.Deliberately not done
GetRendererID()still exists on the resource classes — it is whatPlatform/andRenderer/Debug/are supposed to use, and the graph's diagnostics path reads it on purpose. Item 4 removes every way for the sweep bucket to use one, not the accessor. Deleting it is Phase 8, with the tools relocation....Handlesuffix — Phase 3 reshapes that family forViewHandleanyway.shaderRendererIDkeeps its field name on the POD command structs; it has been anRHI::ResourceHandlesince slice 6, and those 114 mentions are the bulk of the remaining 168.Verification
OloEngine,OloEngine-Tests,OloEditorall build with 0 errors.RHIBoundaryRatchettests green against the lowered baselines.Fluid_*0.24–0.58% vs a 0.24–0.74% floor;WorldOriginRebase_far_beforebyte-identical in an isolated run).OcclusionCull_Deferreddiffers by 2 isolated pixels — the drift already recorded for that image. Churn reverted, pera0ff3ba6's precedent.olo_render_validate→ok: true, 0 hazards, 0 resolve failures, 0 consumed-but-unbacked. Every migrated target captures with real content —ShadowMapCSM(4-layer, depth 0.33–1.0),ShadowMapCSMCascade0/3,ShadowCSMRaw,ShadowAtlasRaw,SceneDepth,SceneNormals,Velocity,WaterRefraction,IrradianceMap. Three posed angles inspected (shadows cast on water, correct materials and lighting).OloEngine.logerror-free;olo_shader_errors0.VirtualGeometryVisualEvidence.RendersUnderDeferredMSAA.Full narrative in
OloEngine/tests/Rendering/rhi_boundary_baseline.jsonanddocs/agent-rules/rhi-abstraction-boundary.md(new "Item 4 (the tail)" section; the step-3 worklist is ticked and its two wrong predictions recorded rather than quietly corrected).🤖 Generated with Claude Code