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

View Frustum culling of children does not work #6834

Closed
simon-heinen opened this issue Jul 14, 2015 · 30 comments · Fixed by #6860
Closed

View Frustum culling of children does not work #6834

simon-heinen opened this issue Jul 14, 2015 · 30 comments · Fixed by #6860
Labels

Comments

@simon-heinen
Copy link

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:

  • We have 1 parent object3d which is set to visible=false and frustumCulled=true
  • We add to this parent 100 textured child-object with 512x512 textures each
  • We switch visible to true in the parent object
  • the renderer crashes because it tries to load all children objects into memory (i guess?) even if they are not in the field of view (is there a way to avoid this?)
  • If we only show x children at once and hide children again it still crashes because the memory is not freed again when a child is not visible anymore (is that correct? how do we free this memory again?)

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 mrdoob added the Bug label Jul 14, 2015
@mrdoob
Copy link
Owner

mrdoob commented Jul 14, 2015

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

@simon-heinen
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

antont commented Jul 15, 2015

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
Copy link
Author

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
Copy link
Contributor

antont commented Jul 15, 2015

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
Copy link
Collaborator

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

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

@mrdoob
Copy link
Owner

mrdoob commented Jul 15, 2015

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

@WestLangley
Copy link
Collaborator

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

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

@gero3
Copy link
Contributor

gero3 commented Jul 15, 2015

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

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

@WestLangley
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Owner

mrdoob commented Jul 15, 2015

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
Copy link
Author

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

@gero3
Copy link
Contributor

gero3 commented Jul 15, 2015

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
Copy link
Owner

mrdoob commented Jul 15, 2015

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
Copy link
Owner

mrdoob commented Jul 15, 2015

Yep, same as @gero3 said.

@mrdoob
Copy link
Owner

mrdoob commented Jul 15, 2015

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

@simon-heinen
Copy link
Author

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
Copy link
Contributor

Usnul commented Jul 17, 2015

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

@dubejf
Copy link
Contributor

dubejf commented Jul 17, 2015

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
Copy link
Owner

mrdoob commented Jul 17, 2015

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

timeline28

Good catch!

@simon-heinen
Copy link
Author

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

@dubejf
Copy link
Contributor

dubejf commented Jul 17, 2015

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
Copy link
Contributor

gero3 commented Jul 17, 2015

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

@dubejf
Copy link
Contributor

dubejf commented Jul 17, 2015

@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
Copy link
Collaborator

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

@titansoftime
Copy link
Contributor

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
Copy link
Contributor

How about an argument to cache or not when loading?

dubejf added a commit to dubejf/three.js that referenced this issue Jul 18, 2015
The global cache should be opt-in. mrdoob#6834
@dubejf
Copy link
Contributor

dubejf commented Jul 20, 2015

Cache is now opt-in, #6876.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants