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

[PoemBot][HOC] Twinkling Stars Foreground Effect #42201

Merged
merged 6 commits into from Aug 30, 2021
Merged

Conversation

epeach
Copy link

@epeach epeach commented Aug 26, 2021

Part of the HoC Block Requests for foregrounds. Randomly seeds a star and fades it in and out (kind of slowly, but I promise, it's happening). Gif shown below:

PeekCirclesPace

If we don't like how this looks yet, we can change: number of stars, color of stars, pace at which they fade in/out.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@epeach epeach requested a review from a team August 26, 2021 01:43
case 'twinkling':
case 'twinkling': {
let stars = [];
const resetStars = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this function (it never gets called)


this.foregroundEffect = () => {
this.p5.push();
if (stars.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here- this never happens since you're not filtering stars out at any point

@ajpal
Copy link
Contributor

ajpal commented Aug 26, 2021

nit: can you change the PR title to be prefixed with [PoemBot][HOC]? that'll help keep everything searchable :)

const color = this.getP5Color(star.color, star.alpha);
this.p5.fill(color);

star.alpha += star.delta;
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine as is, but as an exercise, it could be interesting to turn this into a pure functional approach where the alpha is calculated simply based on which frame number we are up to, rather than doing the sign-flipping and alpha adjustment periodically.

@epeach epeach changed the title PoemBot - Twinkling Stars Foreground Effect [PoemBot][HOC] Twinkling Stars Foreground Effect Aug 26, 2021
Copy link
Author

@epeach epeach left a comment

Choose a reason for hiding this comment

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

New changes look good to me! (I can't approve in the UI since I authored the pull request, but consider this comment approval from my point-of-view).

x: utils.randomInt(0, 400),
y: utils.randomInt(0, 400),
alpha: utils.randomInt(1, 100),
delta: this.p5.random([-6, -5, -4, -3, 3, 4, 5, 6])
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think i understand what delta and the values in this array mean here -- can you add a comment?

@ajpal ajpal merged commit 202725b into staging Aug 30, 2021
@ajpal ajpal deleted the twinkling-starts branch August 30, 2021 22:01
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

4 participants