Re: Creates Skyline_forest smob (issue 7098069)

[address@hidden]

On 26 janv. 2013, at 22:41, address@hidden 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.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.

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.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?

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#newcode27
> 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#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.

Ok.

> 
> 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' ?
> 

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/