Issue 344970043: Add and use a Transform data type
|
Created:
6 years, 9 months ago by dak Modified:
6 years, 9 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAdd and use a Transform data type
As of now, this is just used in stencil-integral. However, a Scheme
level interface would likely make sense for compacting geometric
transformations. The abstraction currently relies on PangoTransform
matrices but could easily be moved to other implementations such as
cairo_matrix_t.
Contains commits:
Add Transform data type
stencil-integral.cc: use Transform
stencil-integral: pass SCM as transform values
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased, reverted to grob rotation for the rotation property. #Patch Set 3 : Refix GC issue #
Total comments: 4
Patch Set 4 : Rework according to Dan's review #
MessagesTotal messages: 17
https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc#newc... lily/stencil-integral.cc:1099: Offset center (robust_scm2double (scm_cadr (rot), 0.0), robust_scm2double (scm_caddr (rot), 0.0)); + center[X_AXIS] = s->extent (X_AXIS).linear_combination (center[X_AXIS]); + center[Y_AXIS] = s->extent (Y_AXIS).linear_combination (center[Y_AXIS]); (or something similar) Here, we've got the rotation centre problem as in issue 5346. Standard regtests can't see it, but PNG based regtests show differences for skyline-grob-rotation.ly and stencil-color-rotation.ly skyline-boxes-ellipses.ly shows up, too, but there are only minimal deviations due to rounding issues.
Sign in to reply to this message.
https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/344970043/diff/1/lily/stencil-integral.cc#newc... lily/stencil-integral.cc:1099: Offset center (robust_scm2double (scm_cadr (rot), 0.0), robust_scm2double (scm_caddr (rot), 0.0)); On 2018/06/17 10:46:37, Be-3 wrote: > + center[X_AXIS] = s->extent (X_AXIS).linear_combination (center[X_AXIS]); > + center[Y_AXIS] = s->extent (Y_AXIS).linear_combination (center[Y_AXIS]); > (or something similar) > > Here, we've got the rotation centre problem as in issue 5346. > > Standard regtests can't see it, but PNG based regtests show differences for > skyline-grob-rotation.ly and > stencil-color-rotation.ly > > skyline-boxes-ellipses.ly shows up, too, but there are only minimal deviations > due to rounding issues. Right. I haven't rebased this patch yet, but I naturally will have to load up another version for review now that the fix is in master.
Sign in to reply to this message.
On 2018/06/17 10:54:38, dak wrote:
> Right. I haven't rebased this patch yet, but I naturally will have to load up
> another version for review now that the fix is in master.
I actually lean towards doing this in a manner similar to your original
approach, namely just using the stencil rotation call. Basically I'd want to
completely replace both sten and s with a rotated copy. This will make a
difference for later uses of s, notably
// we use the bounding box if there are no boxes
// FIXME: Rotation?
if (!boxes.size () && !buildings.size ())
boxes.push_back (Box (s->extent (X_AXIS), s->extent (Y_AXIS)));
The question is whether using the rotated version here is supposedly an
improvement, detrimental or "don't care". Regarding the "relative"
implementation of the origin location, the bounding boxes resulting from empty
extents are likely to be garbage (likely NaN) after rotation.
Sign in to reply to this message.
Rebased, reverted to grob rotation for the rotation property.
Sign in to reply to this message.
Refix GC issue
Sign in to reply to this message.
https://codereview.appspot.com/344970043/diff/40001/lily/include/transform.hh File lily/include/transform.hh (right): https://codereview.appspot.com/344970043/diff/40001/lily/include/transform.hh... lily/include/transform.hh:52: Transform (Offset p0) Do you want to allow implicit conversion from an Offset to a Transform? If not, this should be "explicit". https://codereview.appspot.com/344970043/diff/40001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/344970043/diff/40001/lily/stencil-integral.cc#... lily/stencil-integral.cc:740: if (Transform *tp = unsmob<Transform> (trans)) You've cleaned up a lot, but this recurring optionality could probably be improved. Maybe a function would help: Transform make_transform(const Transform *t) { return t ? Transform (*t) : Transform (); } You could also do it as a constructor, if you prefer its syntax and don't mind implementing yet another one: explicit Transform(const Transform *t) ... I merely suggest.
Sign in to reply to this message.
https://codereview.appspot.com/344970043/diff/40001/lily/include/transform.hh File lily/include/transform.hh (right): https://codereview.appspot.com/344970043/diff/40001/lily/include/transform.hh... lily/include/transform.hh:52: Transform (Offset p0) On 2018/06/20 22:30:07, Dan Eble wrote: > Do you want to allow implicit conversion from an Offset to a Transform? If not, > this should be "explicit". Done. https://codereview.appspot.com/344970043/diff/40001/lily/stencil-integral.cc File lily/stencil-integral.cc (right): https://codereview.appspot.com/344970043/diff/40001/lily/stencil-integral.cc#... lily/stencil-integral.cc:740: if (Transform *tp = unsmob<Transform> (trans)) On 2018/06/20 22:30:07, Dan Eble wrote: > You've cleaned up a lot, but this recurring optionality could probably be > improved. Maybe a function would help: > > Transform make_transform(const Transform *t) > { > return t ? Transform (*t) : Transform (); > } > > You could also do it as a constructor, if you prefer its syntax and don't mind > implementing yet another one: > > explicit Transform(const Transform *t) ... > > I merely suggest. A constructor/function from Transform * seems like a side track when the only use case is conversion from SCM. A constructor avoids an extra copy (but I think C++19(?) or so already states that returned classes are to be considered initializers rather than temporaries). But it would seem weird to have a constructor/convertor that silently takes a non-Transform SCM value to an identity transform. What would your favorite be here? It's not like this pattern occurs often enough (yet) to really require something here or right now.
Sign in to reply to this message.
On 2018/06/21 08:48:32, dak wrote: > A constructor/function from Transform * seems like a side track when the only > use case is conversion from SCM. A constructor avoids an extra copy (but I > think C++19(?) or so already states that returned classes are to be considered > initializers rather than temporaries). But it would seem weird to have a > constructor/convertor that silently takes a non-Transform SCM value to an > identity transform. > > What would your favorite be here? It's not like this pattern occurs often > enough (yet) to really require something here or right now. Answering myself here, this would seem to be between ly_scm2transform and robust_scm2transform . What I don't fancy about the robust variants is that they require creating a default even when it isn't getting used. Maybe if that default generally is a const reference and can be provided statically?
Sign in to reply to this message.
Rework according to Dan's review
Sign in to reply to this message.
> On 21 Jun 2018, at 10:48, dak@gnu.org wrote: > > I think C++19(?) or so already states that returned classes > are to be considered initializers rather than temporaries. Perhaps this is what you have in mind: C+17 returns prvalues without creating temporaries; [1], first note. 1. https://en.cppreference.com/w/cpp/language/copy_elision
Sign in to reply to this message.
Hans Åberg <haberg-1@telia.com> writes: >> On 21 Jun 2018, at 10:48, dak@gnu.org wrote: >> >> I think C++19(?) or so already states that returned classes >> are to be considered initializers rather than temporaries. > > Perhaps this is what you have in mind: C+17 returns prvalues without > creating temporaries; [1], first note. > > 1. https://en.cppreference.com/w/cpp/language/copy_elision That looks like it, yes. At any rate, it seems like reconsidering our coding styles for return types now would be postmature. -- David Kastrup
Sign in to reply to this message.
On 2018/06/21 12:07:01, dak wrote: > Rework according to Dan's review Noticeably improved. I do prefer the function to the constructor.
Sign in to reply to this message.
> On 21 Jun 2018, at 00:30, nine.fierce.ballads@gmail.com wrote: > Maybe a function would help: > > Transform make_transform(const Transform *t) > { > return t ? Transform (*t) : Transform (); > } > > You could also do it as a constructor, if you prefer its syntax and > don't mind implementing yet another one: > > explicit Transform(const Transform *t) ... One can also use a tag type argument in the constructor: struct make_t {}; constexpr make_t make{}; Used in the C++ library, cf. https://en.cppreference.com/w/cpp/utility/optional/nullopt_t
Sign in to reply to this message.
Hans Åberg <haberg-1@telia.com> writes: >> On 21 Jun 2018, at 00:30, nine.fierce.ballads@gmail.com wrote: > >> Maybe a function would help: >> >> Transform make_transform(const Transform *t) >> { >> return t ? Transform (*t) : Transform (); >> } >> >> You could also do it as a constructor, if you prefer its syntax and >> don't mind implementing yet another one: >> >> explicit Transform(const Transform *t) ... > > One can also use a tag type argument in the constructor: > struct make_t {}; > constexpr make_t make{}; What would that be good for? -- David Kastrup
Sign in to reply to this message.
> On 22 Jun 2018, at 11:09, David Kastrup <dak@gnu.org> wrote: > > Hans Åberg <haberg-1@telia.com> writes: > >>> You could also do it as a constructor, if you prefer its syntax and >>> don't mind implementing yet another one: >>> >>> explicit Transform(const Transform *t) ... >> >> One can also use a tag type argument in the constructor: >> struct make_t {}; >> constexpr make_t make{}; > > What would that be good for? There are various uses in the C++ library. I found it useful for in a reference class I wrote: template<class A> class ref { protected: A* data_ = nullptr; public: ... template<class... B> ref(make_t, B&&... bs) : data_(finalize(new (collect) A(bs...))) {} }; A direct constructor taking an operator new pointer would not be able to do the GC registering. It admits a syntax: ref<A> a(make, ...); auto a = ref<A>(make, ...); with the ... arguments passed to the A constructor.
Sign in to reply to this message.
Hans Åberg <haberg-1@telia.com> writes: >> On 22 Jun 2018, at 11:09, David Kastrup <dak@gnu.org> wrote: >> >> Hans Åberg <haberg-1@telia.com> writes: >> >>>> You could also do it as a constructor, if you prefer its syntax and >>>> don't mind implementing yet another one: >>>> >>>> explicit Transform(const Transform *t) ... >>> >>> One can also use a tag type argument in the constructor: >>> struct make_t {}; >>> constexpr make_t make{}; >> >> What would that be good for? > > There are various uses in the C++ library. I found it useful for in a reference class I wrote: > template<class A> > class ref { > protected: > A* data_ = nullptr; > > public: > ... > template<class... B> > ref(make_t, B&&... bs) : data_(finalize(new (collect) A(bs...))) {} > }; > A direct constructor taking an operator new pointer would not be able to do the GC registering. > > It admits a syntax: > ref<A> a(make, ...); > auto a = ref<A>(make, ...); > with the ... arguments passed to the A constructor. What would that be good for concerning this issue? -- David Kastrup
Sign in to reply to this message.
> On 22 Jun 2018, at 14:27, David Kastrup <dak@gnu.org> wrote: > > What would that be good for concerning this issue? Only you know that: you did not like the explicit constructor for some reason, but didn't detail.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
