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

Adding Particle System with many new FX #4506

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

Conversation

DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Jan 19, 2025

this replaces the previous PR (#3823) since I made too many changes and a rebase was needed.
Some info on the particle system:

  • effects look best at 40-80FPS
  • all FX can be overlaid: overlay is auto-detected, blurring may not work properly in multi-overlay
  • PS and its buffers reside in segment data and is dynamically managed to keep a small RAM memory footprint (local render buffers are shared between segments)
  • since most FX are random number based, segments do not run in sync
  • 1M and 2M ESP8266 have the particle effects disabled for memory reasons

there are three compile time defines

  • WLED_DISABLE_PARTICLESYSTEM2D -> disables 2D system and FX, automatically set if WLED_DISABLE_2D is set
  • WLED_DISABLE_PARTICLESYSTEM1D -> disables 1D system and FX
  • WLED_PS_DONT_REPLACE_FX -> re-enables the replaced FX (uses 12k more flash)

Replaced FX
to save on flash, effects that have a particle system based replacement are disabled. This saves 12k of flash. Users who want the originals back can custom compile using compile flag.

  • Ghost Rider -> PS Ghost Rider (with new options and faster rendering)
  • Blobs -> PS Blobs (with new options, optional AR functionality and nicer rendering)
  • Fire 2012 -> PS Fire (1D and 2D versions)
  • Fireworks 1D -> PS Fireworks (1D and 2D versions)
  • Dancing Shadows -> PS Dancing Shadows
  • Fireworks Starburst -> PS Starburst
  • Glitter & Solid Glitter -> PS Sparkler (with overlay and settings tweked to users liking)
  • Sparkle -> PS Sparkler
  • Rolling Balls -> PS Pinball with checkmark "rolling"
  • Multi Comet -> PS Pinball with gravity set to 0

Known issues:

  • ESP8266 truncates the FX info JSON, the last added FX have missing sliders
  • changing segment arrangements or changing FX during transitions can somettimes lead to crashes due memory race conditions/pointer issues
  • replaced FX are not 100% identical

Basic Particle system defining particle structs, some emitters and particle to LED rendering
not yet final version but working
update from another commit that got lost
this somehow also got lost from an earlier commit
particle box now is more random in random mode (still a work in progress)
firework is now more configurable by sliders
particle attractor animation does not work, somthing wrong with pointer allocation, it worked with static variables
still unknown, why more than 256 particles are needed in memory allocation to not make it crash, but it works for now
at the expense of more ram usage, animations now have more options for color control (already used in fireworks now)
improved efficiency for stackup (pushback), added code to correctly determine direction if particles meed (probably overkill but now its there)
untested, need to verify it works
…ables to 32bit for faster calculation

32bit variables are faster on ESP32, so use them whenever a variable is used a lot, it saves one instruction per access.
it made very little difference in performance, but for ESP8266 it may matter so it is set permanently there. graphically the difference is also very small (sometimes a particle gets brighter or less saturated)
particle pile-up did not work correctly, now fixed
@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 22, 2025

While there are some effects in AnimARTrix that could possibly be combined into a single effect with suitable parameters, it does feel a bit wasteful of effect IDs to leave the old ones unused in our default builds.

I need to check the order FX are added but if UM FX are added after the "native" ones, there could be a "cleanup" call after adding the last regular FX, removing all unused entries from the vector and freeing the slots for UM FX (not looking at code RN, so not 100% sure this is easy to do).

I probably need to have a play with the new PS replacements to get a feel for how similar they are to get a sense of how likely it is that folks will want to retain them.

I tried to mimic them as close as possible but since its a completely different approach, its not possible. The main difference for most replacements is that the FX are frame and not time based, so speed depends on number of LEDs if FPS are not the same. There will be users that like the new ones and some that want the old ones back.

If we push the legacy ones into a legacy FX usermod, but don't preserve their original effectID would that be acceptable I wonder, provided it was properly documented in the release notes as breaking change?

From my experience users get annoyed if stuff changes in their existing presets, which is understandable so I think the current approach with keeping the IDs serves users better. I will have a look if unused IDs can be freed up to be available for UM FX.

@blazoncek
Copy link
Collaborator

You could have an option to let user choose old vs. new effects. The question is, where to put that option.
I would recommend to create a new settings page intended to configure effects.

There were requests to "permanently" hide unwanted effects and this could be the place to fulfil that as well.

The implementation may not be trivial however, if someone would also want to configure "default" effect values or other (future) "hidden" parameters.

@blazoncek
Copy link
Collaborator

Before I go too deep into review I'd like to mention this is not yet compatible with effect blending.

Namely, handling of palettes during a transition.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 22, 2025

You could have an option to let user choose old vs. new effects. The question is, where to put that option. I would recommend to create a new settings page intended to configure effects.

it is more about code size: there are no other reasons to disable the old effects (apart from having two doing almost the same). But yes, if both new and old are enabled, that config page to select which ones to hide would be useful.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 22, 2025

Namely, handling of palettes during a transition.

I saw that conflict, did not yet look at what the issue is, will try to work something out that works for both.

wled00/FX.cpp Outdated
@@ -29,7 +34,7 @@
uint8_t *fftResult = nullptr;
float *fftBin = nullptr;
um_data_t *um_data;
if (usermods.getUMData(&um_data, USERMOD_ID_AUDIOREACTIVE)) {
if (UsermodManager::getUMData(&um_data, USERMOD_ID_AUDIOREACTIVE)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is an oversight from @netmindz. AR effects no longer use that approach.

The correct way is: um_data_t *um_data = getAudioData();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah possibly missed during one of the rounds on reactoring we did around that. Example perhaps didn't match the actual code exactly so missed from the find and replace

@blazoncek
Copy link
Collaborator

One more thing. I do not know how others feel but IMO we should start splitting FX.cpp. It is becoming unmanageable.
I would split it into FX.cpp with all legacy effects, FX_2D.cpp with 2D effects, FX_AR.cpp with audio effects (including audio 2D) and finally FX_PS.cpp.

platformio.ini Outdated Show resolved Hide resolved
wled00/FX.cpp Outdated
@@ -29,7 +34,7 @@
uint8_t *fftResult = nullptr;
float *fftBin = nullptr;
um_data_t *um_data;
if (usermods.getUMData(&um_data, USERMOD_ID_AUDIOREACTIVE)) {
if (UsermodManager::getUMData(&um_data, USERMOD_ID_AUDIOREACTIVE)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah possibly missed during one of the rounds on reactoring we did around that. Example perhaps didn't match the actual code exactly so missed from the find and replace

wled00/cfg.cpp Show resolved Hide resolved
wled00/FX.h Show resolved Hide resolved
}

// transfer the frame buffer to the segment and handle transitional rendering (both FX render to the same buffer so they mix)
void transferBuffer(uint32_t width, uint32_t height, bool useAdditiveTransfer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not comfortable with this implementation as it moves pixel blending to a second place while not providing "universal" solution.
Segment blending (as opposed to current pixel blending) should be implemented elsewhere (either by introducing triple-buffering or some other (yet undetermined) method).
I specifically do not approve changing blending mode during effect run.

My original idea was to remove calls to NPB/WS2812FX::setPixelColor() in Segment::setPixelColor() entirely and rely on Segment RGBW buffer (AKA setUpLeds()) entirely (also removing any grouping & spacing logic from Segment::setPixelColor().
When all segments are processed (FX fcn called) blend them in WS2812FX::show() using their opacity and blending mode.
Unfortunately this approach will have huge demands on RAM and CPU (moving pixels between buffers) and will not be suited for S2 or ESP8266.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 23, 2025

tested combinations that work with ESP8266 without touching JSON buffer size: the only thing that does not work is enabling both 1D and 2D particle system, which is now prohibited by an #ifdef. Enabling the replaced FX still produces a working UI.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 23, 2025

@netmindz regarding

it does feel a bit wasteful of effect IDs to leave the old ones unused in our default builds.

the way I understand the addEffect() function in FX.cpp it will already fill up the reserved slots with additional FX, given that they call the function with id=255 (which all usermods currently do)

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 26, 2025

Updated to play nice with blending styles. @blazoncek regarding

I specifically do not approve changing blending mode during effect run.

that (intermediate) disabling is necessary to not have one FX fade out and the other fade in as that is not how PS transitions work anymore when "fade" style is set. It is also only disabled during the buffer transfer. I don't see a better way except if another blending style specifically for the PS is added, but that is just confusing. If you have any other good ideas, I am open to suggestions.

@blazoncek
Copy link
Collaborator

If you have any other good ideas

I do not know if it is a "good idea" but am just thinking out loud:
Forget about blending inside PS system. There should be no notion of two separate runs of PS effects to know of each other (although you think it looks really good but it does so in edge cases only). Leave blending (mixing) to strip logic (as mentioned above) or segment logic (inside setPixelColor()).
It may sound counterproductive or counterintuitive but consider this scenario: 32x32 matrix with 5 segments set up, 0,0-16,16; 16,0-32,16; 0,16-16,32; 16,16-32,32 and 0,0-32,32 (or even a more complex one). Each of them running separate effect, the last segment overlays all others. It will be a lot of work for PS system to correctly blend that.

If done outside of PS it is as simple* as color_blend() for each pixel.
* not really that simple but simpler still

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 26, 2025

thanks for the input. I agree it would be better to do blending in one place BUT: blending between FX inside the PS is only done for one segment, not multiple. The reason it does that is to share the buffer AND the particles for one segment: if you had your proposed scenario, each segment would require two render buffers and twice the particles, which would exceed RAM already at 32x32 and multiple segments + transitions. Disabling the "particle blending" is as easy as changing two or three if's as it already disables it when not using 'fade' style. The "two PS FX can interact" part is more of an addition than a necessity, it could be a user selectable option as well.
edit:
I would like to get some user's feedback on the PS as well as the 'blending' and adjust if requested.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 27, 2025

@netmindz feel free to squash-merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants