Disposing a particle system with subEmitters not dispose all subEmitters

When disposing a particle system with subEmitters, I found some issues:

  1. The attached subEmitters are not disposed.
  2. The existing end type subEmitters are not disposed, and there is not a option to do so.

This is a playground:

When you click the button to dispose the particle system, you could notice that the attached subEmitters still exist. The end type subEmitters are still there until they end. If you uncomment the code I added below, you will see that the issued are fixed.

I think the first question is a bug needs to be fixed by default.

As for the second, we could add an option to choose whether to dispose existing end type sub emitters if I want. Also for the purpose of backwards compatibility. Or maybe we could just make it default also.

This is what I think is a disposing, dispose everything, lol.

I noticed that you are in “code-freeze” stage right now, so I don’t know if it’s appropriate to open a pull request. You could just close it if it bothers you.

The fixed playground:

this might help:

It’s not from me, it’s from my guru :grin: evgeni:

I agree there’s a bug, but from a user standpoint, I would expect that dispose would simply remove the particle system, meaning all particles would disappear right away… Having some particles still alive while the particle system has been removed/destroyed feels wrong to me.

However, it’s how things work currently, so we must keep it for backward compatibility. But I think the new flag passed to dispose should trigger the right behavior: remove everything.

Let’s ask @PatrickRyan and @sebavan their opinion on the subject.

1 Like

Hi, the stop doesn’t help. The attached subEmitters are not disposed, nor even stoped.

Yes, they are. It’s only your ‘first’ (particleSystem1) that isn’t. Sorry though, I rushed the PG a little. This one should dispose of all.
https://playground.babylonjs.com/#9NHBCC#33 :pray:

1 Like

@mawa Yes, your code worked this time. But it’s still by disposing them manually, which I think is a bug, right?

Your solution seems quite hack, lol.

By the way, these code I commented in the playground also works:

        // particleSystem.particles?.forEach((particle) => {
        //     if (particle._attachedSubEmitters) {
        //         for (let i = particle._attachedSubEmitters.length - 1; i >= 0; i -= 1) {
        //             const subEmitter = particle._attachedSubEmitters[i];
        //             subEmitter.dispose();
        //         }
        //     }
        // });

Honestly, I was thinking about this and thinking about replying… but I’m not sure yet. Eventually, I could imagine a use case where keeping with the subemitter for a while (later removing it on a timeout or depending on context) could be something useful… although I admit I do not have this use case in mind at this moment :grin: Still, I appreciate the versatility of the framework (it’s one of the thing I like most) even if it means some additional operations or some ‘weird’ coding/instructions at time. I think it will be good if other people will also give it a second thought before we proceed :thinking: Of course, my opinion only :smiley: Meanwhile, have a great day :sunglasses:

1 Like

Yes, I suppose it does.

1 Like

@mawa Actually I agree with you that there might be some situations the existing particles need to be last a while. In the other hand, I also think the function that disposing all existing particles is an important feature. (But the fact that the attached subEmitters exist forever is definitely a bug. :rofl:)

I also agree with you that the versatility is great. But I think having rich features and ease of use are also important. Of course, my opinion only :smiley:

Wish you a great day too! :sunglasses: :sunglasses: :sunglasses:

1 Like

I agree with @Evgeni_Popov that while the current behavior does feel like a bug, we can’t remove it for back compat sake. There may be someone who saw the sub emitters still alive when the parent emitter is disposed and used that to their advantage. If it were a bug that broke the render loop, I would say to fix it entirely, but this is more of a “shouldn’t work like this” type of bug that we can’t confirm if anyone is using or expects to work this way. Adding a flag to dispose all when the parent emitter is disposed and kill all particles immediately seems like the right move here. Users would want to make sure that their sub emitters have eclipsed their lifecycle and have waited long enough for all particles to die before disposing to make sure particles don’t snap out of existence, but this seems like a logical workflow to dispose the complex system by disposing the parent.

1 Like

So I did not dispose the subEmitters on purpose:) the goal was to let the user decide but I totally agree that it should at least be an option.

Why letting the user decide? Initially I was imaging an use case where the main system could be a bullet and when the bullet hits a wall an explosion subsystem would be triggered (so the bullet is disposed but not the subsystems)

3 Likes

Agree with @Deltakosh too… there’s no way to say how everyone will want to use sub-emitters. Flexibility is usually the rallying cry around here.

1 Like

@PatrickRyan @Deltakosh Agree with both of you, too. Backward compatibility is important.

I made this playground that have an attached sub-emitter as a firework trailing. When I dispose the main particle system, the trailing will not be disposed and will exist forever.

I think it’s because our code just make the sub-emitters disposeOnStop=true and stop them only when the particle they attach is dead:

if (particle.age >= particle.lifeTime) {
    // Recycle by swapping with last particle
    this._emitFromParticle(particle);
    if (particle._attachedSubEmitters) {
        particle._attachedSubEmitters.forEach((subEmitter) => {
            subEmitter.particleSystem.disposeOnStop = true;
            subEmitter.particleSystem.stop();
        });
.....

Clearly it will not work when we dispose a particle system, all the particles in the particle system are disposed. The code above will not be executed. So all the attached sub-emitter are remained and will never be disposed. This is what I think is a bug.

You could try in the play ground by using stop or dispose in line 132 respectively. You will see the difference.

As for the existing end type sub-emitters, I totally agree with both of you that there must be some situations that need them exist until dead. So I add a option to the dispose function in my pull request. Let the user choose. It’s also a good prompt to users that how we handle the particles, by telling them they could choose whether to dispose the end type sub-emitters. The solution to dispose of them may not be easy to find (The loop must iterate in reverse):

            if (this.activeSubSystems) {
                for (let i = this.activeSubSystems.length - 1; i >= 0; i -= 1) {
                    this.activeSubSystems[i].dispose();
                }
            }

So I think it’s a good choice to add it to our code. :grinning:

1 Like

We could have a property like “dispose the particle system when there is no active particles anymore”?Or, maybe, instead of having a new property, it could be a new parameter to the stop method?

When set, we dispose all sub-emitters so that they can’t kick in anymore, and we “wait” (in fact, we continue to render the particle system) for the active number of particles to reach 0 before calling dispose?

This would account for the case where we want the particle system to continue working as long as there are particles to display. Would that work in your case @Luke_Lee?

In that case, we would update dispose to really dispose of everything. It would be a breaking change, but the current behavior is a bug in my opinion. Or we add a flag to dispose to trigger this behavior.

1 Like

I’m not going to add my two cents on top of it. Pretty much everything has been said, so I guess I just wanted to say how much I appreciate how things are handled in my favorite toy :smile: :child:
I trust you Guys will make the right decision.

2 Likes

OK, I agree with your solution.

In summary, there might be 3 situations that we need to deal with:

The sub emitters:

  1. Have a manualCount and still exist when dispose. They will dispose themselves when they are dead.
  2. Have a manualCount and already are disposed themselves when dispose.
  3. No manualCount. They will exist forever cause we handle the disposal only when the particle they link (attached and end type) is dead, which isn’t gonna happen, like I said above.

What we could do respectively:

Situation 1:

  1. Do nothing, then they will dispose themselves when they are dead.
  2. Dispose them immediately when dispose the system.

Situation 2: There is no need to do anything.

Situation 3:

  1. Do nothing, then they will exist forever until the users do it manually.
  2. Dispose them immediately when dispose the system.

So there are two options we have to handle the problem:

  1. Do nothing.
  2. Dispose them immediately.

Then we could add two options for the dispose function

dispose(disposeTexture = true, disposeAttachedSubEmitters = false, disposeEndSubEmitters = false)

As for the situation that the sub emitters will exist forever in Situation 3, just let the users choose.

If you guys agree with that, I will update my pull request. Thanks. @PatrickRyan @Evgeni_Popov @Deltakosh @mawa

We do already have a property to dispose a particle system with no more particles: disposeOnStop

Particle System Examples | Babylon.js Playground (babylonjs.com)

1 Like

Hi, I don’t think they are the same thing. As I mentioned above, there are some situations (linke 3 above) that the particles will not trigger the stop moment when dispose. Then the disposeOnStop will not work.

I beg to differ, If all the systems are flagged like in my PG (disposeOnstop and targetStopDuration set) then all the particle systems will be disposed automatically

But again I’m not pushing back on your intended change. I think it is a good one (the last one you mentioned with ‘dispose(disposeTexture = true, disposeAttachedSubEmitters = false, disposeEndSubEmitters = false)’)

2 Likes