Skip to content

Conversation

@mrxz
Copy link
Collaborator

@mrxz mrxz commented Jul 15, 2025

The PackedSplat and Readback classes used a manual setup to render a quad that covers the entire render target. While this works, the quad consists of two triangles, meaning there's an edge running through the middle. While this might seem inconsequential it can have a negative impact on performance (see this post for details)

This PR replaces the manual setup with the FullScreenQuad from Three.js. Contrary to its name, it actually uses a fullscreen triangle under the hood.

src/Readback.ts Outdated
static mesh = new THREE.Mesh(
Readback.geometry,
// Static full-screen quad for pseudo-compute shader rendering
static fsQuad = new FullScreenQuad(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call it fullScreenQuad and avoid abbreviation ?

Copy link
Contributor

@dmarcos dmarcos Jul 15, 2025

Choose a reason for hiding this comment

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

even better fullScreenTriangle to avoid three misnomer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it to fullScreenQuad. Originally used fsQuad as that is the common name used throughout Three.js. Didn't go for fullScreenTriangle as the snippet fullScreenTriangle = new FullScreenQuad just doesn't read nicely. In a way it being a triangle is an implementation detail, and while that detail is the main motivation for this change, it isn't really needed to understand the code here.

@asundqui
Copy link
Contributor

Nice, yes this will improve things a bit, thanks!

@mrxz mrxz force-pushed the fullscreen-triangle branch from 414180b to 69a6235 Compare July 16, 2025 13:55
@dmarcos dmarcos merged commit 1e697b3 into sparkjsdev:main Jul 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants