#3778 SVG backend: Use bounding box as skylines for markup / Compute SVG skyline based on original data

[Anonymous]

https://codereview.appspot.com/545970043

(needs https://codereview.appspot.com/555760043/)

Compute SVG skyline based on original data

Since stencil data is used as input to formatting decisions, this
will make SVG output more similar to PS/PDF output.

---

Use bounding box as skylines for markup in svg backend

As there is no routine for determining skylines for utf-8-string
stencils, they normally fall back to the grob's bounding box, which is
fine. However, when there is a mixture of utf-8-string and other types
of stencil (which have associated skyline functions) in a single grob,
the entire grob gets a skyline determined only from the non-utf-8-string
stencils. This sometimes causes the text portion of such mixed grobs
(e.g. metronome marks) to collide with other grobs.

While looping over the stencils, check for utf-8-string and if found,
clear the skylines and break out of the loop. Empty skylines forces a
fallback to the grob's bounding box, which restores the behaviour from
before the patch to improve skyline approximations (issue 2148). This
does not fix the issue that there is no routine for determining skylines
for utf-8-strings when the backend is svg, but it does at least remove
the collisions without changing the behaviour in non-broken situations.

Add a suitable (svg backend) regression test.

---

Originally reported here.

the tempo marking in the SVG output is both lower, so that it collides with the stem of the A, and also has insufficient space between the equals sign and the preceding quarter note:

\version "2.18.0"

\relative c'' {
  \tempo 4=76
  a4
}

[Google Importer]

Originally posted by: pnorcks@gmail.com

The lack of space between the notehead and equals sign is a known issue. See issue 1434.

The collision looks like a regression.

Owner: pnorcks@gmail.com

[Google Importer]

Originally posted by: JanRosse...@gmail.com

The severity of this is more than ugly. For scores starting with a rest, followed by music with multiple ledger lines, the end result can become unreadable.

\version "2.18.2"

\relative c''' {
  \tempo "Allegro moderato" 4 = 120
  r1
  \repeat unfold 5 { c4 f e b }
}

[Google Importer]

Originally posted by: JanRosse...@gmail.com

Just to show how ugly this can be in real life: our cello's complained about the rendering of the beginning of the third part of Ravel's Tombeau de Couperin on the SCORA tablets. See attachment.

[Google Importer]

Originally posted by: fedel...@gmail.com

Jan, I agree that your example is really ugly, but in order to be classified as Critical, it should be a regression:
http://lilypond.org/doc/v2.19/Documentation/contributor/issue-classification

I can see that the bug is present in 2.18.2.
But I can't verify 2.16.2, because I'm getting an error similar to that described in issue #3809:

warning: cannot find SVG font #f
/usr/share/lilypond/2.16.2/scm/output-svg.scm:543:7: In procedure string-null? in expression (string-null? expr):
/usr/share/lilypond/2.16.2/scm/output-svg.scm:543:7: Wrong type argument in position 1 (expecting string): #<unspecified>

Can anyone confirm that it's a regression against 2.16.2?

[Google Importer]

Originally posted by: PhilEHol...@googlemail.com

Occurred between 2.17.0 and 2.17.1 so it's a regression with respect to 2.16.0.  I assume it's to do with skylining.  There's an incantation to turn off skylining, I think?  If so, it may serve as a workaround for this.

Labels: -Type-Ugly Type-Critical Regression

[Google Importer]

Originally posted by: JanRosse...@gmail.com

If someone tells me how to turn of skylining, I'll give it a go.

[Google Importer]

Originally posted by: JanRosse...@gmail.com

Just to confirm, this worked flawlessly with 2.16.x.

My biggest problem is the difference between PDF and SVG output. If this would also happen with PDF, we would have spotted this a month ago. I don;t quite well understand how skylining is broken for SVG, but not for PDF...

[Google Importer]

Originally posted by: lilyli...@googlemail.com

I'm not sure what to think about it. But officially for this to be a (blocking) regression, this should have been working on purpose before, that is, someone has done something specific about it to work correctly.

[Google Importer]

Originally posted by: dak@gnu.org

Well, SVG is purposely expected to have the same output than PDF.  Can anybody figure out what exactly is going wrong in the SVG file?

[Google Importer]

Originally posted by: JanRosse...@gmail.com

I'm not sure what to think about it. But officially for this to be a (blocking)
regression, this should have been working on purpose before, that is, someone
has done something specific about it to work correctly.

Sorry, but that just ain’t true. If something was working before—even by accident—it should still be working in a new release.

This being said: the bug is really strange. The tempo marking goes up with certain notes, and not with others. In other words: it’s not always sitting at the same height above the staff. For example in the minimal example I give, when one leaves out the starting rest (r1), it behaves correctly.

To complicate things further, look at the following example, which matches more what one does for larger scores:

\version "2.18.2"

music = \relative c''' {
  \grace s16
  r1
  \repeat unfold 5 { c4 f e b }
}
global = \relative c''' {
  \tempo "Allegro Moderato" 4 = 120
  s1
  s1*4
}

\score {
  \new Staff { << \global \music >> }
}

If one leaves out the leading grace note on the music variable, the result is different. The tempo marking shift up a little bit, but not enough. Having the grace spacer rest, buit not the r1 makes the tempo marking shift up further, but still not enough. It is only removing both the grace spacer rest and the r1 that gives a correct result.

So with the four permutations (grace present or not, r1 present or not), one gets four different results.

Don’t know if this helps…

[Google Importer]

Originally posted by: PhilEHol...@googlemail.com

I'm not sure what to think about it. But officially for this to be a (blocking) regression, this should have been working on purpose before, that is, someone has done something specific about it to work correctly.

Sorry, but that just ain’t true. If something was working before—even by accident—it should still be working in a new release.

Please read http://lilypond.org/doc/v2.13/Documentation/contributor/issue-classification: ‘Regression: it used to work intentionally in an earlier stable release. If the earlier output was accidental (i.e. we didn’t try to stop a collision, but it just so happened that two grobs didn’t collide), then breaking it does not count as a regression.’

[Google Importer]

Originally posted by: dak@gnu.org

Phil, we are not talking about a collision here (namely LilyPond placing things on top of each other) since this only occurs with SVG output. LilyPond does not position things differently depending on backend. What we are talking about here is the SVG output diverging from other backends. That this positioning error results in a collision is just incidental.

The SVG output always was intended to be the same as that of other backends. When the SVG backend now goes off the deep end and places things in places different from where LilyPond thinks it placed them it is absurd to state that the SVG output previously matched the output of other backends by accident rather than by design.

In fact, it would still be an error and regression if such aberrant placement would result in only the SVG output avoiding a collision.

We are not relying on accidents in a backend to get things right or wrong. The backend is supposed to reflect what LilyPond thinks it does. Quite intentionally.

It did so before, it fails to do so now. That’s a regression. It’s conceivable that the error was present always but is now only triggered by different positioning. But while we have no definite analysis, it’s hard to say.

Do all of the presented cases appear with the same version or even commit?

[Google Importer]

Originally posted by: PhilEHol...@googlemail.com

I was making no comment about whether it is a regression or not—simply pointing out that ‘Sorry, but that just ain’t true. If something was working before—even by accident—it should still be working in a new release.’ is not a correct interpretation of the way we develop LilyPond.

[Google Importer]

Originally posted by: PhilEHol...@googlemail.com

There’s something quite odd about how this appears. If you add another line of music below the first line, the first line does not collide but the second does. That appears related to the time signature. See the attached. It would seem there’s some sort of error how in how the locations of the tempo markings are calculated, based on what goes before.

[Google Importer]

Originally posted by: PhilEHol...@googlemail.com

OK, I think I've worked some more out. It looks like the text makes no effort to avoid collisions if there is a notehead in the tempo marking, but the notehead in the tempo signature is placed based on the surrounding objects. See attached.

[Google Importer]

Originally posted by: dak@gnu.org

It really looks as though the whole text is invisible to the positioning.  Do we indeed get different font metrics/behavior depending on the backend?

But wouldn't using more than one backend necessarily lead to the same positioning?

[Google Importer]

Originally posted by: PhilEHol...@googlemail.com

I don’t understand in any way how the output is generated, but here’s some further data. Turning on the skyline debugging shows how the SVG backend fails to skyline the text with a note glyph accurately, whilst using a rectangular skyline if the note is not there. Contrast with how this works with the PDF backend.

[Google Importer]

Originally posted by: dak@gnu.org

Excellent sleuth work. debug-skylines makes it quite clear that it is indeed not just the backend but also LilyPond that has a different notion of the content metrics.

That would seem to suggest that the font access is different, and leads to different results.

Taking some closer look at LilyPond’s options, it would seem that a single run has just one backend but may have different formats. So the output including LilyPond's idea of it (metrics) would seem to depend on the backend.

I'm currently doing a bisection (it’s somewhere between 2.17.0 and 2.17.1).

[Google Importer]

Originally posted by: dak@gnu.org

r28f3294954eff1f263d3b2e3de1c520f4d2fbdfc is the first bad commit

commit r28f3294954eff1f263d3b2e3de1c520f4d2fbdfc
Author: Mike Solomon <mike@apollinemike.com>
Date:   Mon Aug 27 23:47:04 2012 +0200

    Improvements in vertical skyline approximations (issue 2148).

    The file stencil-integral.cc provides a suite of functions that
    traverse a stencil and do linear approximations of its components.
    These are then turned into boxes that are passed to the Skyline
    constructor. This approximation is used for several vertical
    skylines including those of VerticalAxisGroup and System. As a
    result of these more accurate approximations, vertical spacing is
    more snug between grobs.

    Additionally, in axis-group-interface.cc, skylines of grobs are no
    longer compared to a monolithic axis-group skyline but rather all
    of the component skylines of the axis-group, allowing grobs to
    be fit under other ones if there is space instead of always 
    shifted over.

    Two new python scripts allow to visualize the position of skylines.

    All other changes provide functions that allow for better debugging
    of Skylines, better approximations of grobs via skylines, and 
    changes to the measurement of distance between grobs via the new 
    Skyline API.

    This results in a significant time increase in score compilation
    for objects with complex skylines such as all text grobs.  For 
    orchestral scores, the increase is not as steep.

So it would appear that the skylining for SVG fonts is broken and the old behavior has been replaced by a non-working one. The commit is quite humongous, so pinpointing the relevant part is certainly non-trivial.

Cc: mts...@gmail.com

[Google Importer]

Originally posted by: googlegr...@ensemble101.fr

Hmm… the place to look for the difference is make_glyph_string_boxes (stencil-integral.cc). It is likely getting called in one place and not the other.

[Google Importer]

Originally posted by: dak@gnu.org

Previous to the patch, the texts in both SVG and PDF had the same enclosing box, so the SVG version is likely not completely unchanged. I am somewhat surprised that the text handling is different enough for different backends to change LilyPond’s metrics.

\override Score.MetronomeMark.vertical-skylines = #'()

seemingly restores the old behavior, and that output looks at first glance correct though a rectangular bounding box can be achieved even with lots of missing points.

[Google Importer]

Originally posted by: dak@gnu.org

A grep for "backend" leads to the following in lily/pango-font.cc (Pango_font::text-stencil):

  bool to_paths = music_strings_to_paths;

  /*
    Backends with the utf-8-string expression use it when
      1) the -dmusic-strings-to-paths option is set
         and `str' is not a music string, or
      2) the -dmusic-strings-to-paths option is not set.
  */
  if (has_utf8_string && ((to_paths && !music_string) || !to_paths))
    {
      // For Pango based backends, we take a shortcut.
      SCM exp = scm_list_3 (ly_symbol2scm ("utf-8-string"),
                            ly_string2scm (description_string ()),
                            ly_string2scm (str));

      Box b (Interval (0, 0), Interval (0, 0));
      b.unite (dest.extent_box ());
      return Stencil (b, exp);
    }

  return dest;

Could this be related?

[Google Importer]

Originally posted by: dak@gnu.org

In stencil-integral.cc, the routine stencil_traverser looks fishy in several places. While I don’t see how this could affect different backends differently, it still seems worth fixing.

To wit:

  else if (expr == ly_string2scm (""))
  return vector<Transform_matrix_and_expression> ();

The comparison will not in general be useful since ly_string2scm ("") is not guaranteed to be eq? to any other instance of "".

  else
    {
      vector<Transform_matrix_and_expression> out;
      out.push_back (Transform_matrix_and_expression (trans, expr));
      return out;
    }
  warning ("Stencil expression not supported by the veritcal skylines.");
  return vector<Transform_matrix_and_expression> ();

The warning cannot ever be reached. In stencil_dispatcher we have

  else
    {
#if 0
      warning ("Stencil expression not supported by the veritcal skylines.");
#endif
      /*
        We don't issue a warning here, as we assume that stencil-expression.cc
        is doing stencil-checking correctly.
      */
    }

Which seems absurd: if the warning is not supposed to be triggered, one can do a programming_error instead, but outcommenting it seems like ‘let’s hide this problem away’ behavior.

[Kevin Barry]

If you undefine out the warning then it will be printed when the backend is svg because the stencil expression is "utf-8-string" not "glyph-string". There is no routing for handling this type of stencil expression, so either a routing needs to be added or something went wrong before this point that resulted in this expression arriving in this form or at this place.

[Google Importer]

Originally posted by: dak@gnu.org

It’s somewhat hard for me to figure this out definitely, but I think it appears like -dbackend=socket produces output where the tempo marking shown in comment #2 overlaps with noteheads.

That would make it likely that the problem is occuring in any non-Postscript based backend.