feat(rhi): generation-checked RHI::ResourceHandle — the mint and the first migrations (#691 Phase 2 step 3) - #732
Merged
drsnuggles8 merged 14 commits intoAug 1, 2026
Conversation
…step 3, part 1) Phase 1 declared `RHI::ResourceHandle` and specified what it means, but nothing minted one and `GetNativeHandleForDebug` was declared and defined nowhere. This adds the producer. It converts no call sites — the same shape Phase 1 shipped in (a vocabulary plus a ratchet), so Phase 2's checkbox on #691 stays UNTICKED. `Renderer/RHI/RHIResourceRegistry` mints handles with a generation, which is the load-bearing part: GL recycles object names, so `Texture::operator==` (comparing GetRendererID()) can today report a deleted texture and an unrelated later one as EQUAL. `RecycledIndexWithStaleGenerationIsRejected` demonstrates that fixed — it registers, unregisters and re-registers the SAME native name, asserts the slot really was recycled, then asserts the two handles differ. Reads are lock-free (chunked stable-address slots; the generation is read on both sides of the payload so a slot changing hands cannot yield a torn native value); writes take a mutex. `ScopedResourceHandle` gives RAII ownership so no destruction path can forget to unregister, and `UpdateNative` repoints a live handle at recreated storage WITHOUT moving the generation — which makes caching a handle safe across an in-place hot-reload, where TextureInPlaceReloadTest currently warns that a renderer ID must be re-read every frame. ViewHandle and HeapOffset stay deferred to Phase 3 as a matched pair (ADR amendment (11)): a ViewHandle with no heap behind it has one view per resource and therefore detects nothing, and deferring adds two call sites to Phase 3 — not a second sweep — because the ~230 bind sites are already its to delete. Ratchet gains three counters, seeded at their real values rather than at zero: sweep_renderer_id 700 (79 files), facade_native_id_params 68, backend_resolve_hatch 0. Drive the facade one to zero FIRST — same ordering argument section 1.7 makes for stripping the GLenums before counting includes. A full call-site sweep was attempted and reverted; ADR amendment (16) records the four scripted-edit defect classes that stopped it, three of which were found by reading output rather than by any test. The decisive one rewrote a comparison on WaterComponent::m_NoiseTexture — an AssetHandle, not a GPU handle — because SSAORenderPass has a member of the same name that genuinely is one. Handle::IsValid()/HeapOffset::IsValid() gain `constexpr` so the aggregate-init hole can be pinned at compile time: C++20 P0960 makes `ResourceHandle(someId)` well-formed, and the test now asserts the result carries Generation == 0 and is therefore inert, rather than claiming (wrongly) that it cannot be constructed. Verification: 5208 tests run, 5202 passed, 0 failed, 6 skipped (the usual environment-dependent set). No rendering verification is claimed or owed — the registry stores an opaque u64 and never touches a device. The six evidence PNGs the suite regenerated were pixel-identical to HEAD and dropped from the commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ice 1) Additive: GetRHIHandle() sits ALONGSIDE GetRendererID() on all nine resource interfaces, so no call site moves and the tree compiles throughout. The dual currency is deliberate and temporary — a final slice deletes GetRendererID() once every caller has migrated. Ratchet counters are unchanged (700/68/0), which is the correct signature for a slice that makes the new currency available without migrating anything onto it. Declared pure virtual on purpose. It forces every implementor — including the three test stubs — to answer "what is this object's identity?", and their answer (the null handle) is meaningful: a defaulted return would make "stub with no GPU object" indistinguishable from "real resource whose registration silently failed". Backend classes hold a ScopedResourceHandle and Sync() it at all 26 sites that assign a native name. Each was reviewed individually rather than counted; the earlier abandoned sweep showed that "insert a line after this line" is the one scripted edit shape that is reliable, whereas anything encoding brace or include structure is not (OpenGLShader's accessor landed inside another function body and had to be fixed by hand — which turned out for the better, since it needs the same block-finalize as GetRendererID or an async-compiling shader hands back the null handle). ONE REAL BUG, found by the new GL-gated test and worth the slice on its own: ScopedResourceHandle::Sync treated `nativeHandle == 0` as "released" and retired the identity. OpenGLTexture2D::InvalidateImpl zeroes m_RendererID transiently BETWEEN deleting the old GL object and creating the new one, so every in-place hot-reload retired the handle (generation 7) and minted a fresh one (generation 9) — precisely the cached-handle-goes-stale behaviour ADR amendment (12) claims to fix. Sync cannot tell a transient zero from a final one (identical at the call site; only the caller knows), so it is now non-destructive and retirement is RAII-only, with Reset() for a deliberate early release. ADR amendment (12) corrected to record it. RHIHandleNativeIdentityTest is the point of the slice: RHIResourceRegistryTest proves bookkeeping against synthetic u64s and cannot detect a MISSING Sync(), because a handle naming nothing still behaves consistently. The new tests assert against a real driver that a handle resolves to the live GL name, agrees with GetRendererID() during the migration, survives an in-place reload, and goes stale when its resource dies. Four of them passed while the reload bug was live — only a real reload showed it. Caveat recorded rather than papered over: on this driver the GL name was RECYCLED across the reload, so the branch proving the handle follows a CHANGED name did not fire here. That path is covered headlessly by SyncTreatsATransientZeroAsRecreateNotRelease (100 -> 200); the end-to-end combination remains unexercised. The test logs which branch it took. Verification: 5216 run, 5210 passed, 0 failed, 6 skipped (usual environment set). The six evidence PNGs the suite regenerated were pixel-identical to HEAD — the expected result for a slice that adds identity without changing any draw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…slice 2)
Adds RHI::ResourceHandle siblings for the seven bind virtuals (texture, image
texture, uniform/storage buffer, shader program, VAO, framebuffer) beside the
existing u32 forms. Callers migrate in later slices; nothing calls the new forms
yet, so the ratchet is deliberately unmoved at 700/68/0.
WHY BOTH SPELLINGS COEXIST, and why this corrects earlier guidance in this
branch: the conversion is asymmetric. `handle -> native` is a registry lookup,
but `native -> handle` is NOT recoverable — a driver name does not identify a
registry slot. A facade taking only handles therefore cannot serve a caller still
holding a u32, whereas one offering both serves either. So the order is: add
overloads, migrate callers, delete the u32 forms LAST — at which point
facade_native_id_params drops to zero in one step and the boundary becomes
compiler-enforced instead of counter-enforced.
An earlier note in the baseline and ADR said to drive facade_native_id_params to
zero FIRST. That is correct for an all-at-once conversion, where flipping the
facade forces every call site simultaneously — i.e. exactly the unreviewable
change this plan exists to avoid. Both documents are corrected.
Resolution stays inside Platform/OpenGL/: each overload is virtual and only the
backend implementation calls Utils::ResolveNative (reintroduced here, in the
backend, where it belongs). A resolving helper in Renderer/ would have been
simpler and would have breached the boundary this phase exists to close —
backend_resolve_hatch is the counter that keeps that honest, and it stays 0.
MockRendererAPI gains the same seven overrides, resolving and delegating exactly
as the real backend does. That is deliberate beyond satisfying the pure virtual:
a test driving the handle API now observes the native values a real backend would
have used, INCLUDING 0 for a stale handle — so "what does a pass do when it binds
a dead resource?" becomes assertable headlessly. Useful for slices 3+.
The overloads have no callers yet, so two GL-gated tests exercise them directly
rather than shipping untested delegations:
* FacadeHandleOverloadBindsTheSameObjectAsTheLegacyForm reads
GL_TEXTURE_BINDING_2D back and asserts both spellings reach GL with the same
object — a transposed argument in BindImageTexture's seven parameters would
land here.
* FacadeHandleOverloadUnbindsForAStaleHandle binds a live texture, then a DEAD
handle to the same slot, and asserts the unit goes to 0. That is the layer's
central safety claim, previously only inferable from registry unit tests plus
an assumption about how 0 propagates through glBindTextureUnit.
Verification: 5218 run, 5212 passed, 0 failed, 6 skipped (usual environment set).
All seven regenerated evidence PNGs pixel-identical to HEAD.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ep 3, slice 3)
Attachments are separate GPU objects that the engine samples through
ResolveTexture, so "the framebuffer's handle" is the wrong answer to "which
texture is this?". OpenGLFramebuffer now holds a ScopedResourceHandle per colour
attachment plus one for depth, alongside the existing native names.
THIS IS A MIGRATION ROOT, and picking it was a correction. The first attempt at
this slice took HZBGenerator — small, self-contained, GPU-tested — and had to be
reverted. Slice order is a TOPOLOGICAL SORT over the dataflow, not "smallest
first": because `native -> handle` is unrecoverable, a symbol can only migrate
once every producer feeding it has. HZBGenerator sits downstream of
ResolveTexture on BOTH paths — its GetHZBTextureID() returns the externally
supplied id verbatim when one is set — so converting its output made it return
the null handle whenever an external HZB was active. GTAORenderPass sets exactly
that and then binds the result, so ambient occlusion would have silently bound
nothing. It compiled; GTAO's coverage is math-level and would not have caught it.
Found by reading the call site. Recorded in HANDOVER.md with the standing rule:
before starting a slice, list the symbol's producers and confirm all are migrated.
Attachments ARE a root — their producer is Utils::CreateTextures inside
Invalidate(), i.e. the backend itself — and they unblock the next one:
RenderGraph::ResolveTexture returns attachment ids for its framebuffer-view
resources and cannot hand out handles until these do.
ONE SEMANTIC IS DELIBERATELY THE INVERSE OF SLICE 1, and both are now pinned by
tests asserting opposite outcomes:
* texture hot-reload -> identity PRESERVED (the C++ object survives; only its
GL storage is recreated), so materials may cache the handle;
* framebuffer resize -> identity RETIRED (the attachment textures are
genuinely destroyed), so a holder of the old handle sees it go stale rather
than silently following onto a different texture.
Both run through ScopedResourceHandle and differ only in whether the call site
says Sync() or clear()/Reset() — the direct consequence of slice 1's finding that
Sync cannot infer intent. FramebufferResizeRetiresTheOldAttachmentIdentities is
the guard against "simplifying" this into the reload rule by analogy.
Counters unchanged at 700/68/0: additive again, second accessor beside the u32
one, no caller migrated yet. They start moving in the ResolveTexture slice.
Verification: 5220 run, 5214 passed, 0 failed, 6 skipped (usual environment set).
All seven regenerated evidence PNGs pixel-identical to HEAD.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last unmigrated ROOT. CreateTexture2D/CreateTextureCubemap/CreateFramebuffer/ CreateBuffer/CreateVertexArray produce the u32 members passes hold (SSAORenderPass::m_NoiseTexture, ColorGrading's identity LUT, the fluid depth/thickness targets, DDGI atlases, VirtualGeometry debug targets), which feed ImportTexture, which feeds ResolveTexture, which feeds nearly every pass. Nothing above them could migrate until this landed. Spelled `...Handle` rather than overloaded because a creator's signature differs only in return type. Temporary: the final slice deletes the u32 forms and takes the names back. The Delete* siblings ARE overloads, and each does BOTH halves — destroy the object and Unregister the identity. Omitting the second leaves the slot's generation intact so a handle to a destroyed object keeps resolving to a name the driver may reissue; that is the exact omission the abandoned scripted sweep made, and it is why RawCreatorMintsALiveIdentityAndDeleteRetiresIt asserts the post-delete resolve is 0 rather than just that the call compiles. RawBufferAndVertexArrayCreatorsMintDistinctKinds is the cleaner demonstration of what this whole layer buys: GL object names are PER-TYPE, so a buffer, a VAO and a framebuffer created back to back can all legitimately be name 1. Under the old currency those three u32s compare equal while naming completely different objects. The test asserts the three identities differ and that each resolves to a live object of its own type — no delete/create cycle needed to reproduce the conflation ADR 0011 §1.1 describes. This completes the L0 layer of the dependency chart now recorded in HANDOVER.md: Ref<T> resources (slice 1), framebuffer attachments (slice 3), the bind family (slice 2) and the raw creators (here). Every producer in the engine can now hand out a handle. Slice 5 (ImportTexture, L1) is the first that can consume them. Counters unchanged at 700/68/0 — still additive, no caller migrated. They first move at the ResolveTexture slice, which is also the first to change what passes bind and therefore the first owing full rendering verification. Verification: 5222 run, 5216 passed, 0 failed, 6 skipped (usual environment set). All seven regenerated evidence PNGs pixel-identical to HEAD. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…step 3, slice 5)
L1 of the dependency chart. Adds ImportTextureHandle / ResolveTextureHandle
beside the native forms, and a Handle field on PhysicalTexture beside TextureID.
THE TWO FIELDS ARE ALTERNATIVES, NOT DUPLICATES, and that resolves the concern
that motivated the design: there is nothing to keep in sync, because neither is
derivable from the other. `native -> handle` is unrecoverable (a driver name does
not identify a registry slot) and `handle -> native` is a resolution that may
only happen inside Platform/<Backend>/, which RenderGraph is not. Each entry
carries whichever currency its importer had; exactly one is set.
That also fixes the migration GRAIN: per resource, not per layer. A resource's
creator -> import -> resolve -> bind chain moves together inside one pass, which
is why ResolveTextureHandle returns null for a natively-imported entry rather
than inventing something.
Two contract tests pin it in BOTH directions, because the tempting cleanup is to
make either fall back to the other:
* NativeImportResolvesNativelyAndHasNoIdentity — a native import yields no
handle. Fabricating one would name nothing while claiming to name something.
* HandleImportResolvesAsAnIdentityAndNotNatively — a handle import yields 0
from the native form. Returning the underlying GL name there would have
RenderGraph (in Renderer/) performing handle->native resolution: the exact
boundary breach this phase exists to close, and invisible to every counter
because the call would sit inside RenderGraph.cpp rather than at a pass.
Each failure message says why the obvious fix is wrong, not just that the value
differs.
ImportTextureHandle delegates to ImportTexture rather than duplicating it: the
import path does non-obvious bookkeeping (naming, placeholder propagation,
registry invalidation) and forking it would create a second place to fix whenever
those rules change.
A null ResolveTextureHandle result is deliberately NOT recorded as a resolve
failure. During the migration an unmigrated resource legitimately has no
identity, and counting those would bury genuine stale-handle failures in noise
for the whole duration.
Scope note: ResolveTextureHandle covers the framebuffer-ATTACHMENT view path
(migrated in slice 3) and returns null for subresource / multisample-resolve
views, which walk parent chains by name. Those migrate per resource in their own
slices rather than being half-converted here.
Also found and fixed while building this: RGCommandContext.h lacked the RHITypes
include, so `RHI::` was unresolvable and the declaration silently degraded to
OloEngine::ResourceHandle — the render graph's STRING-NAMED resource handle, a
completely different type. It failed loudly only because the definition
disagreed; in a header-only path the substitution would have compiled. ADR 0011
§1.7 predicted this collision at call sites; the header case is worse. A follow-up
commit renames the graph's type to RGResourceHandle, matching every one of its
siblings in the same header.
Verification: 5224 run, 5218 passed, 0 failed, 6 skipped (usual environment set).
All eight regenerated evidence PNGs pixel-identical to HEAD. Counters unchanged at
700/68/0 — no caller migrated yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…andle (#691) Two types named `ResourceHandle` existed in overlapping scopes: the render graph's string-named resource handle (OloEngine::ResourceHandle) and the generation-checked GPU identity added by step 3 (OloEngine::RHI::ResourceHandle). THE COLLISION IS NOT COSMETIC — it turns a missing include into a silent type substitution. RGCommandContext.h declared a member returning RHI::ResourceHandle without including RHITypes.h; the compiler did NOT report an unknown type, because `RHI::` was unresolvable and the name degraded to the unqualified `ResourceHandle`, which was in scope and valid — a completely different concept. It surfaced only because a separately-compiled definition disagreed about the return type. In a header-only path (inline function, or a member defined in the same header) it would have compiled. ADR 0011 §1.7 chose `RHI::`-namespacing so the two could not be confused AT A CALL SITE. That was right and insufficient: where both names can be in scope, they must differ, not merely be qualifiable. Recorded as ADR amendment (17). The new name also makes the type consistent with every sibling in its own header — RGTextureHandle, RGBufferHandle, RGFramebufferHandle, RGResourceDesc, RGResourceFormat, RGLoadAction, RGStoreAction, RGAccessMode, RGQueueType — of which it was the sole unprefixed exception. 664 renames across 117 files. Renderer/RHI/ deliberately excluded: inside `namespace OloEngine::RHI` the unqualified name means the RHI type, and that namespace is opened nowhere else (verified). The target name was checked as unused before renaming into it — the discipline the shaderRendererID -> shaderHandle collision taught earlier in this branch. Two application defects worth recording, both the same root cause — the pattern was correct about C++ identifiers and blind to the context it matched in: * #include paths: Renderer/ResourceHandle.h became Renderer/RGResourceHandle.h in 105 files while the file kept its name. Loud. * prose: three comments in the RHI tests were rewritten to say the opposite of their point — silently, in the files that exist to prevent this very confusion. A flat identifier rename is the reliable scripted edit IN CODE ONLY; comments are the dangerous half because nothing checks them. Verification: 5224 run, 5218 passed, 0 failed, 6 skipped — identical to the pre-rename run. All eight regenerated evidence PNGs pixel-identical to HEAD. A pure rename should change nothing, and nothing changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stall
`cmake --install --prefix` only redirects RELATIVE install destinations. USD's
top-level pxr/CMakeLists installs pxrConfig.cmake to an ABSOLUTE destination
baked from the configure-time CMAKE_INSTALL_PREFIX, which we never passed — so
the generated pxr/cmake_install.cmake ended with
file(INSTALL DESTINATION "C:/Program Files (x86)/usd" ... pxrConfig.cmake)
and the install died with "file cannot create directory ... Maybe need
administrative privileges" on any non-elevated shell.
Two things made this expensive to diagnose, both worth remembering:
* It only fires on a cache MISS, and USD builds once per machine, so it can lie
dormant indefinitely and then surface as "the build randomly broke" long after
the code that caused it was written.
* INSTALL_COMMAND runs Release then Debug, so the Release failure aborted the
whole step before Debug installed at all. The visible symptom was LNK1181 on a
missing install/Debug/lib/usd_m.lib — a file whose absence has nothing to do
with the real error ~10k log lines earlier.
Release is an arbitrary but harmless prefix for the one stray absolute rule:
pxrConfig.cmake is a find_package() shim we never consume (the engine links
usd_m.lib by path), and every destination we DO consume is relative and so still
lands per-config via --prefix.
Found while repairing a wiped USD cache that was blocking issue #691 work; the
fix is unrelated to that migration and stands on its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e 5)
The first slice to move real call sites off the u32 native id rather than add
capability beside it. sweep_renderer_id 700 -> 699 (measured by the ratchet,
baseline followed down).
SSAO's noise texture moves as a WHOLE CHAIN in one commit -- create, configure,
import, bind, delete -- because `native -> handle` is unrecoverable, so a
half-migrated chain has a step that cannot get back to the currency the next
step needs. Its `!= 0` guards become `.IsValid()`. The pass's rawFB attachment
bind moves too, now that attachments mint identities (slice 3).
Migrating a real resource enumerated three facade gaps that reading the facade
had missed, because the earlier survey was organised around the bind and
create/delete families and texture *configuration* is neither:
SetTextureFilter, SetTextureWrap, UploadTextureSubImage2D -- four files each
(RendererAPI, the GL backend, RenderCommand, MockRendererAPI).
It also exposed that a sibling added at the wrong layer is invisible until a
caller needs it: ImportTextureHandle existed on RenderGraph, but passes reach
the graph through RGBuilder, which had no forwarder. Same for
RGCommandContext::BindTexture. Both added here.
FIX -- generation bumping was silently disabled for handle imports.
ImportTextureHandle delegated to ImportTexture(name, 0u, desc) and stamped the
identity on afterwards. AllocateTextureHandle decides whether to retire prior
handles with
resourceChanged = (phys.TextureID != textureID) || (phys.IsHistory != isHistory)
which for a handle import compares 0 != 0 forever. Re-importing a name with a
DIFFERENT texture therefore kept the slot generation, and every cached
RGTextureHandle stayed valid while resolving to the slot's new occupant -- the
recycled-name hazard this phase exists to close, recreated inside the fix for
it. The identity is now threaded into AllocateTextureHandle as a defaulted
parameter, so one change test covers both currencies and the u32 path clears a
stale identity for free (keeping "alternatives, never both" true without a
second assignment site).
No test caught it because SSAO creates its noise texture once in Init and never
recreates it, so the bug is inert in the only migrated pass; a pass that
recreates on resize would have hit it first, as a stale-looking frame rather
than a crash. Pinned in both directions by
RenderGraph.ReimportingANameWithADifferentIdentityRetiresTheOldHandle and
RenderGraph.ReimportingAHandleResourceNativelyClearsTheStaleIdentity.
Verification (this is the first slice touching pass code, so it owes the full
rendering rule):
* 5220 passed / 0 failed, three consecutive full-suite runs.
* Goldens executed with OLOENGINE_GOLDEN_REBASE unset.
* Evidence PNGs pixel-diffed against HEAD after measuring the run-to-run noise
floor on the identical binary. SSAO's own six are byte-identical to HEAD.
Six other PNGs drift; four are at or below their own floor, and the two that
are not (OcclusionCull_Deferred 8/255 over 0.001% of pixels,
WorldOriginRebase 6/255 over 0.000%) were last regenerated 71 commits ago at
41bbdf9 and belong to passes that call none of the migrated entry points --
SSAORenderPass.cpp is the only caller of ImportTextureHandle /
GetColorAttachmentHandle / CreateTexture2DHandle. Reverted rather than
committed, since 4 of 6 are pure noise.
* Live editor: SSAO compiles and links (status 1), 18 entities deserialized,
0 failed, no errors in OloEngine.log.
Also records both generalisable lessons in docs/agent-rules/rhi-abstraction-boundary.md:
that migrating one real resource is the only reliable way to enumerate the
missing facade surface, and that identity passed to a routine as an
afterthought is invisible to that routine's invalidation logic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#691) Scoping finding from starting the Renderer/Commands/ migration and backing it out: it looks like several independent migrations and is exactly one, because material textures, the four shadow paths, snow depth and cloud shadow all key the same CommandDispatchData::BoundTextureIDs redundant-bind cache. Migrating PODMaterialData's texture ids alone does not get past its own file. Records the full unit, the ~180-error blast radius, that every producer involved already mints (so it is unblocked, just indivisible), and the reason it is worth doing rather than deferring: this cache has already shipped a real visual bug of precisely the kind the identity currency prevents -- VirtualGeometryPass binding the Hi-Z pyramid to unit 0 while unit 0 is also u_AlbedoMap, so a material whose albedo GL name matched the stale cache entry silently sampled HZB depth as its albedo. Keyed on identities that collision is unrepresentable, which turns the manual InvalidateTextureSlot/InvalidateTextureBinding calls from load-bearing correctness into a pure redundant-bind optimisation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe renderer adds generation-checked RHI resource handles and a resource registry. OpenGL resources synchronize handles with native objects. Render-graph imports, resolution, and resource-kind references use the new handle model. Tests cover registry lifetime, native identity, graph behavior, and API boundaries. ChangesRHI resource handle migration
OpenUSD configuration
Sequence Diagram(s)sequenceDiagram
participant RenderPass
participant RGBuilder
participant RenderGraph
participant OpenGLRendererAPI
participant ResourceRegistry
RenderPass->>RGBuilder: ImportTextureHandle
RGBuilder->>RenderGraph: import RHI-backed texture
RenderGraph->>ResourceRegistry: retain generation-checked identity
RenderPass->>OpenGLRendererAPI: bind texture handle
OpenGLRendererAPI->>ResourceRegistry: resolve OpenGL native name
ResourceRegistry-->>OpenGLRendererAPI: native resource name
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
OloEngine/src/OloEngine/Renderer/RenderGraph.cpp (1)
2436-2462: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA reused slot keeps a stale
phys.Handle, soResolveTextureHandlecan return another object's identity.
AllocateTextureHandlenow ownsphys.Handleon all three of its paths. The transient paths do not.Trace the sequence:
- A pass imports
"X"throughImportTextureHandle.m_PhysicalTextures[i].Handleholds a valid identity.- A later topology declares
"X"as a transient. This reuse path (Line 2436) finds the existing slot by name, marks it alive, and returns. It never clearsphys.Handle, and it does not bump the generation, so previously cachedRGTextureHandlecopies stay current.MaterializeTransientResourceswritesm_PhysicalTextures[...].TextureID(Line 3426 and Line 3553) but never writesHandle.ResolveTextureHandlereaches Line 1710 and returns the stale identity from step 1.The caller then binds a generation-checked handle that names a different GPU object.
PhysicalTexturedocumentsTextureIDandHandleas alternatives with exactly one set per entry; these paths violate that invariant.
DeclareTransientTexture(name, desc, backingTextureID)at Line 2525 has the same gap: it setsTextureIDandIsHistoryand leavesHandleuntouched.Clear the identity wherever an entry is (re)established on the native currency.
🐛 Proposed fix
Transient reuse path:
if (existingIt->second.Index >= m_PhysicalTextures.size()) m_PhysicalTextures.resize(static_cast<sizet>(existingIt->second.Index) + 1u); + // This entry is now on the native currency. Drop any identity a + // previous handle-based importer left behind, or ResolveTextureHandle + // would hand out an identity naming a different object. + m_PhysicalTextures[existingIt->second.Index].Handle = {}; + const RGTextureHandle handle{ existingIt->second.Index, slot.Generation };Externally-backed transient (Line 2525):
auto& physicalTexture = m_PhysicalTextures[handle.Index]; physicalTexture.TextureID = backingTextureID; + physicalTexture.Handle = {}; physicalTexture.IsHistory = false;Apply the same reset in
MaterializeTransientResourcesnext to eachTextureIDassignment (Line 3426 and Line 3553).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Renderer/RenderGraph.cpp` around lines 2436 - 2462, Clear the stale PhysicalTexture identity whenever a texture becomes transient. In the existing-name reuse path of AllocateTextureHandle, reset the corresponding m_PhysicalTextures entry’s Handle before returning; do the same in DeclareTransientTexture when assigning a backing TextureID, and alongside both TextureID assignments in MaterializeTransientResources. Preserve the invariant that each PhysicalTexture entry has either TextureID or Handle set, never both.OloEngine/src/OloEngine/Renderer/RenderGraphTransientPlanner.cpp (1)
127-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHandle
RGResourceHandle::Kind::Texture3Din the transient planner switches.
RGResourceHandle::Kind::Texture3Dis a declared enumerator inOloEngine/src/OloEngine/Renderer/ResourceHandle.h, butIsAllocatablereturnsfalseandGetSkipReasonreturns"unknown-kind"for non-importedTexture3Ddescriptors. Add explicitTexture3Dcases alongside the existing texture kinds so future non-imported volume descriptions classify correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Renderer/RenderGraphTransientPlanner.cpp` around lines 127 - 158, Add explicit RGResourceHandle::Kind::Texture3D handling to both IsAllocatable and GetSkipReason, alongside the existing texture-kind cases. Apply the same volume-texture validation and allocatable classification used for the other texture descriptors, ensuring non-imported Texture3D resources no longer fall through to false or "unknown-kind".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0011-rhi-neutral-resource-and-binding-model.md`:
- Around line 1180-1187: Update the “Two consequences for how part 2 is done”
section to accurately acknowledge all defect categories listed in the table:
include edits, boolean rewrites, type-side mismatches, and identifier
collisions. Revise the claim that defects are only boolean guards or includes
and that none involve types, while preserving the guidance to review
conditionals and adding explicit manual attention to type-side decisions and
target-name collisions.
- Around line 987-994: Synchronize the migration status in both
docs/adr/0011-rhi-neutral-resource-and-binding-model.md lines 987-994 and
docs/agent-rules/rhi-abstraction-boundary.md lines 352-364: reflect the current
post-part-2 SSAO migration status and update sweep_renderer_id from 700 to 699,
or explicitly mark the ADR text as historical pre-part-2 documentation. Keep the
remaining counters and status consistent across both documents.
- Around line 1198-1205: Update the qualifier-fallback description in the ADR
section discussing RGCommandContext.h: remove the claim that an unresolved RHI::
qualifier degraded to unqualified ResourceHandle, and accurately describe the
declaration or using-directive that implicitly brought RHI names into scope if
that was the actual cause. Preserve the surrounding explanation about the
missing RHITypes include and type mismatch.
In `@docs/agent-rules/rhi-abstraction-boundary.md`:
- Around line 517-528: Revise the ADR guidance to distinguish reload
invalidation from collision-only invalidation: because in-place reload preserves
RHI::ResourceHandle while replacing native storage, retain reload-path
invalidation as a correctness requirement for handle-keyed BoundTextureIDs; only
invalidations used solely to prevent delete/create name collisions may be
treated as optional optimisations.
In `@OloEngine/src/OloEngine/Renderer/RenderGraph.cpp`:
- Around line 1707-1711: Update ResolveTextureHandle to follow
m_VersionAliasTargets using the same depth-capped alias-chain traversal as
ResolveTexture, before reading the physical texture handle. Preserve the
existing native-id fallback for entries without an alias and ensure versioned
reads resolve to the backing resource’s handle.
In `@OloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.cpp`:
- Around line 195-211: In the handle lookup seqlock read section, add an
explicit acquire fence after the relaxed Native and Owner loads and before the
second Generation load. Update the code surrounding the Generation, Native, and
Owner accesses without changing the existing stale-handle checks or return
behavior.
In `@OloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.h`:
- Around line 266-278: Update Sync in ResourceRegistry to check IsLive() after
validating m_Handle and before Rebind; when the handle is not live, use the
existing Adopt(kind, nativeHandle, owner) path instead, while preserving the
current null-handle behavior and only rebinding live handles.
In `@OloEngine/src/Platform/OpenGL/OpenGLUtilities.h`:
- Around line 27-30: The OpenGL handle resolver must validate resource kinds
before returning native names. In
OloEngine/src/Platform/OpenGL/OpenGLUtilities.h:27-30, update ResolveNative to
accept an expected RHI::ResourceKind and reject kind or backend mismatches; in
OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp:1845-1878 and :1923-1960,
pass the appropriate kind for every typed bind, delete, and texture operation,
and ensure delete paths skip both raw GL deletion and Unregister when validation
fails.
In `@OloEngine/tests/Rendering/rhi_boundary_baseline.json`:
- Around line 139-147: Update the documented sweep_renderer_id count from 700 to
the enforced baseline value 699, and verify whether the associated file count of
79 still matches the baseline.
---
Outside diff comments:
In `@OloEngine/src/OloEngine/Renderer/RenderGraph.cpp`:
- Around line 2436-2462: Clear the stale PhysicalTexture identity whenever a
texture becomes transient. In the existing-name reuse path of
AllocateTextureHandle, reset the corresponding m_PhysicalTextures entry’s Handle
before returning; do the same in DeclareTransientTexture when assigning a
backing TextureID, and alongside both TextureID assignments in
MaterializeTransientResources. Preserve the invariant that each PhysicalTexture
entry has either TextureID or Handle set, never both.
In `@OloEngine/src/OloEngine/Renderer/RenderGraphTransientPlanner.cpp`:
- Around line 127-158: Add explicit RGResourceHandle::Kind::Texture3D handling
to both IsAllocatable and GetSkipReason, alongside the existing texture-kind
cases. Apply the same volume-texture validation and allocatable classification
used for the other texture descriptors, ensuring non-imported Texture3D
resources no longer fall through to false or "unknown-kind".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b04c59ac-35cd-4239-99f9-130d96273cbb
📒 Files selected for processing (69)
OloEditor/src/MCP/McpToolsRender.cppOloEngine/src/CMakeLists.txtOloEngine/src/OloEngine/Renderer/ComputeShader.hOloEngine/src/OloEngine/Renderer/DDGI/DDGIProbeUpdatePass.cppOloEngine/src/OloEngine/Renderer/Framebuffer.hOloEngine/src/OloEngine/Renderer/Passes/ColorGradingRenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/FluidIntermediatesPass.cppOloEngine/src/OloEngine/Renderer/Passes/SSAORenderPass.cppOloEngine/src/OloEngine/Renderer/Passes/SSAORenderPass.hOloEngine/src/OloEngine/Renderer/Passes/VolumetricFogPass.cppOloEngine/src/OloEngine/Renderer/RGBuilder.cppOloEngine/src/OloEngine/Renderer/RGBuilder.hOloEngine/src/OloEngine/Renderer/RGCommandContext.cppOloEngine/src/OloEngine/Renderer/RGCommandContext.hOloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.cppOloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.hOloEngine/src/OloEngine/Renderer/RHI/RHITypes.hOloEngine/src/OloEngine/Renderer/RenderCommand.hOloEngine/src/OloEngine/Renderer/RenderGraph.cppOloEngine/src/OloEngine/Renderer/RenderGraph.hOloEngine/src/OloEngine/Renderer/RenderGraphResourceRegistry.cppOloEngine/src/OloEngine/Renderer/RenderGraphTransientPlanner.cppOloEngine/src/OloEngine/Renderer/RenderPipeline.cppOloEngine/src/OloEngine/Renderer/RendererAPI.hOloEngine/src/OloEngine/Renderer/ResourceHandle.hOloEngine/src/OloEngine/Renderer/Shader.hOloEngine/src/OloEngine/Renderer/StorageBuffer.hOloEngine/src/OloEngine/Renderer/Texture.hOloEngine/src/OloEngine/Renderer/Texture2DArray.hOloEngine/src/OloEngine/Renderer/Texture3D.hOloEngine/src/OloEngine/Renderer/UniformBuffer.hOloEngine/src/OloEngine/Renderer/VertexArray.hOloEngine/src/OloEngine/Renderer/VirtualGeometry/VirtualGeometryPass.cppOloEngine/src/Platform/OpenGL/OpenGLComputeShader.cppOloEngine/src/Platform/OpenGL/OpenGLComputeShader.hOloEngine/src/Platform/OpenGL/OpenGLFramebuffer.cppOloEngine/src/Platform/OpenGL/OpenGLFramebuffer.hOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cppOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.hOloEngine/src/Platform/OpenGL/OpenGLShader.cppOloEngine/src/Platform/OpenGL/OpenGLShader.hOloEngine/src/Platform/OpenGL/OpenGLStorageBuffer.cppOloEngine/src/Platform/OpenGL/OpenGLStorageBuffer.hOloEngine/src/Platform/OpenGL/OpenGLTexture.cppOloEngine/src/Platform/OpenGL/OpenGLTexture.hOloEngine/src/Platform/OpenGL/OpenGLTexture2DArray.cppOloEngine/src/Platform/OpenGL/OpenGLTexture2DArray.hOloEngine/src/Platform/OpenGL/OpenGLTexture3D.cppOloEngine/src/Platform/OpenGL/OpenGLTexture3D.hOloEngine/src/Platform/OpenGL/OpenGLTextureCubemap.cppOloEngine/src/Platform/OpenGL/OpenGLTextureCubemap.hOloEngine/src/Platform/OpenGL/OpenGLUniformBuffer.cppOloEngine/src/Platform/OpenGL/OpenGLUniformBuffer.hOloEngine/src/Platform/OpenGL/OpenGLUtilities.hOloEngine/src/Platform/OpenGL/OpenGLVertexArray.cppOloEngine/src/Platform/OpenGL/OpenGLVertexArray.hOloEngine/tests/CMakeLists.txtOloEngine/tests/MCP/McpShaderReloadTest.cppOloEngine/tests/Rendering/MockRendererAPI.hOloEngine/tests/Rendering/PropertyTests/RHIHandleNativeIdentityTest.cppOloEngine/tests/Rendering/RHIBoundaryRatchetTest.cppOloEngine/tests/Rendering/RHIResourceRegistryTest.cppOloEngine/tests/Rendering/RenderGraphTest.cppOloEngine/tests/Rendering/ResourceHazardValidationTests.cppOloEngine/tests/Rendering/TestDeclarativeNode.hOloEngine/tests/Rendering/rhi_boundary_baseline.jsoncmake/vendor/OpenUSD.cmakedocs/adr/0011-rhi-neutral-resource-and-binding-model.mddocs/agent-rules/rhi-abstraction-boundary.md
Comment on lines
+1180
to
+1187
| **Two consequences for how part 2 is done:** | ||
|
|
||
| 1. **Subsystem at a time, with a readable diff.** Not because it is faster — it | ||
| is not — but because the failure mode is a change that compiles and passes | ||
| tests. Review has to be able to see it. | ||
| 2. **Review the conditionals first.** Every defect above is either a boolean | ||
| guard or an include; none is a type. The type changes are the part the | ||
| compiler already checked. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the defect taxonomy.
The table includes include edits, boolean rewrites, type-side mismatches, and identifier collisions. The next paragraph says every defect is a boolean guard or an include and that none is a type. This can cause reviewers to skip the required manual checks for type-side decisions and target-name collisions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0011-rhi-neutral-resource-and-binding-model.md` around lines 1180 -
1187, Update the “Two consequences for how part 2 is done” section to accurately
acknowledge all defect categories listed in the table: include edits, boolean
rewrites, type-side mismatches, and identifier collisions. Revise the claim that
defects are only boolean guards or includes and that none involve types, while
preserving the guidance to review conditionals and adding explicit manual
attention to type-side decisions and target-name collisions.
Comment on lines
+1198
to
+1205
| During step 3, `RGCommandContext.h` declared a member returning | ||
| `RHI::ResourceHandle` without including `RHITypes.h`. The compiler did not report | ||
| an unknown type: `RHI::` was unresolvable, so the name degraded to the | ||
| unqualified `ResourceHandle` — which *was* in scope and *was* a valid type, the | ||
| graph's string-named resource handle. A completely different concept silently | ||
| substituted for the intended one. It surfaced only because a separately-compiled | ||
| definition disagreed about the return type; in a header-only path (an inline | ||
| function, or a member defined in the same header) it would have compiled. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'namespace RHI|RHI::ResourceHandle|using .*ResourceHandle|struct ResourceHandle|class ResourceHandle' \
OloEngine/src/OloEngine/Renderer/RGCommandContext.h \
OloEngine/src/OloEngine/Renderer/RHI/RHITypes.h \
OloEngine/src/OloEngine/Renderer/ResourceHandle.hRepository: drsnuggles8/OloEngineBase
Length of output: 6765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)docs/adr/0011-rhi-neutral-resource-and-binding-model\.md$|OloEngine/src/OloEngine/Renderer/(RGCommandContext|ResourceHandle|RHI/RHITypes)\.h$|OloEngine/src/OloEngine/Renderer/.*ResourceHandle.*\.h$'
echo
echo "== ResourceHandle.h =="
wc -l OloEngine/src/OloEngine/Renderer/ResourceHandle.h
cat -n OloEngine/src/OloEngine/Renderer/ResourceHandle.h
echo
echo "== RGCommandContext.h includes/header scope =="
sed -n '1,110p' OloEngine/src/OloEngine/Renderer/RGCommandContext.h | cat -n
echo
echo "== search declarations of plain ResourceHandle =="
rg -n -C 3 '^\s*(?:class|struct|using|typedef).+\bResourceHandle\b|enum_class\s+\w+ {.*ResourceHandle|ResourceHandle;' OloEngine/src/OloEngine/Renderer OloEngine/src/OloEngine include docs/adr/0011-rhi-neutral-resource-and-binding-model.md || true
echo
echo "== context around ADR lines 1180-1215 =="
sed -n '1180,1215p' docs/adr/0011-rhi-neutral-resource-and-binding-model.md | cat -n
echo
echo "== context around ResourceHandle.h lines 1-80 =="
sed -n '1,80p' OloEngine/src/OloEngine/Renderer/ResourceHandle.h | cat -nRepository: drsnuggles8/OloEngineBase
Length of output: 41993
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search declarations/usages of unqualified ResourceHandle around Renderer include paths =="
rg -n -C 3 '\b(ResourceHandle|namespace\s+RHI|using namespace|RHI::ResourceHandle)' \
OloEngine/src/OloEngine/Core/Base.h \
OloEngine/src/OloEngine/Core/GlobalData.h \
OloEngine/src/OloEngine/Renderer \
OloEngine/src/OloEngine -g '*.h' -g '*.cpp' || true
echo
echo "== RGCommandContext implementation files mentioning Return/ResolveTextureHandle =="
rg -n -C 4 'ResolveTextureHandle|RHI::ResourceHandle|RGCommandContext::' \
OloEngine/src/OloEngine/Renderer -g '*.h' -g '*.cpp' || true
echo
echo "== Base.h relevant prelude =="
sed -n '1,120p' OloEngine/src/OloEngine/Core/Base.h | cat -n
echo
echo "== deterministic C++ qualified-lookup probe =="
if command -v g++ >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
cat > "$tmpdir/qualified_lookup_probe.cc" <<'CPP'
struct ResourceHandle {};
class C
{
public:
return_type RHI::ResourceHandle f();
};
return_type RHI::ResourceHandle g();
int main() {}
CPP
g++ -std=c++17 -fsyntax-only "$tmpdir/qualified_lookup_probe.cc" 2>&1 || true
else
echo "g++ not available"
fiRepository: drsnuggles8/OloEngineBase
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
search_term=$(
rg -n '^using namespace\s+.*RHI\b|^`#define`\s+RHI\b|^struct\s+ResourceHandle\b|^class\s+ResourceHandle\b|^using\s+ResourceHandle|namespace\s+RHI\b' \
OloEngine/src/OloEngine/Core/Base.h 2>/dev/null || true
)
echo "== candidate lines in OloEngine/src/OloEngine/Core/Base.h =="
if [ -n "${search_term##$'\n'}" ]; then
echo "$search_term" | sed -n '1,40p' | cut -d: -f1 | sort -u | xargs -r -I{} sh -c 'echo "-- {} --"; sed -n "$(({} > 0 ? {}-5 : 1)),$(({}+25))p" OloEngine/src/OloEngine/Core/Base.h | cat -n'
else
echo "no RHI-using/ResourceHandle matching lines found in Base.h"
fi
echo
echo "== focused declaration search in OloEngine/src/OloEngine/Core/Base.h =="
rg -n -C 3 '\bResourceHandle\b|namespace\s+RHI\b|using namespace\s+.*RHI\b' OloEngine/src/OloEngine/Core/Base.h || true
echo
echo "== focused declaration search in OloEngine/src/OloEngine/Core ** and OloEngine/src/OloEngine/Renderer ** =="
rg -n -C 3 '^(\s*using namespace\s+).*RHI\b|^\s*struct\s+ResourceHandle\b|^\s*class\s+ResourceHandle\b|^\s*using\s+ResourceHandle\b|namespace\s+RHI\b' \
OloEngine/src/OloEngine/Core OloEngine/src/OloEngine/Renderer | head -n 300 || trueRepository: drsnuggles8/OloEngineBase
Length of output: 1066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - '\
from pathlib import Path\nimport re\nfor path in Path(\"OloEngine/src/OloEngine\").rglob(\"*.h\"):\n text = path.read_text(encoding=\"utf-8\", errors=\"ignore\")\n for m in re.finditer(r\"\\b(?:class|struct)\\s+(ResourceHandle|RHI)\\b|\\bnamespace\\s+(RHI|OloEngine)\\b|\\busing\\s+namespace\\b|\\busing\\s+ResourceHandle\\b\", text):\n line = text.count(\"\\n\",0,m.start())+1\n print(f\"{path}:{line}:{m.group(0)}\")\n' 2>&1 | rg -n '\b(ResourceHandle|RHI|using namespace)\b' | head -n 400 || true
tmpdir="$(mktemp -d)"
cat > "$tmpdir/qualified_lookup_probe.cc" <<'CPP'
struct ResourceHandle {};
class C
{
public:
return_type RHI::ResourceHandle f();
};
return_type RHI::ResourceHandle g();
int main() {}
CPP
g++ -std=c++17 -fsyntax-only "$tmpdir/qualified_lookup_probe.cc" 2>&1 || trueRepository: drsnuggles8/OloEngineBase
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
query = ["ResourceHandle", "RHI", "using namespace"]
for path in Path("OloEngine/src/OloEngine").rglob("*"):
if not path.is_file() or path.suffix not in (".h", ".cpp"):
continue
text = path.read_text(encoding="utf-8", errors="ignore")
for m in re.finditer(r"\b(?:class|struct)\s+ResourceHandle\b|namespace\s+RHI\b|using\s+namespace\b|using\s+ResourceHandle\b", text):
line_no = text.count("\n", 0, m.start()) + 1
line = text.splitlines()[line_no - 1]
if any(q in m.group(0) or q in line for q in query):
print(f"{path}:{line_no}:{line}")
PY
tmpdir="$(mktemp -d)"
cat > "$tmpdir/qualified_lookup_probe.cc" <<'CPP'
struct ResourceHandle {};
class C
{
public:
return_type RHI::ResourceHandle f();
};
return_type RHI::ResourceHandle g();
int main() {}
CPP
g++ -std=c++17 -fsyntax-only "$tmpdir/qualified_lookup_probe.cc" 2>&1 || trueRepository: drsnuggles8/OloEngineBase
Length of output: 2124
Fix the qualifier-fallback description.
RHI::ResourceHandle cannot degrade to unqualified ResourceHandle; an undeclared RHI is not ignored by qualified lookup. If the original symptom came from a missing RHITypes include plus an implicit RHI bring-in, describe that declaration/using-directive instead of RHI::.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0011-rhi-neutral-resource-and-binding-model.md` around lines 1198 -
1205, Update the qualifier-fallback description in the ADR section discussing
RGCommandContext.h: remove the claim that an unresolved RHI:: qualifier degraded
to unqualified ResourceHandle, and accurately describe the declaration or
using-directive that implicitly brought RHI names into scope if that was the
actual cause. Preserve the surrounding explanation about the missing RHITypes
include and type mismatch.
Comment on lines
+517
to
+528
| The reason it is worth doing rather than deferring: **this cache has already | ||
| caused a real, debugged visual bug of exactly the kind the identity currency | ||
| prevents.** The comments on `InvalidateTextureSlot` / `InvalidateTextureBinding` | ||
| record it — `VirtualGeometryPass` binds the Hi-Z pyramid to unit 0 for its cull | ||
| compute, unit 0 is also `u_AlbedoMap`, and "any material whose albedo ID matched | ||
| the stale cache entry silently sampled the HZB depth texture as its albedo." | ||
| The current fix is manual invalidation that every future raw-GL binder must | ||
| remember to call. Keyed on identities the stale entry cannot collide, so the | ||
| invalidation calls stop being load-bearing correctness and become a pure | ||
| optimisation. That is a genuine behavioural win, not just a type change — say so | ||
| in the commit, and keep the invalidation calls (they still avoid redundant | ||
| binds) rather than deleting them as "no longer needed". |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep reload invalidation as a correctness requirement.
The ADR states that in-place reload preserves RHI::ResourceHandle. A handle-keyed BoundTextureIDs entry can therefore still suppress a required rebind after native storage replacement. Handle identity removes delete/create name collisions only. Keep invalidation on the reload path as load-bearing; only collision-only invalidations can become optional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/agent-rules/rhi-abstraction-boundary.md` around lines 517 - 528, Revise
the ADR guidance to distinguish reload invalidation from collision-only
invalidation: because in-place reload preserves RHI::ResourceHandle while
replacing native storage, retain reload-path invalidation as a correctness
requirement for handle-keyed BoundTextureIDs; only invalidations used solely to
prevent delete/create name collisions may be treated as optional optimisations.
Two real defects, two hardening changes, and a documentation correction. ResolveTextureHandle did not follow m_VersionAliasTargets. A WriteNewVersion rename is bookkeeping over the same physical resource — the version's own slot carries no backing, so the identity lives on the base. ResolveTexture walks that chain; the handle resolver fell through instead and returned the null handle. Any migrated resource in a read-modify-write chain would therefore resolve to null on every versioned read, which the header documents as meaning "imported as a native id" — two causes, indistinguishable at the call site. Latent today because nothing migrated is versioned yet, which is exactly why it was worth fixing before the next slice inherits it. Mirrors the native resolver's iterative, depth-capped walk (a malformed alias map must not recurse the stack to death). Seqlock read was missing an acquire fence. The `before` acquire load stops later reads floating above it, but an acquire LOAD does not stop earlier reads from sinking below it — so the relaxed Native/Owner reads could be reordered past the `after` load, and the tearing check would then validate a generation pair while returning a payload read from outside it. std::atomic_thread_fence(acquire) between the payload and the second load supplies the half the two acquire loads cannot express. ScopedResourceHandle::Sync gated on m_Handle.IsValid(), which is a STRUCTURAL test on the handle's own bits and says nothing about whether the registry still holds the entry. After a ResourceRegistry::Clear() every handle still reads as valid while its slot is gone, so Rebind would write into a slot that may since have been handed to another tenant. Now asks the registry (IsLive) and re-adopts when the entry is no longer ours. Backend resolution is now kind-checked. GL object names are per-type, so a texture and a buffer can both legitimately be name 1 — a handle handed to the wrong family resolves to a real, valid, entirely unrelated object, and the generation cannot catch it because BOTH handles are live. Adds ResourceRegistry::GetKind (same seqlock discipline as the native resolve) and Utils::ResolveNativeAs, and routes all 14 typed call sites in the GL backend through it. A mismatch returns 0, landing on the same benign degradation the unchecked form already documents: binding 0 unbinds, glDelete*(0) is a no-op. Also extracts ImportTextureCommon so ImportTexture / ImportTextureHandle share one implementation again. Threading the identity into AllocateTextureHandle had left ImportTextureHandle duplicating the import setup directly beneath a comment explaining why it must not — a comment that contradicts its code is its own defect. The shared helper restores one home for the import rules while keeping the property that mattered: the identity arrives as a parameter, visible to the change detection, rather than being stamped on afterwards. Documentation: the ADR's "nothing is converted onto it yet" paragraph is marked as dating from part 1 rather than silently rewritten, and both it and rhi-abstraction-boundary.md now state the caveat that matters more than the number — sweep_renderer_id matches the ACCESSOR NAME `RendererID`, not the currency, so it is a regression ratchet and not a burn-down (the first fully migrated resource moved it by 0). facade_native_id_params reaching 0 is the completion criterion. Baseline prose synced to 699. Verified: 5226 tests ran, 5222 passed, 0 failed, 4 environmental skips — two fewer than before because building OloEditor (which depends on OloRuntime) let both AppLaunchSmoke launch tests execute instead of skipping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…add -A The suite run that verified the CodeRabbit fixes regenerated ten evidence PNGs and `git add -A` took them along. They are the same run-to-run variance measured during slice 5's verification: diffing two runs of the IDENTICAL binary already showed these files moving by up to 158/255 (VirtualGeometry_Debug_ClusterId) and 80/255 (Fluid_Waterline) with no code change at all, which is why they were reverted rather than committed then. Committing them would put pure noise in the PR diff and imply a visual change that did not happen. Restored to their previous state; the evidence set gets regenerated deliberately after a real visual change, as in 41bbdf9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Worked all 9 findings — 8 applied, 1 applied more broadly than suggested. Pushed in Two were real defects. Thank you — both were latent rather than active, which is exactly when they're cheapest to fix.
Hardening, both accepted:
Documentation findings — all fair, and they caught prose my own commits made stale. Rather than just bump 700 → 699, I marked the ADR paragraph as dating from part 1 (it accurately describes what part 1 shipped) and added the caveat that matters more than the number: Verification: 5226 tests ran, 5222 passed, 0 failed, 4 environmental skips — two fewer than the pre-review runs, because building One deliberate non-change: |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…691) Three slices in a row were scoped without first measuring where the target lives, and all three missed: by resource (SSAO — migrated correctly end-to-end and moved the counter by 0, having five native-id call sites and zero `RendererID` spellings), by pass (HZBGenerator — returns a pass-through of an unmigrated input, so converting it would have silently broken GTAO while compiling green), and by struct (PODMaterialData — cannot migrate alone, its textures bind through a cache shared with four shadow paths, snow, cloud shadow and the UBO/VAO/shader state). So the handoff is a distribution and an ordered worklist rather than prose: * Where the 1196 mentions actually are, including that Platform/OpenGL's 412 are exempt and that `GetRendererID` (325) — not `m_RendererID` (439, mostly backend-internal) — is the consumer-side target. * A suggested order, each item a buildable commit, starting with the 72 attachment consumers because their producer already ships and they need no new facade surface. * The two producer gaps that block their consumers (VertexBuffer/IndexBuffer expose no GetRHIHandle; ShadowMap's CSM/Atlas accessors need handle siblings). Adds OloEngine/tests/scripts/measure_rendererid.py so the distribution is reproducible instead of a number in a document that goes stale — the by-spelling breakdown is the part RHIBoundaryRatchetTest does not give you, and it is the part that identifies the real target. It is an analysis aid, not a test; the ratchet remains authoritative for the counter itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
OloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.cpp (1)
259-269: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
KindOfcurrent withGetKind’s serialization.
KindOfstill has callers, includingRHIResourceRegistryTest. It usesIsLive(handle)once, then readsslot->Kindwithout the generation check insideGetKind, so a concurrentUnregister/Registeron the same slot can make the accessor return the new tenant’s kind. Apply the same seqlock pattern here, or replace these callers withGetKindand remove the duplicate API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.cpp` around lines 259 - 269, Update ResourceRegistry::KindOf to use the same seqlock/generation-validated serialization as GetKind, preventing concurrent Unregister/Register operations from returning a reused slot’s kind; alternatively, migrate its callers, including RHIResourceRegistryTest, to GetKind and remove the duplicate API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cpp`:
- Around line 1923-1945: Guard the registry cleanup in
OpenGLRendererAPI::DeleteTexture, DeleteFramebuffer, DeleteBuffer, and
DeleteVertexArray with the corresponding ResourceKind validation before calling
Unregister. Ensure wrong-kind handles return without unregistering, while valid
handles still resolve, delete the native resource, and unregister normally.
In `@OloEngine/src/Platform/OpenGL/OpenGLUtilities.h`:
- Around line 41-51: Update ResolveNativeAs to store the result of
registry.GetKind(handle), and only emit the existing kind-mismatch warning when
that result is neither expected nor RHI::ResourceKind::Unknown. Preserve the
current return behavior so stale handles remain silent and real live-kind
mismatches still return 0.
---
Outside diff comments:
In `@OloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.cpp`:
- Around line 259-269: Update ResourceRegistry::KindOf to use the same
seqlock/generation-validated serialization as GetKind, preventing concurrent
Unregister/Register operations from returning a reused slot’s kind;
alternatively, migrate its callers, including RHIResourceRegistryTest, to
GetKind and remove the duplicate API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb18f249-6af1-404c-98eb-2b23aa29c9ef
📒 Files selected for processing (9)
OloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.cppOloEngine/src/OloEngine/Renderer/RHI/RHIResourceRegistry.hOloEngine/src/OloEngine/Renderer/RenderGraph.cppOloEngine/src/OloEngine/Renderer/RenderGraph.hOloEngine/src/Platform/OpenGL/OpenGLRendererAPI.cppOloEngine/src/Platform/OpenGL/OpenGLUtilities.hOloEngine/tests/Rendering/rhi_boundary_baseline.jsondocs/adr/0011-rhi-neutral-resource-and-binding-model.mddocs/agent-rules/rhi-abstraction-boundary.md
…existed (#691) Three findings from the second review pass, all still valid against the code. DELETE WITH A WRONG-KIND HANDLE RETIRED SOMEONE ELSE'S REGISTRY ENTRY. A regression introduced by the kind-checking change itself: ResolveNativeAs correctly refused to hand back the wrong family's GL name, but the Unregister that follows is not kind-aware, so the delete left the other resource's GL object alive while killing its identity. That is worse than the unchecked form it replaced — the two halves disagreed. All four Delete* handle overloads now skip both halves together. Pinned by RHIHandleNativeIdentity.DeletingWithAWrongKindHandleTouchesNeitherTheObjectNorTheRegistry, which hands a buffer to DeleteTexture (GL names are per-type, so the buffer's name is very likely also a live texture's) and asserts the buffer survives intact — then that the correctly-typed delete still retires it, since a guard that makes the wrong case safe by breaking the right case would pass the first half and be useless. THE MISMATCH WARNING FIRED ON STALE HANDLES. KindOf reports Unknown for a stale handle, which is != expected, so every legitimate use-after-free degradation — the documented benign path, where resolving to 0 means binding 0 unbinds — would have logged and buried the real signal. Both this and the guard above now share one predicate, Utils::IsWrongKind, true only for a LIVE handle of another family. GetKind WAS A DUPLICATE OF THE EXISTING KindOf. Mine should never have been written: KindOf was already there with 8 test callers, and the header's own thread-safety comment names it. Deleted, callers left alone. KindOf needed hardening regardless — its body was IsLive() followed by a relaxed read, a TOCTOU where the slot can be Unregistered and Registered to a new tenant between the check and the load, so a stale handle would be told the NEW occupant's kind. It now uses the same seqlock discipline (generation read either side of the payload, acquire fence between) as ResolveTaggedForBackend. Verified: 5227 tests ran, 5223 passed, 0 failed, 4 environmental skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Second review pass: all 3 findings verified as still valid and fixed — none skipped. Pushed in 1. Pinned by a new test that hands a buffer to 2. Warning fired on stale handles. Confirmed. 3. Taking the second option in the finding (harden the existing one) rather than migrating callers, because Verified: 5227 tests ran, 5223 passed, 0 failed, 4 environmental skips. |
|
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.
Part of #691 Phase 2 step 3 (ADR 0011 §1.1/§1.3/§1.4): replace the bare
u32 RendererIDcurrency with a generation-checkedRHI::ResourceHandle.#691's Phase 2 checkbox stays UNTICKED and #691 stays open. This is the mint plus the first real call-site migrations, not the completed step.
Why the generation matters
GL recycles object names.
Texture::operator==comparesGetRendererID(), so two different objects can compare equal. That is not hypothetical here —CommandDispatch's own comments record a debugged production bug of exactly this shape:VirtualGeometryPassbinds the Hi-Z pyramid to texture unit 0 for its cull compute, unit 0 is alsou_AlbedoMap, and any material whose albedo GL name matched the stale bind-cache entry silently sampled HZB depth as its albedo.What's here
c9b48f29RHI::ResourceRegistry(lock-free chunked slots, seqlock-style generation read),ScopedResourceHandle, and theGetNativeHandleForDebugthat Phase 1 declared but never defined. 3 new ratchet counters, ADR amendments (11)–(17).153e5617GetRHIHandle()alongsideGetRendererID()— 9 interfaces, 10 backend classes, 26Sync()sites.127a17da81264f14e8816d76cf3c6612RenderGraphcarries both currencies as alternatives, never both.fc30e44cResourceHandle→RGResourceHandleafter a near-miss where a missing include letRHI::ResourceHandlesilently degrade to a different in-scope type.923c5276f914bfcb046527c0The migration is ordered by dataflow, not by size
handle -> nativeis a registry lookup;native -> handleis not recoverable. So a symbol can only take handles once every producer feeding it hands out handles. Two slice attempts were made and reverted for violating this — convertingHZBGenerator's output alone would have madeGetHZBTextureID()return null whenever an external HZB was set, silently breaking GTAO while compiling green.That is also why SSAO moves as a whole chain in one commit (create → configure → import → bind → delete) rather than layer by layer.
Two defects found and fixed en route
Sync()retired a live handle on in-place reload. A texture hot-reload transiently reports native id 0; the first implementation treated that as a release. Caught byRHIHandleNativeIdentity.HandleSurvivesInPlaceReloadAndTracksTheNewGLName.ImportTextureHandledelegated toImportTexture(name, 0u, …)and stamped the identity on afterwards, soAllocateTextureHandle's change test compared0 != 0forever — re-importing a name with a different texture kept the slot generation and every cachedRGTextureHandleresolved to the new occupant. The recycled-name hazard, recreated inside the fix for it. Identity is now threaded in as a defaulted parameter so one change test covers both currencies. Pinned in both directions by two newRenderGraphtests.Neither was reachable from the migrated pass (SSAO creates its noise texture once and never recreates it) — both were found by reading, which is worth knowing for the remaining slices.
Verification
OLOENGINE_GOLDEN_REBASEunset.OloEngine.log.Ratchet
sweep_renderer_id700 → 699, measured and locked into the baseline.facade_native_id_paramsstays 68 — correct, since this adds handle overloads without deleting any u32 form; that counter only falls as each u32 form loses its last caller.sweep_gl_callsandbackend_resolve_hatchremain 0.Worth stating plainly:
sweep_renderer_idmatches the accessor nameRendererID, not the currency, so it is a poor progress meter for this work and a regression ratchet rather than a burn-down.Also included: an unrelated build fix
f914bfcbfixescmake/vendor/OpenUSD.cmakenever passing-DCMAKE_INSTALL_PREFIX.cmake --install --prefixonly redirects relative destinations, so USD bakedC:/Program Files (x86)/usdinto the absolute install rule forpxrConfig.cmakeand the install died without elevation. It only fires on a cache miss, and USD builds once per machine, so it lay dormant until a cache wipe — then surfaced asLNK1181on a missinginstall/Debug/lib/usd_m.lib, because the Release failure aborted the step before Debug installed. Unrelated to #691 but it blocked all work here.What's next (not in this PR)
046527c0documents it: theRenderer/Commands/bind cache is one indivisible unit — material textures, four shadow paths, snow and cloud shadow all key the sameBoundTextureIDsarray, soPODMaterialDatacannot migrate alone. Every producer involved already mints, so it's unblocked, just large (~180 errors from the field-type change). That migration is what turns theInvalidateTextureSlot/InvalidateTextureBindingworkaround above from load-bearing correctness into a pure optimisation.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests