search

onlaj / Piano-LED-Visualizer

Piano LED Visualizer: Connect an LED strip to your Raspberry Pi and create an immersive visual experience for your piano playing
MIT License
531 stars 112 forks source link

Amalgamate color_mode's into a single location #460

Closed stephen322 closed 1 year ago

stephen322 commented 1 year ago

No need to merge this. But looking for feedback into whether I should run with this idea or not.

Plan is to amalgamate as much color_mode code into a single location as possible, to allow for easier addition of a color_mode for the next person, as well as clean up some hard-to-read logic.

This would require a significant reorganization of code in visualizer.py and settings in various places, but I don't want to step on anyone's toes. I can try it if this would be beneficial.

onlaj commented 1 year ago

I agree, this is what this code needs. I should have started refactoring the code much earlier. The current pattern worked well when it was used to display a smaller number of different color modes. Currently there are too many of them and it becomes illegible.

If you feel that this commit is a good basis for further work, I can merge it into a separate branch.

stephen322 commented 1 year ago

Hmm, up to you on how you want to manage it. I can either build on this, or rebase it once it actually does something and looks closer to its final implementation. Either way is okay with me.

onlaj commented 1 year ago

I guess you can just update this PR. That way I can see your progress and help in case you have trouble understanding the logic of the visualizer.py code. I know it's a bit confusing in some places. For example this part is not needed because the same code is here. I overlooked that part when merging.

edit. the other way around, second is not needed

stephen322 commented 1 year ago

I noticed that as well after the fact. :) I was basically copying the Rainbow template, and I suppose it's needed in Rainbow because of timeshift.

stephen322 commented 1 year ago

This branch is ready for review. Two changes unrelated to color_mode (and light mode) rewrites: 1) Rainbow color timeshift fix under Normal fade/light mode 2) Bug fix websettings: will re-init ledsettings, which can cause crashes/problems.

Plan was to continue amalgamating color_mode settings in a dynamically configured way based solely on xml config, and remove much of the manual configuration in ledsettings, but if I get around to it, it can be a different pull request.