[Laszlo-reviews] For Review: Change 20100621-maxcarlson-i Summary: Fix warnings with basecomponent tinting

[Max Carlson]

I was hoping to get rid of get/setColorTransform() because the API is 
terrible, relying on a mystery object with a and b properties - a 
frequent source of confusion.

To fix this, I added two color attributes: shadecolor for a/percentage 
values and tintcolor for b/offset values.  This would work for 95% of 
the cases where one would need a color transformation, and it's much 
easier to understand.

There are two problems with this approach: the a/percentage values are 
allowed to exceed 100% (basecomponent.setTint() relies on being able to 
set them to 114%).  Also, this wouldn't allow for negative transformations.

So, we can't really map the transform value to a color, at least without 
a multiplier.  I'm sending out a change that addresses all this...

Regards,
Max Carlson
OpenLaszlo.org

On 6/23/10 11:27 AM, André Bargull wrote:
> Just for clarification what these 'a' and 'b' values are all about:
>
> ActionScript2 docs:
>>
>> Sets color transform information for a Color object. The
>> |/colorTransformObject/| parameter is a generic object that you create
>> from the |new Object| constructor. It has parameters specifying the
>> percentage and offset values for the red, green, blue, and alpha
>> (transparency) components of a color, entered in the format 0xRRGGBBAA.
>>
>> The parameters for a color transform object correspond to the settings
>> in the Advanced Effect dialog box and are defined as follows:
>>
>>     * |/ra/| is the percentage for the red component (-100 to 100).
>>     * |/rb/| is the offset for the red component (-255 to 255).
>>     * |/ga/| is the percentage for the green component (-100 to 100).
>>     * |/gb/| is the offset for the green component (-255 to 255).
>>     * |/ba/| is the percentage for the blue component (-100 to 100).
>>     * |/bb/| is the offset for the blue component (-255 to 255).
>>     * |/aa/| is the percentage for alpha (-100 to 100).
>>     * |/ab/| is the offset for alpha (-255 to 255).
>>
>
> ActionScript3 docs:
>> The ColorTransform class lets you adjust the color values in a display
>> object. The color adjustment or /color transformation/ can be applied
>> to all four channels: red, green, blue, and alpha transparency.
>>
>> When a ColorTransform object is applied to a display object, a new
>> value for each color channel is calculated like this:
>>
>>     * New red value = (old red value * |redMultiplier| ) + |redOffset|
>>     * New green value = (old green value * |greenMultiplier| ) +
>>       |greenOffset|
>>     * New blue value = (old blue value * |blueMultiplier| ) +
>>       |blueOffset|
>>     * New alpha value = (old alpha value * |alphaMultiplier| ) +
>>       |alphaOffset|
>>
>> If any of the color channel values is greater than 255 after the
>> calculation, it is set to 255. If it is less than 0, it is set to 0.
>>
>
>
>
>> + obj.ra = parseInt(color.substring(1, 3), 16),
>> + obj.ga = parseInt(color.substring(3, 5), 16),
>> + obj.ba = parseInt(color.substring(5, 7), 16)
>
> parseInt(color.substring(n, m), 16) is in range [0, 255], whereas the
> multiplier value is in range [-100, 100] (as2) resp. [0, 100] (as3)
> [1,2]. That change is just wrong.
>
> Why is {set,get}ColorTransform() deprecated at all? "tintcolor" does
> seem to be a fully compatible replacement. "tintcolor" just sets the
> offset, it's no longer possible to affect the multiplier value. (LPP-9124)
>
>
>
> [1] the multiplier range is different between as2 and as3 because we're
> using the deprecated Color class instead of ColorTransform in as2.
> [2] the actual range for ColorTransform is [0, 1] in as3, but see
> LzSprite#setColorTransform
>
>
> On 6/22/2010 1:08 PM, P T Withington wrote:
>> Not approved yet.
>>
>> Issues:
>>
>> 1) I don't follow what you are trying to do in LzUtils.  It might be more straightforward to say:
>>
>>
>>>          var color =
>>>            ((parts[0]&  255)<<  16) +
>>>            ((parts[1]&  255)<<  8) +
>>>            (parts[2]&  255);
>>>
>> `&` being a bit operator will force the strings to be 32-bit ints, and `255` will force them to be in range, solving 2 problems at once.
>>
>> 2) I don't understand your color math.  setColorTransform implies that a tint color is made solely of the 'b' components of a transform, yet $lzc$set_tintcolor uses a transform that is made solely of 'a' components.  In converting the basecomponent tint code from using a transform to a tint, you've used yet a third computation (via hsv).  Shouldn't these all be the same conversion?  Shouldn't they be invertible?
>>
>> 3) It seems to me we ought to have a test that ensures that $lzc$set_tintcolor computes a transform matrix, which if I hand to setColorTransform computes the original tintcolor.  I don't believe the code in your change does that.
>>
>> 4) If the basecomponent code is really setting a tint, why is their so much math required to compute the color to set as the tint?  I'd like to see another test that verifies the old and new code result in the same sprite transform for a range of inputs.  I can't deduce from inspection that this is going to be the case.
>>
>> On 2010-06-21, at 19:36, Max Carlson wrote:
>>
>>
>>> Change 20100621-maxcarlson-i by maxcarlson at friendly on 2010-06-21 16:31:42 PDT
>>>     in /Users/maxcarlson/openlaszlo/trunk-clean
>>>     forhttp://svn.openlaszlo.org/openlaszlo/trunk
>>>
>>> Summary: Fix warnings with basecomponent tinting
>>>
>>> Bugs Fixed: LPP-9132 - Regression: examples/animation/animation.lzx?lzr=swf8&debug=true failing to start up
>>>
>>> Technical Reviewer: ptw
>>> QA Reviewer: hminsky, mdemmon
>>>
>>> Details: LzUtils - Ensure LzColorUtils.fromrgb() returns integers when no alpha component is supplied.
>>>
>>> LaszloView - Update docs, update deprecation warning to not include 'this' which caused swf8 to fail to start up.  Updatei tintcolor setter implementation to actually tint.
>>>
>>> basecomponent - Use LzView.tintcolor instead of setColorTransform().
>>>
>>> Tests: examples/animation/animation.lzx runs in swf8 debug mode, runs as before in swf10 also - but without warnings.
>>>
>>> Files:
>>> M       WEB-INF/lps/lfc/services/LzUtils.lzs
>>> M       WEB-INF/lps/lfc/views/LaszloView.lzs
>>> M       lps/components/base/basecomponent.lzx
>>>
>>> Changeset:http://svn.openlaszlo.org/openlaszlo/patches/20100621-maxcarlson-i.tar
>>>
>>
>>
>>