Skip to content

Conversation

@WestLangley
Copy link
Collaborator

...as proposed in #13752 (comment), plus some clean up.

Calling the dispose() methods significantly frees up gpu memory.

@WestLangley
Copy link
Collaborator Author

Unrelated to this PR...

I am thinking that this pattern is a bit too much:

var pmremGenerator = new THREE.PMREMGenerator( hdrCubeMap );
pmremGenerator.update( renderer );

var pmremCubeUVPacker = new THREE.PMREMCubeUVPacker( pmremGenerator.cubeLods );
pmremCubeUVPacker.update( renderer );

hdrCubeRenderTarget = pmremCubeUVPacker.CubeUVRenderTarget;

hdrCubeMap.dispose();
pmremGenerator.dispose();
pmremCubeUVPacker.dispose();

Pehaps we can hide all of that, and free up memory when we are done, via a simpler API:

var pmrem = new THREE.PMREM(); // Extends WebGLRenderTarget

pmrem.setFromCubeMap( cubeTexture, renderer ); 

pmrem.setFromEquirectangularMap( texture, renderer ); // alternate

material.envMap = pmrem.texture;

/ping @bhouston

@bhouston
Copy link
Contributor

bhouston commented Apr 10, 2018

I like the below:

var pmrem = new THREE.PMREM(); // Extends WebGLRenderTarget

pmrem.setFromCubeMap( cubeTexture, renderer ); 

pmrem.setFromEquirectangularMap( texture, renderer ); // alternate

material.envMap = pmrem.texture;

I can view it as a few separate operations:

  • Convert EquirectangularMap to CubeMap. This is useful outside of the PMREM convolution method. So probably should eventually be separated.
  • Convolve CubeMap into a CubeMap mipmap chain.
  • Convert CubeMap mipmap chain into a CubeUV2 texture. (The UV Packer)

I would actually suggest killing the idea of the PMREM class and replacing it with CubeTextureFactory/CubeTextureUtility class that has these functions on it. I am sure there are others we could add (CubeMap To EquirectangularMap, AngularMap to/from CubeMap, CubeMap to MapCap, CubeMap to/from SphericalHarmonic, etc.)

I think that having them each as separable atomic operations would make it clearer and easier to use, and also lead to more reuse.

@bhouston
Copy link
Contributor

How about:

// converters
CubeMapConverter.equirectangularMapToCubeTextureMap()
CubeMapConverter.cubeTextureMapToEquirectangularMap()
CubeMapConverter.cubeTextureMapToCubeUV2Map()
CubeMapConverter.cubeTextureMapToSphericalHarmonics3()
CubeMapConverter.sphericalHarmonicsToCubeTextureMap()

// convolver?
CubeMapConvolver.convolveCubeMap()

Basically I view the above as more precise and atomic.

@bhouston
Copy link
Contributor

bhouston commented Apr 10, 2018

Or another alternative design would be to follow the Math class examples, where we avoid utility/factory classes and just have a class for each different interpretation of data:

EquirectangularMap em = new THREE.EquirectangularMap();

CubeTexture cubeTexture = new THREE.CubeTexture();
cubeTexture.setFromEquirectangularMap( em );
cubeTexture.setFromSphericalHarmonics( sh );

CubeTexture pmremCubeTexture = new THREE.CubeTexture();
pmremCubeTexture.setFromConvolvedCubeMap( cubeTexture );

CubeUV2Map cubeUv2 = new THREE.CubeUV2();
cubeUv2.setFromCubeMap( pmremCubeTexture);

SphericalHarmonics3 sh = new THREE.SphericalHarmonic3();
sh.setFromCubeMap( cubeMap );

I have to say the above seems the most Three.JS like API - very obvious/simple and usable. Takes the mystery out of the admittedly intimidating "PMREM" name.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Apr 10, 2018

@bhouston Thank you for the suggestions.

For now, this PR allows us to continue to use the existing utilities, and in the HDR example, optionally dispose of ~ 500 temporary geometries and textures.

Is this OK to merge for now?

@mrdoob mrdoob added this to the r92 milestone Apr 12, 2018
@mrdoob mrdoob merged commit dd542a4 into mrdoob:dev Apr 12, 2018
@mrdoob
Copy link
Owner

mrdoob commented Apr 12, 2018

Thanks!

@WestLangley WestLangley deleted the dev-pmrem_dispose branch April 13, 2018 01:08
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.

3 participants