search

xeokit / xeokit-sdk

Open source JavaScript SDK for viewing high-detail, full-precision 3D BIM and AEC models in the Web browser.
https://xeokit.io
Other
728 stars 287 forks source link

Viewer#destroy() files to properly clean up resources #215

Closed tothatt81 closed 4 years ago

tothatt81 commented 4 years ago

Hey Lindsay,

This commit seems to have messed with destroying ...uhm... stuff? Not sure what exactly:

https://github.com/xeokit/xeokit-sdk/commit/2ff7791247eb91c799241f2c0a2fecbea3fbdb35?diff=unified

When I tried to reapply the original order found in the previous version, things work as expected.

Please see these sample codes: https://gist.github.com/tothatt81/fccad6089c60943af3105b221c2f85f2 https://gist.github.com/tothatt81/cfa26603102d6243b568b490d06db6e3

If you try them with the current version of the sdk, you'll get some errors that are very likely due to stuff not being destroyed/cleaned up properly. If you undo the changes in the commit shown above, it will work just fine.

Would you please look into it? Thanks!

xeolabs commented 4 years ago

@tothatt81 Oops - fixed in https://github.com/xeokit/xeokit-sdk/commit/d683ff34b68dccde9726e7b0bbd1d9394fe15e97

tothatt81 commented 4 years ago

Yeah, it seems to work now. It's still broken on npm though, right?

xeolabs commented 4 years ago

Still broken on npm, yes - I have a dependent project on npm that needs to be synchronized with this fix. I'll publish the xeokit fix on npm tomorrow, once that's resolved.

xeolabs commented 4 years ago

@tothatt81 I just did the fix in a slightly different way, which works with the examples you've given, and with my dependent project - could you check it on your end? Then if OK I'll publish to npm.

Latest fix: https://github.com/xeokit/xeokit-sdk/commit/6a3bb7c5fccb7ac07556ab1d05fcd8498894fcf6

tothatt81 commented 4 years ago

Fix 1 & 2 together seems fine to me. I also tried with only the second one that doesn't solve the issue for me.

chloesun commented 4 years ago

Hey Lindsay, I just reinstalled xeokit-sdk with npm package, and also has this destory issue, has this been fixed and you are about to update in npm as well?

xeolabs commented 4 years ago

@chloesun , @tothatt81 I just pushed a fix for this to master and npm package v0.9.1

chloesun commented 4 years ago

Hey Lindsay, after upgrading to the latest npm version, this bug happens again

xeolabs commented 4 years ago

Hi Chloe - gotcha, making this fix the priority for next week. Do you basically get a browser memory leak as you repeatedly destroy and re-instantiate the Viewer?

chloesun commented 4 years ago

I just downgraded to a version I know that works... sorry I didn't check the error message when it crashed. You need me to reproduce that? I guess I can upgrade to the latest version again

xeolabs commented 4 years ago

Nah it's all good, I found what's leaking. Some input event handlers not being unbound in the recent CameraControl rewrite. Fix incoming.

xeolabs commented 4 years ago

@chloesun memory leak now should be fixed in master branch and npm @xeokit/xeokit-sdk 0.9.998