make speed density independent

[cmt218]

Hi @DanielMartinus!

I noticed in my project that devices with low pixel density had much higher particle velocity than I had developed for and realized that this is because velocity is in terms of pixels and not dip in the library. Here is a comparison between 5 inch ldpi and xxhdpi devices with no change on the simple demo:

LDPI: XXHDPI:

as you can see the particles on the ldpi device are traveling 4x as fast as the xxhdpi device. Here is a comparison after the change:

LDPI: XXHDPI:

I would expect that velocity remaining constant across pixel densities is desired behavior, but if you think that some might not want this I would be happy to update the PR so that this is an option passed to the particle system (something like setSpeedPixelIndependent()). Thanks for the great library!

[DanielMartinus]

Hey @cmt218, my bad for not responding earlier to this PR. Thanks a lot for this contribution.

I would expect that velocity remaining constant across pixel densities is desired behavior

You're absolutely right, this is expected behavior to ensure the same effect and experience across different form factors.

but if you think that some might not want this I would be happy to update the PR so that this is an option passed to the particle system (something like setSpeedPixelIndependent()).

Since this behavior is in the library for a long time already I can imagine this would definitely change the desired behavior for some apps since it could change the effect a little bit. If we only do an update with this change and also add setSpeedPixelIndependent it would give other developers some extra time to tweak their animation if any other important updates (such as bug fixes) would follow.

The implementation looks good. If you could add the suggested setSpeedPixelIndependent I'll approve the PR and work on a release.

[cmt218]

@DanielMartinus no worries! Updated this to be configurable via setSpeedDensityIndependent LMK what you think 😄

[DanielMartinus]

@cmt218 tested it and works great!

About the default value for setSpeedDensityIndependent. I was thinking, maybe we should set it by default to true.

The argument for that is that we're sure that everyone implementing it in the future will have the same behavior across all different form factors. The argument against would be that everyone updating to the latest version will visible change their desired behavior probably going unnoticed. They can easily opt-out with the boolean you added.

However, we should not be making non-breaking changes that visibly changes the rendering especially since Konfetti is often implemented in places that don't get tested every day so it can go unnoticed when people update to the latest version.

Option three would be to deprecate setSpeed for now and also offer another function setSpeedIndependent that will later replace setSpeed to make sure it will at some point break for everyone who implemented Konfetti so that they are aware something visibly changes when moving to the new implementation.

Just writing down my thought process here, what do you think of that?
To summarise: Instead of setSpeedDensityIndependent, deprecate setSpeed and add another function setSpeedIndependent that wil later replace setSpeed in a later version breaking Konfetti and forcing people to think about a visible change.

[cmt218]

@DanielMartinus I agree with you 100% that deprecating setSpeed for setSpeedIndependent would be the most ideal to help developers realize that this subtle change in behavior has been made. I actually tried this as my initial approach, but ran in to the problem that acceleration is also affected by device pixel density (even if velocity is scaled to density, the particles on the LDPI device would end up moving noticeably faster than on the XXHDPI device by the time they reach the bottom of the screen).

I could not come up with a satisfying way to also account for acceleration with a new setSpeedIndependent function and found the speedDensityIndependent boolean to be the cleanest approach since it allows us to scale by density in just one place. I do agree that defaulting setSpeedDensityIndependent to true would be best so that everyone has the consistent behavior by default moving forward 👍. What do you think about sticking with this approach, defaulting to true, and calling the behavior change out in notes for the new release? 🤔

[DanielMartinus]

I see, let's go for what you're suggesting indeed and keep it with the boolean but indeed instead defaulting it to true 👍

[cmt218]

Updated! 😄

[DanielMartinus]

Thanks!! 💯