From 49cec0316dad6e38afad5e18e33472fe617ebae1 Mon Sep 17 00:00:00 2001 From: brentyi Date: Sun, 26 Jul 2026 15:53:25 +0000 Subject: [PATCH] Fix close-camera line rendering for drei-based lines under reversed depth three-stdlib LineMaterial's trimSegment near-plane estimate assumes a standard depth projection; under the reversed depth buffer we enable in App.tsx it evaluates to -far/2 instead of -near, so line segments crossing the camera plane get extrapolated hundreds of units away and the part of the line near the camera disappears. Our own carried a per-material onBeforeCompile patch for this (#720), but drei's -- used by camera frustums, Catmull-Rom / cubic Bezier splines, and internally by PivotControls -- constructs its own LineMaterial instances that never got patched. Replace the per-material patch with a vertexShader accessor on LineMaterial.prototype that rewrites the near-plane estimate at construction time for every instance, drei's included. This also stops us from overwriting three-stdlib's own onBeforeCompile, which manages the USE_LINE_COLOR_ALPHA define for transparent materials. Also fixes a PLW0108 lint error (newly stabilized in ruff 0.15) in test_get_render.py. --- .../client/src/CameraFrustumVariants.tsx | 3 + src/viser/client/src/Line.tsx | 25 ++----- .../patchLineMaterialReversedDepth.test.ts | 38 +++++++++++ .../src/patchLineMaterialReversedDepth.ts | 58 ++++++++++++++++ tests/e2e/test_get_render.py | 2 +- tests/e2e/test_line_close_camera.py | 68 +++++++++++++++++++ 6 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 src/viser/client/src/patchLineMaterialReversedDepth.test.ts create mode 100644 src/viser/client/src/patchLineMaterialReversedDepth.ts create mode 100644 tests/e2e/test_line_close_camera.py diff --git a/src/viser/client/src/CameraFrustumVariants.tsx b/src/viser/client/src/CameraFrustumVariants.tsx index 3a2230c19..869747ef0 100644 --- a/src/viser/client/src/CameraFrustumVariants.tsx +++ b/src/viser/client/src/CameraFrustumVariants.tsx @@ -1,3 +1,6 @@ +// drei's Line constructs its own three-stdlib LineMaterial; make sure the +// reversed-depth near-plane patch is installed before that happens. +import "./patchLineMaterialReversedDepth"; import { Line } from "@react-three/drei"; import { useFrame } from "@react-three/fiber"; import React from "react"; diff --git a/src/viser/client/src/Line.tsx b/src/viser/client/src/Line.tsx index e9368942d..826080647 100644 --- a/src/viser/client/src/Line.tsx +++ b/src/viser/client/src/Line.tsx @@ -4,6 +4,7 @@ */ import "./r3f-extend"; +import "./patchLineMaterialReversedDepth"; import * as React from "react"; import * as THREE from "three"; import { ColorRepresentation } from "three"; @@ -106,31 +107,13 @@ export const Line: ForwardRefComponent = [ref], ); - // LineMaterial's trimSegment near-plane estimate assumes standard - // depth; under reversed depth (App.tsx) it explodes and lines smear - // when the camera is close. Switch the formula on sign(a). Three.js - // has an equivalent fix queued for r185 - // (https://github.com/mrdoob/three.js/pull/33572); drop this patch - // once we upgrade to three@>=0.185. - const patchLineMaterialShader = React.useCallback( - (mat: LineMaterial | null) => { - (matRef as React.MutableRefObject).current = mat; - if (!mat) return; - mat.onBeforeCompile = (shader) => { - shader.vertexShader = shader.vertexShader.replace( - "float nearEstimate = - 0.5 * b / a;", - "float nearEstimate = ( a > 0.0 ) ? ( - b / ( 1.0 + a ) ) : ( - 0.5 * b / a );", - ); - }; - mat.needsUpdate = true; - }, - [], - ); + // Reversed-depth near-plane fix for LineMaterial is applied globally + // (all instances, including drei's) in patchLineMaterialReversedDepth. // R3F manages lifecycle for all declarative children -- no manual disposal. const materialJsx = ( { + it("rewrites the near-plane estimate on every new LineMaterial", () => { + // Constructing directly mirrors what drei's does internally; the + // patch must apply without any per-instance setup. + const mat = new LineMaterial(); + expect(mat.vertexShader).not.toContain(BROKEN_NEAR_ESTIMATE); + expect(mat.vertexShader).toContain(FIXED_NEAR_ESTIMATE); + }); + + it("survives clone() without double-applying", () => { + const mat = new LineMaterial(); + const cloned = mat.clone(); + expect(cloned.vertexShader).toBe(mat.vertexShader); + // Exactly one occurrence of the fixed expression. + expect(cloned.vertexShader.split(FIXED_NEAR_ESTIMATE).length - 1).toBe(1); + }); + + it("preserves three-stdlib's own onBeforeCompile hook", () => { + // three-stdlib's LineMaterial assigns an onBeforeCompile that toggles + // USE_LINE_COLOR_ALPHA for transparent materials. Our patch must not + // replace it (the old per-instance patch did). + const mat = new LineMaterial(); + mat.transparent = true; + mat.onBeforeCompile( + { vertexShader: "", fragmentShader: "" } as never, + null as never, + ); + expect(mat.defines.USE_LINE_COLOR_ALPHA).toBe("1"); + }); +}); diff --git a/src/viser/client/src/patchLineMaterialReversedDepth.ts b/src/viser/client/src/patchLineMaterialReversedDepth.ts new file mode 100644 index 000000000..19dc9c208 --- /dev/null +++ b/src/viser/client/src/patchLineMaterialReversedDepth.ts @@ -0,0 +1,58 @@ +/** + * Reversed-depth fix for three-stdlib's LineMaterial, applied to EVERY + * instance via a prototype accessor. + * + * LineMaterial's vertex shader trims line segments that cross the camera + * plane back to a near-plane estimate derived from the projection matrix: + * + * float nearEstimate = - 0.5 * b / a; + * + * That formula assumes a standard depth projection. Under the reversed + * depth buffer we enable in App.tsx, a = near / (far - near) and + * b = far * near / (far - near), so it evaluates to -far / 2 -- segments + * that cross the camera plane get extrapolated hundreds of units into the + * scene and smear across the screen whenever the camera gets close to a + * line. The reversed-depth-safe estimate is -b / (1 + a) = -near exactly; + * we switch on sign(a), which distinguishes the two projection forms. + * + * We can't fix this with a one-off material patch: drei's (used for + * camera frustums, Catmull-Rom / cubic Bezier splines, and internally by + * PivotControls) constructs its own LineMaterial instances that we never + * see. Instead we intercept `vertexShader` on LineMaterial.prototype: the + * ShaderMaterial constructor assigns the shader source with a plain + * `this.vertexShader = ...`, which invokes this inherited setter, so every + * instance is rewritten at construction time. The replacement is a no-op + * for shader sources that don't contain the broken line (idempotent for + * clone()/copy(), and safe if three-stdlib ships its own fix). + * + * three.js fixed its bundled copy of this shader for r185 + * (https://github.com/mrdoob/three.js/pull/33572), but drei and our Line + * component use the fork in three-stdlib, which carries its own copy of + * the shader -- so this patch is needed until three-stdlib ships the + * equivalent fix, independent of the three version we build against. + */ +import { LineMaterial } from "three-stdlib"; + +const BROKEN_NEAR_ESTIMATE = "float nearEstimate = - 0.5 * b / a;"; +const FIXED_NEAR_ESTIMATE = + "float nearEstimate = ( a > 0.0 ) ? ( - b / ( 1.0 + a ) ) : ( - 0.5 * b / a );"; + +const STORAGE_KEY = "__viserPatchedVertexShader"; + +type PatchedMaterial = LineMaterial & { [STORAGE_KEY]?: string }; + +Object.defineProperty(LineMaterial.prototype, "vertexShader", { + configurable: true, + enumerable: true, + get(this: PatchedMaterial) { + return this[STORAGE_KEY]; + }, + set(this: PatchedMaterial, source: string) { + this[STORAGE_KEY] = + typeof source === "string" + ? source.replace(BROKEN_NEAR_ESTIMATE, FIXED_NEAR_ESTIMATE) + : source; + }, +}); + +export { BROKEN_NEAR_ESTIMATE, FIXED_NEAR_ESTIMATE }; diff --git a/tests/e2e/test_get_render.py b/tests/e2e/test_get_render.py index cb90b93b7..99715716d 100644 --- a/tests/e2e/test_get_render.py +++ b/tests/e2e/test_get_render.py @@ -42,7 +42,7 @@ def test_get_render_raises_promptly_on_disconnect( own_server: viser.ViserServer, browser: Browser ) -> None: captured: list[viser.ClientHandle] = [] - own_server.on_client_connect(lambda client: captured.append(client)) + own_server.on_client_connect(captured.append) context = browser.new_context() page = context.new_page() diff --git a/tests/e2e/test_line_close_camera.py b/tests/e2e/test_line_close_camera.py new file mode 100644 index 000000000..a6cab854e --- /dev/null +++ b/tests/e2e/test_line_close_camera.py @@ -0,0 +1,68 @@ +"""Regression test for fat-line rendering when the camera is very close. + +Under the reversed depth buffer (App.tsx), three-stdlib LineMaterial's +``trimSegment`` near-plane estimate evaluates to -far/2 instead of -near, so +any line segment crossing the camera plane gets extrapolated hundreds of +units away from the camera: the part of the line sweeping past the camera +vanishes. viser's own carried a local patch (#720), but drei's +-- used for camera frustums, splines, and internally by PivotControls -- +constructs its own LineMaterial and was still broken. The patch now applies +to every LineMaterial via patchLineMaterialReversedDepth.ts; this test +exercises the drei code path through the built client. +""" + +from __future__ import annotations + +from io import BytesIO + +import numpy as np +from PIL import Image +from playwright.sync_api import Page + +import viser + +from .utils import wait_for_scene_node + + +def test_spline_passing_camera_renders( + viser_server: viser.ViserServer, + viser_page: Page, +) -> None: + """A straight spline passing just beside the camera must sweep across the + screen toward the viewport edge, not vanish near the camera.""" + # drei's CatmullRomLine (unlike add_line_segments) renders through drei's + # , whose LineMaterial viser code never touches directly. + viser_server.scene.add_spline_catmull_rom( + "/spline", + points=np.array( + [[-5.0, 0.0, 0.0], [-2.0, 0.0, 0.0], [2.0, 0.0, 0.0], [5.0, 0.0, 0.0]] + ), + color=(255, 0, 0), + line_width=6.0, + ) + viser_server.scene.world_axes.visible = False + wait_for_scene_node(viser_page, "/spline") + viser_page.wait_for_timeout(300) + + client = list(viser_server.get_clients().values())[0] + # Camera right next to the line (~0.02 to the side), looking along it, and + # offset along x so the curve chunk crossing the camera plane has its + # forward endpoint well in front of the camera (a chunk boundary exactly + # at the camera plane would mask the trimSegment bug). + client.camera.position = (0.25, -0.02, 0.007) + client.camera.look_at = (3.25, 0.0, 0.0) + viser_page.wait_for_timeout(400) + + canvas = viser_page.locator("canvas").first + img = np.array(Image.open(BytesIO(canvas.screenshot())).convert("RGB")).astype(int) + red = ( + (img[:, :, 0] > 150) + & (img[:, :, 0] > img[:, :, 1] + 60) + & (img[:, :, 0] > img[:, :, 2] + 60) + ) + # The section of line sweeping past the camera projects into the + # mid-height left third of the canvas (a region free of the control + # panel, the software-WebGL notification, and the viser logo). With the + # trimSegment bug this region is empty (measured 0 vs ~2000 pixels). + left_mid = int(red[200:500, 0:300].sum()) + assert left_mid > 500, f"line sweep toward viewport edge missing: {left_mid=}"