[Laszlo-reviews] For Review: Change 20100526-maxcarlson-9 Summary: UPDATED: Clean up LzAnimator/LzNode interaction, cpmprehensive
André Bargull
andre.bargull at udo.edu
Fri Jun 4 03:01:23 PDT 2010
>> - 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!
>
Only lz.View will change a null parent to canvas. But it's still _not_
fixed:
> - var animator = new LzAnimator( null, args );
> -
> - return animator;
> + return new LzAnimator( this, args );
> + static var counter:Number = 0;
Why this static field now added? It wasn't added in the previous reviews
and it's also unused (despite updating the counter in updateCounter()).
> /**
> * @access private
> */
> + var counterkey:String;
> + /**
> + * @access private
> + */
> + var onattribute:LzDeclaredEventClass = LzDeclaredEvent;
> +
> + function $lzc$set_attribute(attribute) {
> + this.attribute = attribute;
> + this.counterkey = attribute + '_lzcounter';
> + if (this.onattribute.ready) this.onattribute.sendEvent(attribute);
> + }
I'd like to see this moved to the top where all the other attributes are
declared.
> + // Set to final expected attribute
> + targ.setAttribute(attr, expected[attr]);
> + // clear attribute value
> + delete expected[attr];
setAttribute() may have side effects, for example starting another
animator on the same attribute. In that case, the delete statement will
break some invariants. So you need to retrieve the value first, then
delete the expected-entry and finally call setAttribute(). (The old code
had the same bug.)
> + var aDiff:Number = value - this.currentValue;
> var targ:LzNode = this.target;
> var attr:String = this.attribute;
> - var aDiff:Number = value - this.currentValue;
> + var expected:Object = targ.__animatedAttributes;
> +
This change seems to be unnecessary, it only moves the line with "aDiff"
and adds an empty line.
> + var newto = this.origto - expected[attr];
> + if (this.to != newto) {
> + this.to = newto;
> + // "to" has changed so calc new poles and K value
> + this.calcControlValues();
> + }
I'm ok with this change in resetAnimator(), because to change one of the
lz.Animator attributes while the animator is running will in general
lead to undefined behaviour. But I don't think this change in
prepareStart() is right:
> + var newto:Number;
> if (this.relative) {
> - this.to = this.origto;
> + newto = this.origto;
> } else {
> - this.to = this.origto - targ.getExpectedAttribute(attr);
> -
> - // "to" has changed so calc new poles and K value
> - //this.calcControlValues();
> + newto = this.origto - expected[attr];
> }
> + if (this.to != newto) {
> + this.to = newto;
> + // "to" has changed so calc new poles and K value
> + this.calcControlValues();
> + }
[...]
> - //we already did this if !this.relative -- conditionalize?
> - this.calcControlValues();
Just like in my previous review:
> 'indirect' or 'motion' may have changed in the meantime, so this simple cache check won't work.
calcControlValues() gives different results when 'indirect' or 'motion'
have been changed, so your change will actually break changing 'motion'
or 'indirect' on an animator.
On 6/3/2010 3:15 AM, Max Carlson wrote:
> Change 20100526-maxcarlson-9 by maxcarlson at 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: andre.bargull at udo.edu
> 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
>
More information about the Laszlo-reviews
mailing list