Conversation
…browser test suite - On-demand rendering: SparkRenderer.onDirty now fires for every async result (sort, LoD traverse, chunk landed, mesh initialized, callback exiting with pending work); new docs page, streaming example, and SplatMesh init watching. - Pager races: removed-but-in-flight chunks are retired instead of mapped (retiredSplats/activateSplats); removeSplats purges fetched/lodTreeUpdates/uploads; root-chunk eviction clears rootPage and indices; untraversed paged meshes render nothing. - LoD cleanup: lodDisposeTimeoutMs option (default 3000); visible meshes are never disposed, including ones re-added while a callback runs. - Sort robustness: a failed sort no longer leaves `sorting` stuck; sortDirty re-arms a retry. - Rust hardening: unknown lodId returns an error instead of panicking the worker; traversals bounds-check the root page; get_lod_tree_info/get_lod_tree_ids for introspection. - Test instrumentation: SparkHooks hook points in sort/LoD/pager paths; SplatPager.debugState() and checkInvariants(). - Browser tests: Playwright + SwiftShader suite (sanity, on-demand, churn, orderings, regressions, seeded pager fuzzer) with in-page harness, hook/network seams, pager-vs-Rust cross-check, and synthetic LoD fixtures; manual-only CI workflow. - Docs: on-demand-rendering.md, SparkRenderer onDirty row, LoD guide pointer, pager internals reference. - Fix SplatLoader fileBytes type narrowing so tsc passes.
|
|
||
| Spark does its work from inside `renderer.render()`: that is when it checks the camera, kicks off sorts and LoD updates in its workers, and pages in newly streamed chunks. Results come back asynchronously, and each one needs another render to become visible. In a continuous animation loop the next frame is always coming, so this is invisible. In an on-demand app there is no next frame unless someone asks for one, so Spark needs a way to ask. | ||
|
|
||
| That is what the `onDirty` option on `SparkRenderer` is for. Spark calls it (at most once per rendered frame) whenever it has something new to show or needs another frame to make progress: |
There was a problem hiding this comment.
There's no guarantee that Spark calls it at most once per rendered frame. For example, even in the hello-world example, it's possible to get the following sequence:
- Start of rAF (dirty = false)
setDirty -> onDirtyis called fromupdateInternal- Dirty flag is reset at end of
onBeforeRender - rAF method ends
setDirty -> onDirtyis called fromdriveSort
Probably best to not give this guarantee, instead advising to always coalesce render request, as users are likely to do that anyway. Even if we'd address the above case, Three.js has the unfortunate fact that a single frame might consist of more than one render call. Either through nested render calls (e.g. with mirrors/portals) or by the user manually calling render multiple times. In the latter the renderer.info.render.frame index isn't a reliable indicator either.
There was a problem hiding this comment.
This is a great point, you're right the language is wrong, I will revise it accordingly.
| } | ||
| ``` | ||
|
|
||
| Nothing else is required: R3F renders once on mount, that render kicks off Spark's loading, sorting and LoD work, and each completed step calls `invalidate()` to request the next frame. Anything your own components change (props that move the camera or a `SplatMesh`, adding or removing meshes) should also call `invalidate()`, as usual in demand mode. |
There was a problem hiding this comment.
It might not be immediately clear to users that changes to SplatMesh (e.g. moving them around) isn't automatically tracked by Spark, whereas initialization is. So perhaps this side-note could be made more explicit, or at least also mentioned in the vanilla Three.js case.
There was a problem hiding this comment.
We should probably be more explicit about examples that the user's application will have to trigger re-renders, such as moving splats around. Ironically Spark does track splat movement and whether the accumulated set needs to be re-generated, but that isn't wired into the onDirty callback... I'll make this more explicit in both on-demand examples so users know when they will need to invalidate themselves.
| unhold: (name: string) => | ||
| page.evaluate((n) => window.harness.hooks.unhold(n), name), |
There was a problem hiding this comment.
I kind of expected unhold and release to also assert that the requested hook was indeed held. In part because they are stringly typed, but also to catch any incorrect test setups with uneven/unmatched hold/unhold or release that don't match the actual heldCount.
| // Generators that are still loading change nothing this frame; make sure | ||
| // a render happens once they finish, even without a render loop. | ||
| for (const generator of visibleGenerators) { | ||
| if ( | ||
| generator instanceof SplatMesh && | ||
| !generator.isInitialized && | ||
| !this.initWatched.has(generator) | ||
| ) { | ||
| this.initWatched.add(generator); | ||
| generator.initialized.then( | ||
| () => this.setDirty(), | ||
| () => { | ||
| // Load failures are reported by the mesh itself | ||
| }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it the responsibility of SparkRenderer to keep track of SplatMesh initialization? While it does bridge a gap for on-demand rendering, I'm not sure it actually saves anything for the user.
Let's say a user adds a new SplatMesh to their scene, at that point they'll have to trigger a render themselves. At that point SparkRenderer can pick it up, and call onDirty once the SplatMesh has initialized. So one "user-driven" and one "Spark-driven" request for render.
If the user on the other hand waits for initialization of the SplatMesh and then triggers the render, it'd be just one "user-driven" request for render. Given that a SplatMesh that is initializing shouldn't result in any meaningful difference in the rendered output, this second scenario is preferable for keeping the number of renders down.
Only in simple cases where the scene is setup once, the "user-driven" request for render would coincide with the initial render, in which there's some benefit to this tracking. But ultimately I think it'd be better to move SplatMesh initialization to the user's responsibility to handle, similar to when they add/remove or transform it.
(This would also avoid the edge cases where an uninitialized SplatMesh is removed from the scene before it initialized, yet still triggering an onDirty where it shouldn't)
There was a problem hiding this comment.
Hmm I agree with you that this change could result in additional unnecessary renders... I guess I had taken the internal model that it would be okay to conservatively render a few extra times if we're not sure. I have seen instances where users didn't know that they needed to asynchronously await and re-render when it's initialized, and it seemed like a foot gun that's easy to fix as long as we're okay with potentially a few more frames being renderered.
I think your first example however should not result in an extra render since it's checking generator.isInitialized before watching for its completion?
In the interest of simple solutions and avoiding foot guns for users, do you think it would be okay to have a "conservative" onDirty render like this? It generally seems like few people read the docs carefully these days anyway :(, so if we can avoid pain for some new users this way, I don't mind SparkRenderer doing this...
There was a problem hiding this comment.
In the interest of simple solutions and avoiding foot guns for users, do you think it would be okay to have a "conservative" onDirty render like this? It generally seems like few people read the docs carefully these days anyway :(, so if we can avoid pain for some new users this way, I don't mind SparkRenderer doing this...
I'd expect users looking into on-demand rendering to at least read some docs or the on-demand example. If they'd include splatMesh.initialized.then(() => requestRender()) then I don't think it's going to be much of an issue. Though I could be wrong, of course.
I guess it's fine to have SparkRenderer doing this, it just feels a tad out of place/scope for it.
As an aside, I do think part of the user errors stem from the fact that SplatMesh's initialization is inherently async, which is a departure from how Loaders in Three.js commonly work, which are async while loading but generally give the user a ready to use object. On top of that the relation between SplatMesh and SparkRenderer is somewhat implicit through the scenegraph.
| const hookStart = hookPoint(this.hooks, "sort.start"); | ||
| if (hookStart) await hookStart; |
There was a problem hiding this comment.
We could use a pattern like the following instead:
if (import.meta.env.DEV) {
await hookPoint(this.hooks, "sort.start");
}That way it won't make its way into any of the resulting builds. We could even introduce a specific env var to only be active during tests (or when explicitly set).
Note: there is a difference between not awaiting and await undefined as the latter creates a microtask. Though since these points are supposed to be at async boundaries anyway, the behaviour should be indistinguishable.
There was a problem hiding this comment.
Actually, shouldn't the sort.start hook be right before the await this.readbackDepth line below? Assuming no readPause is configured, that is the actual async boundary.
There was a problem hiding this comment.
I like the approach of checking import.meta.env.DEV or maybe env.SPARK_HOOKS or something like that! I'll add that in. Indeed await undefined is not the same as nothing which is why the hooks were written as two lines with an if statement to guard the await. You are correct that the hook points are at async boundaries anyway so it's moot at this point. However it's not impossible that we would add hooks inside a contiguous "sync" block in the future... I can see that being useful for some situations, even if it introduces an await point that doesn't exist in normal code execution.
Re: sort.start I guess it's semantics a bit :) Technically it's not starting any sort at that point, but I guess I thought of it more as "the macro process of sorting (which includes readback) is starting now". Maybe I can do a pass through the hook point names and see if I can make them clearer.
There was a problem hiding this comment.
So it appears that import.meta.env isn't defined in Node... and with these new tests we are starting to use some of the code in a Node environment so ideally we want code that builds in Vite and Node environments. You would think we could just do import.meta.env?.DEV, but it appears that Vite only handles the non-optional sequence import.meta.env.DEV!!
We could do some trickery to define a const SPARK_HOOKS: boolean in another source file that is then set depending on the environment, but now I feel this roundabout way to remove the hook points in production builds doesn't get us much... The file SparkHooks.ts is tiny and the hook points themselves are tiny amounts of code that execute O(frame) times and amount to two undefined checks basically.
Maybe this is stretching it, but maybe it's actually good to have hook points that are always available, even in the built sources. This could potentially help with debugging sessions for example. And I also think an argument could be made that we'd prefer the production + dev environments to be as identical as possible so there are no surprises later when deploying.
So I'm inclined to leave the hook point infra as-is? WDYT?
There was a problem hiding this comment.
However it's not impossible that we would add hooks inside a contiguous "sync" block in the future... I can see that being useful for some situations, even if it introduces an await point that doesn't exist in normal code execution.
For sync code blocks, we'd better introduce something like a hookPointSync, if we'd need it. Introducing an await point can cause the code in question to be in a state it normally couldn't be in while under test. At best that doesn't cause any problems, but it could easily cause problems like false positive during fuzzing.
That's also why I flagged the sort.start hook location. Though I guess it does align with the readPause async boundary, so not really an issue in that regard.
So it appears that
import.meta.envisn't defined in Node... and with these new tests we are starting to use some of the code in a Node environment so ideally we want code that builds in Vite and Node environments. You would think we could just doimport.meta.env?.DEV, but it appears that Vite only handles the non-optional sequenceimport.meta.env.DEV!!
I believe it should be possible to simply assign process.env to import.meta.env for Node. That said, are we really using some of the code in the Node runtime? AFAICT that isn't the case in this PR, unless I'm overlooking something. The harness runs in a browser environment and the test only import types from the harness.
That would also require us to make more changes to Spark, like avoiding the usage of window and checking the presence of browser APIs before using them. I'd rather we avoid going down this path, or if we really need it, separate out the relevant parts so that its clear which files should avoid browser (and node) specific code.
Maybe this is stretching it, but maybe it's actually good to have hook points that are always available, even in the built sources. This could potentially help with debugging sessions for example. And I also think an argument could be made that we'd prefer the production + dev environments to be as identical as possible so there are no surprises later when deploying.
Don't really think it's going to be that useful for in the wild debugging sessions. The built-in debugger/dev tools are going to get you a long way, and if not you're likely looking at making a minimal reproduction anyway which you can easily hook up to a build/dev version with the hooks.
As for the differences between production and dev environments, you'll always have differences, but none should be behavioural. I view these hooks akin to assertions, which should be safe to eliminate. Of course you can introduce a side-effect in an assertion that, once stripped, causes different behaviour. And if we go for something like env.SPARK_HOOKS we could even have it off by default on dev and only on for testing.
So I'm inclined to leave the hook point infra as-is? WDYT?
If the above really isn't feasible, I'm okay with the hook point infra as-is. IMHO slightly cleaner to not ship it, but not a big deal. Definitely wouldn't want to jump through hoops to strip it. The benefit of something like import.meta.env.DEV is that people will be familiar with it.
|
Went over the PR and left some comments. Have yet to look at the tests more in depth. When I did try and run the tests they were considerably flaky. Throughout a couple runs a couple would fail each time, though not the same ones consistently. Granted the system was not at-rest, and it seemed to be mostly timeouts of some steps, but it would be interesting to see if they are stable in the CI.
Don't really like how much test/debug code is being introduced on the Spark source side of things. In an ideal situation the tests and/or harness would be able to instrument and inspect what it needs itself. Granted, race conditions are harder to represent in tests that way, so likely no easy alternative possible for Additionally there are ways to have Vite (and other bundlers) eliminate code from production builds. That way we can include test/assertion code in Spark with zero impact once published. Good candidates for this could be invariants, as having them in the code itself in relevant functions serve as self-documentation and would be verified every time not only when explicitly done in a test.
There's quite a bit of useful information in there, though I'm not sure about the longevity, and therefore usefulness, of parts these files. How do you envision these to evolve over time? For example, most tests now explicitly mention/refer the identified defect ID, is this a scheme we're going to continue for new tests and how would these IDs be determined. What to name/ID tests that don't arise from a defect, but cover existing (working) behaviour? |
| // The pipeline drains on its own (only onDirty-driven renders happen here): | ||
| // nothing stays queued that would need an application render to be consumed. | ||
| expect(await h.waitIdle({ timeoutMs: 10_000 })).toBe(true); | ||
| expect(await h.isBusy()).toBe(false); |
There was a problem hiding this comment.
This check can be removed, as waitIdle already implies that isBusy will be false.
| } | ||
|
|
||
| /** Release up to `count` parked executions at `name` (FIFO). Returns released count. */ | ||
| release(name?: string, count = 1) { |
There was a problem hiding this comment.
Nitpick: the naming can be a tad confusing as release really seems like a natural counterpart of hold, rather than unhold. Can't really think of a better naming scheme of the top of my head, so no big issue if left unchanged, but if a less ambiguous name could be found that would be nice.
There was a problem hiding this comment.
You're right that can be confusing, like "hold" vs "held" is also confusing in the code. Let me think about better names...
|
@mrxz thank you for your thorough review! Let me iterate on this PR a bit...
That's unfortunate that they were flaky. I ran them many times locally and always ran successfully and to completion and unfortunately I assumed they were reliable. Of course this is often not the case with browser-based testing :(. It could be timeouts too short, but fiddling with timeouts until it "usually works" is not a good solution. Let me think about how to make them more reliable. Thank you so much for your thorough review! Some of the code is obviously LLM-written and I would not expect super thorough review on the testing stuff for example, under the assumption that we will be able to revise and improve it easily over time.
Yeah it feels a bit dirty to pollute the code with this testing infra, but I do think the hook points are very short and simple, and in some ways helps comment/delineate the code so I don't mind them? They could be useful for other purposes in the future as well. I can move the invariant code outside the main source files so they are more cleanly delineated from the "core code". Let me do a pass to improve that.
The
My thinking was to leave docs/internals/pager/index.html if we merged this in, but remove fixes-2026-09.html, which was more of an artifact useful for understanding the fixes in this PR. I was even hoping over time to add more docs/internals/X/index.html pages, as a way for people to learn the nitty-gritty details of how the system works. It feels like these sorts of docs would be too much for the average user on our normal docs hosting system. I made them self-contained HTML files so they could have mermaid diagrams and potentially LLM-generated animations or other useful visuals, which I think is hard to accomplish with our doc-generation system, hence docs/internals//.html self-contained files. What do you think? |
It's definitely an acceptable level of code for the benefits it gives. Testing specific sequence of async events is otherwise very difficult to achieve. I would be somewhat wary of expanding their usage, as the preference should IMO still go to first finding ways to test by instrumenting/observing from the outside in, rather than adjusting the code to accommodate testing, where possible.
Yes, that would do trick. It would be nice to have a way to introduce assertions going forward without having to worry about bundle size or performance impact (in published builds). By default the dead code elimination of Vite removes these entirely (given the tested env variable isn't set/true).
Sounds good. Agreed that its best to keep this out of the normal docs, as they do go into internal details that shouldn't be relevant to most users. |
2 participants
In addition to fixing some known (and unknown) race conditions with the Spark LoD/paged system, this PR adds a new test suite
npm run test:browserthat uses Playwright coupled with a headless (software WebGL2 rendered) Chromium to run some automated browser-based testing. The framework includes "hook points" within the Spark Renderer that enables us to deterministically control async progression across the concurrent rendering/sorting/LoDing/fetching processes, and allows us to "fuzz" the system to find new issues. Note: heavily LLM-generated test suite but it looks reasonable and overall a good starting point for this sort of testing.Also adds some single-page HTML docs for internal systems in docs/internals/pager (load it up localhost) that document how the paging system works, along with descriptions of the bugs identified and fixed in docs/internals/pager/fixes-2026-09.html.
Commit summary:
Fix paged LoD churn races and add on-demand rendering support with a browser test suite
sortingstuck; sortDirty re-arms a retry.