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

Reduce slider range for Particles preprocess to discourage setting a preprocess time of 10 minutes #100378

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Dec 13, 2024

Fixes: #100098

When we expose a slider range, the implicit promise to users is that the whole range is valid. Technically that is true, but in this case using the upper range creates a huge performance footgun, as well as a risk of crashing (especially on lower power systems).

Further, the huge range isn't useful. Once you get above 1 or so, you can no longer edit by small increments, so you end up needing to input a value manually anyway.

This PR reduces the range to an amount that allows you to meaningfully adjust the slider to get your desired value, while still allowing users to put in values as big as they want.

Bonus:

While looking at this I realized that it is pointless to set the preprocess time to be greater than the lifetime of the particles because the particles will reset at the end of their lifetime. Accordingly we can modulo this with the lifetime and save a ton of processing time. In the OP the preprocess time was 600, but the lifetime was only 1. So the end result would always look the same as if preprocess was disabled.

I'll open a follow up PR to make that change as technically it is a behavioral change and I want to test my theory without blocking these simple changes

@Calinou
Copy link
Member

Calinou commented Dec 13, 2024

Accordingly we can modulo this with the lifetime and save a ton of processing time.

Shouldn't we use min(lifetime, preprocess_time) instead? If you have particles with a lifetime of 1 second and preprocess set to 1.5 seconds, I'd expect the full lifetime to be preprocessed instead of preprocessing only 0.5 seconds.

@clayjohn
Copy link
Member Author

Accordingly we can modulo this with the lifetime and save a ton of processing time.

Shouldn't we use min(lifetime, preprocess_time) instead? If you have particles with a lifetime of 1 second and preprocess set to 1.5 seconds, I'd expect the full lifetime to be preprocessed instead of preprocessing only 0.5 seconds.

No, the purpose of preprocess is to start your shader mid way through the lifetime of the particles. Using min would totally defeat the purpose of using preprocess altogether. It would guarantee that the particles behaviour is the same as if preprocess wasn't used at all.

@JoNax97
Copy link
Contributor

JoNax97 commented Dec 14, 2024

Would it make sense to expose this as a bounded 0-1 range that repents a % of the particle lifetime instead of absolute time in seconds?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 14, 2024

Using min would totally defeat the purpose of using preprocess altogether.

But the min would ensure only that preprocess doesn't exceed lifetime.

We can also set a dynamic property range based on lifetime.

@clayjohn
Copy link
Member Author

Would it make sense to expose this as a bounded 0-1 range that repents a % of the particle lifetime instead of absolute time in seconds?

Perhaps. My understanding of preprocess is that it is used to get the particle simulation to a certain point before it becomes visible, in which case I think absolute time or absolute frames are necessary for artists to actually control the effect. Making it a % of lifetime means that it would need to manually be adjusted each time that lifetime is adjusted.

Using min would totally defeat the purpose of using preprocess altogether.

But the min would ensure only that preprocess doesn't exceed lifetime.

I should clarify, there are two conditions with a min:

  1. Preprocess is less than lifetime: Min does nothing
  2. Preprocess is greater than lifetime: preprocess does nothing

We can also set a dynamic property range based on lifetime.

I think this would be very nice too, but it won't help people that use RenderingServer.particles_set_pre_process_time() directly. However, it would probably be much nicer for the editor. How do you set a dynamic property range?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 17, 2024

Preprocess is greater than lifetime: preprocess does nothing

Only if explosiveness is 1, so all particles disappear with lifetime. Otherwise you have particles spawned in-between lifetimes and preprocessing full lifetime has visible effect.

@clayjohn
Copy link
Member Author

Preprocess is greater than lifetime: preprocess does nothing

Only if explosiveness is 1, so all particles disappear with lifetime. Otherwise you have particles spawned in-between lifetimes and preprocessing full lifetime has visible effect.

Oh, right. I was confusing a cycle with lifetime. After a cycle all the particles will be back to the same position they would have been in if you only cycled once. Cycling 1.5 times is the same as cycling 0.5 times for example.

Indeed, with explosiveness at 0 (and one shot disabled) then a cycle is 2 * lifetime and preprocessing up to 2 * lifetime will do something.

At any rate, this PR doesn't even change that, so no reason to hold this PR back...

@akien-mga akien-mga merged commit 90555e6 into godotengine:master Dec 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@clayjohn clayjohn deleted the particles-preprocess branch December 18, 2024 16:32
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.

Setting preprocess to 600 on a GPUParticles2D causes it to take forever to load. (4.4 Dev 6)
6 participants