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

fix(Snowflake): Fixed decimal random generation and opacity updates #84

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

Conversation

KirillTregubov
Copy link

@KirillTregubov KirillTregubov commented Dec 31, 2024

Fixes #83

This PR adds two related improvements that I discovered while playing around with the opacity property.

  1. Fix decimal random number generation.
    The current decimal random number generation does not work as intended. The current implementation uses Math.random() * (max - min + 1) + min to generate a pseudorandom number in the range min (inclusive) to max (inclusive). Most importantly, the +1 part is what allows the range to be inclusive of the maximum. However, when returning decimals, this results in the largest outputs being off by up to 1. In other words, when users currently supply opacity={[0, 0.5]} , this library sets the opacities between 0 and 1.5 (specifically, 1.499 with 9 repeated).
    You could do some trickery to keep the max inclusive, including replacing the 1 with 0.005 or Numbers.EPSILON, but I am not satisfied with such approaches and feel it is simpler to make random() return numbers exclusive of the maximum when dealing with decimals.

  2. Update opacities to reflect new range when changed.
    Currently, when the opacity range is changed, the snowflakes are not affected. From my understanding, the snowflake image opacity is only set once, upon initialization. Therefore, even if the opacity config is changed, the snowflake opacities only reflect the range that was specified when they were initialized.
    I found two approaches to solve this issue. The first, before commit #9cc6669, re-generates all the snowflake opacities once a new opacity range is detected. The second way (current) sets a hasNextOpacity flag during the opacity config change, and the snowflake's opacity is only re-generated when this flag is set and it is reset to the top of the canvas.
    The first aoproach is more reactive, while the second one is more gradual and maintains a consistent snowflake opacity while it's visible. You should consider both approaches before settling on a behaviour.

Additionally, I wanted to mention that the codebase needs to be formated with prettier (it is out-of-date), and your eslint is broken because eslint-config-prettier and eslint-plugin-prettier need to be updated to latest.

Feel free to leave comments for me to make any changes or additions to the code before accepting and merging this PR.
Cheers!

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.

opacity not works
1 participant