-
-
Notifications
You must be signed in to change notification settings - Fork 20.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add particles_updated
and particles_expired
signals to CPUParticles
#61273
base: master
Are you sure you want to change the base?
Add particles_updated
and particles_expired
signals to CPUParticles
#61273
Conversation
eec34e8
to
15e52bd
Compare
particles_updated
signal to CPUParticlesparticles_updated
and particles_expired
signals to CPUParticles
15e52bd
to
78c9283
Compare
for (int i = 0; i < pcount; i++) { | ||
Particle &p = parray[i]; | ||
Ref<CPUParticle3D> cpu_particle = memnew(CPUParticle3D); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is using dynamic allocation for each particle within a particle system per frame, then it is a really really bad idea. Particles should usually be super lightweight data orientated. If they absolutely have to be objects they should be pooled. But really they should just be data imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a Particle struct, but I can't emit it to scripting using a signal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the signal not come from the particle system, rather than the particle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the signal not come from the particle system, rather than the particle?
This is already what happens, but the issue is passing the data to the signal in a way that is compatible with the scripting API. GDScript does not have a struct
type, so type-safe data must be passed via custom objects.
We could emit an array of dictionaries in the signal, but it's not strongly typed. This means there's no autocompletion and errors will only occur at run-time in scripts (instead of compile-time). Parts of the Godot API use dictionaries for that reason, but I feel this is something that should be avoided in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the particle system super inefficient to work around bugs / deficiencies in the scripting seems a case of the tail wagging the dog. Imo it is important the particle system works efficiently, then problems in gdscript can be addressed in further PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a second commit that switches to a Dictionary[]
return type.
This can be used for various effects and even gameplay purposes: - Add lights that follow individual particles. - Add sound effects that follow individual particles. - Add particle nodes that follow individual particles (either CPUParticles3D or GPUParticles3D). - Detect collisions with areas or physics bodies, and make the areas or physics bodies react accordingly. - Individual particle reporting is read-only, so the particles themselves cannot react to collisions this way. - Spawn lights, sound effects or other particles at the location of a particle that just expired. TODO: - Add Max Particles Reported property which limits the number of particles reported in total during the particle's lifetime. This would affect both `particles_updated` and `particles_expired`. Spread out the particles reported over the lifetime smoothly. If set to 0 (default), the signal is never emitted to avoid incurring any performance cost. - Port to CPUParticles2D.
78c9283
to
52536bd
Compare
Rebased and tested again, it works as expected. See OP for updated MRPs, both for the initial commit and the second commit that switches to a |
cd85e65
to
052d43c
Compare
This can be used for various effects and even gameplay purposes:
particles_updated
, which can therefore perform better when you don't need to know particle transforms on every particle tick. If we follow along this design, I expectparticles_collided
to also be a separate signal fromparticles_updated
if CPUParticles collision is implemented.This closes godotengine/godot-proposals#3349.
Testing project: test_cpuparticles3d_signal_dictionary.zip
Testing project for old commit: test_cpuparticles3d_signal_class.zip
Preview
Local/Global coordinates support
cpuparticles3d_signal_1.mp4
Explosiveness
cpuparticles3d_signal_2.mp4
Lights reflected in VoxelGI
cpuparticles3d_signal_3.mp4
Spawning boxes on expiration locations
https://www.youtube.com/watch?v=rOLA_-IZ4jM
TODO
particles_updated
andparticles_expired
. Spread out the particles reported over the lifetime smoothly. If set to 0 (default), the signal is never emitted to avoid incurring any performance cost.