Re: Creates Skyline_forest smob (issue 7098069)

[k-ohara5a5a]

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.hh#newcode25
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.hh#newcode30
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#newcode27
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#newcode123
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#newcode132
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' ?

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