fix particle brightness distribution with new gamma correction #4710
Conversation
Particle System depends on linear brightness distribution, gamma corrected values are non-linear. - added invers gamma table - reversing gamma after brightness distribution makes it linear once again
WalkthroughThe changes introduce support for inverse gamma correction alongside existing gamma correction. This includes adding inverse gamma lookup tables and accessors, updating particle rendering functions in 1D and 2D systems to apply gamma and inverse gamma corrections, and modifying configuration and settings comments to reflect filling multiple lookup tables. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/colors.cpp (1)
579-584: Optimize inverse table generation.Currently each loop iteration divides by
gamma. Pulling out1.0f/gammaonce reduces redundant operations:void NeoGammaWLEDMethod::calcInverseGammaTable(float gamma) { - for (size_t i = 0; i < 256; i++) { - gammaT_inv[i] = (int)(powf((float)i / 255.0f, 1/gamma) * 255.0f + 0.5f); //inverse - } + float invGamma = 1.0f / gamma; + for (size_t i = 0; i < 256; i++) { + gammaT_inv[i] = (int)(powf((float)i / 255.0f, invGamma) * 255.0f + 0.5f); + } }wled00/FXparticleSystem.cpp (1)
1547-1552: 1D inverse gamma correction properly implemented.The implementation correctly applies inverse gamma correction to both pixel brightness values in the 1D system, maintaining consistency with the 2D approach while adapting to the 1D rendering model.
Given that the PR mentions a minor FPS impact, consider monitoring the performance in particle-heavy scenarios. The additional lookup table operations (gamma8 and gamma8inv) are called for every particle during rendering, which could accumulate in scenes with many particles. If performance becomes a concern, you might consider:
- Caching gamma-corrected values for frequently used brightness levels
- Using approximations for less critical particles
- Implementing a quality setting that allows users to trade accuracy for performance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/FXparticleSystem.cpp(5 hunks)wled00/cfg.cpp(1 hunks)wled00/colors.cpp(1 hunks)wled00/fcn_declare.h(1 hunks)wled00/set.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
wled00/cfg.cpp (1)
wled00/colors.cpp (4)
calcGammaTable(572-577)calcGammaTable(572-572)calcInverseGammaTable(579-584)calcInverseGammaTable(579-579)
wled00/set.cpp (1)
wled00/colors.cpp (4)
calcGammaTable(572-577)calcGammaTable(572-572)calcInverseGammaTable(579-584)calcInverseGammaTable(579-579)
wled00/colors.cpp (1)
wled00/fcn_declare.h (2)
gamma(161-161)gamma(162-162)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (12)
wled00/fcn_declare.h (4)
162-162: LGTM: Inverse gamma table calculation method declaration.The method signature is consistent with the existing
calcGammaTablemethod and follows the established pattern.
164-164: LGTM: Inline inverse gamma lookup method.The implementation mirrors the existing
rawGamma8method and provides efficient table lookup for inverse gamma correction.
167-167: LGTM: Inverse gamma lookup table declaration.The static array declaration follows the same pattern as the existing
gammaT[]array. The 256-byte memory usage for this lookup table is reasonable and documented in the PR.
171-171: LGTM: Convenient macro for inverse gamma correction.The macro provides a clean API consistent with the existing
gamma8(c)macro, maintaining code readability and ease of use.wled00/cfg.cpp (1)
535-536: LGTM: Proper initialization of both gamma and inverse gamma tables.The addition of the inverse gamma table calculation immediately after the gamma table calculation ensures both lookup tables are properly initialized with consistent gamma values during configuration loading. This maintains the mathematical relationship where inverse gamma uses the reciprocal of the gamma value.
wled00/set.cpp (1)
344-345: Ensure inverse gamma table is recalculated alongside gamma.Adding
calcInverseGammaTableimmediately aftercalcGammaTableguarantees both lookup tables stay in sync whengammaCorrectValchanges. This aligns with the new particle system requirements and maintains consistent behavior across effects.wled00/colors.cpp (1)
567-569: Declare inverse gamma lookup table.Introducing
gammaT_inv[256]next to the existinggammaTarray correctly reserves RAM for the inverse gamma table. The placement and naming are consistent with the forward table.wled00/FXparticleSystem.cpp (5)
587-587: Brightness offset adjustment looks appropriate for gamma correction.The reduction from 20 to 5 in the fire mode brightness calculation appears to be a necessary adjustment to compensate for the gamma correction applied later in the rendering pipeline.
603-604: Gamma correction implementation is correct.Applying gamma correction to the brightness before rendering is the right approach. Combined with the inverse gamma correction applied to the brightness distribution values, this ensures linear brightness distribution in the final output.
1479-1480: 1D particle system gamma correction matches 2D implementation.Good consistency - the 1D particle system applies the same gamma correction approach as the 2D system, ensuring uniform behavior across different particle effects.
680-687:✅ Verification successful
Inverse gamma correction correctly applied to brightness distribution.
The implementation properly applies inverse gamma correction to all four pixel brightness values after distribution calculation. This ensures that the brightness distribution remains linear after the full gamma correction pipeline.
Let me verify that the
gamma8invfunction is properly defined and accessible:
🏁 Script executed:
#!/bin/bash # Description: Verify gamma8inv function is defined and accessible # Search for gamma8inv definition ast-grep --pattern 'gamma8inv($_) { $$$ }' # Also search with ripgrep for any definition or declaration rg -A 3 'gamma8inv'Length of output: 1503
Inverse gamma correction is correctly applied and
gamma8invis available.
- Verified
gamma8inv(c)is defined inwled00/fcn_declare.has
#define gamma8inv(c) NeoGammaWLEDMethod::rawInverseGamma8(c).- All four
pxlbrightnessentries are properly adjusted before rendering, ensuring a linear distribution post–gamma pipeline.
680-687:✅ Verification successful
Verify inverse gamma table initialization.
The implementation relies on
gamma8inv()function which requires an inverse gamma lookup table. According to the PR description, this adds 256 bytes of RAM usage.Let me verify that the inverse gamma table is properly initialized when gamma settings are configured:
Also applies to: 1551-1552
🏁 Script executed:
#!/bin/bash # Description: Verify inverse gamma table initialization in configuration and settings # Check cfg.cpp for gamma table initialization rg -A 10 -B 5 'gamma.*table|gamma8' wled00/cfg.cpp # Check set.cpp for gamma settings handling rg -A 10 -B 5 'gamma.*table|gamma8' wled00/set.cpp # Check if calculateGammaTable handles both normal and inverse tables ast-grep --pattern 'calculateGammaTable($$$) { $$$ }'Length of output: 1466
Inverse gamma table initialization confirmed
The inverse lookup table is populated by calls to
NeoGammaWLEDMethod::calcInverseGammaTable(gammaCorrectVal)inwled00/cfg.cppNeoGammaWLEDMethod::calcInverseGammaTable(gammaCorrectVal)inwled00/set.cppNo further changes needed.
|
You could have used calcGammaTable() to fill both gamma and inverse gamma tables instead of adding another function that needs to be called at the same time. for loop is the same for both. |
|
Good point. |
…4710) Particle System depends on linear brightness distribution, gamma corrected values are non-linear. - added invers gamma table - reversing gamma after brightness distribution makes it linear once again Conflicts: wled00/FXparticleSystem.cpp
2 participants
Particle System depends on linear brightness distribution, gamma corrected values are non-linear.
there is no other way to fix this unfortunately. It comes at the cost of a second lookup table (additional 256bytes of RAM used) and a slight hit to FPS in PS effects but its nothing to worry about.
Summary by CodeRabbit
New Features
Bug Fixes
Chores