Skip to content
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 finished signal to GPUParticles #76859

Merged
merged 1 commit into from Jul 14, 2023

Conversation

HolonProduction
Copy link
Contributor

@HolonProduction HolonProduction commented May 8, 2023

Followup to #76853.

bugsquad edit: Fixes: #54964

@mr-sihc
Copy link

mr-sihc commented Jul 5, 2023

when?
threaded render ist still broken without this

@Calinou
Copy link
Member

Calinou commented Jul 5, 2023

when? threaded render ist still broken without this

There's no ETA for merging this. 4.1 is in feature freeze, and this pull request needs a review before this can be merged.

@YuriSizov YuriSizov changed the title Add emission_finished signal to GPUParticles. Add emission_finished signal to GPUParticles. Jul 7, 2023
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 7, 2023
@YuriSizov YuriSizov requested a review from a team July 7, 2023 13:02
@YuriSizov YuriSizov changed the title Add emission_finished signal to GPUParticles. Add emission_finished signal to GPUParticles Jul 7, 2023
@HolonProduction HolonProduction force-pushed the emission-finished-gpu branch 2 times, most recently from a6c3b07 to dcff477 Compare July 10, 2023 14:34
clayjohn
clayjohn previously approved these changes Jul 11, 2023
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current iteration looks good to me. I have tested locally with GPUParticles2D and GPUParticles3D and the behaviour appears to match CPUParticles (with #76853)

As we discussed, it would be worth opening a new proposal/PR to deal with the awkwardness of particles not resetting when emitting is toggled off then on again.

Note for mergers, this PR should be merged at the same time as #76853

@clayjohn clayjohn dismissed their stale review July 11, 2023 10:10

One more thing!

@clayjohn
Copy link
Member

One thing I just realized after hitting "approve". Is "emission_finished" the best name for this signal? As it fires after the last particle dies rather than after the last particle is emitted. The way I see it, there are two stages for a one_shot particle 1) Particles emitting (and some active) and 2) no particles emit anymore, but some particles still active. This is fired after 2), but "emission_finished" seems to infer that it signals between 1) and 2)

It would be nice to have feedback from the particles team CC @QbieShay

@QbieShay
Copy link
Contributor

I'd maybe just call it "finished" or "cycle done". What happens if it's not oneshot? will it still fire at the end of each cycle of emission?

@HolonProduction
Copy link
Contributor Author

Im open to renaming the signal. I just took the name from the old implementation in #7887. The signal is one shot only so finished seems fitting.

@HolonProduction
Copy link
Contributor Author

I can extend the documentation to mention that it is one_shot only if desired.

@clayjohn
Copy link
Member

I like "finished". "cycle_done" seems like it would fire every time a cycle finished (which would match this behaviour for one_shot particles, but would fire many times for non one_shot particles).

Let's do two things then:

  1. Rename the signal to "finished" ("finished"already exists as a SceneStringName, so you can just use it and delete "emission_finished"
  2. Add to the docs to clarify when the signal is fired (e.g. "emitted when all active particles have finished processing. When [member one_shot] is disabled, particles will process continuously, so this is never emitted")

@QbieShay
Copy link
Contributor

For more context on my proposal: godotengine/godot-proposals#4770

@QbieShay
Copy link
Contributor

Clarification: my proposal that everything has a "finished" signal is out of the scope of this PR.

@HolonProduction
Copy link
Contributor Author

But we could start by naming this one finished.

@clayjohn
Copy link
Member

My ideal resolution would be:

  1. rename "emission_finished" to just "finished" (delete "emission_finished" from SceneStringName)
  2. Open proposals for other types of signals (e.g. signal on cycle, signal on emission finishing, etc.)

My only concern is the name "finished" might imply that it will signal whenever the particle system finishes, so some users may expect it to fire after a user turns emitting to false and the last particle dies. But I may be overthinking things at this point

@QbieShay
Copy link
Contributor

If that's the case we can find a way to emit the signal at that point too. I think it would make a lot of sense, but it doesn't need to be this PR.

@clayjohn
Copy link
Member

If that's the case we can find a way to emit the signal at that point too. I think it would make a lot of sense, but it doesn't need to be this PR.

Fair enough. We should document that this only gets emitted with one_shot particles, and then we can remove that limitation if there is demand in the future :)

@HolonProduction To summarize, we have circled back to my earlier comment #76859 (comment) if you implement the two things there in this PR and the CPUParticles PR, we should be ready to merge :)

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both this and #76853 should be ready to merge now.

One note, since they both delete "emission_finished" from SceneStringNames, github require rebasing one of the PRs after the other has been merged. But it also might be able to gracefully handle this type of conflict

@YuriSizov YuriSizov changed the title Add emission_finished signal to GPUParticles Add finished signal to GPUParticles Jul 14, 2023
@YuriSizov YuriSizov merged commit 0f7625a into godotengine:master Jul 14, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

But it also might be able to gracefully handle this type of conflict

To my surprise it did!

matorin57 added a commit to matorin57/godot that referenced this pull request Sep 12, 2023
matorin57 added a commit to matorin57/godot that referenced this pull request Sep 12, 2023
Co-authored-by: HolonProduction <holonproduction@gmail.com>
matorin57 added a commit to matorin57/godot that referenced this pull request Sep 12, 2023
Co-authored-by: HolonProduction <holonproduction@gmail.com>
@Gatada
Copy link
Contributor

Gatada commented Oct 18, 2023

In 4.2 beta 1 should the finished signal from a one shot emitter allow me to immediately set emitting = true to get new particles? Cause that is not working in my test project without manually delaying for a bit longer.

Maybe this is more related to #79689?

@clayjohn
Copy link
Member

@Gatada In short, no. A signal is just a notification that something has happened. It doesn't impact the emitting property in any way.

You seem to be describing desired functionality that is the subject of this proposal godotengine/godot-proposals#7322, I suggest you follow up there

@Gatada
Copy link
Contributor

Gatada commented Oct 19, 2023

I suspect I wasn't clear: the finished signal is transmitted at what time? And has it been implemented in beta 1?

In other words: when finished-signal is received, all particles are dead and emitting == false?

@Gatada
Copy link
Contributor

Gatada commented Oct 19, 2023

@Gatada In short, no. A signal is just a notification that something has happened. It doesn't impact the emitting property in any way.

You seem to be describing desired functionality that is the subject of this proposal godotengine/godot-proposals#7322, I suggest you follow up there

Surely the signal isn't emitted without the emitter having changed state? The finished signal isn't very useful if I still risk interrupting or interfering with the particles life-span even after having received it.

The docs states: "Emitted when all active particles have finished processing."

Which I assume means:

• emitting is false
• all particles have expired
• emitter is ready to emit

This however is not the case for beta 1, hence my question: is my assumption wrong?

@HolonProduction
Copy link
Contributor Author

tl;dr
Try using restart() instead of emitting=true.

Since we are talking about GPU particles here the answer is a little bit complicated. Those particles are processed on the GPU and querying the GPU every frame to find out whether the particles are still processing is to performance heavy. The signal is just fired based on a timer with a time estimated from you particle configuration.

However I don't think you issue has something to do with this PR. While I was working on this I also had problems with restarting. If I recall correctly setting emitting=true would only work after a certain time. Which is basically the time your particles need but it does not factor in explosiveness or lifetime randomness. So when using those properties (which I suspect you are doing) there is a time when no particles are visible but restarting this way does not work (idk why just observed the fact; no guaranty has been some time since I worked on this).

Setting emitting to true has no effect if some internals think the system is still running, which I suspect is happening. On the other hand restart tells all internals to restart the system from the start, discarding all previous particles. So using restart should solve the problem. In theory it could also remove existing particles but since finished is emitted when no particles are visible anymore this is not a concern in this case.

@Gatada
Copy link
Contributor

Gatada commented Oct 19, 2023

Key point being: finished is emitted when no particles are visible; restart() is therefore the preferred approach—as invisible particles still being processed can be removed without issues.

Documentation should reflect this. Currently it says "all active particles have finished processing", which is not actually the case. Clarifying this will also make it easier to understand why restart() is the preferred call when pacing matters.

@Gatada
Copy link
Contributor

Gatada commented Oct 19, 2023

Documentation should reflect this.

Made a doc PR to explain the signal more accurately.

@HolonProduction HolonProduction deleted the emission-finished-gpu branch December 9, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulkan GPUParticles3D related error spam with multithreaded renderer
9 participants