fixed palette FX to more closely match original 1D version #4263
Conversation
- rotation scale is now exactly 180° (divide slider input by 255 instead of 256) - removed shift offset: offset is now zero at slider 0, to hit 128 on touch input devices is really hard - added a 90° shift to input rotation, enabling to rotate from 0 to 180° instead of +90 to -90 (which is not useful in 1D) - changed default settings values to more closely match the old 1D effect
|
There was some initial discussion about this in the original PR: #3683 |
I agree with your reasoning 👍 should be for 0.15.0, to avoid breaking previous behaviour. |
|
@TripleWhy @dosipod can you please check if this looks ok to you and if its the desired improvement? |
wled00/FX.cpp
Outdated
| @@ -1940,7 +1940,7 @@ uint16_t mode_palette() { | |||
| using angleType = unsigned; | |||
| constexpr mathType sInt16Scale = 0x7FFF; | |||
| constexpr mathType maxAngle = 0x8000; | |||
| constexpr mathType staticRotationScale = 256; | |||
| constexpr mathType staticRotationScale = 255; | |||
There was a problem hiding this comment.
rotation scale is now exactly 180° (divide slider input by 255 instead of 256)
Mirroring does work in 2D, so I don't think it is necessary to provide an exact 180° rotation.
In any case this change breaks every other "nice" angle. With this change it is impossible to produce 90°, 45° etc.
So you are adding an additional way to achieve 1 nice angle, and for that trade in 254 angles that were nice before and become ugly now.
Also it was not possible to make it match exactly: the moving direction was inverted. The shifted rotation angle now makes it possible to choose the direction.
If that is really the underlying issue, I'd prefer inverting the shift direction.
There was a problem hiding this comment.
thx for the review! really good point, did not think of that (I ran 1D tests mostly, as that is the issue addressed in this PR).
you are right, with mirroring this can be done also in 1D. So this was an unnecessary change.
| @@ -2016,7 +2016,7 @@ uint16_t mode_palette() { | |||
| colorIndex = ((inputSize - 112) * colorIndex) / 16; | |||
| } | |||
| // Finally, shift the palette a bit. | |||
| const int paletteOffset = (!inputAnimateShift) ? (inputShift-128) : (((strip.now * ((inputShift >> 3) +1)) & 0xFFFF) >> 8); | |||
| const int paletteOffset = (!inputAnimateShift) ? (inputShift) : (((strip.now * ((inputShift >> 3) +1)) & 0xFFFF) >> 8); | |||
There was a problem hiding this comment.
removed shift offset: offset is now zero at slider 0, to hit 128 on touch input devices is really hard
As mentioned, the offset was introduced by @blazoncek. I believe he didn't provide an opinion on removing it again in the previous discussion?
There was a problem hiding this comment.
@TripleWhy FYI I stepped down as a maintainer & developer of WLED and my decisions (past and present) may be overruled.
The original reasoning I had was that for existing/old presets (which lack new effect sliders/options) the default value for sliders/options should not interfere/clash with effect display (i.e. if c1 is unset in preset it's value should be 128 and not 0 which would be similar as if sx is missing).
Looking at the bigger picture (and certain PRs in the queue) quite a few effects are ripe for consolidation and pruning. This will inevitably break compatibility so no matter how you look at it some users will be upset.
My 2c.
wled00/FX.cpp
Outdated
| @@ -1961,7 +1961,7 @@ uint16_t mode_palette() { | |||
|
|
|||
| const int inputShift = SEGMENT.speed; | |||
| const int inputSize = SEGMENT.intensity; | |||
| const int inputRotation = SEGMENT.custom1; | |||
| const int inputRotation = SEGMENT.custom1 + 128; | |||
There was a problem hiding this comment.
Move the +128 into the theta computation:
- These lines here are meant to give the raw input values names for better readability. If you modify the values here, they lose that purpose and now themselves contribute to bad readability because the names no longer speak for their content.
- The offset should only be applied to static rotations, not animated rotations (where it determines the speed, not the offset).
…ft direction - inverting the shift direction in signed int is computationally safe as it is cast into an uint8_t and it matches the original FX in 1D
|
if there are no objections to the latest commit, this is ready to be added |
We frequently get confused beta users because rotation is enabled by default and sliders need to be fiddled with to arbitrary but exact positions to get an approximate match of the original 1D palette effect. Also it was not possible to make it match exactly: the moving direction was inverted. The shifted rotation angle now makes it possible to choose the direction.
This does not fix everything but tries to match it as closely as possible, an exact match is not possible unfortunately.
Existing presets will match worse with this update, but at least it is possible to make them match again. It is basically a question of which version will annoy more users: having an approximate match on old presets but being unable to match it exactly or have a complete missmatch but the possibility to set it the same again.
Another downside: presets made in any 0.15 beta will also not match anymore...
with the new default values, selecting the palette effect will give a close match to the original in 1D but will be different than the 2D beta version up to this point.
If this PR is to be merged it better be done so before 0.15 release. The minimum that should be changed are the default check settings.