Issue 582010043: Issue 3778: Use bounding box as skylines for markup in svg backend (Closed)
DescriptionIssue 3778: 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.
Patch Set 1 #Patch Set 2 : Remove regression test #
MessagesTotal messages: 14
Remove regression test
Sign in to reply to this message.
please don't submit; I'm rearranging this file completely. Can you add your regression test + instructions on how to reproduce the problem?
Sign in to reply to this message.
On Sat, May 02, 2020 at 12:25:15AM -0700, hanwenn@gmail.com wrote: > please don't submit; I'm rearranging this file completely. > > Can you add your regression test + instructions on how to reproduce the > problem? I did add a regression test in the first patch set, but it breaks testing. It seems we have no way to include the SVG backend in regression tests (I can see one other example that tries to, using inkscape, but it was disabled in 2008). I had to remove the regression test in the second patchset in order to not break make test/check/etc. Do you have any suggestion about how to deal with it? The issue in the tracker shows how to reproduce the problem - do you want that added to the code somewhere? > > https://codereview.appspot.com/582010043/
Sign in to reply to this message.
On 2020/05/02 07:34:29, barrykp wrote: > On Sat, May 02, 2020 at 12:25:15AM -0700, mailto:hanwenn@gmail.com wrote: > > please don't submit; I'm rearranging this file completely. > > > > Can you add your regression test + instructions on how to reproduce the > > problem? > > I did add a regression test in the first patch set, but it breaks > testing. It seems we have no way to include the SVG backend in > regression tests (I can see one other example that tries to, using > inkscape, but it was disabled in 2008). I had to remove the regression > test in the second patchset in order to not break make test/check/etc. > Do you have any suggestion about how to deal with it? > > The issue in the tracker shows how to reproduce the problem - do you > want that added to the code somewhere? > > > > > https://codereview.appspot.com/582010043/ the .ly in the tracker is sufficient. Thanks.
Sign in to reply to this message.
On 2020/05/02 07:37:31, hanwenn wrote: > On 2020/05/02 07:34:29, barrykp wrote: > > On Sat, May 02, 2020 at 12:25:15AM -0700, mailto:hanwenn@gmail.com wrote: > > > please don't submit; I'm rearranging this file completely. > > > > > > Can you add your regression test + instructions on how to reproduce the > > > problem? > > > > I did add a regression test in the first patch set, but it breaks > > testing. It seems we have no way to include the SVG backend in > > regression tests (I can see one other example that tries to, using > > inkscape, but it was disabled in 2008). I had to remove the regression > > test in the second patchset in order to not break make test/check/etc. > > Do you have any suggestion about how to deal with it? > > > > The issue in the tracker shows how to reproduce the problem - do you > > want that added to the code somewhere? > > > > > > > > https://codereview.appspot.com/582010043/ > > the .ly in the tracker is sufficient. Thanks. I don't completely understand, though: if we put the "utf-8-string" directly into the SVG output, the SVG browser might make other font choices, making the outline potentially incorrect. Is this a problem in practice?
Sign in to reply to this message.
On 2020/05/02 07:25:16, hanwenn wrote: > please don't submit; I'm rearranging this file completely. Do you really need to hold this up after it went through the full cycle? If it fixes a problem now and doesn't regress somewhere else, I think we should take it now.
Sign in to reply to this message.
Not necessarily, but the refactoring means I'll likely have to overwrite the fix wholesale. Because there is no regtest, it willl depend on my diligence anyway to fix this again. On Sat, May 2, 2020 at 9:46 AM <jonas.hahnfeld@gmail.com> wrote: > > On 2020/05/02 07:25:16, hanwenn wrote: > > please don't submit; I'm rearranging this file completely. > > Do you really need to hold this up after it went through the full cycle? > If it fixes a problem now and doesn't regress somewhere else, I think we > should take it now. > > https://codereview.appspot.com/582010043/ -- Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
On 2020/05/02 07:38:58, hanwenn wrote: > I don't completely understand, though: if we put the "utf-8-string" directly > into the SVG output, the SVG browser might make other font choices, making the > outline potentially incorrect. Is this a problem in practice? There isn't really an "outline" anymore; the bounding box is used. That is based on the specified font, but if a font were substituted, there will be slight differences. They wouldn't be too big though and so wouldn't be a problem.
Sign in to reply to this message.
On Sat, May 02, 2020 at 01:16:30AM -0700, beauleetienne0@gmail.com wrote: > On 2020/05/02 07:38:58, hanwenn wrote: > > I don't completely understand, though: if we put the "utf-8-string" > directly > > into the SVG output, the SVG browser might make other font choices, > making the > > outline potentially incorrect. Is this a problem in practice? "utf-8-string" never appeared directly in the svg output and that doesn't change. Typically (without changing options) you would end up with <text font-family="serif"></text>. > There isn't really an "outline" anymore; the bounding box is used. That > is based on the specified font, but if a font were substituted, there > will be slight differences. They wouldn't be too big though and so > wouldn't be a problem. Yes, and to expand on this: - lilypond was *already* falling back on the bounding box for pure utf-8-string stencils - but when a grob mixed utf-8-string stencils with something else (a note for example), it would give the grob a skyline *just* from the note, meaning the text portion of the grob would collide with other grobs - this patch changes the behaviour of stencils that mix utf-8-string and other kinds of stencil to be the same as pure utf-8-string (i.e. fall back to the bounding box). > > https://codereview.appspot.com/582010043/
Sign in to reply to this message.
On Sat, May 02, 2020 at 09:59:18AM +0200, Han-Wen Nienhuys wrote: > Not necessarily, but the refactoring means I'll likely have to > overwrite the fix wholesale. Because there is no regtest, it willl > depend on my diligence anyway to fix this again. OK, so what should we do? I am happy to take responsibility for making sure it stays fixed after your work (even if that means another patch), but if there's some way I can rebase it off your work that would be fine too.
Sign in to reply to this message.
Let me see what I can come up with today On Sat, May 2, 2020 at 10:53 AM Kevin Barry <barrykp@gmail.com> wrote: > > On Sat, May 02, 2020 at 09:59:18AM +0200, Han-Wen Nienhuys wrote: > > Not necessarily, but the refactoring means I'll likely have to > > overwrite the fix wholesale. Because there is no regtest, it willl > > depend on my diligence anyway to fix this again. > > OK, so what should we do? I am happy to take responsibility for making > sure it stays fixed after your work (even if that means another patch), > but if there's some way I can rebase it off your work that would be fine > too. -- Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
See http://codereview.appspot.com/545970043 note: 1) Depending on the renderer, the skylines diverge from the text. Inkscape reproduces exactly, but EOG messes up the text formatting. 2) The lack of regression testing is worrying. We could have a section of regtests that is powered by inkscape --export-background-opacity=1.0 --export-background=white --export-type="png" svg.cropped.svg and then do imagemagick diffs on the result. On Sat, May 2, 2020 at 10:53 AM Kevin Barry <barrykp@gmail.com> wrote: > > On Sat, May 02, 2020 at 09:59:18AM +0200, Han-Wen Nienhuys wrote: > > Not necessarily, but the refactoring means I'll likely have to > > overwrite the fix wholesale. Because there is no regtest, it willl > > depend on my diligence anyway to fix this again. > > OK, so what should we do? I am happy to take responsibility for making > sure it stays fixed after your work (even if that means another patch), > but if there's some way I can rebase it off your work that would be fine > too. -- Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
I will close this rietveld issue now that you are taking care of it. Whatever discussion needs to take place can happen at the one you have opened. On Sat, May 02, 2020 at 11:54:14AM +0200, Han-Wen Nienhuys wrote: > See http://codereview.appspot.com/545970043 > > note: > 1) Depending on the renderer, the skylines diverge from the text. > Inkscape reproduces exactly, but EOG messes up the text formatting. > > 2) The lack of regression testing is worrying. We could have a section > of regtests that is powered by > > inkscape --export-background-opacity=1.0 --export-background=white > --export-type="png" svg.cropped.svg > > and then do imagemagick diffs on the result. > > On Sat, May 2, 2020 at 10:53 AM Kevin Barry <barrykp@gmail.com> wrote: > > > > On Sat, May 02, 2020 at 09:59:18AM +0200, Han-Wen Nienhuys wrote: > > > Not necessarily, but the refactoring means I'll likely have to > > > overwrite the fix wholesale. Because there is no regtest, it willl > > > depend on my diligence anyway to fix this again. > > > > OK, so what should we do? I am happy to take responsibility for making > > sure it stays fixed after your work (even if that means another patch), > > but if there's some way I can rebase it off your work that would be fine > > too. > > > > -- > Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
