Creates Skyline_forest smob

Creates Skyline_forest smob

First commit in a series to eliminate the translate_axis call in
axis-group-interface.cc, which will make it easier to work on
system spacing.

Also creates more modular and generalized code.

[dak, 2013-01-23 09:20:51]

https://codereview.appspot.com/7098069/diff/1/lily/skyline-forest.cc
File lily/skyline-forest.cc (right):

https://codereview.appspot.com/7098069/diff/1/lily/skyline-forest.cc#newcode2
lily/skyline-forest.cc:2: This file is part of LilyPond, the GNU music
typesetter.
And the license makes 95% of all the comments in this file adding new
functionality.  There is no comment either here or in the  .hh file that
explains what use case this code is designed for, why it is necessary, and how
it does the job it has been written for.

There is no definition of a "skyline forest", what differentiates it from a
skyline, what operations it does provide for which use cases under which cost
and considerations.

There is no why, what, and how explained anywhere.  It is just a dump of code
without any rationale or explanation.

This code does not even try to be comprehensible to or maintainable by anybody
but Mike, so it should not leave the repository of Mike in this state.

[dak, 2013-01-26 12:51:02]

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc
File lily/skyline-forest.cc (right):

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
lily/skyline-forest.cc:28: A skyline forest is, conceptually, a skyline pair
that can have
"forest" is a strange choice of word here.  In computer science, a "forest" is a
collection of trees without common root, but we don't have trees in here.  So it
is more like an object composition or a skyline pair compound or whatever.

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
lily/skyline-forest.cc:68: other skylines in a forest.
That does not make sense.  If you are talking about "other skylines in a
forest", presumably the skyline pair under consideration is considered to be
part of the forest.  Why would it need to move to avoid "other skylines" then? 
Are skylines in a forest supposed to intersect?  That's not what (physical)
trees do.  And what is "minimum distance"?  According to which metric?  And why
"distance_to_avoid"?  Sounds rather like the distance to move rather than to
avoid.  How is a "forest" created?  It sounds like you try to make it consist of
non-overlapping objects?

What is the purpose of a forest?  Is the focus on arranging a set of skyline
pairs non-overlappingly, or is the focus on being able to move it as a whole?

[Keith, 2013-01-26 21:41:29]

So in   { g4\> g'_"pico"  g' g\! } 
the Staff including notes is one forest-dweller, and the hairpin is another. 
The text "pico" enters the forest to try to find a home.

Maybe this is a simple-enough concept to pull into a separate class, but I'm not
sure.  Too many classes can create spaghetti code, with method calls being the
modern substitute for GOTO.  Like GOTO, classes help if readers can avoid
thinking about what happens at the target of the GOTO, and hurts if readers will
have to go find the other end.

https://codereview.appspot.com/7098069/diff/4001/lily/include/skyline-forest.hh
File lily/include/skyline-forest.hh (right):

https://codereview.appspot.com/7098069/diff/4001/lily/include/skyline-forest....
lily/include/skyline-forest.hh:25: class Skyline_forest
I cannot see what 'forest' means in the name.  The name Skyline_set would make
more sense because this seems to be analogous to Interval_set.   Maybe
Skyline_array because you provide an operator[] and implement as a C array,
though conceptually the Skyline_pairs are not ordered.

There is also Interval_minefield, which seems to implement similar hole-finding.

https://codereview.appspot.com/7098069/diff/4001/lily/include/skyline-forest....
lily/include/skyline-forest.hh:30: vector<Real> skyline_pairs_horizon_padding_;
Storing an individual horizon_padding for each skyline makes sense, because (for
historical reasons, or maybe necessity) *each* skyline has its own padding
applied.
Storing the individual padding here confuses me.  LilyPond leaves room for just
one 'pad' between objects, and its thickness is determined by the padding of the
object being placed.  Why store the padding of already-placed objects?

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc
File lily/skyline-forest.cc (right):

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
lily/skyline-forest.cc:27: /*
Emacs will destroy the indenting, unless you protect with 
/*
 *
 */
Honestly, though, this comment is overkill.

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
lily/skyline-forest.cc:123: // so that it doesn't intersect with other skylines
in the
'other' is confusing.  The skyline being placed 'v_skyline' is not a member of
the Skyline_forest, at least not yet and not so far as this class knows.

https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
lily/skyline-forest.cc:132: Real pad = (padding + skyline_pairs_padding_[i]);
Oh! Maybe we double pad now.  I guess that's new with patch set 47 to
https://codereview.appspot.com/5626052/

Where do we use 'pad' ?

[mike7, 2013-01-27 09:35:05]

On 26 janv. 2013, at 22:41, k-ohara5a5a@oco.net wrote:

> So in   { g4\> g'_"pico"  g' g\! }
> the Staff including notes is one forest-dweller, and the hairpin is
> another.  The text "pico" enters the forest to try to find a home.
> 

Exactly.

> Maybe this is a simple-enough concept to pull into a separate class, but
> I'm not sure.  Too many classes can create spaghetti code, with method
> calls being the modern substitute for GOTO.  Like GOTO, classes help if
> readers can avoid thinking about what happens at the target of the GOTO,
> and hurts if readers will have to go find the other end.
> 

Ah, here, I have no clue.

It is all part of a gimungous project that I hope to finish sometime during the
summer to improve how LilyPond handles cross-staff grobs.

In general, if there is an algorithm that somehow doesn't seem essential to a
class, I like the idea of it being a separate class.  In general, anything that
uses some sort of ::solve () or ::distance () logic I like as a separate class. 
This is why I liked box-quarantine (RIP).

This class facilitates some work I'll be doing, but if you don't think it merits
classhood, I could drop it.  In general, in my work, this code will eventually
be used in Side_position_interface and maybe even in Page_layout_problem.  So it
needs to move out of the Axis_group_interface, where it is currently hanging
out.  Not unlike the Simple_spacer, even though it is only used in a couple
contexts, it seems worth it to refactor the code into a class rather than
shuffling the code around as I modify the design.

> 
>
https://codereview.appspot.com/7098069/diff/4001/lily/include/skyline-forest.hh
> File lily/include/skyline-forest.hh (right):
> 
>
https://codereview.appspot.com/7098069/diff/4001/lily/include/skyline-forest....
> lily/include/skyline-forest.hh:25: class Skyline_forest
> I cannot see what 'forest' means in the name.  The name Skyline_set
> would make more sense because this seems to be analogous to
> Interval_set.

You're right...Skyline_set is better.

> Maybe Skyline_array because you provide an operator[]
> and implement as a C array, though conceptually the Skyline_pairs are
> not ordered.
> 
> There is also Interval_minefield, which seems to implement similar
> hole-finding.
> 
>
https://codereview.appspot.com/7098069/diff/4001/lily/include/skyline-forest....
> lily/include/skyline-forest.hh:30: vector<Real>
> skyline_pairs_horizon_padding_;
> Storing an individual horizon_padding for each skyline makes sense,
> because (for historical reasons, or maybe necessity) *each* skyline has
> its own padding applied.
> Storing the individual padding here confuses me.  LilyPond leaves room
> for just one 'pad' between objects, and its thickness is determined by
> the padding of the object being placed.  Why store the padding of
> already-placed objects?

Excellent question...if for nothing else this is a good reason to make an
abstraction of these things.  This is a holdover from the current code in
axis-group-interface (from which this method comes).  That code is, for this
reason, not coherent with other uses of padding in the code base and should get
a makeover.

> 
> https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc
> File lily/skyline-forest.cc (right):
> 
>
https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
> lily/skyline-forest.cc:27: /*
> Emacs will destroy the indenting, unless you protect with
> /*
> *
> */
> Honestly, though, this comment is overkill.
> 

AHHHH!!!!!  I still don't get this whole comment thing. 
Underkill....overkill....
For now, I'm leaving overkill - I'd rather people accuse me of explaining things
too thoroughly than worry that I haven't explained them sufficiently.

>
https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
> lily/skyline-forest.cc:123: // so that it doesn't intersect with other
> skylines in the
> 'other' is confusing.  The skyline being placed 'v_skyline' is not a
> member of the Skyline_forest, at least not yet and not so far as this
> class knows.

Ok.

> 
>
https://codereview.appspot.com/7098069/diff/4001/lily/skyline-forest.cc#newco...
> lily/skyline-forest.cc:132: Real pad = (padding +
> skyline_pairs_padding_[i]);
> Oh! Maybe we double pad now.  I guess that's new with patch set 47 to
> https://codereview.appspot.com/5626052/
> 
> Where do we use 'pad' ?
> 

pad is used in the calculations of up and down distances (they're calculated a
few lines below pad).

See above...this double padding has been in the axis-group-interface code for a
few years now, but it's not conceptually coherent with other parts of the code. 
I have no clue who implemented it, but it does work quite well...I hope that
it's eliminatable while being able to get the same nice layout.

In general, there are several places in the Lilypond code where conceptually
similar things are done in slightly different ways.  The double padding for
outside staff objects is not good...it is confusing because, as you correctly
state, almost all other algorithms just use the padding of the object being
placed.

> https://codereview.appspot.com/7098069/