Value Curve in Spiral Wrap V10 Broken

[byaky]

Just downloaded V10.  Noticed that my existing sine wave value curves on my Spiral Wraps do not change to the same full degree/angles as previous xLight versions.


Sent from my iPhone using Tapatalk

[kevinp]

I noticed another issue with the Value Curves after the recent fix for another problem with the value curves.  I opened a issue ticket on Github for Keith to look into.  Maybe the two are related.

[keithsw1111]

I will take a look when I get a chance. Some behaviour change may be necessary because the code was wrong before but it is also possible I broke something.


Sent from my iPhone using Tapatalk

[Gilrock]

Keith I checked in the debugger and when using a flat value curve set to 11.0 the GetValueCurveInt call returns 11 but without the value curve and using 11.0 in the textbox the function returns 110.

I also noticed when I set the value curve to 10.0  got a 9 so we may want to add a round function when the float is converted to an int.

[Gilrock]

Yeah I think most of us designed our effects to work with the undivided value.  Like my Fan rotations it returns me a 360 which represents 360 degrees in all my calculations but it shows as 1.0 revolution on the slider.  So that divisor is causing it to return a 1.0 when its a value curve when I still need to be getting a 360.

[keithsw1111]

I am not so sure Gil. The text box has the divided value. And my looking into the spirals effect I think the behaviour is now actually the correct behaviour for the algorithm.

I am less sure about other effects but Spirals I am pretty sure is ok.

[Gilrock]

No it's broken.  If you don't get the same return value from the GetValueCurveInt call when there is no curve vs having the exact same value in the curve then it's not right.  The problem is you changed RenderableEffect to call GetOutputValueAtDivided whereas all the effects were already designed to work with GetOutputValueAt.  You can't divide the value when it's a value curve and then not divide it when it's the textbox value.  GetOutputValueAtDivided is not called when the curve is not active.  If you force it to be called all the time then all the effects that used it will need to be modified.  What was the original problem you were trying to solve because I'm pretty sure my effects that used a decimal point were already working before that code change.

[keithsw1111]

But the text box value is already divided. I am not 100% on this yet but I am pretty close and the easiest test is how kevin found it in the first place. Set the value curve to flat ... and check the value returned matches when the value curve is off.


Sent from my iPhone using Tapatalk

[Gilrock]

I tested yesterday and the text box value is not being divided.  For instance a Fan effect defaults to 2.0 revolutions.  The GetValueCurveInt function returns 720 when the curve is not active and it returns 2 when it is active.  That's a problem.  It's been returning 720 all along until the recent code change which is causing it to return a 2 when the curve is active because its dividing 720 by the 360 divisor.  If you divide it then I lose precision and would never be able to set fractional rotations unless I change the effect to use GetValueCurveDouble and then I multiple the divisor back into it so that the current logic will work.

[Gilrock]

But the text box value is already divided. I am not 100% on this yet but I am pretty close and the easiest test is how kevin found it in the first place. Set the value curve to flat ... and check the value returned matches when the value curve is off.

Yes that's exactly what I'm saying...they don't match.

[keithsw1111]

I have applied some fixes in .11 ... it turns out both Gil and I were right ... in some circumstances.