Templates - Migrate macros to templates, simplify effects table, add effectId() to all derived effects by davepl · Pull Request #748 · PlummersSoftwareLLC/NightDriverStrip

davepl · 2025-08-18T04:45:59Z

Migrate macros to templates, simplify effects table, add effectId() to all derived effects

I read the contribution guidelines in CONTRIBUTING.md.
I understand the BlinkenPerBit metric, and maximized it in this PR.
I selected main as the target branch.
All code herein is subjected to the license terms in COPYING.txt.

robertlipe

This is just really hard to review because I think it includes pieces of two other sweeping changes (new audio math, new config system) and there are changes unrelated to the goal mixed in. There are thus some entire files I just didn't look at in this PR.

For what I think is the focal point, I like the direction, but I'd like to peel some of the preprocessing away even more. While the templates are thick, the call sites and the flexibility offered in configurations are pretty nice.

It's a good step forward. This PR specifically is messy, but once I ignore the construction mess, I like the direction.

robertlipe · 2025-08-19T01:27:58Z

+
+// Adds a default and JSON effect factory, but the default effect will be disabled upon creation.
+template<typename TEffect, typename... Args>
+inline EffectFactories::NumberedFactory& AddEffectDisabled(EffectFactories& factories, Args&&... args)


Optional food for thought: Is is really worth having separate AddEffectDisabled()s for all of these? Should there perhaps be an additional argument that defaults to kEnabled?

Alternately, use the Named Paramater Idiom so you could invoke AddEffect(blah, arg1, arg2, arg3).disabled()

No idea. I'll defer to Rutger on that!

Short answer: no. Particularly with the Disabled() decorator that one would use with RegisterAll(), the separate Add...Disabled() functions are unnecessary.

In my effectid-template branch, I've already removed them.

rbergen

As (partially) mentioned already in an out-of-band exchange, I think the switch to a template-based approach is an improvement.

I'm approving with some important side notes, and I do think we should make some further changes after this PR is merged.

Specifically:

I did not review changes that are part of PR #746 - I've commented on those changes there (the empty soundanalyzer.cpp probably being the one exception...)
There is a number of effects that do not yet adopt the new approach - I think all those that are not currently pulled into an effect set in effects.cpp. I think we should apply this pattern comprehensively if we choose to apply it.
There are quite a few whitespace-glitches in the modified effects code (Copilot somehow always seems to struggle with this, even when specifically instructed to fix it.) I didn't comment on these; a comprehensive white-spacing clean-up is part of the branch I will mention in my next point.
The per-effect declaration and definition of the effect-specific kId and effectId() members causes a lot of repetitive code, which relies on developer discipline and is prone to copy/paste errors. I've created a effectid-template branch in my fork that proposes a template-based approach for setting an effect's EffectId, which I think is cleaner.
With the increased flexibility in effect set compositions, I think the simple approach of an integer version number is no longer the way to distinguish between a persisted effect set and the "standard" set. I've created a factories_hash branch in my fork to propose a hash-based approach instead.

Both branches I just mentioned can be turned into PRs independently after this PR has been merged; I'll happily take care of any merge conflicts that arise if we do indeed adopt both of them - I'm pretty sure there would be some in effectsupport.h.

rbergen · 2025-08-19T07:56:23Z

 class PatternAlienText : public LEDStripEffect
 {
+public:
+  static constexpr EffectId kId = idMatrixAlienText;


I know this is a matter of preference and opinions vary, but I'd prefer kId to be called Id, or something else without the k - adding it seems like a disclosure of a class-internal implementation decision to me.

This obviously applies to all other effects as well.

rbergen · 2025-08-19T07:59:22Z

 {
 public:
-  PatternRainbowFlag() : LEDStripEffect(EFFECT_MATRIX_RAINBOW_FLAG, "RainbowFlag")
+  PatternRainbowFlag() : LEDStripEffect(idMatrixRainbowFlag, "RainbowFlag")


The kId/effectId() combo is missing here, which you get away with because the effect is currently not included in any effect set.

rbergen · 2025-08-19T08:01:11Z

  public:

-    PatternPulse() : LEDStripEffect(EFFECT_MATRIX_PULSE, "Pulse")
+    PatternPulse() : LEDStripEffect(idMatrixPulse, "Pulse")


Missing kId/effectId() combo.

rbergen · 2025-08-19T08:02:29Z

 public:

-    PatternQR() : LEDStripEffect(EFFECT_MATRIX_QR, "QR")
+    PatternQR() : LEDStripEffect(idMatrixQR, "QR")


Missing kId/effectId() block.

rbergen · 2025-08-20T10:40:04Z


 public:
-  PatternSpiro() : LEDStripEffect(EFFECT_MATRIX_SPIRO, "Spiro")
+  PatternSpiro() : LEDStripEffect(idMatrixSpiro, "Spiro")


kId/effectId() pair is missing.

rbergen · 2025-08-20T10:46:49Z

+
+// Adds a default and JSON effect factory, but the default effect will be disabled upon creation.
+template<typename TEffect, typename... Args>
+inline EffectFactories::NumberedFactory& AddEffectDisabled(EffectFactories& factories, Args&&... args)


Short answer: no. Particularly with the Disabled() decorator that one would use with RegisterAll(), the separate Add...Disabled() functions are unnecessary.

In my effectid-template branch, I've already removed them.

rbergen · 2025-08-20T10:58:05Z

+        return false;
+    }

    // Helper functions for known setting types, as defined in SettingSpec::SettingType


@robertlipe assuming you're talking about std::variant, maybe. With all the testing of which reference is held by the variant and having to align that with the template parameters for the types that are supported, I'm not exactly sure how much cleaner, or clearer, the code would be.

And we're not talking about a dozen overloads, but seven. :)

davepl added 8 commits August 17, 2025 13:13

Change from macros to templates

2b4deb8

Add kId everywhere

f593b46

Make radial fire dynamically update, fix psram array allocator

9b11683

DEFINE to kConst

e0ce809

Move accumulate out of std, various tweaks

b807b67

ELECROW use I2S_AUDIO, trim dead code

b4987d9

Finish adding effectId() to effects, removes belt project

e51a8c4

Fix Init base class call

3417e4f

davepl requested a review from rbergen August 18, 2025 23:52

robertlipe reviewed Aug 19, 2025

View reviewed changes

rbergen mentioned this pull request Aug 19, 2025

Templates #747

Merged

4 tasks

rbergen approved these changes Aug 20, 2025

View reviewed changes

rbergen added 2 commits August 20, 2025 22:18

Merge remote-tracking branch 'upstream/main' into templates

3d2c7ba

Remove empty soundanalyzer.cpp

48a31a2

davepl merged commit 035ad49 into main Aug 21, 2025
141 checks passed

This was referenced Aug 21, 2025

Refactor: Apply Class Template Argument Deduction (CTAD) #749

Merged

Refactor: Centralize polar coordinate calculations for radial effects #752

Closed

Simplify effect ID implementation #754

Merged

robertlipe mentioned this pull request Aug 23, 2025

Refactor: Centralize polar coordinate calculations for radial effects #757

Merged

4 tasks

Uh oh!

Conversation

davepl commented Aug 18, 2025

Uh oh!

robertlipe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbergen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rbergen left a comment •

edited

Loading