Skip to content

Conversation

@WestLangley
Copy link
Collaborator

The intention is for this to replace THREE.EquirectangularToCubeGenerator, but for now, the new class is simply added to the file.

Usage is as follows:

var options = {
	resolution: 512,
	generateMipmaps: true,
	minFilter: THREE.LinearMipMapLinearFilter,
	magFilter: THREE.LinearFilter
};

var rtCube = new THREE.CubemapGenerator( renderer ).fromEquirectangular( texture, options );
   ...

var pmremGenerator = new THREE.PMREMGenerator( rtCube.texture );
   ...

This differs from the existing generator in that it does not modify the input format, type, or encoding.

I feel this is a much simpler API.

/ping @jsantell @richardmonette

@WestLangley
Copy link
Collaborator Author

I also think THREE.CubemapGenerator should be promoted into the library. We will use it to support LDR and HDR equirectangular scene backgrounds.

@richardmonette
Copy link
Contributor

Would be great to see this promoted to the standard library and using THREE.CubeCamera seems like a reasonable way to reduce the amount of duplicate code.

@WestLangley
Copy link
Collaborator Author

Live equirectangular scene background:

https://raw.githack.com/westlangley/three.js/dev-cubemap_gen/examples/webgl_materials_cubemap_dynamic.html

@jsantell
Copy link
Contributor

👍 to the API change and for this being promoted. This would run into the same leak as #15288 though, times 6 as its a CubeCamera. Saving the Scene and CubeCamera per CubemapGenerator instance and replacing the camera's renderTarget on cubemap generation would alleviate that, though.

@WestLangley
Copy link
Collaborator Author

@jsantell Is the leak actually the renderer's responsibility to fix? Also, as you implied, most users will use this utility once.

@mrdoob If this were promoted in to the core library, where should the new file CubemapGenerator.js reside?

@jsantell
Copy link
Contributor

@WestLangley AFAIK, there's no way to dispose the build up cache of WebGLState other than calling WebGLRenderer's dispose, but in my case, the renderer is still in use and do not want to dispose it. I don't know the background of caching WebGLState for every camera/scene pair, but maybe there's a way to render without storing the state, e.g.:

renderer.autoSaveState = false
generator.fromEquirectangular(texture);
renderer.autoSaveState = true;

Not an ideal API, but a combination of unexpected memory growth (it was a discovery to me that a new scene/camera trigger a new WebGLState to be permanently cached), as well as no way to clean up this state made this a particular difficult bug to track down. Fixing this in the renderer is probably the ideal long term fix, but not familiar enough with the WebGLState if that is something possible.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 28, 2018

I don't know the background of caching WebGLState for every camera/scene pair,

This happens right now to ensure correct uniform data of lights (because some of these data are view dependent). three.js defines a single combination of a camera and scene as a WebGLRenderState. A single render state manages its corresponding lighting uniforms with an instance of WebGLLights.

@WestLangley
Copy link
Collaborator Author

Thanks, guys. Hopefully, @mrdoob will chime in...

@mrdoob If this were promoted in to the core library, where should the new file CubemapGenerator.js reside?

@mrdoob mrdoob added this to the r99 milestone Nov 29, 2018
@mrdoob mrdoob merged commit 36e2f63 into mrdoob:dev Nov 29, 2018
@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2018

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2018

@mrdoob If this were promoted in to the core library, where should the new file CubemapGenerator.js reside?

I'm not sure how this pattern would apply later to PMREMGenerator but one option could be this:

var options = {
	generateMipmaps: true,
	minFilter: THREE.LinearMipMapLinearFilter,
	magFilter: THREE.LinearFilter
};
var background = new THREE.WebGLRenderTargetCube( 512, 512, options )
	.fromEquirectangularTexture( renderer, texture );

Hmm, we should rename THREE.WebGLRenderTargetCube to THREE.WebGLCubeRenderTarget also...

However, I always thought that these objects could have a more render-like API...
Maybe is a good time to create new objects?

var mirror = new THREE.WebGLRenderBuffer( renderer );
mirror.setSize( width, height );
mirror.render( scene, camera );

var background = new THREE.WebGLCubeRenderBuffer( renderer );
background.setSize( width, height );
background.renderEquirectangularTexture( texture );

@WestLangley
Copy link
Collaborator Author

@mrdoob Maybe you did not understand my question... I am proposing that THREE.CubemapGenerator be promoted to core. If so, in what directory should CubemapGenerator.js be placed?

@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2018

I was proposing turning THREE.CubemapGenerator into a method in THREE.WebGLRenderTargetCube.

@WestLangley
Copy link
Collaborator Author

I see. I'll file a new PR with your suggestion. :-)

@WestLangley WestLangley deleted the dev-cubemap_gen branch January 11, 2019 23:43
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 3, 2019

I see. I'll file a new PR with your suggestion. :-)

I'm not going to convert THREE.CubemapGenerator into a module and provide the respective TS file if the logic goes to WebGLRenderTargetCube anyway.

BTW: I was never a fan of the current API since the following line

new THREE.CubemapGenerator( renderer ).fromEquirectangular( texture, options );

is actually not consistent to the project's API style. A from*() method should always return this and not a new object (like WebGLRenderTargetCube). It's not a factory method.

@WestLangley
Copy link
Collaborator Author

I was proposing turning THREE.CubemapGenerator into a method in THREE.WebGLRenderTargetCube.

I have done that. I just haven't filed the PR yet.

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.

5 participants