Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGLRenderer: Fix binding __webglFramebuffer when mipmaps are used. #30746

Merged
merged 4 commits into from
Mar 31, 2025

Conversation

haxiomic
Copy link
Contributor

@haxiomic haxiomic commented Mar 17, 2025

Closes #30745

Related issue: #29779

Description

It's possible render into specific mipmap levels when we set the target.texture.mipmaps array. This works great for color-only render targets, however if we give this target a depth buffer (either through depthBuffer: true or an explicit depthTexture), rendering will fail with a crash when rendering

The crash is caused by three.js assuming .__webglFramebuffer is a single framebuffer, however, when .mipmaps is set it becomes an array:

if (texture.mipmaps && texture.mipmaps.length > 0) {
  for (let level = 0; level < texture.mipmaps.length; level++) {
    setupFrameBufferTexture(renderTargetProperties.__webglFramebuffer[level], renderTarget, texture, _gl.COLOR_ATTACHMENT0, glTextureType, level);
  }
} else {
  setupFrameBufferTexture(renderTargetProperties.__webglFramebuffer, renderTarget, texture, _gl.COLOR_ATTACHMENT0, glTextureType, 0);
}

This causes a crash on this line

state.bindFramebuffer( _gl.FRAMEBUFFER, renderTargetProperties.__webglFramebuffer );

And

state.bindFramebuffer( _gl.FRAMEBUFFER, framebuffer );

Maybe other places too

This PR resolves this by instead setting the depth attachment for only mipmap 0.

I will also think about setting a depth attachment for each mip level

I've tried to use idomatic three.js style which comes out quite verbose but let me know if we want something tidier!

@haxiomic
Copy link
Contributor Author

haxiomic commented Mar 17, 2025

Draft for now to get the ball rolling – I want to complete a working demo before I finish

Copy link

github-actions bot commented Mar 17, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.08
78.29
336.36
78.34
+271 B
+56 B
WebGPU 530.63
147.4
530.63
147.4
+0 B
+0 B
WebGPU Nodes 530.09
147.29
530.09
147.29
+0 B
+0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.15
112.18
465.44
112.23
+281 B
+54 B
WebGPU 602.3
163.4
602.3
163.4
+0 B
+0 B
WebGPU Nodes 557.34
152.84
557.34
152.84
+0 B
+0 B

@haxiomic haxiomic marked this pull request as ready for review March 25, 2025 16:23
@haxiomic
Copy link
Contributor Author

haxiomic commented Mar 25, 2025

I can confirm this is good to go now!

CC @Mugen87

@haxiomic
Copy link
Contributor Author

haxiomic commented Mar 31, 2025

Motivating examples for what becomes possible: better bloom & AO and others!

Screen.Recording.2025-03-31.at.10.48.06.mov
Screenshot 2025-03-31 at 10 42 49

@Mugen87 Mugen87 changed the title Fix binding __webglFramebuffer when mipmaps are used WebGLRenderer: Fix binding __webglFramebuffer when mipmaps are used. Mar 31, 2025
@Mugen87 Mugen87 added this to the r176 milestone Mar 31, 2025
@Mugen87 Mugen87 merged commit 541455a into mrdoob:dev Mar 31, 2025
12 checks passed
@mrdoob
Copy link
Owner

mrdoob commented Mar 31, 2025

@haxiomic Wow these look stunning! Are you planning to send a PR with these improvements too?

@haxiomic
Copy link
Contributor Author

haxiomic commented Mar 31, 2025

Thanks @mrdoob!

It would be great to get these improvements in, there's a few interesting pieces

That demo relies on a some utilities which I've open sourced here: https://github.com/haxiomic/haxiomic-engine, might be a few bits three can benefit from – for example there's a fast gaussian blur implementation (uses the hardware bilinear sampling to reduce fetches and pre-computes texture coordinates to enable early fetch optimisations)

For anyone looking to replicate, the pipeline looks like this

bloomMipmapsMaterial = new BloomMipmapsMaterial();

...

// render scene to texture
Rendering.renderPass(renderer, {
    target: mainTarget,
    scene: scene,
    camera: camera,
    toneMapping: NoToneMapping,
});

// blur (linear color space) into the mipmap chain
generateBlurredMipmaps(renderer, {
    target: mainTarget,
    blurKernel_heightFraction: 0.0173, // size of mip 0 -> mip 1 blur radius relative to height
    truncationSigma: 0.567,
});

// copy to screen (sRGB) while applying mipmap bloom
Rendering.shaderPass(renderer, {
    target: null,
    shader: bloomMipmapsMaterial,
    toneMapping: ACESToneMapping,
    uniforms: {
        source: mainTarget.texture,
        bloomFalloff: 0.386,
        bloomStrength: 0.0328,
        minLod: 1,
    }
});

I will need to think how to use with EffectCompositor. I tend to use this Rendering.renderPass() approach a lot these days, the idea there is to let you perform rendering operations without worrying about how this might change global state other operations rely on.

Shader passes are such a common operation it's nice to have a primitive for this so we don't need to think about setting up a scene or camera

@haxiomic
Copy link
Contributor Author

AO is still in development but I can take a look at making three.js examples once that's done

@mrdoob
Copy link
Owner

mrdoob commented Mar 31, 2025

AO is still in development but I can take a look at making three.js examples once that's done

That would be great!🙏

@mrdoob
Copy link
Owner

mrdoob commented Mar 31, 2025

@sunag FYI

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.

Rendering into a framebuffer with .mipmaps set crashes if depth is used
3 participants