shinyblink / sled

Satanic/Sexy/Stupid/Silly/Shiny LED matrix controller
https://shinyblink.github.io/sled/
ISC License
121 stars 25 forks source link

gfx_voronoi #151

Closed br-olf closed 1 year ago

br-olf commented 1 year ago

I rebased and opened this new pull request which is a successor of https://github.com/shinyblink/sled/pull/150

br-olf commented 1 year ago

Addressing https://github.com/shinyblink/sled/pull/150#pullrequestreview-1201325344

1. Can you rebase on master? There are a lot of commits that have already been merged.

2. `p3`, `p4`, `p5` and `p6` dist functions all seem pretty similar and come down to `powf(dx, n) + powf(dy, n)` if I'm not mistaken. Could you maybe just use that instead?

3. In general, there are a few codestyle nit-picks, I'd like you to change the following:

* Remove the blank lines following function definitions (like in `static void myinit()`)

* Add a space between curly braces and function definitions or keywords (like line 239, 241, etc..)

* Pick whether you want to have the curly braces on the same line or on the next, but please don't mix. Personally, I prefer them on the same line because it's not the 80's and we have wide monitors.

* Unless there is a reason for doing so, could you have the `p` functions and the switch in order? :)

I know it's a lot, but it's all really minor stuff. If you disagree with some of the changes or don't feel like it, it's fine, but let me know. Thank you!!

Thanks for the feedback :)

  1. Rebase is done.
  2. I thought this may be faster but I'm thinking about changing to the pow() function and also moving around the exponent. I'll test this on time.
  3. Fixed. But is it okay for you to have the curly braces on function definitions on the next line but in loops, ifs, ... they stay on the same line. Take a look at a134e545579a5d414dc533f6b5424e4acd3f8af2 and give me feedback.

Overall, I think I'll work a little bit more on this effect like I pointed out in 2 or would you recommend me to move this idea into another effect?

I also think about adding some blur to smooth out the sharp edges. Is there some sort of convolve, fftconvolve or blur already built into sled?

vifino commented 1 year ago

Hey! Sorry for taking a while to get back to you, but here i am.

1) Thank you! 2) Do you have any concern regarding speed? I am not a fan of the "Premature Optimization is the root of all evil" quote, but don't overthink it unless it runs "slow" in your opinion! 3) While it's certainly not my style, it's your code. As long as you are consistent, I'm fine with it.

Feel free to work on it how long you wish, you're the artist! :)

No, I don't think there is anything like that ready to use. Feel free to make a "library" and link it in. Or maybe a header-only library for speed? Measure if it makes a difference or if the functions are big enough that the call overhead doesn't matter.

Thanks!

br-olf commented 1 year ago

I wrote a new voronoi2 effect that now also uses the threadpool to optimize performance. It's my preferred variant of the effect but I didn't want to drop the old voronoi effect completely. So I feel like both variants are ready for mergeing.