I have uploaded a new patch set for review. Something went wrong with git cl: "Incorrect username or password". I may have mistyped it, but I don't think so.
5th patch set up for review. It has one snippet in snippets/new, the only one that needed updating (outside of changes made by the automatic convert-ly update, please disregard the changed snippets in the snippets directory, these were just automatic convert-ly changes). I did not run makelsr.py.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Taking a look at the results makes me skeptical that we are doing people a favor by making the current implementation the default. Maybe we should reconsider. The staggered whiteout is pretty sure to _never_ lead to pleasing results for curved material like letters whenever there is significant overlap with solid objects. And when there isn't, it is not much of a difference to the box whiteout.
So at least until we have the engineering chops for a smooth outline (which will more likely than not require digging around in the C++ code to get a reasonable solution), it does not seem to make a lot of sense to offer this as the primary solution.
Well, any patches are likely to go in quite a different direction than this issue, so it may make sense opening a new issue but recording its eventual resolution here.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ah, because it’s a ‘patch issue’. Of course.
Still, we should discuss here if we want to keep the current (new) whiteout function as default, or modify the default parameters it uses, or revert to using whiteout-box by default and the new ‘outline’ whiteout as an alternative.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The default didn't really change. The patch uses a convert-ly rule to change existing \whiteout to \whiteout-box, specifically because I found so many uses of \whiteout\pad-around as in comment %19. The documentation uses \whiteout-box because of the convert-ly rule.
I would recommend \whiteout over \whiteout-box for most situations. The square whiteout quite generally causes artifacts worse than shown in comment #19. See, for example the workaround for issue 621 at https://code.google.com/p/lilypond/issues/detail?id=621#c11
There is a faint ghost of the barline; zoomed in below, we see that \whiteout-box artifact compared with the new \whiteout.
The ghost barline from \whiteout-box is a bit distracting when seen on the music stand. The jagged outline of \whiteout looks like the messy pattern of ink in the metal that was pushing up by the 'd' engraving die, and does not bother me at normal scale. It would only bother the lilypond engraver, and only if he chooses to zoom in quite far.
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes tests. Includes a full make doc
Labels: -Patch-new Patch-review
Originally posted by: pkx1...@gmail.com
(No comment was entered for this change.)
Owner: pkx1...@gmail.com
Originally posted by: pkx1...@gmail.com
Looks like this needs work?
Labels: -Patch-review Patch-needs_work
Originally posted by: paulwmor...@gmail.com
I have uploaded a new patch set for review. Something went wrong with git cl: "Incorrect username or password". I may have mistyped it, but I don't think so.
https://codereview.appspot.com/236480043
Originally posted by: pkx1...@gmail.com
Passes make, make check - reg test diff attached.
I'll run a full make doc, so assume no update means it has passed
Labels: -Patch-needs_work Patch-review
Originally posted by: pkx1...@gmail.com
I am leaving this on review based on the long response from Keith in Rietveld
Originally posted by: paulwmor...@gmail.com
New patch set uploaded for review (3rd one):
https://codereview.appspot.com/236480043
Originally posted by: dak@gnu.org
Since there are unaddressed points in the review, putting back to Needs_work.
Labels: -Patch-review Patch-needs_work
Originally posted by: paulwmor...@gmail.com
4th patch set uploaded for review.
Originally posted by: pkx1...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-needs_work Patch-new
Originally posted by: pkx1...@gmail.com
The patch needs to make sure that snippets are edited/added via snippets/new/.. not in snippets directly.
Can you redo that and make a new patch?
Don't run the makelsr.py, I can do that when I test the patch
Labels: -Patch-new Patch-needs_work
Originally posted by: paulwmor...@gmail.com
5th patch set up for review. It has one snippet in snippets/new, the only one that needed updating (outside of changes made by the automatic convert-ly update, please disregard the changed snippets in the snippets directory, these were just automatic convert-ly changes). I did not run makelsr.py.
Originally posted by: pkx1...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-needs_work Patch-new
Originally posted by: pkx1...@gmail.com
Passes make, make check and a full make doc - reg test diff as per comment #5
Labels: -Patch-new Patch-review
Originally posted by: pkx1...@gmail.com
Patch on countdown for June 17th
Labels: -Patch-review Patch-countdown
Originally posted by: pkx1...@gmail.com
Patch counted down - please push
Labels: -Patch-countdown Patch-push
Originally posted by: pkx1...@gmail.com
commit [r66a7d343eb27beccd73f8b579d5ef5a635b7e542]
Author: Paul Morris <paulwmorris@gmail.com>
Date: Wed Jun 10 01:40:41 2015 -0400
Issue 4418/6 edits to docs, tests, and changelog
commit [rc5c06029fc0b17266d36b40255c631ac1d31a370]
Author: Paul Morris <paulwmorris@gmail.com>
Date: Wed Jun 10 01:11:56 2015 -0400
Issue 4418/5 whiteout -> whiteout-box in scm/
commit [r5178fc01c82223a2fa1a7dbbce55772022612210]
Author: Paul Morris <paulwmorris@gmail.com>
Date: Wed Jun 10 00:58:48 2015 -0400
Issue 4418/4 run scripts/auxiliar/update-with-convert-ly.sh
commit [ra112b40657ce03ab64fc1d3ffbad794615cb3e3f]
Author: Paul Morris <paulwmorris@gmail.com>
Date: Wed Jun 10 00:42:44 2015 -0400
Issue 4418/3 convert-ly rule: whiteout -> whiteout-box
commit [r8df18ed7b493871d72e52650022fee129789d176]
Author: Paul Morris <paulwmorris@gmail.com>
Date: Wed Jun 10 00:16:16 2015 -0400
Issue 4418/2 add and use new whiteout function
commit [r43ebcfb1b2d94e49cba66549083f4abcccfc7c07]
Author: Paul Morris <paulwmorris@gmail.com>
Date: Tue Jun 9 23:45:40 2015 -0400
Issue 4418/1 add boolean-or-number? predicate
Labels: -Patch-push FIxed_2_19_22
Status: Fixed
Originally posted by: fedel...@gmail.com
(No comment was entered for this change.)
Status: Verified
Originally posted by: dak@gnu.org
Mail by Karol Majewski on lilypond-user:
Subject: What happened to whiteout?
Message-ID: <559faefa7f8b11.37014485@wp.pl>
For some reason this gives horrible output:
=========BEGIN============
\version "2.19.22"
one = {
\clef treble \key c \major \time 2/4 \tempo "Andantino"
a'2 _\markup { \whiteout \pad-markup #0.5 { \dynamic p \italic "dolce" } } a'2
}
two = {
\clef bass \key c \major \time 2/4
c2 c2
}
\score {
\new PianoStaff <<
\new Staff = "one" \one
\new Staff = "two" \two
>>
}
=======END===========
Taking a look at the results makes me skeptical that we are doing people a favor by making the current implementation the default. Maybe we should reconsider. The staggered whiteout is pretty sure to _never_ lead to pleasing results for curved material like letters whenever there is significant overlap with solid objects. And when there isn't, it is not much of a difference to the box whiteout.
So at least until we have the engineering chops for a smooth outline (which will more likely than not require digging around in the C++ code to get a reasonable solution), it does not seem to make a lot of sense to offer this as the primary solution.
Originally posted by: dak@gnu.org
Mailing archive link for comment #19:
<URL:http://lists.gnu.org/archive/html/lilypond-user/2015-07/msg00124.html>
Originally posted by: simon.al...@mail.de
Should we reopen this issue or add a new one, then?
Originally posted by: dak@gnu.org
Well, any patches are likely to go in quite a different direction than this issue, so it may make sense opening a new issue but recording its eventual resolution here.
Originally posted by: simon.al...@mail.de
Ah, because it’s a ‘patch issue’. Of course.
Still, we should discuss here if we want to keep the current (new) whiteout function as default, or modify the default parameters it uses, or revert to using whiteout-box by default and the new ‘outline’ whiteout as an alternative.
Originally posted by: simon.al...@mail.de
See issue 4502 for follow-up on smooth outlines.
Related
Issues: #4502
Originally posted by: k-ohara5...@oco.net
The default didn't really change. The patch uses a convert-ly rule to change existing \whiteout to \whiteout-box, specifically because I found so many uses of \whiteout\pad-around as in comment %19. The documentation uses \whiteout-box because of the convert-ly rule.
I would recommend \whiteout over \whiteout-box for most situations. The square whiteout quite generally causes artifacts worse than shown in comment #19. See, for example the workaround for issue 621 at
https://code.google.com/p/lilypond/issues/detail?id=621#c11
There is a faint ghost of the barline; zoomed in below, we see that \whiteout-box artifact compared with the new \whiteout.
The ghost barline from \whiteout-box is a bit distracting when seen on the music stand. The jagged outline of \whiteout looks like the messy pattern of ink in the metal that was pushing up by the 'd' engraving die, and does not bother me at normal scale. It would only bother the lilypond engraver, and only if he chooses to zoom in quite far.