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

Change count logic to density logic #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cyrilchapon
Copy link

@cyrilchapon cyrilchapon commented Dec 9, 2022

Hi,

Reading the warning about mobile performance; it came to me the idea that count is practically a nonsense in a resizable environment.

When implementing "snow" effect for example; what you want is a constant visual density, instead of a constant "number of flakes". Thus, I implemented a new density logic. I defaulted this to keep the same count as of today on a classic fullscreen desktop container (1400 * 660).

It will recalculate the count, which is now dynamic based on density x resolution.

It's also less dangerous on mobile; based on the naïve principle of "a larger screen resolution basically often mean a stronger GPU calculation capacities".


This is a breaking change, non backward compatible

@simeydotme
Copy link
Owner

hi @cyrilchapon , I understand and like this modification ... however, what if we didn't make it breaking/incompatible?

if I'm not mistaken, this line;

this.count = Math.ceil((this.resolution / BASE_RESOLUTION) * this.settings.density);

could be changed to;

this.count = Math.ceil(
  typeof this.settings.density !== "undefined" ? 
    this.resolution / BASE_RESOLUTION * this.settings.density : 
    this.settings.count
  );

which would allow for anyone using count to continue doing so?

@cyrilchapon
Copy link
Author

Hey @simeydotme,

This is not stupid at all; I'd slightly change it though, maybe making (and documenting) the opposite (density prioritized over count), to make density the new-normal; but keep backward compatibility with count. Maybe marking count deprecated in the code, but still working.

(the reason being simple : density being way less dangerous than count for lower resolution, despite the warning on homepage)

@simeydotme
Copy link
Owner

I wrote that quite late, but I think it is doing just as you propose; if density is undefined, then use count. but if density is defined; use density. and all the other code (default density, and document density) remains. so new users will be using density, and old users will have no problems.

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

2 participants