Analog clock 2d branch0 15

[bandi13]

Added a 2D Analog Clock effect. It needed some antialiased lines and circles, so I've implemented that as well. On my 48x48 pixel display it looks like this:

Also cleaned up some function naming to be more consistent with the majority of the source code.

[blazoncek]

Please revert all unnecessary changes (renamed functions).

[bandi13]

There aren't any unnecessary changes. I needed to create 2 new functions drawCircleAntialiased and drawLineAntialiased. The coding style for the circle functions were an anomaly compared to the rest of the code so I modified them to be like the rest of the code base.

[blazoncek]

You are not requested to change existing code unless necessary.
There is no need to change existing functions.

[softhack007]

Hi, thanks for taking the time to add this nice new effect:-)

If you need anti-aliased pixel drawing, you might want to look into existing variants of setPixelColorXY that take a "float" argument. These functions are already able to perform anti-aliased plotting. It should be simple to create a line drawing/arc drawing routine based on these.

The coding style for the circle functions were an anomaly compared to the rest of the code so I modified them to be like the rest of the code base.

Please focus on the feature you would like to add - I agree with @blazoncek that correcting coding style is not needed. There are other forks that sometimes like to cherry-pick single commits; unfortunately the "coding style fix" will make it very hard to do so for future commits that come after your PR.

[bandi13]

I don't agree with that mentality, but not my repo so I will do as you ask. As for the setPixelColorXY my drawCircleAntialiased function does use that.

Do you want me to add -DWLED_USE_AA_PIXELS into platformio.ini's build_flags as this effect depends on those gates being enabled.

[softhack007]

Coding style: please re-write this for loop into a while{} or do{}while loop that does not rely on existing with break. The gcc optimizer will thank you ;-)

[bandi13]

This function's style is nearly identical to the existing drawLine() including the for (;;) loop.

[bandi13]

Do you need anything from me to merge this PR?

[blazoncek]

Do you need anything from me to merge this PR?

I will be adding some optimisations and modifications later. Once I'm done, you'll check if everything is still working and then it can be merged.

[blazoncek]

I've had a thorough look and test of this new effect and changes made to WLED.

I do not like drawCircleAntialiased() and the issue it creates when you want to draw a thin anti-aliased circle. It cannot do it.
I will update drawCircle() and drawLine() to allow anti-aliased circles and lines that are thin (1 pixel) without using floating point math.

I also do not like the idea that an effect is conditionally compiled. This is unacceptable for single effect IMO.
Another problem with anti-aliased approach taken in this effect is that it looks awful on anything less than 20x20 sized segment so I added an option to disable anti-aliasing while testing and it looked acceptable on 10x10 segments (without seconds hand).

I'll move the PR into draft until AA circle and line are finished and included in 0_15 branch.

[blazoncek]

@bandi13 please review and comment.

[blazoncek]

Reworked & approved

[bandi13]

I left a few comments, most are for my understanding. Seems to work though the default colors are terrible.

[bandi13]

Any way to make the default colors actually correct? On startup, Hour=white, Minute=black, Second=black for me.

[blazoncek]

Unfortunately you can't but I guess you are not using every effect using default values. Or are you?
If you are not, you should consider using presets which save every configuration item.

[softhack007]

Effect metadata format is documented here:
https://kno.wled.ge/interfaces/json-api/#effect-metadata

It looks like you can define default colors in metadata, too.

Edit: you can only specify the names of the 3 colors that appear below the color wheel, sorry.

[bandi13]

The default values should be ones that work for most cases out of the box. When you buy a car/phone/computer/anything it should work out of the box without you having to edit parameters to make it functional. Note that I didn't say customized, just functional out of the gate.

Maybe you could have a special case, that if the Hour=White and Minute=Second=Black that it would set the colors to Red, Green, Blue respectively?

[blazoncek]

When you buy it, yes. If you get it for free, it is just a nice touch.

Maybe you could have a special case, that if the Hour=White and Minute=Second=Black that it would set the colors to Red, Green, Blue respectively?

You can do that in effect initialisation code that starts as:

if (SEGENV.call == 0) {
  SEGMENT.colors[0] = <hour_color>;
...
}

But I would only change those if they a black.
There are examples elsewhere in the code.

The other option is to enhance metadata. That will be a lot more work IMO.

[blazoncek]

Do we pursue this further or do we close? As far as I'm concerned this is completed.

BTW none of the effects have "default" colors but some have default (some even hardcoded) palettes.

[bandi13]

I put in the default colors. Seems to work well enough.

Sorry it took some time, I'm only doing this as a hobby. Weekends = family time. Strictly enforced. ;-)

[blazoncek]

One more thing occurred to me tonight. WLED has "analog clock overlay" feature (for strips) and since segments now can support overlapping (if coded properly) it would be ok IMO to move that overlay code into this effect and make it 1D + 2D.

This would add 2 benefits to analog clock overlay:

1. it could be turned on/off using segment power button (or JSON API) instead of in settings
2. it could only encompass part of the strip instead of entire strip

@softhack007 what do you think?

BTW @bandi13 do you think I'm doing WLED professionally? No, I do not. My sleep and spare time suffer so that I can do WLED. 😀

[bandi13]

Should that overlay code refactor be a separate PR?

[blazoncek]

IMO if this effect (analog clock) can replace existing overlay, no. I'll remove overlay code once effect can handle 1D and 2D.

@softhack007 @Aircoookie your opinion?

[blazoncek]

Reference usermod: #2736