Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1006)

Issue 567580043: Stop smobifying Transform

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by hanwenn
Modified:
2 months ago
Reviewers:
dak
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Stop smobifying Transform

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -36 lines) Patch
M lily/include/transform.hh View 3 chunks +1 line, -13 lines 0 comments Download
M lily/transform.cc View 2 chunks +0 lines, -23 lines 0 comments Download

Messages

Total messages: 5
dak
Rationale? This negates work done as part of issue 5347 with the long-term goal of ...
2 months ago (2020-05-09 11:03:12 UTC) #1
hanwenn
On 2020/05/09 11:03:12, dak wrote: > Rationale? This negates work done as part of issue ...
2 months ago (2020-05-09 12:00:01 UTC) #2
dak
On 2020/05/09 12:00:01, hanwenn wrote: > On 2020/05/09 11:03:12, dak wrote: > > Rationale? This ...
2 months ago (2020-05-09 14:56:51 UTC) #3
hanwenn
On Sat, May 9, 2020 at 4:56 PM <dak@gnu.org> wrote: > > On 2020/05/09 12:00:01, ...
2 months ago (2020-05-09 16:19:59 UTC) #4
dak
2 months ago (2020-05-09 17:12:07 UTC) #5
On 2020/05/09 16:19:59, hanwenn wrote:
> On Sat, May 9, 2020 at 4:56 PM <mailto:dak@gnu.org> wrote:
> >
> > On 2020/05/09 12:00:01, hanwenn wrote:
> > > On 2020/05/09 11:03:12, dak wrote:
> > > > Rationale?  This negates work done as part of issue 5347 with the
> > long-term
> > > goal
> > > > of making transforms a better integrated and accessible part of
> > stencils.
> > > This
> > > > will be increasingly important when we move to Cairo since Cairo
> > cannot
> > > > represent rotations by multiples of 90 degrees cleanly other than by
> > using an
> > > > explicit transform matrix since (in defiance of all other graphical
> > systems
> > > like
> > > > Postscript, PDF, METAPOST) it refuses to represent angles in any
> > other form
> > > than
> > > > as radians (an actual patch implementing angles as degrees was
> > proferred and
> > > > rejected on what can hardly be called anything but philosophical
> > grounds), and
> > > > PI does not have an exact representation in those.
> > >
> > > Could you sketch how SCM-accessible Transform help with implementing
> > Cairo
> > > support?
> >
> > I have not said that SCM-accessible transforms help with implementing
> > Cairo support.  The problem with Cairo is that it cannot properly
> > represent rotations by multiple of 90 degrees (a common use case) except
> > by using transform matrices, and so the in-stencil (and thus
> > Scheme-accessible) support of transformations, currently mostly
> > constrained to translations and rotations, would have to be moved over
> > to whole transform matrices.
> >
> > > As you can see from the current state, it is not necessary to export
> > them to
> > > Scheme, and
> > > I actually think the state from before
> > https://codereview.appspot.com/344970043
> > > is preferable
> > > to SCM encapsulation. With SCM encapsulation, we create GC overhead,
> > and lose
> > > the type checking that C++ gives us.
> >
> > That statement is not borne out by facts since a Simple_smob does not
> > incur _any_ GC or Scheme overhead or loses any of the type checking of
> > C++ unless you actually call smobbed_copy on it.  Overhead occurs once
> 
> Well, the change I we are discussing, you are actually calling
> smobbed_copy, for example in line 412 here:
> https://codereview.appspot.com/344970043/diff/60001/lily/stencil-integral.cc

Which actually is a pretty good use of it since passing around an SCM expression
(and the stencil expression being passed in parallel also is an SCM expression)
is cheaper than passing a whole transform matrix by value as done before.  And
there is only rather occasional change needed (necessitating the creation of a
new object) since most stencil expressions do not change the current transform.

> 
> > > It is extremely hard to  deprecate or change APIs once they gain use
> > in Scheme,
> > > so everything being equal, the safe option is to not make them
> > accessible IMO.
> >
> > They aren't available from Scheme as of yet.  This is just done in
> > preparation of changes that are sensible to make in future.  And stencil
> > expressions, albeit expressed in terms of Scheme data structures, don't
> > really are manipulated to any significant degree from the user.
> >
> > Which is good since it would make a whole lot of sense to implement them
> > in terms of, say, Cairo data structures in the long run.  And the
> > refactoring of the transform matrix and its use (at the current point in
> > terms of a Pango transform matrix, but changing that detail would be
> > comparatively trivial even though annoyingly the respective structures
> > don't have the same internal layout) lends itself to that purpose very
> > well in that regard.
> >
> > As I stated: performance cannot possibly be an issue before smobbed_copy
> > () is even called since the memory layout and management and access is
> > completely unchanged until one does.
> >
> > So undoing work that has been done for the sake of future extensibility
> > would warrant an actually valid reason.  In the long run, I'd be in
> > preference of moving the SCM reflection of data types now represented by
> > class derivation from Simple_smob (namely expressions without an SCM
> > identity that sensibly can be eq?-compared and a straightforward storage
> > layout) to a templated mechanism that allows confining the Scheme
> > reflection to files actually concerned with it.
> >
> > As long as we are not there, however, I don't see the point in making
> > life harder for those who would want to use transforms from the Scheme
> > side.  We have enough graphical entities represented by Scheme lists
> > already, an appallingly expensive storage representation for material of
> > fixed size.
> 
> I sitll don't understand. You want to have Transform as a smob in
> support of some future functionality. Can you explain what that
> functionality is, and how it would use a smobbed transform?

Stencil expressions during interpretation currently need to recreate transforms
on the fly.  It would be easier to just store them along with the expression.

> This is currently dead code, and the normal treatment of dead code is
> to delete it.

Han-Wen, you don't accept long-term plans of other developers without proof of
concept.  And you throw out intermediate steps working towards that goal and
declare them dead code.

The right point of time for making objections to other people's contributions is
when they were put up for review and discussed.  You did not participate in
those discussions.  The next thing you are doing (when git blame clearly points
to available information about the contributions, their time, their issues,
their rationale and likely point of time) is deleting them without offering a
rationale for that, indicating that you have no respect of previous decisions,
previous discussions, and previous work.

The last decade of LilyPond development has largely been marked by the goal to
make much of LilyPond's internals workable and accessible to power users at the
Scheme level.  This has been a significant driver of LilyPond-based solutions
and of an active and helpful user base.  It has enabled significant
contributions by community members that are not core C++ programmers.

I don't see the point in reversing this course and locking access to LilyPond's
internals to anybody but C++ programmers.  Rewinding every effort in that
direction throws away the potential of a quite larger community of contributors
than the C++ programmers, and in this particular case the C++ programmers do not
even get anything in return.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b