Jump to content

What's going on with node.onDispose?


fenomas
 Share

Recommended Posts

The implementation of mesh.onDispose seems to make it impossible to trigger more than one event.


On the one hand, it's not a regular property - it has a setter but no getter, so you can't hook it like a normal callback:

var old = mesh.onDispose
mesh.onDispose = function() {
    if (old) old() // doesn't work, mesh.onDispose always undefined
    // ...
}

But on the other hand, it's not an event / observable either - there's no way to listen to it, and assigning a callback just overwrites any previous callback:

var mesh = new BABYLON.Mesh('', scene)
mesh.onDispose = function() { console.log('never happens') }
mesh.onDispose = function() { console.log('happens') }
mesh.dispose()
// > happens

 

What is the purpose of this pattern, and how is it meant to be used?

And why not just use a regular property or a regular event?

Link to comment
Share on other sites

Hmm. Okay, I see the Observables docs now, so I get the latter version.

But if onDispose is there for backwards compatibility, why is implemented as a write-only pseudo-property? Isn't this incredibly confusing to users?

I mean, it looks like a regular callback - but if you use it like one, then eventually you'll get weird memory leaks that are incredibly hard to debug (because the second callback you tried to add silently overwrote the first, with no obvious way for the user to know it happened).

Link to comment
Share on other sites

Not sure to follow you.

It should behave exactly like a callback.

if you set node.onDispose = func1 ==> func1 is added to the observable and a link to the observer is kept.

and then node.onDispose = func2==> previous observer is removed from the list (so no one hold a link to the observer so the GC can collect it) and only func2 will be there. Where do you see a memory leak here?

https://github.com/BabylonJS/Babylon.js/blob/master/src/babylon.node.ts#L82

Link to comment
Share on other sites

1 hour ago, Deltakosh said:

It should behave exactly like a callback.

It doesn't! Please see the first example in my first post. Normal callback properties can be hooked, and that's generally the idiomatic way to use them AFAIK.

Whereas, the current "onDispose" looks like a property, and looks like it can be hooked. And code like my first example will look like it's working, until more than one callback gets added to the same mesh. Then callbacks start getting silently thrown away, and the user starts seeing memory leaks. Otherwise there's no way a user will notice that onDispose isn't a regular property.

 

Basically, write-only properties are bad juju. Take it from the folks at Microsoft :D 

It would be better for events like this to be regular properties. (Or if they're deprecated or something, just removed?)

Link to comment
Share on other sites

If code is easier, here is my use case (now fixed to work around the current implementation):

// add mesh to the octree
_octree.dynamicContent.push(mesh)

// function that removes mesh from the octree
var remover = fastSplice.bind(null, _octree.dynamicContent, mesh)

// call remover when mesh gets disposed
if (mesh.onDisposeObservable) {
    // babylon 2.4+
    mesh.onDisposeObservable.add(remover)
} else {
    // babylon 2.3 and below - breaks from 2.4+
    var old = mesh.onDispose
    mesh.onDispose = function () {
        if (old) old()
        remover()
    }
}

 

I don't think it's possible to do the above without separate handling for ~2.3 and 2.4+

Hope this makes sense!

Link to comment
Share on other sites

6 minutes ago, adam said:

So you would be fine if there was just a getter for onDispose, right?

I'm fine regardless now that I'm working around it :)  It's just bad juju for other users.

My guess is that a getter returning _onDisposeObserver.callback would fix things, but I haven't looked inside Observable very closely.

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Loading...
 Share

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...