vasturiano / three-globe

WebGL Globe Data Visualization as a ThreeJS reusable 3D object
https://vasturiano.github.io/three-globe/example/links/
MIT License
1.22k stars 147 forks source link

Memory leak when updating Globe.tilesData() #49

Closed realtomaszkula closed 2 years ago

realtomaszkula commented 2 years ago

To Reproduce Update the Globe.tilesData() in a setInterval(). After some time it will crash the browser because of the lack of memory.

I've created a simple repo to showcase the issue: https://stackblitz.com/edit/web-platform-85n1dt

It does nothing special, just extends the default demo of the tiles layer with the folowing code:

setInterval(() => {
  Globe.tilesData(
    tilesData.slice(0, Math.floor(Math.random() * tilesData.length))
  );
}, 1000);

If you open up the developer tools, in the memory tab, you will see that the memory usage will keep growing until it crashed the browser. Screenshot 2022-02-28 at 13 51 17 1

Expected behavior It should not crash the browser because of the lack of memory.

Desktop:

The issue is caused by creating a THREE.SphereBufferGeometry object in an animation callback function. https://github.com/vasturiano/three-globe/blob/638f4fa3ae3003e74ca57b157208cc319242dcf9/src/layers/tiles.js#L81-L95 If you comment out the lines 81-89 the memory usage is constant, but the animation stops working (duh).

Ideally the object would be created once, outside of the animation callback, and then only updated in the animation callback. But I'm too big of a Threejs noob to help out here.

I did manage to make it work in my app, by making the following workaround:

  1. call the applyPosition() function outside of the animation, just once.
  2. animate the creating of the tile by tweening the scale of the sphere.

The diff generated by patch-package looks like this:

diff --git a/node_modules/three-globe/dist/three-globe.module.js b/node_modules/three-globe/dist/three-globe.module.js
index ff966a1..252b829 100644
--- a/node_modules/three-globe/dist/three-globe.module.js
+++ b/node_modules/three-globe/dist/three-globe.module.js
@@ -2437,6 +2437,11 @@ var TilesLayerKapsule = Kapsule({
           height: 0
         });

+        var animate = (td) => {
+          obj.scale.x = td.scale;
+          obj.scale.y = td.scale;
+        }
+
         if (Object.keys(targetD).some(function (k) {
           return currentTargetD[k] !== targetD[k];
         })) {
@@ -2445,7 +2450,8 @@ var TilesLayerKapsule = Kapsule({
             applyPosition(targetD);
           } else {
             // animate
-            new TWEEN.Tween(currentTargetD).to(targetD, state.tilesTransitionDuration).easing(TWEEN.Easing.Quadratic.InOut).onUpdate(applyPosition).start();
+            applyPosition(targetD);
+            new TWEEN.Tween({ scale: 0 }).to({ scale: 1 }, state.tilesTransitionDuration).easing(TWEEN.Easing.Quadratic.InOut).onUpdate(animate).start();
           }
         }
       }
vasturiano commented 2 years ago

@zetsnotdead thanks for finding this mem leak case and for your detailed report. Much appreciated.

It should be fixed in the new version of the lib. Please try it out. The cause was that the sphere geometries weren't being properly disposed of before replacing with new ones.

Your suggestion of scaling the sphere is interesting (in fact that's how that layer was initially developed). The issue with it is that you want the tiles to grow sideways on the surface of the globe. While if you scale the sphere object they will rise up from the center of the globe and only become visible in it its final state, while animating the entering objects. That's why it's required to create a new sphere object each time, to achieve the intended visual effect.

realtomaszkula commented 2 years ago

That was quick 😮 I can confirm it works 🎉 Great work! 😍 Thanks a lot.