Enable polar noise patterns on more products.

[jasoncoon]

Enable for other products after discussion.

The polar noise patterns should be enabled on any product that has angle and radius (or a radius proxy).

Thoughts on consolidating HAS_POLAR_COORDS and HAS_RADIUS_PROXY completely?

[henrygab]

Additional question:

Should we change the constant data here, to be able to remove both RADII_SCALE_DIVISOR and RADII_SCALE_MULTIPLIER?

It would require:

• Fib512 -- add new uint8_t radiusProxy[] that stores physicalToFibonacci[] / 2 (+512 bytes ROM)
• Fib128 -- add new uint8_t radiusProxy[] that stores physicalToFibonacci[] * 2 (+128 bytes ROM)
• Fib64 -- add new uint8_t radiusProxy[] that stores physicalToFibonacci[] * 2 (+64 bytes ROM)
• Fib32 -- add new uint8_t radiusProxy[] that stores physicalToFibonacci[] * 2 (+32 bytes ROM)

Doing the above, would remove all references to RADII_SCALE_DIVISOR and RADII_SCALE_MULTIPLIER. I also believe it makes the code cleaner.

thoughts?

[henrygab]

You wrote:

Thoughts on consolidating HAS_POLAR_COORDS and HAS_RADIUS_PROXY completely?

Yes, I think removal of the defines for HAS_POLAR_COORDS and HAS_RADIUS_PROXY would be a good thing. Any board that defines HAS_COORDINATE_MAP should simply define the following:

• coordsX[NUM_PIXELS]
• coordsY[NUM_PIXELS]
• angles[NUM_PIXELS]
• radiusProxy[NUM_PIXELS]
• RADII_SCALE_DIVISOR and RADII_SCALE_MULTIPLIER -- but see above comment on removal of these?

Of course, radiusProxy could normally still be a reference to an existing array, to avoid double-storage costs.

At the same time, I think keeping the radiusProxy variable named as such would be good for at least two reasons:

1. It makes it clear that the radius proxy does not have to be the X/Y values converted to a radius. For some props, it's better to have a manually defined proxy for the radius value, rather than having it hard-coded to use the actual radius values.
2. It also avoids any confusion in later-written effects, which might otherwise expect radiusProxy[i] to mean the actual offset from center for the pixel.

I think Kraken64 may not use the actual radius for the pixels. In essence, it's an example of the above.

[jasoncoon]

Should we change the constant data here, to be able to remove both RADII_SCALE_DIVISOR and RADII_SCALE_MULTIPLIER?

Yes, I think so. Whether we call it radius or radiusProxy, I would rather it be explicit, and always in a one byte (0-255) scale, like the other coords.

[jasoncoon]

I think we could actually get rid of the physicalToFibonacci arrays, as the only places they are used should be using either radius or fibonacciToPhysical instead. 😆

I'll try to get these changes made and tested before merging this PR.

[henrygab]

I think we could actually get rid of the physicalToFibonacci arrays, as the only places they are used should be using either radius or fibonacciToPhysical instead. 😆

OK, although the compiler will optimize them away if they are never referenced, so it shouldn't cause any change in the final binary size. Then again, since they are dead code, it makes sense to drop them.

[jasoncoon]

I still need to test on all the different products, but everything builds without errors or warnings.

[henrygab]

Very nice! I like how the standardized radius range (always 0..255) allows simplification of the clock hand calculations, and removing the HAS_POLAR_COORDS is another "Good Thing:tm:"

Nicely done!