Re: [boost] [tweener] Preliminary Submission

Subject: Re: [boost] [tweener] Preliminary Submission
From: Michael Marcin (mike.marcin_at_[hidden])
Date: 2013-03-02 13:54:35

Klaim - Joël Lamotte wrote:
> Hi, some comments on your remarks Micheal:
>
> On Sat, Mar 2, 2013 at 10:28 AM, Michael Marcin <mike.marcin_at_[hidden]>wrote:
>
>>
>> How do you do looping tweens? (Both N and infinite loops)
>>
>> How do you do relative tweens, i.e. instead of tween from -> to, tween by.
>> I guess this might not matter as it seems the tweener only tweens its own
>> internal value. In many cases it would seem nice to update a memory
>> location directly. For instance a member variable of the class that owns
>> the tween.
>>
>
> Looks to me like advanced features that are not necessary for a first
> version.
> In particular, in my experience checking a value in memory have been less
> useful (and harder to make safe) than keeping a copy of the target value,
> but allow it to be updated from user code.
>

Tweening a copy and then assigning from it every update is acceptable as 
this is pretty easy to accomplish from user code.

The looping behavior is something that is not easy to implement in user 
code. For example HOTween supports N or infinite loops of these types:

// When a tween completes, rewinds the animation and restarts (X to Y, 
repeat).
Restart,

// Tweens to the end values then back to the original ones and so on (X 
to Y, Y to X, repeat).
Yoyo,

// Like <see cref="LoopType.Yoyo"/>, but also inverts the easing 
(meaning if it was <c>easeInSomething</c>, it will become 
<c>easeOutSomething</c>, and viceversa).
YoyoInverse,

// Continuously increments the tween (X to Y, Y to Y+(Y-X), and so on),
// thus always moving "onward".
Incremental

>
>>
>> domain_type feels awkard. It's used as both a time_point and duration to
>> borrow boost::chrono terminology.
>>
>>
> I think you assume that tweening/interpolation is relative to time. It is
> not. This library should never depends on time.
> It could provide extensions later to work with time but I feel it's
> unnecessary.
>

Sorry I don't mean actual time, although internally it uses time 
terminology. Using duration as a [0,1] ratio or as a 0 to int 
numItemsToLoadForProgressBar is just as valid. I only mean to imply that 
the difference type of domain_type - domain_type might not necessarily 
be domain_type.

If the interface reflected this it would then be easier to tell when 
something is a relative change (delta) vs an absolute value. And as a 
plus you could use chrono types directly I think as even though time 
isn't the only domain_type it's certainly a common one.

I think you can just get domain_difference_type from domain_type using 
result_of or typeof.

>
>
>> Why does base_tweener have a virtual destructor, virtual do_clone and
>> virtual do_is_finished members? It seems you could easily just use CRTP
>> here.
>>
>>
> Not sure it's possible in this case, but that would be cool.
>

It seems like it's all for the sake of tweener which is I believe 
essentially type erasure for tweens. I don't think this virtual overhead 
should be paid for every tween just for this sake.

The only time you should need type erasure is if you're making a tween 
group or tween sequence composed of single_tweeners with different 
value_types. Or if you're compositing single_tweeners, tween_sequences, 
and tween_groups.

>
>> I think tweener_sequence/tweener_group should be a container adapter or at
>> least switched to use a std::vector. I know I wouldn't want to pay for a
>> std::list here in any use case I can imagine.
>>
>> FWIW if you're using a std::list (like in tweener_group.ipp) instead of:
>> const iterator_type tmp(it);
>> ++it;
>> m_tweeners.erase(tmp);
>> you can do:
>> it = m_tweeners.erase(it);
>>
>> I think the order of tween updates for sequences/groups can be important.
>> There should be a guarantee of first-in first-update order and I think the
>> name insert should change to push_back to reflect the ordered nature of the
>> operation.
>>
>>
> I agree that vector instead of list would be far better.
>
>
>> It seems all tweeners are one shot in that you cannot restart them.
>> tween_sequence and tweener_groups do_update is destructive and prevents
>> being able to restart the group.
>>
>>
> I'm not sure to understand what you mean here.
>

If you look inside their do_update calls they remove things from the 
lists as you're going through them. This means you can't for instance 
create a tween group that scales a menu button and changes it's color 
and play it every time the user does a mouse-down event. You'll have to 
recreate it every time.

>
>> Why are all the callbacks boost::functions? It's already a template can't
>> I just use my own callback type without paying for the type erasure and
>> dispatch of a boost::function if I don't need it?
>>
>>
> I think the functions should be template but inside they still have to use
> boost::function for storing the callback.
>

I don't think that's necessarily true. You can just store a copy of the 
callback in the tweener so long as the type of the tweener depends on 
the type of the callback. Although I could be convinced it might just be 
easier and cleaner to pay the overhead for the type erasure here.

>
>> There appears to be no interface to set the current time_point (m_date)
>> directly you can only move it a relative amount. There is also no interface
>> to query it. This is a very useful feature, it allows you for instance to
>> bind your tween to a slider and scrub back and forth through it.
>>
>>
> I don't understand why you are talking about time and then about non-time
> based interpolation.
> Do you mean a way to set manually the interpolation state from outside?
>

The actual type of the domain_type (be it time or otherwise) is 
irrelevant. I just mean a way to set the value of the internal m_date 
member and get the appropriate value out of the tween. I think the 
example makes the desired behavior pretty straight forward.

Come to think of it there is also no support for playing the tweens 
backwards. A negative delta passed to update doesn't work correctly.

>> Supporting multiple callbacks for on_finished seems unnecessarily
>> expensive an inconsistent (only one update callback supported).
>>
>>
> Not  sure about this.
>

iTween, HOTween, TweenLite don't have multiple callbacks, never felt 
limited without them. Also it's a pretty easy feature to add from user 
code but it's impossible to avoid the cost of multiple callbacks if 
you're only using one when it's implemented internally.

The cost is much higher than I would want to pay, for instance:

template<typename S>
void boost::tweener::base_tweener<S>::notify_finished() const
{
   // If one of the callbacks deletes the tweener, then m_on_finished 
will not be
   // available. Since we still want to execute all the callbacks, we 
iterate on
   // a copy of it.
   const std::list<finish_callback> callbacks(m_on_finished);

   for ( std::list<finish_callback>::const_iterator it=callbacks.begin();
         it!=callbacks.end(); ++it )
     (*it)();
} // base_tweener::notify_finished()

>
>> init/end seems like a strange name pairing. Maybe from/to, begin/end,
>> initial/final? Although using end here at all seems a bit dubious as it is
>> so often used in the as a semi-open interval [begin,end) whereas here it is
>> a closed interval [init,end].
>>
>>
> I quite agree, I'm just not sure begin/end wouldnt be misleading. I tend to
> prefer "target" as the end value name, as it should be modifiable (moving
> target).
>

I'm not sure how mutable target works. If t=0.5 with a linear tween from 
from 0 to 10 (so current position is 5), what happens when I change the 
target to 20?

Does my t stay at 0.5 and my position change to 10?
Does my position stay at 5 and my t change to 0.25?
Does something else happen?

Thanks for the comments.