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

Question: how to dispose textures cleanly on unmount #533

Closed
brianc opened this issue Apr 16, 2019 · 4 comments
Closed

Question: how to dispose textures cleanly on unmount #533

brianc opened this issue Apr 16, 2019 · 4 comments

Comments

@brianc
Copy link

brianc commented Apr 16, 2019

Howdy - I'm one of the authors of regl-worldview. First of all, thanks for regl...it's great!

I have a question...in our app we have a "map" similar to google maps consisting of a grid of tiles, each w/ an image as a texture. When the user pans the scene we call texture.destroy() on any of the tiles which are too far from the viewport, within the individual tile's componentWillUnmount callback. This works great...until the entire <Worldview /> component unmounts. At that point we call regl.destroy() within the <Worldview />'s componentWillUnmount callback....immediately after this react calls the tiles unmount and we call texture.destroy() on each of the textures from within each tile...but since regl has already cleaned up itself & everything it's managing (including the textures) we get a console.error saying "must not double-destroy texture". In some apps, we unmount and remount <Worldview /> somewhat regularly based on where the user is at in the app. This causes a lot of console.error spamming and floods our monitoring tools w/ error messages.

I put a try/catch around the call to texture.destroy() in an effort to suppress this error but regl has console.error hard-coded into the check function's path.

Would you be interested in a PR to remove the hard-coded console.error and just rely on the throw statement immediately below it?

If not...we were thinking of wrapping the call to regl.destroy() in a requestAnimationFrame within the componentWillUnmount of <Worldview /> which would give children (and grandchildren, etc) time to do any cleanup while they unmount before we destroy regl itself. I'm a bit concerned about doing that though because if we call regl.destroy() in an animation frame callback after <Worldview /> unmounts the canvas element will already be unmounted by react & I don't want to get into a weird state of leaking GPU memory or something by regl not being able to fully dispose the canvas context since the canvas is already removed by the browser. Is it safe to call regl.destroy in an animation frame after the canvas element has been unmounted by react?

@brianc brianc changed the title Question: how to dispose textures Question: how to dispose textures cleanly on unmount Apr 16, 2019
@rreusser
Copy link
Member

Thanks for the detailed description! That seems like a completely valid issue, but just for the sake of argument, is there a reasonable way that you might be able to track destruction of regl on the application side? If you could set regl._destroyed = true, might you be able to check if regl is destroyed when calling texture.destroy() (presuming access to the regl instance where the texture is destroyed, of course)?

My second thought is way grosser… no further comment necessary here, I think :)

function doItQuietly (action) {
  var tmp = window.console.error;
  window.console.error = function () {}
  action();
  window.console.error = tmp;
}
doItQuietly(texture.destroy);

Beyond that, it does seem reasonable to me to rely on throwing rather than forcing things into the console and throwing. My only hesitation is that it's a pretty central error handler so that it might have an effect on things like the readability of glsl errors or something. Without exhaustive investigation it does seem reasonable to consider, though it's at the very least easiest if there's a workaround.

@brianc
Copy link
Author

brianc commented Apr 16, 2019

I understand - I'm hesitant to change regl as well. So, just wanted to open up w/ a question. I was also thinking of possibly switching on a flag in our react context to say regl has been torn down. It's a bit more work but it's the cleanest approach. Haha your "gross" approach was the quick fix we already implemented. 😋

One other option would be to put a flag onto the regl.texture instance itself like texture.destroyed or texture.isDestroyed or set the texture.destroy() function to a noop when regl calls destroyTextures(). I looked at just checking if (texture._texture) { texture.destroy() } but don't like relying on "private" internals of libraries as it's not stable.

I think I'll look at addin a flag to our context for now & thread it down into the map tiles and check it there. Thanks for your thoughts!

Please feel free to close this as we can work around it in our app, but wanted to share w/ you to bring it to your attention and see if we were missing anything. Thanks again. 😄

@deluksic
Copy link

deluksic commented May 4, 2020

Just my two cents, you could also simply mock the error method temporarily ;)

@brianc
Copy link
Author

brianc commented May 13, 2020

yeah that's what I ended up doing. Just monkey-patched over it during dispose.

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

No branches or pull requests

3 participants