Re: [Libpapyrus-devel] Problems when switching from papyrus 0.7 to 0.9

[Rick L. V. Jr. <rvi...@cs...>]

Hello.

I cc'd the mailing list so that there is an archive of the breakdown of
the affine transforms in case anyone else has the same questions, or would
like to provide feedback as well.

Perhaps I should copy and paste part of the explanation below into the docs?

Tim Niemueller wrote:
> Hi Rick.
>
> I have just converted our application from papyrus 0.7 to 0.9 and ran
> into a few problems that I wanted to tell you about:
>
> - The new API documentation hard to read, dark brown on only slightly
> lighter brown for the headings, and yellowish brown for black text isn't
> very readable. I'd suggest giving the backgrounds a shift to white,
> keeping the base color. Currently I have a hard time following the text.

Yup. They're pretty bad... that's high on the todo list. I lost the
previous css and hacked one up really quick picking some colors that
didn't turn out to be very easy on the eyes.

> - Currently compilation needs -std=c++0x when using Papyrus on Fedora.
> This isn't a nice thing if you do not want to use an incomplete standard
> just now. The problem is the smart pointer. As I mentioned earlier I
> would suggest to use the Cairo::RefPtr. This is available everywhere
> where libpapyrus would be useful and would remove the requirement of
> either Boost (pretty fat for just smart pointers), C++0x or TR1, have
> the same effect a nicely mix with Cairo.

IIRC, Cairo::RefPtr is missing the weak pointer (std::weak_pointer,
std::tr1::weak_pointer or boost::weak_pointer) which is needed in several
places within papyrus. For example one of the new containers, Watcher,
utilizes the weak pointer so that it doesn't take ownership of the object.

If you don't compile with -std=c++0x it should default to looking for tr1,
and if tr1 isn't found it should default to looking for boost.

I looked at including the smart pointer within papyrus, but extracting the
implementation from any of the aforementioned sources just looked like a
real pain.

> - Why is it set_rotation() bit then translate(). I'd suggest to either
> have rotate() and translate() or to have set_rotation() and
> set_translation(). I'd vote for the latter for the following reason:
> When I first read the new documentation about translate() I thought
> "Damn, now I need to do all kinds of relative maths", expecting that
> translate() would be added every time, like it is in Cairo. Of course
> that's nonsense because it's executed on different objects, but still
> set_translation() would be much clearer in that it sets a certain
> translation. I haven't checked if translate adds the given translation
> instead of setting it. If that is the case you might want to have both,
> rotate() and translate() *and* set_*(). One will add to the current
> rotation/translation, the other one will set it explicitly.

For each of the affine transforms there are sets of methods that provide
absolute and relative transformation, plus there are sets of accessors to
retrieve the current values.

For the accessors I used a mixture of 'affine-noun'() and
get_'affine-noun'(). Part of the reason was to expose simple access
methods for the x and y position with x() and y() allowing code like:
  if ( drawable->x() > 5 and drawable->x() < 10 ) ...

Affine translations were the only ones to use the simpler form, and the
other transforms (scaling, rotation, skewing, et. al.) used the
get_'affine-noun'() form:
  x()
  y()
  get_position(x,y)
  get_scale_x()
  get_scale_y()
  get_scale(sx,sy)
  get_rotation()
  get_skew_x()
  get_skew_y()
  get_skew(wx,wy)

What I should have probably done was not use 'position' but rather
'translation' or 'xy'. I think I'd prefer to use the latter to stay
consistent with the x() and y() methods, so it would look like this:
  x()
  y()
  xy(x,y)
  get_scale_x()
  get_scale_y()
  get_scale(sx,sy)
  get_rotation()
  get_skew_x()
  get_skew_y()
  get_skew(wx,wy)

But, I thought it made the code a bit harder to read, as it turned this:
  if ( drawable->x() > 5 and drawable->x() < 10 ) ...

into this:
  if ( drawable->get_translate_x() > 5 and drawable->get_translate_x() <
10 ) ...

The latter just felt cumbersome. I suppose an alternative would be to
provide aliases for x() and y():
  x()
  y()
  xy(x,y)
  get_translation_x(): alias for x()
  get_translation_y(): alias for y()
  get_translation_xy(): alias for xy()
  get_scale_x()
  get_scale_y()
  get_scale(sx,sy)
  get_rotation()
  get_skew_x()
  get_skew_y()
  get_skew(wx,wy)

For the relative transforms I used a form of 'affine-verb'(parameters):
  translate(tx, ty)
  scale(sx, sy)
  rotate(r)
  skew(wx, wy)

What is probably missing on this list is the individual transforms such as:
  translate_x(tx)
  translate_y(ty)
  scale_x(sx)
  scale_y(sy)
  skew_x(wx)
  skew_y(wy)
But, I was trying to keep the set of relative transforms to a minimum. I
picked verbs to indicate action upon the drawable.

For the absolutes I used set_'affine-noun'() so they are:
  set_x(x)
  set_y(y)
  set_position(x,y) (again, probably better xy)
  set_scale_x(sx)
  set_scale_y(sy)
  set_scale(sx, sy)
  set_rotation(r)
  set_skew_x(wx)
  set_skew_y(wy)
  set_skew(wx, wy)

So, for example with rotation there is:
  rotate(r): rotates by r (rotation = rotation + r)
  get_rotation(): returns the current rotation
  set_rotation(r): sets the current rotation to an absolute r ( rotation = r)

I like using the action verbs since it allows you to write this:
  drawable->rotate( M_PI );
rather than:
  drawable->set_rotation( drawable->get_rotation() + M_PI );

As a side note, I should probably use 'shear' instead of 'skew'.

> - After conversion and running it I got 100% CPU load, and the graph
> (which is updated with about 30Hz) showed only up until after my
> application stopped updating the graph. I haven't checked but it seems
> that the new version introduced a performance bottle neck compared to
> the old version. Or is there something that has changed from before that
> it expects me to do some drawing operations differently that could cause
> it to cause a high load?

The biggest change has been in the extents calculation. I'm offloading
that to cairo, and I haven't calculated the load related to it. If I had
to guess, that's where it's spending it's time.

I'd be interested in seeing a profile to see what methods it's spending
the most time in.

> I'll let you know once I know more, today there is a deadline
> approaching so probably not before tomorrow.

Sounds good.

---

Rick