Re: [boost] [tweener] Preliminary Submission

Subject: Re: [boost] [tweener] Preliminary Submission
From: Julien Jorge (julien.jorge_at_[hidden])
Date: 2013-03-03 13:12:48

Le samedi 02 mars 2013 à 03:28 -0600, Michael Marcin a écrit :
> Julien Jorge wrote:
> > Hello,
> >
> > I would like to propose the Boost.Tweener library for preliminary
> > submission.
> >
> > The purpose of the Boost.Tweener library is to provide solutions to
> > interpolate the intermediate values between two given values during a
> > time interval. Various easing functions can be applied to control the
> > computation of the intermediate values. I felt the need of such a
> > library while developing games but I have also seen this need in various
> > animation softwares.
> >
> > The library provides the following functionalities:
> > - Interpolation is done on the value of a given variable or using an
> >    user-provided callback function,
> > - The interpolated values are computed using a predefined easing
> >    function or any user-provided function respecting the contract,
> > - Several tweeners can easily be executed simultaneously or in a row,
> > - The type of the interpolated values are user defined.
> >
> > Please follow this link to download a Zip of the library:
> > http://sourceforge.net/projects/libclaw/files/boost/boost-tweener-rc1.tar.gz/download
> >
> > Can you check that the library works as expected or tell me what should
> > be fixed before submitting for review? There are various example
> > programs in the archive.
> >
> 
> I would prefer Boost.Tween to Boost.Tweener.

That's a change I can do.

> How do you do looping tweens? (Both N and infinite loops)

Currently I would do an N-iterations loop by inserting the tweener in a
tweener sequence N time, and an infinite loop by creating a new tweener
on the on_finished() callbacks.

I think I can add a tweener_loop in the library.

> 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.
> 

A tween "by" is can be done by using the single_tweener. It has a
constructor that takes a reference to a variable (that would be the
member variable of the class in your example). Nevertheless it is very
limited since the initial value is stored in the constructor. For
example, it cannot be done during a sequence on the same variable.

The workaround is currently to convert the "by" to a "from -> to" and to
use the on_finished() callbacks to create the tweeners with the fresh
values.

Adding this functionality is not obvious, so I would prefer not to add
it to the library yet.

> domain_type feels awkard. It's used as both a time_point and duration to 
> borrow boost::chrono terminology.
> 

I agree on the awkwardness of the terminology here. I needed a type T
that can multiply the value_type of the tweeners; scalar_type may have
been a better name?

Actually there is no need for time-related types. All the single_tweener
needs to know is the percentage of progression between the initial and
final value. Their duration in the groups and sequence is more a weight
in the container but "duration" seems a more classic vocabulary. I am
still open for suggestions here.
 
> 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.

That would be nice but it seems incompatible with the ability to insert
any tweener class in the groups and the sequences.

> 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.
> 

OK to switch to std::vector. I never had the need to manipulate the
groups and the sequences as containers, so I am not convinced that it
would be interesting to go so far.

> 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 totally agree for the sequences. About the groups, I think that they
should be understood as if all tweeners inside are updated on parallel,
maybe with one thread for each. A guarantee of first-in first-update
here is more a side effect of the current implementation than a
conceptual behaviour.

> 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 think I can add a reset() function to the tweeners.

> 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 use boost::functions to make the signature of the callbacks obvious in
the interface of the tweeners. As you have noticed it stops the user of
the library to use custom callback type and function objects. I can add
a template argument to the tweener classes to select the type of the
callbacks but the pros do not win the cons for me:

Pros:
 - you use the type of callback that best fits your needs

Cons
 - failing to use the right callback type will display a hard to
understand error message on a line inside the implementation of the
tweener classes
 - the callback type will be shared among the tweener classes, so the
function objects won't be usable in practice.

Globally I am not convinced that the lost in readability would be
compensated by the flexibility. Nevertheless I am still open to comments
and suggestions on the subject.

> 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.

OK to add this on single_tweener. I think I can add a similar feature on
the tweener_sequence, maybe also on the tweener group. I do not know for
the tweener_loop.

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

I remember that I needed this but I do not remember why… I'm OK to
remove it but I am not sure it is the best decision. It seems to me that
it is usual to be able to add multiple callbacks. Maybe should I allow
to have multiple update callbacks? Or replace the callbacks with
boost::signals?

> 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 totally agree. What about origin/target or initial/target?

> I personally think a method chaining interface ala HOTween's TweenParms 
> is very nice for configuring tweeners.
> 

I will have a look at TweenParms but I am not familiar with HOTween.

> 
> As someone who has used TweenLite, iTween, HOTween, and AniMate as well 
> as rolled his own (poorly) I'm very happy to have such a library 
> proposed to Boost.
> 
> Thank you.
> 

Thanks for the feedback.

Julien Jorge