Transformation methods
Piglop
(translate, rotate, scale and skew). I started it originally to allow
PrawnVectorImport to import matrices from vector files.
It's there : http://github.com/piglop/prawn/tree/transformations
I added methods:
* to save and restore graphics states (the former accepts a block),
* to apply a transformation matrix (rather low level, so it should
probably be private, but PrawnVectorImport will need it) and
* to apply common transformations (translate, rotate, scale and skew).
I had to rename the private "translate" methods. I chose "bind" but
there's probably a better name.
I added an example here : http://github.com/piglop/prawn/blob/transformations/examples/graphics/transformations.rb
In the sample, I had to remove the margin because otherwise the
rectangle method alters the coordinates according to the bounding box.
It should probably use transformations instead.
Gregory Brown
> I wrote a few methods to manipulate the transformation matrix
> (translate, rotate, scale and skew). I started it originally to allow
> PrawnVectorImport to import matrices from vector files.
Neat!
> It's there : http://github.com/piglop/prawn/tree/transformations
Definitely will look at this for possible merging into 0.8, too busy
right now to do a full review.
> I added methods:
> * to save and restore graphics states (the former accepts a block),
> * to apply a transformation matrix (rather low level, so it should
> probably be private, but PrawnVectorImport will need it) and
> * to apply common transformations (translate, rotate, scale and skew).
sounds fine.
> I had to rename the private "translate" methods. I chose "bind" but
> there's probably a better name.
to_absolute_coords() or just to_absolute probably makes sense.
> I added an example here : http://github.com/piglop/prawn/blob/transformations/examples/graphics/transformations.rb
API looks a bit opaque to me, but I need to actually try it out before
complaining. Thanks for working on this, looks interesting.
-greg
Brad Ediger
<gregory...@gmail.com> wrote:
> On Sat, Dec 19, 2009 at 2:55 PM, Piglop <pig...@gmail.com> wrote:
>> I wrote a few methods to manipulate the transformation matrix
>> (translate, rotate, scale and skew). I started it originally to allow
>> PrawnVectorImport to import matrices from vector files.
>
> Neat!
>
>> It's there : http://github.com/piglop/prawn/tree/transformations
>
> Definitely will look at this for possible merging into 0.8, too busy
> right now to do a full review.
We probably want a ticket in the tracker so it doesn't get lost before
0.8. I don't think any of us have time to do a proper review right
now.
-be
Daniel
are very good (especially the presence of well conceived specs). Here
are a few changes:
i. We don't need a separate save_graphics_state and
restore_graphics_state since they must always be paired. A single
graphics_state method that accepts a block would be good.
ii. I would suggest not leaving it up to the user to set the graphics
state before using the shortcut methods (translate, rotate, scale, and
skew). Instead, make each of those accept a block and call
graphics_state themselves, and all but translate would also accept an
optional :origin option that would control a translate block within
the shortcut method. This will make for a cleaner API.
That is, instead of:
save_graphics_state do
translate 100, 0
save_graphics_state do
draw_origin
rotate 10
draw "10° rotation"
end
end
You would have:
rotate 10, :origin => [100, 0] do
draw_origin
draw "10° rotation"
end
iii. I've always been one for very explicit method names, which
sometimes become longer than what Prawn has leaned towards, but
instead of 'bind', what about 'map_to_absolute'? That certainly tells
me what the method is doing.
This isn't something you can change now, but with respect to the
visibility of the transformation_matrix method, if Greg's experiment
to separate the low level developer API from the public API is
successful, then transformation_matrix should go into the developer
API.
Cheers,
Daniel
Daniel
> optional :origin option that would control a translate block within
> the shortcut method.
Actually, instead of controlling a translate block the :origin option
would be used to customize the transformation_matrix call from the
shortcut (since it can handle all the various transformations at
once), that would keep things simple within the shortcut methods.
-Daniel
Piglop
> i. We don't need a separate save_graphics_state and
> restore_graphics_state since they must always be paired. A single
> graphics_state method that accepts a block would be good.
I'm not sure. For example one may want to save the graphic state in a
method and restore it in another method. It sounds like a bad practice
but forcing the use of a block doesn't sound good either.
> ii. I would suggest not leaving it up to the user to set the graphics
> state before using the shortcut methods (translate, rotate, scale, and
> skew). Instead, make each of those accept a block and call
> graphics_state themselves, and all but translate would also accept an
> optional :origin option that would control a translate block within
> the shortcut method. This will make for a cleaner API.
>
> That is, instead of:
> save_graphics_state do
> translate 100, 0
> save_graphics_state do
> draw_origin
> rotate 10
> draw "10° rotation"
> end
> end
>
> You would have:
> rotate 10, :origin => [100, 0] do
> draw_origin
> draw "10° rotation"
> end
Allowing a block in transformation calls is a good idea. If a block is
given, the state is saved before the transformation, and restored
after the yield.
But I don't think forcing it would be a good thing when you have to
use multiple transformations in the same saved graphic state. For
example you may want to save the state, translate, rotate, draw
something, translate again, draw something else, and only then restore
the previous state. You don't want to save the state on each
transformation.
If you have to use a block for each transformations, you'll end up
with many levels that will break the logic and the code will be hard
to read and maintain.
Combining transformations into single calls (ie. rotate 10, :origin =>
[100, 0]) may be a good thing. But is it really better than calling
translate(100, 0) and then rotate(10)?
> iii. I've always been one for very explicit method names, which
> sometimes become longer than what Prawn has leaned towards, but
> instead of 'bind', what about 'map_to_absolute'? That certainly tells
> me what the method is doing.
It sounds good.
> This isn't something you can change now, but with respect to the
> visibility of the transformation_matrix method, if Greg's experiment
> to separate the low level developer API from the public API is
> successful, then transformation_matrix should go into the developer
> API.
PrawnVectorImport doesn't use it yet so it can change.
I think it's better if the PrawVectorImport output doesn't have to
call private methods. The output code should be understandable and
reusable easily in your own code.
Piglop
Combining multiple transformations into a single matrix is not easy
and I don't think it'd be clearer. For example, translating and
rotating is different than rotating and translating. It'd be easier to
understand the transformation method if it explicitly calls the
translate and rotate methods to build the combined matrix.
Gregory Brown
merging this feature, but here are a few quick thoughts from me...
On Thu, Jan 14, 2010 at 2:25 AM, Piglop <pig...@gmail.com> wrote:
> On 8 jan, 18:42, Daniel <dnelso...@gmail.com> wrote:
>> i. We don't need a separate save_graphics_state and
>> restore_graphics_state since they must always be paired. A single
>> graphics_state method that accepts a block would be good.
>
> I'm not sure. For example one may want to save the graphic state in a
> method and restore it in another method. It sounds like a bad practice
> but forcing the use of a block doesn't sound good either.
I think the block form should be implemented in terms of save and
restore state, personally. I agree that there's no real need to force
the block form.
That sounds a bit obscure, but I think that having both is OK.
> If you have to use a block for each transformations, you'll end up
> with many levels that will break the logic and the code will be hard
> to read and maintain.
I never really buy into this argument. Show me complex nested code
and I'll show you how to break it down into something readable and
maintainable. It is never the case that you *need* deep nesting, and
readability actually goes up when you write some helper functions.
That having been said, it's a non issue if you build the block form on
top of the more manual API.
Then folks can choose the way they want to organize things.
Gregory Brown
> Combining transformations into single calls (ie. rotate 10, :origin =>
> [100, 0]) may be a good thing. But is it really better than calling
> translate(100, 0) and then rotate(10)?
Yes, definitely.
Daniel Nelson
> method and restore it in another method. It sounds like a bad practice
> but forcing the use of a block doesn't sound good either.
Remember that graphics state isn't like color, font, or undo. If a
graphics state is saved and not restored, or vice versa, the document
is broken. I don't know when there would be a situation to save in one
method and restore in another that would result in good design (just
have another method that calls both within a graphics state block).
If someone really wanted to save the graphics state in a different
place than they restore it, let them extend Prawn. Prawn itself should
encourage good practices.
> But I don't think forcing it would be a good thing when you have to
> use multiple transformations in the same saved graphic state. For
> example you may want to save the state, translate, rotate, draw
> something, translate again, draw something else, and only then restore
> the previous state. You don't want to save the state on each
> transformation.
Why not? Since the transformations are manipulating the graphics state
behind the scenes, the fact that the graphics state is saved on each
transformation is transparent to the user. The user simply calls
rotate, scale, etc. This makes for Prawn code that is clearer and less
prone to introducing hard-to-find bugs.
> If you have to use a block for each transformations, you'll end up
> with many levels that will break the logic and the code will be hard
> to read and maintain.
The common use scenario is simply to use a single transformation
(usually rotate, or scale), in which case the approach I propose is
easier to read and maintain: a single rotate block instead of a rotate
block within a graphics state block. This also means a clearer API for
the common use situation: a single, obvious method instead of a method
that must always be used in conjunction with another method.
If one finds oneself in a case with too many nested levels, just break
some of them out into methods. Given that it isn't apparent from Prawn
calls what is being drawn, this would be good practice anyway.
> Combining transformations into single calls (ie. rotate 10, :origin =>
> [100, 0]) may be a good thing. But is it really better than calling
> translate(100, 0) and then rotate(10)?
Yes, because it lends itself to self-explanatory code. Reading code
that translates before it rotates does not tell me that the
translation is being performed to provide an origin around which to
rotate.
Best,
Daniel
Gregory Brown
>> I'm not sure. For example one may want to save the graphic state in a
>> method and restore it in another method. It sounds like a bad practice
>> but forcing the use of a block doesn't sound good either.
>
> Remember that graphics state isn't like color, font, or undo. If a
> graphics state is saved and not restored, or vice versa, the document
> is broken. I don't know when there would be a situation to save in one
> method and restore in another that would result in good design (just
> have another method that calls both within a graphics state block).
> If someone really wanted to save the graphics state in a different
> place than they restore it, let them extend Prawn. Prawn itself should
> encourage good practices.
I don't know... this is sort of like arguing File#close() shouldn't
exist. I almost *always* use the block form, but I think it's fine
to have both. I agree we should encourage good practices, but we're
not going to make people go through hoops like Rails does because of
something we think is "uncommon". I think the right solution is to
implement the block form in terms of save and restore graphics state,
and leave the low level methods available for those that want it.
>> But I don't think forcing it would be a good thing when you have to
>> use multiple transformations in the same saved graphic state. For
>> example you may want to save the state, translate, rotate, draw
>> something, translate again, draw something else, and only then restore
>> the previous state. You don't want to save the state on each
>> transformation.
>
> Why not? Since the transformations are manipulating the graphics state
> behind the scenes, the fact that the graphics state is saved on each
> transformation is transparent to the user. The user simply calls
> rotate, scale, etc. This makes for Prawn code that is clearer and less
> prone to introducing hard-to-find bugs.
Right, we should make these blocks implicitly save state when nested. I agree
Daniel Nelson
> implement the block form in terms of save and restore graphics state,
> and leave the low level methods available for those that want it.
Okay, sounds good.
Best,
Daniel
jouni
is going to be called several times. This would be the case ie. on
table rendering or similar. It makes a really huge difference on
performance if you limit your usage of yield blocks.
Gregory Brown
> I personally try to avoid using blocks when possible, especially if it
> is going to be called several times. This would be the case ie. on
> table rendering or similar. It makes a really huge difference on
> performance if you limit your usage of yield blocks.
Benchmarks please.
Jouni Airaksinen
Attached the benchmark:
With ruby 1.8.7:
lgm110:dev jouni$ ruby blockbm.rb
5.890000 0.000000 5.890000 ( 5.900553)
3.480000 0.010000 3.490000 ( 3.490343)
lgm110:dev jouni$ ruby blockbm.rb
5.880000 0.010000 5.890000 ( 5.900996)
3.480000 0.000000 3.480000 ( 3.492033)
lgm110:dev jouni$ ruby blockbm.rb
5.880000 0.000000 5.880000 ( 5.893387)
3.480000 0.010000 3.490000 ( 3.495055)
With Ruby 1.9.1:
lgm110:dev jouni$ ruby blockbm.rb
1.350000 0.000000 1.350000 ( 1.362072)
1.270000 0.010000 1.280000 ( 1.268874)
lgm110:dev jouni$ ruby blockbm.rb
1.350000 0.000000 1.350000 ( 1.360003)
1.270000 0.000000 1.270000 ( 1.268398)
lgm110:dev jouni$ ruby blockbm.rb
1.360000 0.000000 1.360000 ( 1.360144)
1.270000 0.000000 1.270000 ( 1.267842)
Especially issue when nesting as the blocks easily multiply. ie. if you
have multiple columns on a table, you nest several times per column and
have several pages of table content. Plus if you have to do that for
thousands of documents on a batch run, you'll save several minutes, if
not hours for not using blocks if it's not neccessary.
Of course not everybody have similar large scale corporate needs where
this cumulates into hours of time, but would be good to have the option
to choose optimization over cleaner code.
Gregory Brown
I guess I've been spoiled by using Ruby 1.9 too much, there, what
you're showing is negligible compared to the actual work being done in
the blocks.
If they were doing anything more interesting than multiplication it'd
be line noise.
> Of course not everybody have similar large scale corporate needs where this
> cumulates into hours of time, but would be good to have the option to choose
> optimization over cleaner code.
Yeah, there's also just no reason *not* to do it, since you can build
the block form on top of the low level calls. Daniel probably wants
to make the simple case look good and reflect good practices, whereas
others might want more flexibility. There is no reason we can't have
both, and so we will.
-greg
Piglop
We keep the save_graphic_state and restore_graphic_state methods with
optional block form on the former.
We add an optional block to each transformation method. When given,
the method saves the state, applies the transformation, yields and
restores the state.
We add options to the transformation methods.
The only option for now is a :origin for the rotation. It applies a
translation before the rotation.
If this is ok, I'll make a new patch.
Do you prefer a single big patch, or a patch per change like I did[1]?
Piglop
> So, if I understood correctly, the summary of the discussion is:
>
> We keep the save_graphic_state and restore_graphic_state methods with
> optional block form on the former.
>
> the method saves the state, applies thetransformation, yields and
> restores the state.
>
> We add options to thetransformationmethods.
>
> The only option for now is a :origin for the rotation. It applies a
> translation before the rotation.
And we rename the private method "translate" to "map_to_absolute"
instead of "bind".
Daniel Nelson
Either patching approach is fine with me on this one.
Best,
Daniel
Gregory Brown
> So, if I understood correctly, the summary of the discussion is:
>
> We keep the save_graphic_state and restore_graphic_state methods with
> optional block form on the former.
>
> We add an optional block to each transformation method. When given,
> the method saves the state, applies the transformation, yields and
> restores the state.
>
> We add options to the transformation methods.
>
> The only option for now is a :origin for the rotation. It applies a
> translation before the rotation.
Sounds right to me.
> Do you prefer a single big patch, or a patch per change like I did[1]?
>
> [1] http://github.com/piglop/prawn/commits/transformations/
topic branches are preferred for anything that's not a one-off bug fix
or tiny feature. So yeah, get synchronized with sandal/prawn@master,
branch, and then get up to date before sending a pull request.
Piglop
http://github.com/piglop/prawn/commits/transformations_revised_and_rebased/
The code and the specs need refactoring but I couldn't find a good way
to do it.
It'd be good if we could change the behavior of a transformation
method without having to update 2 tests. For example to implement
the :origin option to rotate, it'd be nice to write only one test, not
one with the block form and one without it.
And it would be nice if the transformation methods could simply call
transformation_matrix with a block if a block is given. The code would
be cleaner.
A good way to do this may be to use a mocking library that can verify
the block given to a mocked method. I couldn't find a way do that with
Mocha.
It works with RR, but the spec is not very nice :
http://github.com/piglop/prawn/commit/afdaef74c7e9e8464ab79a4d837e5081b6ce8d18
Gregory Brown
> It works with RR, but the spec is not very nice :
> http://github.com/piglop/prawn/commit/afdaef74c7e9e8464ab79a4d837e5081b6ce8d18
I probably won't be able to look at this in detail until later this
week, so maybe someone else can give you details, but we aren't going
to have parallel mocking frameworks, so another solution needs to be
used.
-greg
Daniel Nelson
def translate(x, y, &block)
transformation_matrix(1, 0, 0, 1, x, y, &block)
end
(and the same for each of the others)
This would also mean that you would only need to test
transformation_matrix with both the block and non-block forms. For the
others, you would just need to test one of the forms.
Best,
Daniel
Piglop
Yes, that's what I tried here : http://github.com/piglop/prawn/commit/afdaef74c7e9e8464ab79a4d837e5081b6ce8d18
The problem is there's no way to test that the block is passed to
transformation_matrix (with Mocha). But since we can't use another
mocking framework, it sounds good enough.
I updated the branch : http://github.com/piglop/prawn/commits/transformations_revised_and_rebased/
And made a cleaner branch here : http://github.com/piglop/prawn/commits/transformations_cleanup
Daniel Nelson
I've merged your contributions into master. Note that I moved the
methods into their own modules, and added some API documentation. I
also took the opportunity to replace all the instances in Prawn in
which 'q' or 'Q' were manually written to the document using the save_
and restore_graphics_state methods you provided.
Thank You,
Daniel
Piglop
> methods into their own modules, and added some API documentation. I
> also took the opportunity to replace all the instances in Prawn in
> which 'q' or 'Q' were manually written to the document using the save_
> and restore_graphics_state methods you provided.
Cool. Note there's still a problem with transformations when the
margin is not zero, probably when map_to_absolute is used. I'm not
sure how to resolve it.
Daniel Nelson
> margin is not zero, probably when map_to_absolute is used. I'm not
> sure how to resolve it.
Oh, I didn't realize that. I'll need to figure that out in order for
the :rotation text box option I added to work.
Kind Regards,
Daniel
Daniel Nelson
the fact that all transformation matrices have the absolute 0,0 as
their origin. I've now accounted for this when handling the :origin
option that I added to the rotate method. See
http://github.com/sandal/prawn/commit/7d8ce371cefb9e0fec3b3fbc44aa48218af0aa83