Fix on-demand paged LoD streaming. Add headless browser rendering tests. by asundqui · Pull Request #449 · sparkjsdev/spark

asundqui · 2026-09-22T05:37:52Z

This PR is meant to supersede #428 (will be closed once all fixes are in) and addresses #316 . Additional PRs will follow later that address other bugs/race conditions discovered while developing #428 .

When rendering paged LoD splats in on-demand mode, chunks that are received don't trigger onDirty unless the viewpoint changes or something else triggers a render. This PR fixes it by calling back from SplatPager into SparkRenderer.setDirty when new pages are decoded.

In addition, this PR adds documentation and a concrete example for how to do "on-demand rendering". The new example examples/on-demand/ wouldn't load the whole scene unless the user rotated the view. With this fix it loads to completion by itself.

Finally, this PR introduces basic CI browser tests that use a headless browser, rendering using CPU-based WebGL2, and tests some basic rendering scenarios, LoD rendering, and paged LoD and compares them against reference images, similar to Three.js. These "Spark CI Browser" tests (ci-browser) run automatically in this branch, but for now will only run on manual dispatch once merged in. They have been tested and appear to run reliably both locally and via GitHub Actions.

The browser tests can be run locally using npm run test:browser and uses Playwright to run Chromium with SwiftShader (a CPU-based WebGL2 implementation) to do 256x256 canvas renders. Comparisons against reference images are expected to be pixel-perfect but with minimal differences from platforms (6-16 pixels per 64K image were different between MacOS and Linux). All tests use the Spark example asset fuzzy-logo-pedestal that is downloaded in global-setup.ts and built into RAD + chunked RAD files using build-lod.

Current minimal browser tests include:

Basic rendering: straight render, render transformed, render 3 instances
LoD rendering: render at 1K/10K/100K splat count targets
Paged LoD rendering: render at 1K/10K/100K with chunks loaded in on-demand (previously failing before these fixes)

The tests use a common fixture harness.fixture.ts that calls into harness.ts running in the browser, with a simple API that allows tests to create a SparkRenderer (with options), cameras, SplatMeshes, render the scene, and wait for Spark to "settle" before reading back the pixels and comparing against the reference.

Future PRs will build on this, adding hooks into Spark and tests that deterministically trigger known race conditions. Fixes will then exercise these conditions and make sure they are reliably fixed and stay fixed.

asundqui · 2026-09-22T05:39:19Z

Example workflow run with successful browser tests: https://github.com/asundqui/spark/actions/runs/35689443120

mrxz · 2026-09-22T13:55:48Z

+        run: npm run test:browser
+
+      - name: Upload Playwright report
+        if: always()


Using always() has the downside that it runs even when the job gets cancelled. It's not uncommon to see alternatives like success() || failure() or "!cancelled()" for this reason.

Good call, changed to !cancalled() in the next commit.

mrxz · 2026-09-22T14:00:17Z


 | **Parameter**     | Description |
 | ----------------- | ----------- |
+| **onDirty**       | Callback invoked (at most once per rendered frame) when Spark needs another render to show new results, e.g. a completed sort or LoD update, or a newly streamed chunk. Use this to drive [on-demand rendering](on-demand-rendering.md). (default: `undefined`)


Nitpick: this still includes the "at most once per rendered frame"

Whoops, you're right! I've corrected this in the next commit (along with two other places in docs/comments).

mrxz · 2026-09-22T14:05:03Z

  private readonly ownsTimer: boolean;
  lastFrame = -1;
-  updateTimeoutId = -1;
+  updateTimeoutId: ReturnType<typeof setTimeout> | undefined = undefined;


Is there a specific reason for this change? The spec guarantees the returned value is a positive integer (timer initialization step 2). And while ReturnType<typeof setTimeout> does make it clear what the variable will hold, it doesn't provide any benefit in terms of type safety, as it evaluates to the type number.

Yes, the reason is because harness.ts, which is used both in the Browser and in the Node testing environment, references SparkRenderer + type SparkRendererOptions, so that the testing environment has access to things like the options for constructing the SparkRenderer.

The alternative would be to have separate definitions for the browser + node environments, but then we have the problem that we'd need to make sure these definitions are sync. Having one "source of truth" that works across both environments simplifies things a lot and could avoid issues in the future.

We import @node/types so that we can have TS checking on the new tests, but apparently once you import that it will default to using those type definitions rather than the browser's... So when running tsc to check types we get the error src/SparkRenderer.ts:759:11 - error TS2322: Type 'Timeout' is not assignable to type 'number'.

These changes in SparkRenderer allow the types to work in both environments. I actually think it looks nicer than the -1 constant for when the timer isn't set. I hope you agree it's not bad!

mrxz · 2026-09-22T14:19:35Z

+   * Called when a fetched chunk is ready to be paged in and a frame is
+   * needed to consume it.
+   */
+  onDirty?: () => void;


Nitpick: I don't think the onDirty name fits what it does in the context of SplatPager. It's the SparkRenderer that is concerned with flagging the dirty state, and having lodTreeUpdates/newUploads in the pager "just so happens" to be one of the conditions for the SparkRenderer. But from the SplatPager it's more a notification/event.

I see your point. I've changed it to onUpdate, which I think is a bit better! We could also have something like onFetchSettled but I think future PRs addressing race conditions may be a better fit with onUpdate.

mrxz · 2026-09-22T15:09:41Z

+declare global {
+  interface Window {
+    harness: Harness;
+  }
+}


Adding const harness: Harness to the global would allow directly referencing the harness without needing to go through the window object. Normally that isn't the greatest idea, but for test code it can be useful.

So you'd have:

- const h = window.harness; - h.createSpark({ lodSplatCount }); + harness.createSpark({ lodSplatCount });

Of course harness is more verbose than h, but personally I don't think that's bad in this case. Though maybe we should go even further and make the relevant methods directly available, eliminating the need for harness. entirely.

That's nice as well to just be able to say harness rather than h = window.harness, but if we made that change to add const harness: Harness then this would also be available in the Node global context that the tests run in. Same if we added all the harness API methods to the global scope... I feel it's probably better to keep them namespace-contained inside Harness, and I like making it explicit where it appears in the browser context! I also like the pattern of assigning h and then using that - I feel it makes the tests themselves succinct and clean looking!

mrxz · 2026-09-22T15:15:47Z

Awesome work, looks good. Ran the tests locally and this time they weren't flaky and succeeded consistently.

Left some comments, but none are blockers, mostly naming and small suggestions. Really like how the browser tests are shaping up. It seems like a good base to add more tests to.

oscarlorentzon · 2026-09-22T16:37:38Z

Three.js splits its tests into test/unit/ and test/e2e/. Would it make sense to move test/utils.test.ts into test/unit/ and switch the script to --dir test/unit, so it names the folder instead of needing the --exclude? And should test/browser/ be renamed test/e2e/ to match, or stay as it is alongside test:browser and the browser workflow?

oscarlorentzon

Look good in general, this is a really good base for e2e testing 👍 . See the inline comments and structural suggestions.

oscarlorentzon · 2026-09-22T16:15:53Z

+    "test": "vitest run --reporter=dot --exclude \"test/browser/**\" test/",
+    "test:browser": "playwright test -c test/browser/playwright.config.ts",
+    "test:browser:update": "playwright test -c test/browser/playwright.config.ts --update-snapshots",
+    "types": "tsc -p tsconfig.json --noEmit && tsc -p test/browser/tsconfig.json"


Maybe it would be clearer to separate these into types and types:browser scripts?

I had it that way originally, but decided to combine them since I feel we should just have all the types be checked all the time... Right now the browser tests are optional so there's a chance they would diverge/break. If these tests were slow I would say maybe we should separate them but they run so quickly, why not just test it all!

Sounds good, let's keep them combined.

oscarlorentzon · 2026-09-22T16:48:28Z


    this.autoDrive = options.autoDrive ?? true;
    this.numFetchers = options.numFetchers ?? 3;
+    this.onDirty = options.onDirty;


dispose() leaves onDirty set, so unfinished fetches still call it after dispose() returns. Setting it to undefined in dispose() would stop that.

Good catch, thank you! I've made the update in the next commit.

oscarlorentzon · 2026-09-22T17:17:20Z

   */
  numFetchers?: number;
+  /**
+   * Called when a fetched chunk is ready to be paged in and a frame is


onDirty fires from the .finally() block, so it also runs when a chunk fetch fails. Do we want to inform about the failure case in this description too?

That's right, I've updated the comment in the next commit!

oscarlorentzon · 2026-09-22T17:32:57Z

@@ -0,0 +1,75 @@
+import { expect, test } from "./harness.fixture.js";


Playwright's default testMatch covers .test.ts as well as .spec.ts, so these could be basic.test.ts and so on, matching test/utils.test.ts. Do we want one extension across both suites?

Good call, more consistent that way. I've renamed them in the next commit.

asundqui · 2026-09-22T19:21:04Z

@oscarlorentzon :

Three.js splits its tests into test/unit/ and test/e2e/. Would it make sense to move test/utils.test.ts into test/unit/ and switch the script to --dir test/unit, so it names the folder instead of needing the --exclude? And should test/browser/ be renamed test/e2e/ to match, or stay as it is alongside test:browser and the browser workflow?

Okay I've moved the unit test to test/unit/ for the future and to make the test selection simpler! As for e2e I actually think E2E implies a bit more than these tests do... like users clicking etc. Although it doesn't mirror Three.js's e2e, I think test/browser actually is appropriately named. Not opposed to changing this in the future as these tests evolve.

…ming - Browser tests: Playwright + SwiftShader harness, exact pixel snapshots for basic, transforms, instances, LoD and paged LoD - SplatPager/SparkRenderer: fire onDirty when a chunk lands, add pending(); portable setTimeout types - CI: ci-browser.yml with HTML report artifact, CPU-only build-lod - Docs/examples: on-demand rendering guide, example, onDirty option docs

- playwright.config: keep threshold 0, allow maxDiffPixelRatio 0.001 (65 px); CI showed 6-16 scattered pixel diffs vs macOS snapshots - ci-browser.yml: bump actions to current majors (checkout/setup-node/upload-artifact v7, cache v6) to clear Node 20 deprecation warnings

…once merged into main.

- SplatPager: rename onDirty to onUpdate, clear it in dispose() - Docs: onDirty may fire several times per frame; schedule a single render - Tests: move unit test to test/unit (vitest --dir), rename browser specs to *.test.ts - CI: upload report with !cancelled() instead of always()

asundqui requested review from dmarcos, mrxz and oscarlorentzon September 22, 2026 05:37

asundqui self-assigned this Sep 22, 2026

mrxz reviewed Sep 22, 2026

View reviewed changes

oscarlorentzon approved these changes Sep 22, 2026

View reviewed changes

asundqui added 4 commits September 22, 2026 13:38

Only run ci-browser for this branch. Add option to dispatch workflow …

63bedb8

…once merged into main.

asundqui force-pushed the fix-paged-lod-browser-test branch from b54958f to c20a1d5 Compare September 22, 2026 20:44

Shared pngBuffer snapshot helper and drop 1K LoD cases

e3df4d7

asundqui mentioned this pull request Sep 23, 2026

Fix paged root eviction issue #451

Open

		@@ -0,0 +1,75 @@
		import { expect, test } from "./harness.fixture.js";

Conversation

asundqui commented Sep 22, 2026

Uh oh!

asundqui commented Sep 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrxz commented Sep 22, 2026

Uh oh!

oscarlorentzon commented Sep 22, 2026

Uh oh!

oscarlorentzon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asundqui commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants