mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
103.05k stars 35.41k forks source link

View Frustum culling of children does not work #6834

Closed simon-heinen closed 9 years ago

simon-heinen commented 9 years ago

It seems to us that the frustum culling does not work for children of an object in the scene. Here is a simplified problem description:

Our testing devices are mobile devices (the fastest it crashes on an samsung s3 but the same problems seem to occur on desktop machines after the memory limits are reached)

JSFiddle to reproduce the problem: http://jsfiddle.net/vao125wx/13

mrdoob commented 9 years ago

I'm having a hard time to follow your issue description. Can you create a jsfiddle that reproduces the issue?

simon-heinen commented 9 years ago

Sorry I upated my original text and tried to make it a little bit more clear how our tests look like currently. we will provide the example as code in the next hours

simon-heinen commented 9 years ago

Ok here is the jsfiddle to reproduce the problem: http://jsfiddle.net/vao125wx/13 We now show only one image at a time. On a Samsung S3 it can load about 30-40 images before it crashes On a Samsung S5 it can load about 110 images before it crashes

When we don't apply the image as an texture it does not crash ( http://jsfiddle.net/vao125wx/14/ ) so we assume it must be the graphics memory and not the normal device memory which causes the problem. Maybe freeing graphics memory on mobile phones is broken or different? on the desktop we didn't manage to crash it

antont commented 9 years ago

AFAIK visible=false is not even meant to free memory, just to configure whether to render or not. So you can for example quickly temporarily hide it and then show again, with not delay for the reloading in the meantime.

To free memory you need to actually remove the object from the scene and dispose the meshes, textures or both.

http://stackoverflow.com/questions/21851349/freeing-memory-in-three-js is a bit old but gives you the idea.

simon-heinen commented 9 years ago

To release the memory we do exactly what is posted in your stackoverflow link, the visible flag we dont use in the simplified example ( http://jsfiddle.net/vao125wx/13/ ) now, but it still crashes on the mobile devices, because the graphics memory is not freed for some reason

antont commented 9 years ago

I recall seeing an issue recently about memory not being freed, might be a bug in current version. It did work earlier. My flight is leaving now so can't check more - will search for that issue later unless others have provided info already.

WestLangley commented 9 years ago

Removed errors/warnings from fiddle: http://jsfiddle.net/vao125wx/17/

renderer.info.memory.textures is increasing for some reason...

mrdoob commented 9 years ago

Using the dev version in case we indirectly fixed it already...? http://jsfiddle.net/vao125wx/19/

WestLangley commented 9 years ago

Using the dev version in case we indirectly fixed it already...?

renderer.info.memory.textures is increasing using the dev version, too.

gero3 commented 9 years ago

You need to dispose of textures manually because they can be used by multiple materials.

http://jsfiddle.net/vao125wx/21/

WestLangley commented 9 years ago

Thanks, @gero3. Here is an equivalent way:

mesh.geometry.dispose();
mesh.material.map.dispose();
mesh.material.dispose();
scene.remove( mesh );

http://jsfiddle.net/vao125wx/23/

simon-heinen commented 9 years ago

Uh nice, so only this line "mesh.material.map.dispose();" was missing, but I don't get why material.dispose() does not not trigger material.map.dispose() as well, why is that so?

mrdoob commented 9 years ago

but I don't get why material.dispose() does not not trigger material.map.dispose() as well, why is that so?

As @gero3 said, because the texture could be used in another material.

simon-heinen commented 9 years ago

But couldn't the material/texture figure this out itself? Something like a TextureManager which checks such references and releases memory automatically?

gero3 commented 9 years ago

What if you just want to create a new material with the same texture while disposing of the material?? It is pretty hard to predict what the need for textures is.

mrdoob commented 9 years ago

Yes and no. I can easily imagine a use case were someone is changing materials but using the same texture and we're disposing and uploading the texture wastefully.

mrdoob commented 9 years ago

Yep, same as @gero3 said.

mrdoob commented 9 years ago

We should, however, add a note about this in Material.dispose in the docs.

simon-heinen commented 9 years ago

If I run now the latest http://jsfiddle.net/vao125wx/25/ with renderer.info.memory.textures constantly staying at 1 on the Samsung S3 it works much longer (it makes it to 100-120 loaded textures but then still crashes) so there must be another object we do not release :/ Maybe its now not the graphics memory but the normal browser memory?

Usnul commented 9 years ago

Reference counting for textures could be a useful thing to consider?

dubejf commented 9 years ago

http://jsfiddle.net/vao125wx/27/ Convert BoxGeometry to BufferGeometry manually. There is probably a memory leak when letting the renderer take care of the conversion. See #6729. Not a big problem here.

timeline27

http://jsfiddle.net/vao125wx/28/ Clear the THREE.Cache!

timeline28

Also, if you use Chrome, you can monitor the GPU memory usage in chrome://memory

mrdoob commented 9 years ago

http://jsfiddle.net/vao125wx/28/ Clear the THREE.Cache!

timeline28

Good catch!

simon-heinen commented 9 years ago

Oh ok thanks for the tip with chrome://memory :+1: Do I break something in a normal scene with many objects if I frequently clear the cache after removing one of the objects?

dubejf commented 9 years ago

You can clear the cache in your onLoad callback, or in the render loop if you want. You could also monkey patch it if you don't want to use it at all: THREE.Cache.add = function () {}.

gero3 commented 9 years ago

You should be able to enable or disable the cache don't you think so @dubejf

dubejf commented 9 years ago

@gero3 Overwriting THREE.Cache.add does just that, but you could also add .enabled and maybe .maxBytes and .maxItems to remove LRU item, or maybe just shout a warning that that hey three.js has a cache and it's getting huge.

Also, maybe the cache should not be global and be per-loader instance instead, to limit growth to the lifetime of the loader. Previous discussion #5650

WestLangley commented 9 years ago

@simon-heinen How about changing the title and content of your original post so it is descriptive of the issue.

titansoftime commented 9 years ago

This bit me as well. Could not for the life of me figure out where 40MB of ArrayBuffer data was coming from. Turns out THREE.Cache had a retained my CompressedTexture.image data even after disposing of the textures.

Having the ability to enable or disable the cache would be nice (I just replaced the THREE.Cache.add function), but you would have to know about this behavior to begin with.

Perhaps the docs should describe in ImageUtils or somewhere what is happening.

MasterJames commented 9 years ago

How about an argument to cache or not when loading?

dubejf commented 9 years ago

Cache is now opt-in, #6876.