Skip to content

Add SceneDepthReader, reading a camera's scene depth back to the CPU - #9274

Merged
mvaligursky merged 14 commits into
mainfrom
mv-gsplat-scene-depth
Sep 2, 2026
Merged

Add SceneDepthReader, reading a camera's scene depth back to the CPU#9274
mvaligursky merged 14 commits into
mainfrom
mv-gsplat-scene-depth

Conversation

@mvaligursky

@mvaligursky mvaligursky commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

The depth a camera renders for its effects is only reachable from shaders. SceneDepthReader renders a requested region of it through the same chunk the effects sample it with, and reads that back, so an application can use it too:

const reader = new pc.SceneDepthReader(camera.camera);
const rect = new pc.Vec4(0.45, 0.45, 0.1, 0.1);

app.on('update', () => {
    reader.read(rect, 8, 8)?.then((samples) => {
        const hit = samples.filter(Number.isFinite);
        console.log(hit.length ? Math.min(...hit) : 'nothing in view');
    });
});

Reads are asynchronous and land a frame or two later, and any number may be in flight, so one can be issued every frame without waiting. A sample is the distance from the camera to the surface at that point, in world units, measured along the camera's view direction. Samples where nothing was rendered read as Infinity, as do the few which land within a hair of the far clip, that being the depth an empty pixel reports.

Public API

One new export, marked @alpha:

export class SceneDepthReader {
    constructor(camera: CameraComponent);
    read(rect: Vec4, width: number, height: number, target?: Float32Array): Promise<Float32Array> | null;
    destroy(): void;
}

Nothing else is exposed - the per-camera depth record on Camera, the new CameraShaderParams flag and the ShaderUtils helpers are all @ignore (and Camera and CameraShaderParams are @ignore as classes).

Why the camera records its own depth

The uniform holding the scene depth is global, so with more than one camera rendering a depth the last one to render owns it. Anything wanting a particular camera's depth therefore has to read it from that camera, which is what Camera#publishSceneDepthMap is for. Both producers - the prepass and the scene pass - publish through it.

The scene depth is now accumulated in reciprocal space

Previously the scene pass stored a coverage weighted mean of the depths with the background pinned at the far clip. That returns a depth no surface is at wherever coverage is partial: at a far clip of 1000, a half covered pixel over a surface at 5 units stored 502, and a 90% covered one stored 145. In a splat capture that is every silhouette, and the depth of field turns those into visible artefacts.

Averaging the reciprocals instead keeps the background's contribution proportionate - its reciprocal is small - so a thinly covered pixel falls off smoothly toward the background rather than being dragged most of the way out to it. The same pixels now read 95 and 55.

The write became:

pcFragColor{SLOT} = vec4(alpha / max(linearDepth, 1e-6), 0.0, 0.0, alpha);

Premultiplied blending makes the coverage weights sum to one on their own, so this needs no second channel and the attachment stays single channel. The clear is 1 / farClip, which makes the background a surface at that distance picking up whatever coverage the splats leave over, and a pixel nothing covered still reports the far clip.

Consumers are unaffected: the volumetric fog, depth aware blur, DOF circle of confusion, SSAO, TAA resolve, compose, soft particles and the reader all go through getLinearScreenDepth, which inverts the average. The encoding is declared by whichever pass produced the map through a new sceneDepthMapReciprocal shader parameter, so the prepass keeps writing the depth outright and its consumers keep decoding it that way. The shader key distinguishes the two so no variant is shared across encodings.

Two consequences of that encoding, both deliberate

A camera which needs the prepass does not get the scene texture depth. The two producers publish to the same uniform and the shaders sampling it are generated once, from a single declaration of the encoding - and they no longer store it the same way. So they cannot coexist, and the prepass wins, as it is the one the materials rendering in the scene pass sample. This widens a restriction which already existed for the packed RGBA8 case. A debug build names the reason, and the comment above it records what lifting it would take: a uniform of its own for the passes which consume the depth after the scene pass, selected per consuming pass rather than per camera.

A half float scene depth is accurate over a camera range of about 0.000015 to 16384. What has to be representable is one over the distance, not the distance itself, and a half float carries that well while it stays normal. Past the far end of the range a distant depth loses accuracy and a pixel nothing covers stops reading as far away as it is, so an uncovered sample can come back as a large distance rather than Infinity.

Nothing refuses the scene depth on that basis, deliberately. A camera turned away from it falls back to the prepass, and a scene of splats and nothing else has no opaque geometry for the prepass to render - the depth comes out empty and the fog and the depth of field end up bounded by nothing at all, which is a worse outcome than a bounded inaccuracy at the far end, on exactly the scenes this is for. The range is documented on GSplatParams#sceneDepthWrite, where an application decides whether to turn the splat depth on, and on SceneDepthReader#read, whose promise about uncovered samples is what stops holding. What to do about it is left to the application, which may know its far clip is nowhere near the limit, or would rather leave the depth effects off on such a device.

Nothing watches the clip planes for it either. Which producer renders the depth is settled when the camera frame is built, and rebuilding that recreates every pass which reads the depth - each of which resolves its decode defines in its constructor, so it recompiles those shaders. CameraFrame applies changes when update() is called rather than by watching state.

Examples

  • gaussian-splatting/depth-effects (new) - volumetric fog and depth of field over a Gaussian splat capture with no proxy geometry of any kind, both reading a depth the splats write themselves. Autofocus reads the depth through SceneDepthReader.
  • test/scene-depth-read (new) - reads back the depth of three boxes at known distances and a patch of empty space, once for each encoding the depth can arrive in: the grab pass, the scene textures and the prepass. The encoding which actually turned up is checked against the one each phase set out to reach, so a device falling back to another cannot read as a pass for a case which never ran.

Notes for reviewers

Anyone sampling uSceneDepthMap directly in a custom shader, rather than calling getLinearScreenDepth, will need to invert what they read. The supported path is unaffected.

A known limitation, visible in the new example: where splats thin out, the depth fades toward the far clip, and a focus distance landing inside that fade puts a thin in focus contour through an otherwise blurred background - the depth crosses the whole focus range within a pixel or two there. The example caps how far its autofocus will focus and colours its reticle when that cap is hit. Fixing it properly means letting the circle of confusion weight by coverage rather than trusting the depth alone, which is a change to the DOF pass and is not attempted here.

A read is answered when the camera it reads finishes a frame. Disabling that camera after issuing one therefore leaves it unanswered until the camera renders again, and destroy() settles anything still queued.

Testing

Verified on WebGPU and WebGL2. test/scene-depth-read passes on both across all three encodings, reading 10.00, 15.00 and 20.00 for the three boxes and infinity for the empty patch in each. The new example was checked with the scene depth and DOF circle of confusion debug views alongside the composed frame.

🤖 Generated with Claude Code

The depth a camera renders for its effects is only available to shaders.
SceneDepthReader renders a requested region of it through the same chunk
the effects sample it with and reads the result back, so an application
can use it too - autofocus being the obvious case. Reads are asynchronous
and any number may be in flight, so one can be issued every frame.

The depth is recorded on the camera as it is published, because the
uniform holding it is global: with more than one camera rendering a depth,
the last to render owns that uniform, so anything wanting a particular
camera's depth has to read it from the camera instead.

The scene pass now accumulates the depth in reciprocal space. Previously
it stored a coverage weighted mean of the depths with the background
pinned at the far clip, which returns a depth no surface is at wherever
coverage is partial - a half covered pixel over a surface at 5 units with
a far clip of 1000 stored 502. Averaging the reciprocals instead keeps the
background's contribution small, so such a pixel falls off toward the
background smoothly rather than being dragged out to it. Consumers all go
through getLinearScreenDepth, which inverts the average; the encoding is
declared by the producer through a new shader parameter, so the prepass
keeps writing the depth outright.

Adds two examples: gaussian-splatting/depth-effects, where volumetric fog
and depth of field both read a depth the splats write themselves with no
proxy geometry, and test/scene-depth-read, which checks the read against
boxes at known distances.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Public API report

This PR changes the public API surface (+4 / −0), per the docs' rules (@ignore / @Private / undocumented are excluded).

Show API diff
+SceneDepthReader.constructor(camera: CameraComponent)
+SceneDepthReader.destroy(): void
+SceneDepthReader.read(rect: Vec4, width: number, height: number, target?: Float32Array<ArrayBufferLike>): Promise<Float32Array<ArrayBufferLike>> | null
+class SceneDepthReader

Informational only — this never fails the build.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Build size report

This PR changes the size of the minified bundles.

Bundle Minified Gzip Brotli
playcanvas.min.js 2398.1 KB (+6.8 KB, +0.28%) 616.9 KB (+1.9 KB, +0.31%) 478.5 KB (+1.3 KB, +0.27%)
playcanvas.min.mjs 2395.5 KB (+6.8 KB, +0.28%) 615.5 KB (+1.8 KB, +0.30%) 478.0 KB (+1.5 KB, +0.31%)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds CPU-accessible per-camera scene-depth readback and reciprocal depth accumulation for splat rendering.

Changes:

  • Adds the alpha SceneDepthReader API with asynchronous GPU readback.
  • Introduces reciprocal depth encoding across GLSL/WGSL.
  • Adds functional and Gaussian-splat depth-effects examples.

Reviewed changes

Copilot reviewed 20 out of 24 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/scene/shader-lib/wgsl/chunks/render-pass/frag/scene-depth-read.js Adds WGSL depth readback shader.
src/scene/shader-lib/wgsl/chunks/gsplat/frag/gsplat.js Updates depth accumulation commentary.
src/scene/shader-lib/wgsl/chunks/common/frag/screenDepth.js Decodes reciprocal WGSL depth.
src/scene/shader-lib/wgsl/chunks/common/frag/scene-textures.js Writes reciprocal WGSL depth.
src/scene/shader-lib/shader-utils.js Adds reciprocal shader variants.
src/scene/shader-lib/glsl/chunks/render-pass/frag/scene-depth-read.js Adds GLSL depth readback shader.
src/scene/shader-lib/glsl/chunks/gsplat/frag/gsplat.js Updates depth accumulation commentary.
src/scene/shader-lib/glsl/chunks/common/frag/screenDepth.js Decodes reciprocal GLSL depth.
src/scene/shader-lib/glsl/chunks/common/frag/scene-textures.js Writes reciprocal GLSL depth.
src/scene/renderer/render-pass-forward.js Publishes depth per camera.
src/scene/graphics/frame-pass-depth-grab.js Records grabbed camera depth.
src/scene/camera.js Stores published depth metadata.
src/scene/camera-shader-params.js Tracks reciprocal encoding.
src/index.js Exports SceneDepthReader.
src/framework/graphics/scene-depth-reader.js Implements depth readback API.
src/extras/render-passes/render-pass-prepass.js Publishes prepass depth.
src/extras/render-passes/frame-pass-camera-frame.js Configures reciprocal scene depth.
examples/src/examples/test/scene-depth-read.example.mjs Adds functional readback test.
examples/src/examples/gaussian-splatting/depth-effects.example.mjs Demonstrates splat depth effects.
examples/src/examples/gaussian-splatting/depth-effects.controls.jsx Adds example controls.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +94 to +95
// this pass writes the depth outright, rather than the reciprocal average the scene pass accumulates
shaderParams.sceneDepthMapReciprocal = false;
Comment on lines +252 to +254
for (let i = 0; i < requests.length; i++) {
this._readRequest(requests[i]);
}
// The median rather than the nearest sample, as captures are full of faint floaters which would
// otherwise grab the focus, and thinly covered pixels read too far - the splat depth is weighted by
// transmittance, so it blends toward the value the depth was cleared to where coverage is partial.
depthReader.read(focusRect, FOCUS_SAMPLES, FOCUS_SAMPLES, focusSamples)?.then((samples) => {
Comment on lines +20 to +26
/**
* How close to the far clip a sample has to be to count as nothing having been rendered, as a fraction
* of the far clip. Both producers report the far clip itself for a pixel nothing covered - the prepass
* clears to it, and the scene pass clears to its reciprocal, which decodes back to it - so the margin
* is only here to absorb the rounding of that round trip.
*/
const _farLimitFraction = 1 - 1.5e-3;
Comment on lines +216 to +220
const promise = new Promise((resolve) => {
request.resolve = resolve;
});
this._requests.push(request);
return promise;

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 24 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

src/extras/render-passes/frame-pass-camera-frame.js:555

  • This camera-wide flag also affects shaders rendered before the scene-depth attachment is published. When prepassEnabled and sceneTextureDepth are both true, uSceneDepthMap still refers to the prepass's ordinary linear depth during the scene pass, but soft-particle shaders are generated with SCENE_DEPTHMAP_RECIPROCAL and invert it. This breaks depth softening (for example, depth 10 is decoded as 0.1). The two producers need the same encoding while they coexist, or the decode must be selected per producer/pass rather than on the shared camera parameters.
            // the scene pass accumulates an average of the reciprocals, as the blended splats
            // contribute to it - unlike the prepass, which writes the depth outright
            shaderParams.sceneDepthMapReciprocal = true;

src/framework/graphics/scene-depth-reader.js:430

  • The in-flight requests have already been removed from _requests, so destroying the reader does not mark them as canceled. If a staging copy has already completed, its success callback still writes and resolves the real samples, contrary to the documented guarantee that in-flight reads report an empty region. Track destruction/in-flight requests and make both completion paths resolve Infinity after destruction.
     * @private
     */
    _returnBuffer(buffer) {
        const byteLength = buffer.bytes.byteLength;
        const pool = this._buffers.get(byteLength) ?? [];

examples/src/examples/gaussian-splatting/depth-effects.example.mjs:71

  • This explanation describes the old encoding. The scene pass now stores reciprocal linear depth, not depth in linear view-space units, so the stated R16F precision model is misleading. Update the comment to describe reciprocal storage and why the fallback still uses the tighter far clip.
// The scene depth is stored in linear view space units, in R32F where a float render target can be
// blended into and R16F where it cannot - and half float quantizes with distance, stepping by a quarter
// of a unit at 500. So the far clip is generous on the format which can carry it and stays tight on the
// fallback, which costs the far mountains there rather than the precision the fog and the DOF need.

src/framework/graphics/scene-depth-reader.js:203

  • If a camera that previously rendered depth is disabled, this cached flag remains true because disabling removes the camera from its layers/system and no further postrender event calls _process. Subsequent reads therefore enqueue promises that never settle, and repeated calls accumulate indefinitely. Reject/resolve queued work on camera deactivation and return null while the component or entity is disabled.

        Debug.assert(width > 0 && height > 0, 'SceneDepthReader#read needs a positive number of samples.');
        const count = width * height;

Comment on lines +307 to +310
this.renderTarget.colorBuffer.read(0, 0, width, height, {
renderTarget: this.renderTarget,
data: buffer.bytes
}).then(() => {

@mvaligursky mvaligursky left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR review by Codex (GPT-5).

I reviewed the latest head (bf1944ac7) across the public API, camera/render-pass lifecycle, depth producer/consumer encodings, WebGL2/WebGPU readback behavior, half-float precision, resource use, examples, compatibility, and test coverage. Local npm run lint and npm test pass (2534 passing, 2 pending), and CI is green.

I found three correctness issues inline. The mixed prepass + scene-texture pipeline can decode every reciprocal scene-depth value as a linear distance; R16F still loses the empty sentinel and severe precision at the large far clips this PR newly permits; and a read queued just before a camera is disabled or removed can remain pending forever. The existing Copilot comment about the autofocus example reusing one target across overlapping reads also remains valid: the latest comment acknowledges that it violates the API contract, while the subsequent filter allocates another array anyway. These should be addressed before merge, with regression coverage for mixed producers, camera shutdown, and an R16F large-far-clip case.

This review was generated and posted automatically by Codex (GPT-5).

// the WGSL screenDepth chunk implements no packed decode, as WebGPU always supports
// rendering to float textures
Debug.assert(!(device.isWebGPU && shaderParams.sceneDepthMapPacked));
shaderParams.sceneDepthMapReciprocal = false;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] This still leaves the mixed-producer pipeline with the wrong decode variant. setupRenderPasses first sets sceneDepthMapReciprocal = true when a scene depth attachment is present, but createPasses constructs this prepass first and this assignment resets the camera-wide flag to false; all later post-process passes and SceneDepthReader are then built without SCENE_DEPTHMAP_RECIPROCAL, even though the scene pass unconditionally writes alpha / linearDepth. This occurs with configurations such as an explicit prepass plus DOF/splat depth, so reciprocal values are consumed as tiny linear distances. The encoding needs to be selected per consuming pass, or both producers must use a compatible encoding and decode path when they coexist.

// a disabled camera renders nothing, so it would never service the read - the request would sit
// in the queue unanswered rather than the caller being told there is nothing to read
const { camera } = this;
if (!this._depthRendered || !camera.enabled || !camera.entity.enabled) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Checking the enabled state only when enqueuing does not settle a request if the camera stops later in the same frame. For example, one update listener can call read(), a later listener can disable/remove the camera before rendering, and then no camera postrender event ever calls _process; the returned promise and request remain pending indefinitely. Please observe the camera disable/removal lifecycle or add an end-of-frame fallback that resolves queued requests for cameras that did not render.

this.gridId.setValue(gridValue);

const halfFloat = internal.sceneDepthMap.format === PIXELFORMAT_R16F;
this.farId.setValue(internal.farClip * (halfFloat ? _farLimitFractionHalf : _farLimitFractionFull));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] The fixed half-float margin still cannot identify the clear value at the large far clips the removed guard now permits. Using the engine's half conversion, farClip = 1,000,000 stores 1 / far as 1.01327896e-6, which decodes to about 986,895; that is below this 998,500 cutoff, so a completely empty pixel is returned as a finite hit. Precision also degrades sharply in the subnormal range (at 10,000,000 the decoded value is 8,388,608, and above 16,777,216 the reciprocal can underflow to zero), contrary to the uniform-relative-precision premise. Compare against the producer's actually encoded clear/sentinel before inversion and retain a format-aware support limit where valid scene depths are no longer usable.

@mvaligursky mvaligursky left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated re-review by Codex (GPT-5) of the changes since bf1944ac7.

The update resolves the earlier mixed-producer failure by falling back to the prepass whenever the two incompatible encodings would coexist, restores a safe R16F far-clip limit, pre-sizes the read target, and uses a bottom-origin target so varying-height WebGPU viewports and readbacks address the same physical rows. I do not find the latest Copilot viewport warning applicable after tracing RenderTarget#flipY through WebGPU setViewport: RENDERTARGET_ORIGIN_BOTTOM leaves the viewport at native row zero, matching Texture#read(…, 0, …).

Two promise-lifecycle issues remain inline. The existing autofocus-example target-aliasing comment also remains valid, and the PR description / example comments should be updated to reflect the restored R16F limit, the new prepass fallback, and reciprocal rather than linear half-float storage. Local npm run lint and npm test pass (2534 passing, 2 pending), and all CI checks are green.

This review was generated and posted automatically by Codex (GPT-5).

// a disabled camera renders nothing, so it would never service the read - the request would sit
// in the queue unanswered rather than the caller being told there is nothing to read
const { camera } = this;
if (!this._deviceValid || !this._depthRendered || !camera.enabled || !camera.entity.enabled) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] The added device-destroy handling does not address the camera lifecycle from the previous review. A request can be queued while these checks pass and then the camera or its entity can be disabled before its render step; no camera postrender event fires, so the promise remains in _requests forever. Removal is worse: CameraComponentSystem calls onDisable() and destroys the internal camera but leaves this retained component's enabled flag true, so subsequent read() calls can also pass this test and accumulate permanently. Listen for component removal/disable and add an end-of-frame fallback for requests whose camera did not render, or otherwise settle every queued request when the camera leaves the render set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partly fixed, partly a deliberate limitation.

Removal is fixed. You are right that onBeforeRemove() leaves enabled set - it calls the internal onDisable() and destroys the internal camera, so the check here could not see it and reads would accumulate against a camera which will never render again. That is now picked up from the component's beforeremove event, which ComponentSystem.removeComponent fires before the component sheds its own listeners, so it is still reached.

Disable is accepted as-is. A read queued while the camera is enabled, followed by a disable before that camera renders, does sit unanswered - but it is deferred rather than leaked. The subscription is on the scene filtered by camera, so nothing unsubscribes on disable and the queue drains on the camera's next render. Only a camera which never renders again leaves it pending, and destroy() settles everything queued.

That case is also self-inflicted and avoidable from the application: check the camera before reading, or destroy the reader. Picker, which reads back the same way, holds no camera lifecycle awareness at all. So the residual is one unanswered promise in an ordering the application chose, with no data lost - not worth an end-of-frame sweep over requests which may legitimately be waiting several frames for a slow readback.

this.camera.system.app.scene.off(EVENT_POSTRENDER, this._onPostRender);
this.device.off('destroy', this._onDeviceDestroy);

this._settleRequests();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] _settleRequests() only sees requests still waiting for _process; _process removes every request immediately after starting Texture#read, so neither destroy() nor _onDeviceDestroy settles reads already in flight. On reader destruction a successful callback can still overwrite the target with real samples, contrary to this method's contract. On WebGL device destruction it can hang permanently: WebglGraphicsDevice#readTextureAsync deliberately returns from its completion handler without resolving or rejecting once _destroyed is true, so this class's .catch() never runs. Track in-flight requests, settle them exactly once on either destruction path, and make late callbacks observe cancellation without modifying the resolved target.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of the concrete failures here are now fixed, one in this PR and one upstream.

Overwriting the target after destruction - the readback's completion handler now checks the reader's validity first and reports the region as empty rather than unpacking through buffers which have been let go of. So a late callback no longer writes real samples into a caller's array after destroy().

The WebGL hang - WebglGraphicsDevice#readTextureAsync returning from its completion handler without resolving or rejecting was the root cause, and it is fixed in #9275, now merged and included here. In-flight reads therefore settle on both backends: WebGPU already rejected when its staging buffer could not be mapped, and WebGL now does the same, which this class turns into an empty region.

What remains is that destroy() does not settle an in-flight read synchronously - it settles when the readback completes, which now always happens. The doc says that rather than claiming otherwise:

Reads which have not been rendered yet report their region as empty, and one already in flight does the same once it completes, rather than writing samples read through resources this has let go of.

Tracking in-flight requests separately to settle them immediately would buy only the timing, so it is left out.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Camera disablement can leave reads unresolved, resources leak on destruction, and half-float behavior contradicts the stated compatibility.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/framework/graphics/scene-depth-reader.js:524

  • RenderPassShaderQuad does not override the no-op FramePass.destroy(), so this call leaves its QuadRender (including WebGPU uniform/bind-group resources) alive after the reader is destroyed. Clear shader first; its setter releases the quad while leaving the cached shader owned by the program library.

src/framework/graphics/scene-depth-reader.js:240

  • The enabled checks only cover the instant read() is called. If the camera component or its entity is disabled later in the same frame, this camera never emits EVENT_POSTRENDER, so the queued promise remains unresolved indefinitely (or is serviced by an unrelated future frame after re-enabling). Pending requests need to be settled when the camera stops being renderable, not only when it is removed.
        // a disabled camera renders nothing, so it would never service the read - the request would sit
        // in the queue unanswered rather than the caller being told there is nothing to read
        const { camera } = this;
        if (!this._valid || !this._depthRendered || !camera.enabled || !camera.entity.enabled) {
            return null;

src/extras/render-passes/frame-pass-camera-frame.js:365

  • This reintroduces a stricter half-float far-clip guard (about 16384) even though the PR explicitly states that the old 65504 guard was removed and subnormal reciprocals remain supported. On R16F-only devices, cameras beyond this limit fall back to the prepass, so splats stop contributing to fog/DOF. Either retain subnormal support as described (and adjust empty detection appropriately) or update the stated behavior and justify this new regression.
        if (this.sceneDepthFormat === PIXELFORMAT_R16F &&
            1 / this.cameraComponent.camera.farClip < _minHalfFloatNormal) {
            const limit = Math.floor(1 / _minHalfFloatNormal);
            return `the far clip of this camera is too far for a half float scene depth - keep it below ${limit}, as the depth is stored as its reciprocal`;

examples/src/examples/gaussian-splatting/depth-effects.example.mjs:72

  • This explanation describes the old direct-depth encoding. The new scene attachment stores reciprocal depth, and R16F is selected only when it is renderable and blendable; saying it “cannot” be blended is therefore incorrect. Please describe the reciprocal encoding and the actual precision tradeoff behind the fallback far clip.
// The scene depth is stored in linear view space units, in R32F where a float render target can be
// blended into and R16F where it cannot - and half float quantizes with distance, stepping by a quarter
// of a unit at 500. So the far clip is generous on the format which can carry it and stays tight on the
// fallback, which costs the far mountains there rather than the precision the fog and the DOF need.
const FAR_CLIP = device.textureFloatBlendable ? 1000 : 200;
  • Files reviewed: 20/24 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/scene/camera-shader-params.js Outdated
Comment on lines +40 to +43
* True when the linear scene depth map holds a coverage weighted average of the reciprocals of the
* depths, which a consumer inverts to recover the depth. This is how the scene pass accumulates a
* depth the blended gaussian splats contribute to, and a zero marks a pixel nothing was rendered
* to. Only meaningful when {@link CameraShaderParams#sceneDepthMapLinear} is set.
…ions

The pass has no teardown of its own - FramePass#destroy does nothing and
RenderPassShaderQuad does not override it - so destroying the reader left
the QuadRender it holds alive, along with what that holds on the GPU.
Clearing the shader is what releases it, and leaves the shader itself with
the program library which cached it.

The scene depth encoding was described as marking an uncovered pixel with
a zero. It does not: such a pixel holds the reciprocal of the far clip the
map is cleared to, and reads back as the far clip. Zero is only the guard
the decode takes before dividing.

The far clip in the depth effects example was explained in terms of how a
half float quantizes a distance, which is not what is stored there either.
What caps it on that format is one over the far clip having to stay a
normal half float.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Null-device readback can leave promises unresolved, and R16F reciprocal bounds are not fully enforced.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/extras/render-passes/frame-pass-camera-frame.js:954

  • This uses the camera's live farClip, but the R16F support check at setup is not rerun when farClip changes. A CameraFrame created below the 16384 limit can therefore keep its R16F scene-depth path after the camera is moved past the limit, clearing to a subnormal reciprocal instead of falling back to the prepass as documented. Detect clip-plane changes and rebuild/reselect the depth producer before applying an unsupported clear value.
            clearValue.r = 1 / this.cameraComponent.camera.farClip;

src/framework/graphics/scene-depth-reader.js:345

  • Texture.read() returns undefined on NullGraphicsDevice because NullTexture has no read implementation, so this unconditional .then() throws during the camera's post-render event and leaves the request promise unresolved. The null backend advertises renderable float textures, so a depth producer can publish a map and reach this path; handle an unavailable readback by resolving the request as empty (or add a null-backend read implementation).
        this.renderTarget.colorBuffer.read(0, 0, width, height, {
            renderTarget: this.renderTarget,
            data: buffer.bytes
        }).then(() => {
  • Files reviewed: 20/24 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +362 to +366
if (this.sceneDepthFormat === PIXELFORMAT_R16F &&
1 / this.cameraComponent.camera.farClip < _minHalfFloatNormal) {
const limit = Math.floor(1 / _minHalfFloatNormal);
return `the far clip of this camera is too far for a half float scene depth - keep it below ${limit}, as the depth is stored as its reciprocal`;
}
A half float carries the reciprocal of a distance well over a camera range
of about 0.000015 to 16384, and past the far end of that a distant depth
loses accuracy and a pixel nothing covers stops reading as far away as it
is. Refusing the scene depth there costs far more than it saves though: a
scene of splats and nothing else has no opaque geometry for the prepass to
render, so the depth comes out empty and the fog and the depth of field
are bounded by nothing at all rather than being slightly out at the far
end.

So the range is documented where an application decides whether to turn
the splat depth on, and where a caller reads the samples back, and the
choice of what to do about it is left to the application - which may know
its far clip is nowhere near the limit, or would rather leave the effects
off on such a device.

Nothing watches the clip planes for it either. Which producer renders the
depth is settled when the camera frame is built, and rebuilding that
recompiles the shaders of every pass which reads the depth - not something
to trigger because a near clip was tweened.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The functional test performs an unbalanced depth-map request decrement, corrupting the camera request count.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/extras/render-passes/frame-pass-camera-frame.js:377

  • The PR description still says an R16F camera with farClip > 16384 falls back to the prepass, but this code now has no far-clip guard and continues using reciprocal scene depth with reduced accuracy. Please update the “half float scene depth” consequence to match the implemented behavior and the new API documentation.
        if (this.needsInSceneDepth(options)) {
            return 'the depth prepass this camera also needs stores the depth differently, and the two cannot be told apart by the shaders sampling them';
  • Files reviewed: 21/25 changed files
  • Comments generated: 1
  • Review effort level: Balanced

name: 'depth prepass',
expect: 'linear',
setup: () => {
camera.camera.requestSceneDepthMap(false);
The request the grab phase makes is accumulative, and the phase after it
already gives that one back. Giving it back a second time took the count
the camera keeps below zero, which it asserts against, and left anything
requesting the depth afterwards unable to turn it on.

Nothing measured changed, which is why the test went on passing - by that
phase the camera frame renders the depth whatever the count says.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

WebGPU reads the vertically mirrored depth region, and additional API and example edge cases remain unresolved.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

examples/src/examples/gaussian-splatting/depth-effects.example.mjs:334

  • An empty region is returned as all Infinity, so filtering first leaves no values and preserves the previous focus distance. This contradicts the example's stated behavior that looking into the distance pulls focus to the configured cap; treat an all-empty result as an infinite median and clamp it like any other out-of-range read.
    src/framework/graphics/scene-depth-reader.js:237
  • Fractional sample counts pass this assertion, but they are later used as texture, viewport, copy, and buffer dimensions, all of which require integers. This can produce backend validation failures or mismatched buffer lengths; reject non-integer dimensions at the API boundary.

src/scene/shader-lib/wgsl/chunks/render-pass/frag/scene-depth-read.js:24

  • The public rectangle uses bottom-left coordinates, but WebGPU texture coordinates are top-left. Unlike the other depth-consuming quad passes, this path bypasses getImageEffectUV, so it samples the vertically mirrored region on WebGPU (the current test's vertically symmetric box placement does not expose this). Convert the UV before snapping to the depth texel.
        let uv = uniform.uDepthReadRect.xy + cell * uniform.uDepthReadRect.zw;
  • Files reviewed: 21/25 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

The region is given with its origin in the bottom left, as the camera's
own rect is, while the depth is addressed however the graphics API stores
it. The read worked out its own coordinates and skipped the conversion
every other image effect makes, so on WebGPU it sampled the region
mirrored about the middle of the view.

The functional test could not see it: three boxes at one height are
mirrored onto themselves, and the patch of empty space above them is
mirrored onto empty space below. They now stand at heights of their own,
where a mirrored read lands on nothing at all - which fails the check on
every one of the three encodings before the conversion is applied, and
passes on both backends after.

Two smaller ones alongside it. A region with nothing in it read as every
sample infinite and was dropped, leaving the focus of the example
wherever it happened to be, where the point of the clamp is that looking
into the distance pulls it out to the limit. And a fractional number of
samples passed the check at the API boundary and then went on to be the
dimensions of a texture.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It adds an avoidable architecture dependency and incomplete public guidance for CameraFrame users.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/framework/graphics/scene-depth-reader.js:42

  • CameraComponent#requestSceneDepthMap is documented as ignored whenever framePasses is used, which includes CameraFrame. Following this public guidance on a CameraFrame camera without a depth-consuming effect therefore still leaves no depth to read. Mention CameraFrame.rendering.sceneDepthMap as the required option in that case.
    src/scene/renderer/render-pass-forward.js:90
  • This new field stores a CameraComponent inside src/scene, adding another scene→framework dependency. The module hierarchy explicitly prohibits lower-level modules from using higher-level instances and asks not to extend the existing CameraComponent exception (AGENTS.md:51-65). Store the underlying scene Camera here instead, pass this.cameraComponent.camera from the camera-frame setup, and publish directly on that object.
  • Files reviewed: 21/25 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

The pass which publishes the scene textures held the camera component to
do it, which has the scene level holding an instance from the framework
level above it. Every use of it was to reach the camera underneath, which
is where the depth is recorded and which the scene level owns, so that is
what it holds now and what the camera frame hands it. The other producer
already records the depth this way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mvaligursky
mvaligursky merged commit 571f68d into main Sep 2, 2026
10 checks passed
@mvaligursky
mvaligursky deleted the mv-gsplat-scene-depth branch September 2, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: graphics Graphics related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants