ngl
ngl copied to clipboard
fix: don't dispose structure
This is a fix for a regression introduced in 3405abbd0a8ec1de35aa63d0e90126beb5d07b44.
I went through the code and noticed that in commit 3405abbd0a8ec1de35aa63d0e90126beb5d07b44 the file structure-representation.ts
was changed. Even though the commit was supposed to only update new Typescript rules it changed behaviour, causing the bug.
When updating ngl
in our project from 2.0.0-dev.39
to 2.0.0-dev.40
I noticed that adding new representations after removing old ones didn't work anymore. The old representations were simply removed and new ones didn't appear in the viewer.
It is working as expected in version 2.0.0-dev.39
as you can see in this Codesandbox:
https://codesandbox.io/s/pedantic-dan-z9ycdv
https://user-images.githubusercontent.com/5798652/183840658-6c47f0d5-4a7a-4844-b015-57cb7e3bd640.mp4
After installing version 2.0.0-dev.40
(it happens in 2.0.0
too but that isn't available on npm) it looks like this:
https://codesandbox.io/s/upbeat-danny-o5td2v
https://user-images.githubusercontent.com/5798652/183840902-874b6e74-e0d1-413a-840d-a3660bec973e.mp4
My change keeps the TS change and restores the previous behaviour.
Interesting. The original code deleted both the structure and the view; that change switched to disposing them both. I expect your fix won't cause any problems directly, but it would be good to understand whether it could cause a memory leak (keeping a ref to the structure) before merging.
I ran some benchmarks in Chrome by taking multiple heap snapshots in the Memory Tab of Devtools. I made a page with the fixed ngl version that removes and adds representations every 200ms and let it run over several minutes. Memory usage did not increase, so I assume the QC is doing it's job.

RepresentationElement
visible in the screenshot never changes in shallow or retained size.
Good spot, thanks @sto3psl and @garyo for reviewing. The StructureRepresentation
instance owns a StructureView
instance, created here (which provides the selection behaviour for the representation) and it makes sense to dispose that when clearing up the representation, but the StructureRepresentation
doesn't own the Structure
instance so calling dispose on that is indeed a bug.