lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Rewrite Skyline code (issue 547980044 by address@hidden)


From: hanwenn
Subject: Re: Rewrite Skyline code (issue 547980044 by address@hidden)
Date: Fri, 24 Apr 2020 10:56:35 -0700

On 2020/04/24 17:12:48, hanwenn wrote:
>
https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh
> File lily/include/skyline.hh (right):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh#newcode152
> lily/include/skyline.hh:152: }
> On 2020/04/24 16:33:04, hahnjo wrote:
> > Why do you need all of this in the header file? As far as I can see,
nobody
> else
> > is calling these methods, so the argument of inlinining does not
apply.
> 
> trimmed this a bit.
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc
> File lily/skyline-pair.cc (left):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc#oldcode100
> lily/skyline-pair.cc:100: Skyline_pair::print_points () const
> On 2020/04/24 16:33:04, hahnjo wrote:
> > You should not delete a function without removing the declaration in
> > lily/include/skyline-pair.hh. Also this seems unrelated to the
performance
> > improvements.
> 
> the print() function prints the building in terms of y-intercept at
x=0.0 +
> slope. Since we don't store the y-intercept anymore, that is useless
now. The
> points are also more convenient for debugging, which is the only
objective of
> this function.
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc
> File lily/skyline.cc (right):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc#newcode780
> lily/skyline.cc:780: Skyline::to_segments (Axis horizon_axis) const
> On 2020/04/24 16:33:04, hahnjo wrote:
> > What's the advantage of this code over the old method?
> 
> the buildings making up the skylines have become non-contiguous, so
you can't
> simply connect all the points any more.  (If you'd do that, there
would be
> diagonal line segments printed in debug output that aren't really
there.)
> 
>
https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm#newcode1657
> scm/define-grobs.scm:1657: (stencil . ,ly:paper-column::print)
> On 2020/04/24 16:33:04, hahnjo wrote:
> > Is this change intentional? And intentionally in this review?
> 
> thanks, no. I was already wondering why the cell count shot up in the
profile
> :-)

grmbl.- and now the speed difference is gone again.  I hate my
autoscaling CPU.


https://codereview.appspot.com/547980044/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]