TransformControls: Added geometry and material dispose to dispose(). #16065
Conversation
arodic
force-pushed
the
patch/TransformControls-memory-leak
branch
from
d872779
to
7970c45
Compare
arodic
force-pushed
the
patch/TransformControls-memory-leak
branch
from
7970c45
to
0f39a18
Compare
|
Note: this fix hasn't been confirmed yet! Do not merge yet. |
Hint: Consider to do a draft PR in such cases: https://github.blog/2019-02-14-introducing-draft-pull-requests/ |
mrdoob
changed the title
|
Thanks! |
|
|
||
| if ( object.material ) { | ||
|
|
||
| object.material.dispose(); |
There was a problem hiding this comment.
This is going to crash on array materials.
There was a problem hiding this comment.
Disposing objects needs to be consolidated into a single place. For example, this is what I currently have and I re-use it everywhere: https:/infamous/infamous/blob/39ab9e3854f32911c1bc53ac0dca0d1696f4242d/src/utils/three.ts#L10-L61
It is better than writing duplicate dispose code all over the place.
And also, now that I realize based on discussion in #15743, this does not dispose textures (which may be something you intend to do in some cases).
There was a problem hiding this comment.
This is going to crash on array materials.
TransformControls does not assign multiple materials to its renderable 3d (helper) objects so this should be no issue.
There was a problem hiding this comment.
Disposing objects needs to be consolidated into a single place.
I think it's better to define just an interface for disposal operations ( dispose()) and let the respective class decide how to free things. In this way, it's not necessary to centralize component-specific stuff into a single place.
This should fix #15743