On Feb 13, 2012, at 5:41 AM, Joe Neeman wrote:
Wow, that's quite some work you've done. I haven't read it carefully yet, but here are some broad suggestions/comments:
On Thu, Feb 9, 2012 at 9:50 AM, address@hidden <address@hidden>
I did some experiments with caching that are up on:
Fresh branch up at dev/skylines-cached. This patch should only increase compilation time of a LilyPond score by 1-2 seconds for every minute.
You can thank inclement weather and late trains!
Regarding your comment in axis-group-interface.cc:802, if it gets called twice, there's a bug (usually caused because we've asked for the extent of a cross-staff grob before line-breaking has happened). I'm fine with trying to detect the bug and continue, but I think there should be a programming_error (at least for NDEBUG builds; see the way cyclic dependencies are reported in grob-property.cc).
The reason the function is called twice is because it'd be called through Axis_group_interface::calc_skylines and then again via Hara_kiri_group_spanner::calc_skylines, potentially before one of these finishes to execute. It doesn't seem like this could be related to a line breaking problem.
Since you seem to be expanding the scope of skylines, you may want to add some more flexible constructors instead of doing everything with boxes. For example, it should be easy to add a skyline constructor that builds a skyline from a collection of line segments. Then you can approximate beziers with line segments instead of boxes. I think I even had some code for this lying around, but I can't find it now...
I agree that this is doable. I'd like to investigate this as a second patch, just cuz it seems like an isolatable problem and it doesn't seem like it'd improve spacing to the naked eye in the majority of cases.
add_grobs_of_one_priority looks ok, but a bit complicated. It would be simpler if you got rid of the boxes altogether and only used skylines. You could "hardcode" ly:grob::vertical-skylines-from-stencil as a default for vertical-skylines in the same way that ly:grob::stencil-height is hardcoded as a default for Y-extent. Then you can assume that every grob has a skyline.
I also agree here, but if it's OK with you, I'd like to hold off until this initial code is in the code base.