Fix GPUParticles3D emitting finished signal on ready

[TCROC]

Fixes a bug introduced by PR: #92089

The bug being that all gpu_particle_3ds emit the finished signal on ready.

Discussion in rocket chat can be found here: https://chat.godotengine.org/channel/vfx-tech-art/thread/WhhDnzLmTke5vYHF9

Details are as follows.

@QbieShay 's pr changes this method:

void GPUParticles3D::set_one_shot(bool p_one_shot) {
	one_shot = p_one_shot;
	RS::get_singleton()->particles_set_one_shot(particles, one_shot);

	if (is_emitting()) {
		if (!one_shot) {
			RenderingServer::get_singleton()->particles_restart(particles);
		}
	}
}

To

void GPUParticles3D::set_one_shot(bool p_one_shot) {
	one_shot = p_one_shot;
	RS::get_singleton()->particles_set_one_shot(particles, one_shot);

	if (is_emitting()) {
		if (!one_shot) {
			restart();
		}
	}
}

Specifically the part RenderingServer::get_singleton()->particles_restart(particles); being changed to restart(); is what introduces this bug. If we inspect the restart method, we see that some fields get set:

void GPUParticles3D::restart(bool p_keep_seed) {
	if (!p_keep_seed && !use_fixed_seed) {
		set_seed(Math::rand());
	}
	RenderingServer::get_singleton()->particles_restart(particles);
	RenderingServer::get_singleton()->particles_set_emitting(particles, true);

	emitting = true;
	active = true;
	signal_canceled = false;
	time = 0;
	emission_time = lifetime * (1 - explosiveness_ratio);
	active_time = lifetime * (2 - explosiveness_ratio);
	set_process_internal(true);
}

These fields are then evaluated in void GPUParticles3D::_notification here:

if (time > active_time) {
	if (active && !signal_canceled) {
		emit_signal(SceneStringName(finished));
	}
	active = false;
	if (!emitting) {
		set_process_internal(false);
	}
}

But you still may be wondering, who calls the set_one_shot method? That gets called by the constructor as can be seen by my debugger:

In summary, the issue is:

constructor calls set_one_shot -> calls restart -> sets fields incorrectly indicating particle system started -> GPUParticles3D::_notification sees that the particle system isn't running but incorrectly thinks it was started so it incorrectly emits the finished signal.

By reverting void GPUParticles3D::set_one_shot it goes back to behaving how it was before. To be thorough, I checked GPUParticles2D and can confirm this is how it is currently implemented over there so there is no bug with GPUParticle2D.

Thank you for the help debugging this @QbieShay! ❤️

[TCROC]

I discovered the issue because I have a script that emits all child particles when the parent particle emits his finished signal. As a result, all child particles emit on scene load.

@tool
extends Node3D
class_name GPUParticleSubeEmitterGroup3D

func _ready() -> void:
    (get_parent() as GPUParticles3D).finished.connect(on_parent_finished);

func on_parent_finished() -> void:
    for child in get_children():
        child.restart()
        if child is GPUParticles3D:
            child.emitting = true

@QbieShay realized the appropriate fix and I dug in to find the details as to why :)

[clayjohn]

This is going to need to update the seed still otherwise the one shot particle will look the exact same every time which is a change in behaviour

[TCROC]

This is going to need to update the seed still otherwise the one shot particle will look the exact same every time which is a change in behaviour

Makes sense. I'll go update it to update the seed

[TCROC]

@clayjohn It has been done 😎

[QbieShay]

@clayjohn i'm not sure it's necessaary, this happens in the setter of oneshot and not on reset

[TCROC]

Whatever the Godot powers that be command of me, I shall do 😎

[TCROC]

I do agree with @QbieShay though. I personally don't care that much about it one way or the other. Yes doing this introduces (imo) an unnecessary call to randomize the seed since this is just a setter. But I don't call set_one_shot often (never actually in my case) to care one way or the other. So I didn't bother bringing it up as I'd rather the PR just go through. But if I am to offer some input, I do agree with @QbieShay that randomizing the seed should be left to the restart method rather than this setter. In most cases, doing it here will be redundant.

[TCROC]

So I guess with that said, @clayjohn do you like it as it is (your suggestion) or would you like me to go back to the way the pr was (@QbieShay and my suggestion).