Menu

#4418 Patch: add stencil-whiteout-outline function

Verified
nobody
Enhancement
2015-07-15
2015-05-24
Anonymous
No

Originally created by: *anonymous

Originally created by: paulwmor...@gmail.com
Originally owned by: pkx1...@gmail.com

add stencil-whiteout-outline function

clean up code formatting in stencil-whiteout

http://codereview.appspot.com/236480043

Related

Issues: #2071

Discussion

  • Google Importer

    Google Importer - 2015-05-24

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

    Patchy the autobot says: passes tests.  Includes a full make doc

    Labels: -Patch-new Patch-review

     
  • Google Importer

    Google Importer - 2015-05-25

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

    (No comment was entered for this change.)

    Owner: pkx1...@gmail.com

     
  • Google Importer

    Google Importer - 2015-05-28

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

    Looks like this needs work?

    Labels: -Patch-review Patch-needs_work

     
  • Google Importer

    Google Importer - 2015-05-28

    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

     
  • Google Importer

    Google Importer - 2015-05-29

    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

     
  • Google Importer

    Google Importer - 2015-05-31

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

    I am leaving this on review based on the long response from Keith in Rietveld

     
  • Google Importer

    Google Importer - 2015-06-01

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

    New patch set uploaded for review (3rd one):
    https://codereview.appspot.com/236480043

     
  • Google Importer

    Google Importer - 2015-06-03

    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

     
  • Google Importer

    Google Importer - 2015-06-07

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

    4th patch set uploaded for review.

     
  • Google Importer

    Google Importer - 2015-06-07

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

    (No comment was entered for this change.)

    Labels: -Patch-needs_work Patch-new

     
  • Google Importer

    Google Importer - 2015-06-07

    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

     
  • Google Importer

    Google Importer - 2015-06-09

    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. 

     
  • Google Importer

    Google Importer - 2015-06-11

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

    (No comment was entered for this change.)

    Labels: -Patch-needs_work Patch-new

     
  • Google Importer

    Google Importer - 2015-06-11

    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

     
  • Google Importer

    Google Importer - 2015-06-14

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

    Patch on countdown for June 17th

    Labels: -Patch-review Patch-countdown

     
  • Google Importer

    Google Importer - 2015-06-16

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

    Patch counted down - please push

    Labels: -Patch-countdown Patch-push

     
  • Google Importer

    Google Importer - 2015-06-17

    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

     
  • Google Importer

    Google Importer - 2015-06-30

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

    (No comment was entered for this change.)

    Status: Verified

     
  • Google Importer

    Google Importer - 2015-07-10

    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.

     
  • Google Importer

    Google Importer - 2015-07-13

    Originally posted by: simon.al...@mail.de

    Should we reopen this issue or add a new one, then?

     
  • Google Importer

    Google Importer - 2015-07-13

    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.

     
  • Google Importer

    Google Importer - 2015-07-13

    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.

     
  • Google Importer

    Google Importer - 2015-07-13

    Originally posted by: simon.al...@mail.de

    See issue 4502 for follow-up on smooth outlines.

     

    Related

    Issues: #4502

  • Google Importer

    Google Importer - 2015-07-15

    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.

     
Monday.com Logo