What's going on with node.onDispose?

[fenomas]

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?

[adam]

I'm thinking this was for backwards compatibility.

Use:

node.onDisposeObservable.add(callback);

 

[fenomas]

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).

[GameMonetize]

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

[fenomas]

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  

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

[fenomas]

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!

[adam]

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

[GameMonetize]

As long as we protect the backward compat I'm fine

[fenomas]

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.