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

@remotion/noise #1401

Merged
merged 42 commits into from
Oct 18, 2022
Merged

@remotion/noise #1401

merged 42 commits into from
Oct 18, 2022

Conversation

@vercel
Copy link

vercel bot commented Oct 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
remotion ✅ Ready (Inspect) Visit Preview Oct 18, 2022 at 11:37AM (UTC)

@JonnyBurger
Copy link
Member

Awesome, you nailed even the details! 🙌 Quality work!
I'm planning on doing the final polishing tomorrow and am also considering to simplify the API:

- const noise = createNoise2D('my-seed')(px, frame)
+ const noise = noise2D('my-seed', px, frame)

And cache the expensive initialization via WeakMap.

@satelllte
Copy link
Contributor Author

satelllte commented Oct 17, 2022

Thanks @JonnyBurger. I'm glad you liked it.
Oh, I see what you mean. I din't pay the attention to the function signature initially.
Do you need me to tweak it?

Also, I'd actually consider to leave both implementations:

In first case scenario, user wants to use it on the go:

const n1p1 = noise2D('seed-1', x1, y1);
const n1p2 = noise2D('seed-1', x2, y2);
const n2p1 = noise2D('seed-2', x1, y1);
const n2p2 = noise2D('seed-2', x2, y2);

But on the other hand I think it would be helpful to have the ability to just create the "instanced" noise function which doesn't require seed argument no more:

const myNoise = createNoise2D('my-seed');
const p1 = myNoise(x1, y1);
const p2 = myNoise(x2, y2);

@JonnyBurger
Copy link
Member

@satelllte No action required on your side I think, I'll confirm that by tomorrow and get it shipped myself!

I like it more if there is just one way of doing it, also to make it easier to teach in the future. I prefer the pure functional way over the two-step and think that sometimes the seed is redundant is an okay tradeoff. But if you strongly believe I should not change it, I have an open ear.

@satelllte
Copy link
Contributor Author

@JonnyBurger thanks Jonny. As you wish, it's your repo :)

@JonnyBurger
Copy link
Member

Thanks for understanding! Tried to simplify the example as well, to hopefully make it easy to grasp what noise is all about. I see you contributed to simplex-noise as well, this is awesome!

Ready to ship 🚀 Thanks a lot for this super excellent PR 🙌

@JonnyBurger JonnyBurger merged commit 116f0c9 into remotion-dev:main Oct 18, 2022
@satelllte satelllte deleted the noise branch October 18, 2022 13:41
@JonnyBurger
Copy link
Member

@satelllte Do you remember how long you spent on this issue?

BTW, going to also make a Twitter post about it an honor your packages once I have time to prepare all the announcements 😄

@satelllte
Copy link
Contributor Author

@satelllte Do you remember how long you spent on this issue?

BTW, going to also make a Twitter post about it an honor your packages once I have time to prepare all the announcements 😄

@JonnyBurger I think it's about a day or so. Cool, thank you!

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.

[Techical Debt][TypeScript] create RemotionSeed type Implement a @remotion/noise package
2 participants