[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:12:48 -0700 |
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 :-)
https://codereview.appspot.com/547980044/
- Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/19
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), dak, 2020/04/19
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), nine . fierce . ballads, 2020/04/21
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/21
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/21
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden),
hanwenn <=
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), dak, 2020/04/24
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/27
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/27
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/28
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), hanwenn, 2020/04/28
- Re: Rewrite Skyline code (issue 547980044 by address@hidden), jonas . hahnfeld, 2020/04/28