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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add starts_immediately related methods #174

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zArubaru
Copy link

This change makes it possible to create a single burst of particles over a time window, that doesn't fire immediately. My workaround before was to have it inactive at first, but this seemed counter-intuitive, as we have the once function.

Perhaps an additional function we could add, is something like burst_once_over_time, pending for a better name 馃槄

@djeedai
Copy link
Owner

djeedai commented Mar 30, 2023

Hi- Thanks for the PR!

This change makes it possible to create a single burst of particles over a time window, that doesn't fire immediately.

How is that different from the spawn_on_command.rs example which also uses a once spawner and reset() to trigger the spawning when necessary? I'm not necessarily opposed to that API but I'd like to understand what's the use case.

@zArubaru zArubaru force-pushed the feature/spawner-time-manipulation branch from 42ae875 to 9e67d27 Compare March 31, 2023 06:56
@zArubaru
Copy link
Author

zArubaru commented Mar 31, 2023

Hey, thanks for getting back to me!

I added an example, see it here.

I may also be missing something, but as far as I could tell, with the current API in main I could only get this to work with having it deactive on start.

As a direct comparison how it's different to spawn_on_command:

spawn_on_command

... and burst_over_time_on_command (the example in the README.md is intentionally more different):

burst_over_time_on_command

@zArubaru zArubaru force-pushed the feature/spawner-time-manipulation branch from 9e67d27 to 10dc7ff Compare March 31, 2023 07:03
Copy link
Owner

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

That looks like a useful addition, that will probably be cleaner that what I did in #176, however #176 moves some things around so this needs to be rebased.

examples/burst_over_time_on_command.rs Outdated Show resolved Hide resolved
src/spawn.rs Show resolved Hide resolved
@zArubaru zArubaru force-pushed the feature/spawner-time-manipulation branch from c981a0e to 283b1ea Compare April 3, 2023 07:16
Remove `with_time` from `Spawner`, and move `set_time` and `get_time` to
`EffectSpawner`.

Update `burst_over_time_on_command.rs` example.

Change how `starts_immediately=false` is handled (with infinity).
@zArubaru
Copy link
Author

zArubaru commented Apr 4, 2023

@djeedai I made some changes, dropped with_time and added starts_immediately related functionality. See the latest commit (a3ac6ea) :). I tested that all the examples work correctly (and updated my new example also). What do you think?

Also not sure do we really need get_time and set_time but I can imagine there can be some cases where they are useful if someone needs temporal precision 馃槄馃

@zArubaru zArubaru changed the title Add with_time, set_time, and get_time functions to Spawner Add starts_immediately related methods Apr 11, 2023
@zArubaru zArubaru requested a review from djeedai April 20, 2023 07:37
Copy link
Owner

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

I'm still not convinced, sorry 馃槄

  • The added methods for starts_immediately are probably fine.
  • The time ones are... maybe ok
  • The fact that the infinite time has a special effect though, that one I'd like to be more convinced that I am now that it's a good thing. See other comment for details.

@@ -442,6 +465,16 @@ impl EffectSpawner {
self.active
}

/// Set accumulated time since last spawn.
pub fn set_time(&mut self, time: f32) {
self.time = time;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd add at least a debug check for robustness, because passing a negative time will likely break everything.

Suggested change
self.time = time;
debug_assert!(time >= 0.);
self.time = time;

0.
} else {
f32::INFINITY
Copy link
Owner

Choose a reason for hiding this comment

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

So this is nice (or not; see below) because it works with non-once spawners too. However this means there's now an implicit API contract which is that set_time(f32::INFINITY) is equivalent to / duplicates set_starts_immediately(false), which is both unexpected and undocumented. I liked the time-based approach better because this makes sense to users, and can be useful for precise control even beyond the current burst use case.

And on the fact it works for non-once spawners: thinking more about it I think that's a mistake, because this is redundant with the active state of the spawner itself. The once spawner is very special because once it emitted a burst it goes to "sleep" yet is still active, but the other spawners (anything with a finite period) should conceptually emit particles if active, and now with that infinite time trick they don't, and I think that makes things confusing for the user.

Actually, as I was writing, I'm now wondering if any of this is needed. If you want a burst-over-time effect you can simply call new() with non-zero count and time, but leave the period as infinite to get a non-repeating emission. There's no need to manipulate the time, is there?

@djeedai
Copy link
Owner

djeedai commented Apr 20, 2023

Also not sure do we really need get_time and set_time but I can imagine there can be some cases where they are useful if someone needs temporal precision 馃槄馃

It's always good to expose this kind of things, both for testing and for tooling (some future VFX editor), even if not used much at runtime in games/apps, provided it doesn't force any design or behavior that is detrimental to runtime usability/performance. I'd probably keep those.

@zaqxsw-dev
Copy link

zaqxsw-dev commented May 21, 2023

I think you can do it in current api using Spawner or add this new function in Spawner implementation

pub fn until_timeout(count: Value<f32>, spawn_immediately: bool, timeout: f32) -> Self {
        let mut spawner = Self::new(count, 0.0.into(), timeout);
        spawner.starts_immediately = spawn_immediately;
        spawner
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants