I still can't approve because of the following 2 issues. Issues:
1) You still have the situation that set_tintcolor is constructing a colortransform that maps the tint to offsets, yet set_colortransform updates tintcolor by reading out the multipliers. This can't be right. set_tintcolor is cheating because it calls set_colortransform and then smashes the tintcolor that the transform created with the actual value. There should be an assertion in set_tintcolor that after calling set_colortransform the value of tintcolor === the value you are trying to set. 2) If the point of tintcolor is to do the same thing as was done in basecomponents, then maybe we should just copy the algorithm from there and have basecomponents use tintcolor directly. I'm not a color scientist, and I don't play one on TV, but from what I've read, our use of `tint` in basecomponents (and in view now) is non-standard. It seems basecomponents has a _very_ narrow application, which is a grayscale asset that it is colorizing. It's more like the color is the base and the grayscale is creating 'tints' of the base color. I think this is why it works with the multiplier. Comment: 1) I just have one curiosity: Why do we encode alpha as a fraction? As you say, this is fragile (it's effectively a feature of binary floating point representaions of decimal fractions). Why not encode the alpha in the 'top byte' like it is done in most hardwares? Seems like we are asking for trouble here... On 2010-06-24, at 20:24, Max Carlson wrote: > Change 20100623-maxcarlson-M by maxcarl...@friendly on 2010-06-23 16:46:00 PDT > in /Users/maxcarlson/openlaszlo/trunk-clean > for http://svn.openlaszlo.org/openlaszlo/trunk > > Summary: UPDATED: Correct 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: Updating to address Tucker's comments: > > 1) I'd like to see us use a more modern format like the as3 colorTransform > (i.e., name the fields {red,green,blue}{offset,multiplier} instead of > {r,g,b}{a,b}). Since you are adding a new API here (the colortransform > attribute), let's use the opportunity to make the type saner. The old, > deprecated API can still take the old format -- it should be simple to map to > the new format. > > Done. > > 2) I think in rgbatoint you should clip each of the input values (& 0xff) > just for cleanliness. > > Done. > > 3) findAlpha looks wrong. Elsewhere alpha is encoded by `fraction = alpha / > 25500` but in findAlpha you are decoding with `alpha = fraction * 25600`. > > This breaks the rounding, which is quite fragile. I added a comment about > needing to do this. > > Otherwise: > > Updating to address comments from Andre and Tucker. Since there is no good > replacement for LzView.setColorTransform(), I moved it to a setter. I also > fixed the tinting implementation to work consistently across swf8 and swf10. > > LzUtils - Ensure LzColorUtils.fromrgb() returns integers when no alpha > component is supplied. > > LzUtils - Update/refactor LzColorUtils > > swf/LzSprite - setColorTransform() converts multipliers to percentage > > swf9/LzSprite - setColorTransform() takes values literally > > LaszloView - Update docs, add colortransform setter to be used in place of > setColorTransform(), update tintcolor when possible. Update tintcolor setter > to tint correctly in swf8 and swf10. > > basecomponent - Use LzView.colortransform instead of setColorTransform(). > > Tests: See LPP-9132 for a testcase that passes in swf8 and swf10. > test/lfc/lzunit-lzutils.lzx passes across all runtimes. > > Files: > M WEB-INF/lps/lfc/kernel/swf/LzSprite.as > M WEB-INF/lps/lfc/kernel/swf9/LzSprite.as > 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/20100623-maxcarlson-M.tar
