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

Texture bug on r137 #23353

Closed
Karbust opened this issue Jan 27, 2022 · 16 comments · Fixed by #23361
Closed

Texture bug on r137 #23353

Karbust opened this issue Jan 27, 2022 · 16 comments · Fixed by #23361

Comments

@Karbust
Copy link

Karbust commented Jan 27, 2022

Describe the bug

DDS and PNG textures lose all color, I don't know how to really explain, I attached screenshots bellow. Since I only use these 2 types of textures, I can't test other types.

If the browser has DDS support, it will be used, otherwise fallback to PNG. This happens on all browsers and devices I tested.

To Reproduce

Steps to reproduce the behavior:
Go to either of the live examples below and press on a weapon or amor icon, the differences will be visible.

Code

A code snippet of how I'm using it.

		const manager = new THREE.LoadingManager(() => {
			const loadingScreen = document.getElementById('loading-screen')
			if (loadingScreen) {
				loadingScreen.classList.add('fade-out')

				loadingScreen!.addEventListener('transitionend', onTransitionEnd)
			}
		})
		manager.addHandler(/\.dds$/i, new DDSLoader())

		const mtlLoader = new MTLLoader(manager)
		mtlLoader.setPath(url!)
		mtlLoader.load(`${model}.mtl`, (materials) => {
			materials.preload()

			const objLoader = new OBJLoader(manager)
			objLoader.setMaterials(materials)
			objLoader.setPath(url!)
			objLoader.load(`${model}.obj`, (object) => {
				object.scale.setScalar(1 / 200)

				object.traverse((child) => {
					child.frustumCulled = false
					if ((child as any).material instanceof THREE.MeshPhongMaterial) {
						(child as any).material.side = THREE.DoubleSide
					}
				})
				scene.add(object)
			}, undefined, (error) => console.error(error))
		})

Live example

Screenshots

Public version with r136:
Public version with r136

Dev version with r137
Dev version with r137

Platform:

  • Device: Desktop, Mobile
  • OS: Windows, Android
  • Browser: Tested on Vivaldi (both Desktop and Android), Chrome, Firefox and Edge
  • Three.js version: r137
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 27, 2022

This issue is related to the latest alpha change, see #23230.

The problem is that your asset is blended with the HTML background since for some reasons the DDS texture holds alpha values < 1.

One workaround to solve the issue is to add this block to your onLoad() callback:

object.traverse( function ( child ) {

     if ( child.material ) child.material.alphaWrite = false;

} );

This will define the material as opaque by setting a 1 alpha value to all fragments.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 27, 2022

Here is a complete code example that should load the sword as expected. Note that since r137 you have to use an sRGB workflow when loading assets with OBJ/MTL loader. That means you need renderer.outputEncoding = THREE.sRGBEncoding;.

import * as THREE from 'three';

import { MTLLoader } from './jsm/loaders/MTLLoader.js';
import { OBJLoader } from './jsm/loaders/OBJLoader.js';
import { DDSLoader } from './jsm/loaders/DDSLoader.js';
import { OrbitControls } from './jsm/controls/OrbitControls.js';

let camera, scene, renderer;

init();
animate();

function init() {

	const container = document.createElement( 'div' );
	document.body.appendChild( container );

	camera = new THREE.PerspectiveCamera( 45, window.innerWidth / window.innerHeight, 1, 2000 );
	camera.position.set( 250, 250, 0 );
	camera.lookAt( 0, 0, 0 );

	// scene

	scene = new THREE.Scene();
	scene.background = new THREE.Color( 0xcccccc );

	const ambientLight = new THREE.AmbientLight( 0xcccccc, 0.4 );
	scene.add( ambientLight );

	const pointLight = new THREE.PointLight( 0xffffff, 0.8 );
	camera.add( pointLight );
	scene.add( camera );

	// model

	const manager = new THREE.LoadingManager();
	manager.addHandler( /\.dds$/i, new DDSLoader() );

	new MTLLoader( manager )
		.setPath( 'models/obj/temp1/' )
		.load( '00270.mtl', function ( materials ) {

			materials.preload();

			new OBJLoader( manager )
				.setMaterials( materials )
				.setPath( 'models/obj/temp1/' )
				.load( '00270.obj', function ( object ) {

					object.traverse( function ( child ) {

						if ( child.material ) child.material.alphaWrite = false;

					} );

					scene.add( object );

				}  );

		} );

	//
	//

	renderer = new THREE.WebGLRenderer();
	renderer.outputEncoding = THREE.sRGBEncoding;
	renderer.setPixelRatio( window.devicePixelRatio );
	renderer.setSize( window.innerWidth, window.innerHeight );
	container.appendChild( renderer.domElement );

	//

	window.addEventListener( 'resize', onWindowResize );

	const controls = new OrbitControls( camera, renderer.domElement );

}

function onWindowResize() {

	camera.aspect = window.innerWidth / window.innerHeight;
	camera.updateProjectionMatrix();

	renderer.setSize( window.innerWidth, window.innerHeight );

}


//

function animate() {

	requestAnimationFrame( animate );
	render();

}

function render() {

	renderer.render( scene, camera );

}

@Karbust
Copy link
Author

Karbust commented Jan 27, 2022

Perfect, it worked.

I was reading the migration guide but I wasn't able to pinpoint where the problem was coming from.

Thank you.

@Karbust Karbust closed this as completed Jan 27, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

Makes me wonder if this line:

alphaWrite: material.alphaWrite || material.transparent,

Should be this instead:

alphaWrite: material.alphaWrite && material.transparent,

This stuff hurts my brain a bit.

/cc @donmccurdy

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 27, 2022

I was reading the migration guide but I wasn't able to pinpoint where the problem was coming from.

I've added a note that OBJ/MTLLoader now requires a sRGB workflow and also the alphaWrite solution.

But I fear many user will not understand that 3D objects are potentially blended with the HTML background and thus don't understand the note in the migration guide. It seems these cases will require some support.

@Karbust
Copy link
Author

Karbust commented Jan 27, 2022

alphaWrite: material.alphaWrite || material.transparent,

Makes me wonder if this line:

alphaWrite: material.alphaWrite || material.transparent,

Should be this instead:

alphaWrite: material.alphaWrite && material.transparent,

This stuff hurts my brain a bit.

/cc @donmccurdy

It didn't work, I edited it on the node_modules folder, also tried setting it to false, it stayed the same... (I kept the sRGBEncoding, only removed the alphaWrite's on the my side).

image
image

image

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

Oh, hmm...

Then maybe the problem is that material.blending is set to NormalBlending by default.

Maybe if transparent === false it should use NoBlending internally?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 27, 2022

Having a different value of material.blending will not solve this particular issue. If the shader writes fragments with alpha values < 1, they will be blended with the HTML background. The only way to prevent this is to write/have 1 alpha values in the default framebuffer.

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

You're right: https://jsfiddle.net/zfgebmux/

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

So if transparent === false we have to add gl_FragColor.a = 1.0 chunk at the end then.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 27, 2022

Maybe we should reconsider #23166 (comment) for a long-term and glTF compliant solution.

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

So if transparent === false we have to add gl_FragColor.a = 1.0 chunk at the end then.

Hmm, I already tried that.

And now that we are setting alpha: true in the context I'm re-reading this conversation with new eyes...

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

As far as I can see the only issue of #18631 is that it broke this use case:

Confirmed. This breaks AdditiveBlending when texture.a < 1 and material.transparent is false.

In this new world of alpha: true, I think I'm totally okay with breaking that use case.

And then we'll be able to get rid of material.alphaWrite.

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2022

Fixed: https://jsfiddle.net/wsuhyr52/ 😬

@mrdoob
Copy link
Owner

mrdoob commented Jan 28, 2022

@Karbust If you update to 0.137.4 you shoudn't need the alphaWrite code anymore.

@Karbust
Copy link
Author

Karbust commented Jan 31, 2022

@Karbust If you update to 0.137.4 you shoudn't need the alphaWrite code anymore.

Correct, it's working without alphaWrite. Thank you

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 a pull request may close this issue.

3 participants