Change 20100526-maxcarlson-9 by maxcarl...@friendly on 2010-05-26 16:35:11 PDT
in /Users/maxcarlson/openlaszlo/trunk-clean
for http://svn.openlaszlo.org/openlaszlo/trunk
Summary: UPDATED: Clean up LzAnimator/LzNode interaction, cpmprehensive
Bugs Fixed: LPP-9020 - Support CSS3 transitions (partial)
Technical Reviewer: [email protected]
QA Reviewer: ptw
Details: Updated to address Andre's latest round of comments:
> - for( var p in moreargs) args[p] = moreargs[p];
> + if (moreargs != null) {
> + for( var p in moreargs) args[p] = moreargs[p];
> + }
There is no need for the additional if-clause, it just bloats code.
Fixed.
> - var animator = new LzAnimator( null, args );
> -
> - return animator;
> + return new LzAnimator( this, args );
This will cause memory leaks. The parent of the newly created animator must be
null, otherwise the animator will be added to the parent's subnodes array, but
it'll never be destroyed. In general, no one cares about the animator returned
lz.Node#animate(), people just call animate() and that's it.
Fixed. For some reason, I thought passing null as for the parent arg to LzNode
owuld attach to the canvas!
> - this.__LZUID = "__U" + ++LzNode.__UIDs;
> + var uid = "__U" + ++LzNode.__UIDs;
> + this.__LZUID = uid;
> + // add a weak link for this UID
> + LzNode.__fromUID[uid] = this;
This will cause memory leaks, too. The reason is the same as above.
Fixed.
> /**
> + * @access private
> + */
> + static var __fromUID:Object = {};
__fromUID isn't even used in the complete change? (Apart from adding and
removing entries.)
Fixed.
> + /**
> + * Tracks expected values and counters
> + * @access private
> + */
> + static var __animatedAttributes = {};
Memory leak. For every node which gets animated an entry is added to this
object. But that entry will never be deleted. Why didn't you leave the
animatedAttrs object on lz.Node?
I moved __animatedAttributes bqck to LzNode.
> - function calcControlValues (cval = null) :void {
> + function calcControlValues (cval = 0) :void {
calcControlValues() is never called with an argument, so why does it have an
optional argument at all?
Since the arg is never used, I changed to a constant in the function.
> + var to = this.to;
> + if (to === this.__lastto) return;
> + this.__lastto = to;
'indirect' or 'motion' may have changed in the meantime, so this simple cache
check won't work. I'd just leave the code as it is.
Fixed.
> - if (! this.isactive) return;
> + //if (! this.isactive) return;
> + this.isactive = false;
Why did you comment out the check? Calling stop() on an inactive animator
should be a no-op, so the check is required.
Fixed.
>From the previous review round:
> 3) It seems to me if your counter goes negative, you have a bug. Why are we
> papering over this?
>
> Agreed. I added a warning to catch this in debug mode for now.
What happened to warning you wanted to add?
I did some testing, and read through the code - I couldn't find a way to get a
negative counter, so I removed the test/warning.
> + var counter:Number = expected[counterkey] - 1;
> + expected[counterkey] = counter;
> + if (counter == 0) {
> // "to" has changed so calc new poles and K value
> - this.calcControlValues();
Comment should be moved to the new position, too.
Fixed.
> - targ.setAttribute(attr, from);
> - var d:Number = from - targ.getExpectedAttribute(attr);
> - targ.addToExpectedAttribute(attr, d);
> + targ.setAttribute(attr, Number(from));
> + expected[attr] += Number(from);
This seems to be wrong.
it was: exp' = exp + d = exp + (from - exp) => exp' = from
but now it is: exp' = exp + from
with exp the old expected value and exp' the new expected value
Good catch! Fixed.
> - // set current to zero since all animators are now relative
> - this.currentValue = 0;
Comment should be added to calcControlValues(), I know it did help me in the
past to understand the code in lz.Animator
Fixed.
Otherwise, the same:
Updated to address Andre's comments:
>> LzAnimatorGroup.__LZhalt() also sends onstop. Bret asked me to add
>> this to ensure onstop is fired after the screen has had a chance to
>> update, and this seemed like the cleanest way for now.
Could you split changes which introduce new behaviour in different change sets?
This makes it easier to test changes and to back out these changes if necessary
due to regression and so forth.
Also, moved all animator state into a single object -
LzAnimator.__animatedAttributes.
Otherwise:
Updated to address Tucker's comments:
1) Since we're removing all the substrate from LzNode, the comments that say
'inlined LzNode...' make no sense any more.
Done.
2) The logic with expectedvalue is just too convoluted for me to follow. This
expression (expectedvalue != null ? expectedvalue : targ[attr]) is repeated
over and over, which can't be efficient. AFAICT, the only time expected value
will be null is if the attribute is transitioning from not animated to
animated. So, wouldn't it be simpler to first test if there is a counter
entry, and if there is not, set the counter to 0 and the expected value to the
current value? Then the rest of the logic can simply work with the counter and
the expected value without all the redundant conditionalization. When all the
animations are done, you remove the counter (and optionally the expected
value), so that you know the attribute is no longer animating.
Agreed.
3) It seems to me if your counter goes negative, you have a bug. Why are we
papering over this?
Agreed. I added a warning to catch this in debug mode for now.
Issue:
1) I don't understand why you have to introduce another delegate to call halt
from finalize from stop? All halt does is unregister the animator.
LzAnimatorGroup.__LZhalt() also sends onstop. Bret asked me to add this to
ensure onstop is fired after the screen has had a chance to update, and this
seemed like the cleanest way for now.
Otherwise, the same:
Updated to address Andre and Tucker's comments:
> + this.__animExpected = {};
> + this.__animCounter = {};
Only a few nodes will be animated in an application, so both hashes should be
built lazily. Otherwise every node construction will cost +2 Objects.
Fixed.
> [...]
> + var __animExpected = {};
> [...]
> + var __animCounter = {};
Every node construction will cost +4 Objects for swf9/10
(http://jira.openlaszlo.org/jira/browse/LPP-5232).
Fixed.
> @@ -218,23 +227,32 @@
> var attr:String = this.attribute;
>
> var from:* = this.from;
> + var expected = targ.__animExpected;
> + var expectedvalue = (expected[attr] != null ? expected[attr] :
> targ[attr]); // inlined targ.getExpectedAttribute(attr);
> if (from != null) {
> targ.setAttribute(attr, from);
> - var d:Number = from - targ.getExpectedAttribute(attr);
> - targ.addToExpectedAttribute(attr, d);
> + var d:Number = from - expectedvalue;
> + // inlined targ.addToExpectedAttribute(attr, d);
> + var current = expected[attr];
> + expected[attr] = (current != null ? current : targ[attr]) + d;
> }
>
> if (! this.relative) {
> - this.to = this.origto - targ.getExpectedAttribute(attr);
> + this.to = this.origto - expectedvalue;
This seems to be wrong:
expected[attr] is updated (inlined call to addToExpectedAttribute), but later
"expectedvalue" is used again.
Fixed.
On 5/25/2010 4:54 PM, P T Withington wrote:
> Comments:
>
> 1) I'd just remove all the animator functions you commented out of LzNode.
> They are clutter and will (already for one of them) rot. svn can find them
> if we really need them back.
>
You mean to rot like setExpectedAttribute() which wasn't updated to use
__animExpected ;-)
Fixed.
> 2) Rather than creating multiple object hashes for the various animation
> attributes, why not define a class (data structure) that holds all the
> animation properties of an attribute? Thus, when animating an attribute, you
> will just say something like:
>
> var aas = this.__animatedAttributes;
> if (! aas[name]) { aas[name] = new AnimationAttribute(start, end, ...);
>
> ?
Fixed.
Also in stop()
> var chash = targ.__animCounter;
> var counter = chash[attr];
> if (counter == null || counter - 1<= 0) {
> chash[attr] = 0;
> targ.__animExpected[attr] = null;
> } else {
> chash[attr] = counter - 1;
> targ.__animExpected[attr] = this.to - this.currentValue;
> }
>
> /*
> var e_prop:String = "e_" + (this.attribute cast String);
> if (! targ[e_prop].c) {
> targ[e_prop].c = 0;
> }
> targ[e_prop].c -= 1; //decrement animation counter for prop
> if (targ[e_prop].c<= 0) {
> targ[e_prop].c = 0;
> targ[e_prop].v = null;
> } else {
> targ[e_prop].v -= this.to - this.currentValue;
> }
> */
>
1) the complete commented out block seems to be unnecessary
Fixed.
2)
targ[e_prop].v -= this.to - this.currentValue;
is changed to
targ.__animExpected[attr] = this.to - this.currentValue;
Shouldn't that be -= ?
Good catch. Fixed.
And I still think you cannot reuse "expectedvalue" here:
> if (! this.relative) {
> - this.to = this.origto - targ.getExpectedAttribute(attr);
> + this.to = this.origto - expectedvalue;
>
Fixed.
LzNode - Remove all animator-related code.
LzAnimatorGroup - Update comments, remove cruft, update types. Prevent
duplicate calls to doStart() or stop().
LaszloAnimation - Use LzAnimator.__animatedAttributes to track animation
counter and expected values. Shorten lookups. Fix bugs in reviewer comments.
Tests: Visual inspection, animators work as before across all runtimes.
Files:
M WEB-INF/lps/lfc/core/LzNode.lzs
M WEB-INF/lps/lfc/controllers/LzAnimatorGroup.lzs
M WEB-INF/lps/lfc/controllers/LaszloAnimation.lzs
Changeset:
http://svn.openlaszlo.org/openlaszlo/patches/20100526-maxcarlson-9.tar
