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

MeshPhysicalMaterial: transmission rendering is buggy for high-roughness values #25485

Closed
WestLangley opened this issue Feb 12, 2023 · 9 comments · Fixed by #25494
Closed

MeshPhysicalMaterial: transmission rendering is buggy for high-roughness values #25485

WestLangley opened this issue Feb 12, 2023 · 9 comments · Fixed by #25494

Comments

@WestLangley
Copy link
Collaborator

WestLangley commented Feb 12, 2023

Description

Small changes in the camera position can cause huge changes in the color of the transmissive material.

Reproduction steps

See webgl_materials_physical_transmission.html and set roughness to 0.8 or greater.

Code

git bisect points to #23450 as the start of this behavior.

Live example

https://threejs.org/examples/webgl_materials_physical_transmission.html

Screenshots

Screenshot 2023-02-12 at 12 03 41 AM

Screenshot 2023-02-12 at 12 03 55 AM

Version

r149

Device

Desktop

Browser

Chrome, Firefox, Safari

OS

MacOS

@WestLangley
Copy link
Collaborator Author

#23450 was in r138. For comparison, see r137.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2023

Thanks for bringing this issue up. I guess I've noticed the same glitch some time ago here: #18629 (comment)

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2023

It seems going back to a fixed resolution like 1024x1024 does the trick.

@WestLangley For testing, can you confirm that the following restores the old behavior?

function renderTransmissionPass( opaqueObjects, scene, camera ) {

		const isWebGL2 = capabilities.isWebGL2;

		if ( _transmissionRenderTarget === null ) {

			_transmissionRenderTarget = new WebGLRenderTarget( 1024, 1024, {
				generateMipmaps: true,
				type: extensions.has( 'EXT_color_buffer_half_float' ) ? HalfFloatType : UnsignedByteType,
				minFilter: LinearMipmapLinearFilter,
				samples: ( isWebGL2 && _antialias === true ) ? 4 : 0
			} );

		}

		//

		const currentRenderTarget = _this.getRenderTarget();
		_this.setRenderTarget( _transmissionRenderTarget );
		_this.clear();

		// Turn off the features which can affect the frag color for opaque objects pass.
		// Otherwise they are applied twice in opaque objects pass and transmission objects pass.
		const currentToneMapping = _this.toneMapping;
		_this.toneMapping = NoToneMapping;

		renderObjects( opaqueObjects, scene, camera );

		_this.toneMapping = currentToneMapping;

		textures.updateMultisampleRenderTarget( _transmissionRenderTarget );
		textures.updateRenderTargetMipmap( _transmissionRenderTarget );

		_this.setRenderTarget( currentRenderTarget );

	}

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2023

I could imagine that with a non-squared dimension, the sampling and processing of the low-resolution mipmap levels does not work as expected 🤔 .

@WestLangley
Copy link
Collaborator Author

@WestLangley For testing, can you confirm that the following restores the old behavior?

Confirmed -- and in conjunction with #25483, it's visually far superior.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2023

But if we restore the POT dimensions, we introduce asymmetrical warping glitches back. They are mentioned in #22009 (comment) and #22731 (comment).

Not sure what is more severe. These glitches or the problem reported in this issue.

@WestLangley
Copy link
Collaborator Author

The artifacts reported in this post are unacceptable, IMO.

I'd have to see a live example of the so-called "asymmetrical warping glitches" to pass judgement on their severity. But I expect they can be mitigated at the app-level by maintaining a canvas that's relatively square (not necessarily perfectly square).

@LeviPesin
Copy link
Contributor

Maybe we can set width and height of the render target to both Math.min( width, height ) or Math.max( width, height )?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2023

The artifacts reported in this post are unacceptable, IMO.

Agreed! Let's revert to POT for now. Maybe find later a solution that works for NPOT, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants