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

WebGLTextures: Support custom cubemap mipmaps #17072

Merged
merged 12 commits into from
Aug 7, 2019
Merged

WebGLTextures: Support custom cubemap mipmaps #17072

merged 12 commits into from
Aug 7, 2019

Conversation

liamlangli
Copy link
Contributor

@liamlangli liamlangli commented Jul 23, 2019

Pre-Filtered Environment Map is the typical approach While simulate image base lighting specular reflections roughness.

I have saw that there is another pull request to accomplish this #15014. But haven't been merged because of some simple typo error.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 23, 2019

Can you please remove the modified package-lock.json from your PR?

@liamlangli
Copy link
Contributor Author

resolved

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 25, 2019

AFAIK, there is no official example so far that uses the new code path. I think it would be great to have one.

The missing example was probably one reason why the other PR was never merged.

@liamlangli liamlangli closed this Jul 26, 2019
@liamlangli liamlangli reopened this Jul 26, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 26, 2019

Can you please add the example to files.js? Otherwise it won't be visible in the side bar.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 26, 2019

Both rows of spheres look identical now. Is this the intended result?

@liamlangli
Copy link
Contributor Author

With camera position change, texture roughness will change slightly because of texture mipmap level change.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 29, 2019

Okay, looks good to me then 👍

@WestLangley
Copy link
Collaborator

Okay, looks good to me then

@Mugen87 What is this supposed to demonstrate? I would have expected it would demonstrate typical mipmap usage.

The CubeMap constructor signature has changed? It no longer parallels that of Texture. Is that change needed?

What about docs? How do you define mips for a cube map? How does that compare to textures?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 29, 2019

What is this supposed to demonstrate?

Manual setting mipmaps for uncompressed cube textures.

The CubeMap constructor signature has changed? It no longer parallels that of Texture. Is that change needed?

Well, no. The example does not even make use of this option. Doing customizedCubeTexture.mipmaps = cubeTextures; is totally sufficient.

@WestLangley WestLangley requested a review from Mugen87 July 29, 2019 20:31
@WestLangley
Copy link
Collaborator

What is the format of this file: specular_cubemap_256_luv.bin? It has its own custom loader.

@Mugen87 A Texture does not have a Texture as a mipmap, but here, each CubeTexture mipmap is another CubeTexture. Why do you think that is a good idea?

The code creates 20 materials. Only two are needed. Here is a proposed rewrite for clarity.

function init() {

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

	camera = new THREE.PerspectiveCamera( 50, window.innerWidth / window.innerHeight, 100, 100000 );
	camera.position.set( - 1750, 0, 500 );

	scene = new THREE.Scene();

	//load cubemap

	loadCubeTexture( 'textures/cube/angus/specular_cubemap_256_luv.bin', 256 ).then( function ( cubeTexture ) {

		//model

		var sphere = new THREE.SphereBufferGeometry( 100, 128, 64 );

		var material = new THREE.MeshBasicMaterial( { color: 0xffffff, envMap: cubeTexture } );
		material.name = 'manual mipmaps';

		var offset = 250;
		var originX = - 1150;

		for ( var i = 0; i < 10; i ++ ) {

			var mesh = new THREE.Mesh( sphere, material );
			mesh.position.set( originX + i * offset, - 150, 0 );
			scene.add( mesh );

		}

		//webgl mipmaps

		material = material.clone();
		material.name = 'auto mipmaps';

		var originalCubeTexture = cubeTexture.clone();
		originalCubeTexture.mipmaps = [];
		originalCubeTexture.generateMipmaps = true;
		originalCubeTexture.needsUpdate = true; // this is needed, but I'm not sure why... something is not right

		material.envMap = originalCubeTexture;

		for ( var i = 0; i < 10; i ++ ) {

			var mesh = new THREE.Mesh( sphere, material );
			mesh.position.set( originX + i * offset, 150, 0 );
			scene.add( mesh );

		}

	} );

	//renderer
	renderer = new THREE.WebGLRenderer( { antialias: true } );
	renderer.setPixelRatio( window.devicePixelRatio );
	renderer.setSize( window.innerWidth, window.innerHeight );
	container.appendChild( renderer.domElement );

	//controls
	var controls = new OrbitControls( camera, renderer.domElement );
	controls.minDistance = 500;
	controls.maxDistance = 100000;

	//stats
	stats = new Stats();
	container.appendChild( stats.dom );

	window.addEventListener( 'resize', onWindowResize, false );

}

I allowed zooming. Are the mips supposed to be hot pink?

Screen Shot 2019-07-29 at 8 08 02 PM

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 30, 2019

A Texture does not have a Texture as a mipmap,

Maybe we should change this. Right now, loaders create custom objects which is no good approach.

Are the mips supposed to be hot pink?

I don't see this pink color on my computer. Have you done this screenshot with your proposed example code?

@liamlangli
Copy link
Contributor Author

I have found that specular_cubemap_256_luv.bin will appears pink at windows 10 platform. So I replace that with separated jpeg image file.

And remove the mipmaps argument which require by CubeTexture constructor.

@WestLangley
Copy link
Collaborator

@Mugen87 Yes, using my proposed code on macOS.

@AngusLang Please respond to my code suggestions posted above.

@mrdoob mrdoob added this to the r108 milestone Jul 31, 2019
@liamlangli
Copy link
Contributor Author

liamlangli commented Jul 31, 2019

@WestLangley example code base has been updated with your suggestions.

@WestLangley
Copy link
Collaborator

@Mugen87 I have done all I can on this issue.

I could add that I really do not see the need for 20 spheres in this example when only 2 will suffice.

@liamlangli
Copy link
Contributor Author

The original commit of this example was implement with shader material which set the cube map LOD of each sphere manually. So that you could
found the difference between each sphere in the same row. With built-in material, I
can’t control the LOD of each sphere, so spheres in the same row might be the same.

@WestLangley
Copy link
Collaborator

I understand. Two spheres are now sufficient when zooming is allowed, IMO.

@liamlangli
Copy link
Contributor Author

Ok, I Will remove the unnecessary spheres.

@mrdoob mrdoob changed the title set cube texture with Pre-Filtered Environment Map(mipmaps). WebGLTextures: Support cutstom cubemap mipmaps Aug 7, 2019
@mrdoob mrdoob merged commit f2ddfc7 into mrdoob:dev Aug 7, 2019
@mrdoob
Copy link
Owner

mrdoob commented Aug 7, 2019

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Aug 7, 2019

@AngusLang sometimes the example fails, do you know why that is?

Screen Shot 2019-08-07 at 4 24 55 PM

@liamlangli
Copy link
Contributor Author

I can't reproduce that issue on my computer. But i used to got this warning before use await Promise.all to ensure all the cube maps was loaded. Because width & height of image object was not correct before image loaded, so that webgl will output not-pot warning.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 15, 2019

I can reproduce on the dev branch, too:

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_materials_cubemap_mipmaps.html

As @mrdoob said, it does not break each time you open the example. Sometimes it works, sometimes not. In this case you don't see the second sphere but some warnings in the browser console.

@AngusLang Do you mind having a look at this issue?

@jee7
Copy link
Contributor

jee7 commented Aug 16, 2019

Could this solution also work, when the assigned mipmaps are CubeCamera's render targets?
Let's say I want to calculate the different radiance maps on the fly given an input environment map.

For example like this:
https://codepen.io/jee7/pen/WNexbgQ?editors=0010

I would expect the cube on the right to be teal. With a varying level of brightness depending on zoom (used mipmap). For some reason it is yellow like the left cube, but they should have nothing in common.

@jee7
Copy link
Contributor

jee7 commented Aug 16, 2019

FYI, I tried to get render targets working by copying their contents to the mipmaps through framebuffers, but this caused weird issues. Mipmaps did not work and when I rendered again with a CubeCamera, one or two faces were completely black (or memory noise).

if (mipmaps.length > 0) {
	for ( var j = 0; j < mipmaps.length; j++ ) {
		var mipmap = mipmaps[ j ];
		var mipmapTex = properties.get( mipmap ).__webglTexture;
		console.log(_gl.isTexture(mipmapTex));
	
		var cfb = _gl.getParameter( _gl.FRAMEBUFFER_BINDING );
		var fb = _gl.createFramebuffer();
		_gl.bindFramebuffer( _gl.FRAMEBUFFER, fb );
		_gl.framebufferTexture2D( _gl.FRAMEBUFFER, _gl.COLOR_ATTACHMENT0, 34069 + i, mipmapTex, 0 );
		_gl.copyTexImage2D( 34069 + i, j, glInternalFormat, 0, 0, mipmap.image.width, mipmap.image.height, 0 );
		_gl.bindFramebuffer( _gl.FRAMEBUFFER, cfb );
		//_gl.deleteFramebuffer( fb );
		
		console.log(texture.name, j, mipmap.image.width, mipmap.image.height);
	}
} else {
	state.texImage2D( 34069 + i, 0, glInternalFormat, glFormat, glType, cubeImage[ i ] );
	console.log(texture.name, 'no mipmaps');
}

Kinda like this:
image

Also, the renderer.readRenderTargetPixels() did not work with the created WebGLRenderTargetCube, because needs the format to be RGBAFormat, but the WebGLRenderTargetCube has RGBFormat when it is created by the CubeCamera. When changing the format, there was another error and I gave up.

@jee7
Copy link
Contributor

jee7 commented Aug 16, 2019

Furthermore, if I understood correctly, WebGL wants all the mipmaps down to 1x1. If that is true, then there could be a fallback that if the user does not provide all the mipmaps, but only from 0 to some specific level n, then the rest could be generated from the n-th one.

@mrdoob
Copy link
Owner

mrdoob commented Aug 20, 2019

I can't reproduce that issue on my computer. But i used to got this warning before use await Promise.all to ensure all the cube maps was loaded. Because width & height of image object was not correct before image loaded, so that webgl will output not-pot warning.

That's correct. loadCubeTextureWithMipmaps() doesn't always load the mipmaps in the same order. Needs to be fixed or rewritten.

@liamlangli
Copy link
Contributor Author

liamlangli commented Aug 20, 2019

Yes. I found that example crashed almost every time when you have a bad network situation. The problem is that i assume that bigger size texture alway resolve before smaller size textures, that was not correct.

I will add PR to fix this.

@liamlangli
Copy link
Contributor Author

#17286

@jee7
Copy link
Contributor

jee7 commented Aug 20, 2019

I tried copying the render target texture solution again and it did not produce the problem I had before.
So, would it be possible to add the check that if the mipmap is a reference to the render target texture on the GPU, then the library does the copyTexImage2D approach (copying the texture to the mipmap on the GPU via framebuffer) instead of texImage2D?

@Mugen87 Mugen87 changed the title WebGLTextures: Support cutstom cubemap mipmaps WebGLTextures: Support custom cubemap mipmaps Aug 25, 2019
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.

None yet

5 participants