Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1753)

Issue 4661061: Adds glissando stems to Lilypond.

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 12 months ago by MikeSol
Modified:
7 years, 11 months ago
Reviewers:
mike, Graham Percival, james.lowe, carl.d.sorensen, Neil Puttock, c_sorensen, reinhold, bernardobarros2, pkx166h, hanwenn
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Adds glissando stems to Lilypond.

Patch Set 1 #

Patch Set 2 : Fixes naming. #

Total comments: 9

Patch Set 3 : Addresses comments from Han-Wen and Neil. #

Patch Set 4 : Fixes issue with beam slopes. #

Patch Set 5 : Fixes some tremolo issues. #

Patch Set 6 : Documents stem placement function. #

Patch Set 7 : Adds event property definition for glissando-stem #

Patch Set 8 : Nixes message in stem-tremolo. #

Total comments: 1

Patch Set 9 : Minimal implementation of glissando stems. #

Patch Set 10 : Creates new GlissandoStem grob. #

Total comments: 28

Patch Set 11 : Merge with new stem code. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+362 lines, -7 lines) Patch
A input/regression/glissando-stem.ly View 1 2 3 4 5 6 7 8 9 1 chunk +90 lines, -0 lines 0 comments Download
M lily/beam.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M lily/glissando-engraver.cc View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -0 lines 0 comments Download
M lily/include/script-interface.hh View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M lily/include/stem.hh View 1 2 3 4 5 6 7 8 9 10 1 chunk +8 lines, -0 lines 0 comments Download
M lily/script-engraver.cc View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -1 line 0 comments Download
M lily/script-interface.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M lily/simple-spacer.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M lily/stem.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +161 lines, -1 line 0 comments Download
M lily/stem-engraver.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M ly/music-functions-init.ly View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -0 lines 0 comments Download
M scm/define-context-properties.scm View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M scm/define-grobs.scm View 1 2 3 4 5 6 7 8 9 10 1 chunk +57 lines, -0 lines 0 comments Download
M scm/define-music-properties.scm View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 35
MikeSol
I have a functional version of this feature up with a regtest. I think the ...
7 years, 12 months ago (2011-06-30 09:02:48 UTC) #1
pkx166h
hello, I'm in no way qualified to comment on code, but am uncomfortable with the ...
7 years, 12 months ago (2011-06-30 09:21:21 UTC) #2
Graham Percival
On Thu, Jun 30, 2011 at 09:21:21AM +0000, pkx166h@gmail.com wrote: > \startGlissandoStem > > while ...
7 years, 12 months ago (2011-06-30 09:25:49 UTC) #3
MikeSol
> Long-term, I'll be pushing for a different naming scheme, but I > have no ...
7 years, 12 months ago (2011-06-30 09:31:58 UTC) #4
hanwenn
can you show png examples of what you're trying to do? I think this patch ...
7 years, 12 months ago (2011-06-30 15:29:23 UTC) #5
Neil Puttock
Hi Mike, The interface is an improvement on the patch you originally posted, but it ...
7 years, 12 months ago (2011-06-30 15:29:30 UTC) #6
mike_apollinemike.com
On Jun 30, 2011, at 5:29 PM, hanwenn@gmail.com wrote: > can you show png examples ...
7 years, 12 months ago (2011-06-30 15:46:13 UTC) #7
mike_apollinemike.com
On Jun 30, 2011, at 5:29 PM, n.puttock@gmail.com wrote: > Hi Mike, > > The ...
7 years, 12 months ago (2011-06-30 16:00:58 UTC) #8
mike_apollinemike.com
Hey Han-Wen, I see the to_spanner of which you speak in gdb.cc, but I'm not ...
7 years, 12 months ago (2011-06-30 16:09:54 UTC) #9
MikeSol
New patch-set uploaded addressing all minor concerns so far. The biggest change is that, as ...
7 years, 12 months ago (2011-06-30 16:49:09 UTC) #10
Colin Campbell
Added issue 1727 for tracking
7 years, 12 months ago (2011-07-01 02:22:14 UTC) #11
mike_apollinemike.com
Hey all, I've uploaded a new patchset that kinda sorta makes tremolos work. There are ...
7 years, 12 months ago (2011-07-01 09:33:50 UTC) #12
pkx166h
On 2011/07/01 09:33:50, mike_apollinemike.com wrote: > Hey all, > I've uploaded a new patchset that ...
7 years, 12 months ago (2011-07-01 10:47:17 UTC) #13
mike_apollinemike.com
On Jul 1, 2011, at 12:47 PM, pkx166h@gmail.com wrote: > On 2011/07/01 09:33:50, mike_apollinemike.com wrote: ...
7 years, 12 months ago (2011-07-01 11:51:20 UTC) #14
hanwenn
sorry - disregard; brainfart. On Thu, Jun 30, 2011 at 1:09 PM, mike@apollinemike.com <mike@apollinemike.com> wrote: ...
7 years, 12 months ago (2011-07-01 15:12:41 UTC) #15
hanwenn
On Thu, Jun 30, 2011 at 12:46 PM, mike@apollinemike.com <mike@apollinemike.com> wrote: >> can you show ...
7 years, 12 months ago (2011-07-01 15:50:25 UTC) #16
mike_apollinemike.com
Hey Han Wen, I'm responding to your e-mail backwards, with answers to the technical parts ...
7 years, 12 months ago (2011-07-01 17:17:20 UTC) #17
Graham Percival
On Fri, Jul 01, 2011 at 07:17:08PM +0200, mike@apollinemike.com wrote: > I'm responding to your ...
7 years, 12 months ago (2011-07-01 17:46:01 UTC) #18
bernardobarros2_gmail.com
2011/7/1 Han-Wen Nienhuys <hanwenn@gmail.com>: > If we add patches like this for every composer's favorite ...
7 years, 12 months ago (2011-07-01 19:01:19 UTC) #19
mike_apollinemike.com
On Jul 1, 2011, at 7:45 PM, Graham Percival wrote: > On Fri, Jul 01, ...
7 years, 12 months ago (2011-07-01 20:12:38 UTC) #20
c_sorensen
On 7/1/11 2:12 PM, "mike@apollinemike.com" <mike@apollinemike.com> wrote: > > I am a proponent of using ...
7 years, 12 months ago (2011-07-01 21:42:52 UTC) #21
mike_apollinemike.com
On Jul 1, 2011, at 5:50 PM, Han-Wen Nienhuys wrote: > On Thu, Jun 30, ...
7 years, 11 months ago (2011-07-03 17:13:36 UTC) #22
Carl
Looking at the details of the code, it seems fine. But I tend to agree ...
7 years, 11 months ago (2011-07-05 04:10:12 UTC) #23
mike_apollinemike.com
On Jul 5, 2011, at 6:10 AM, Carl.D.Sorensen@gmail.com wrote: > Looking at the details of ...
7 years, 11 months ago (2011-07-05 07:58:52 UTC) #24
mike_apollinemike.com
On Jul 5, 2011, at 9:58 AM, mike@apollinemike.com wrote: > > 3) Make the lines_ ...
7 years, 11 months ago (2011-07-05 09:02:24 UTC) #25
reinhold_kainhofer.com
Am Dienstag 05 Juli 2011, 06:10:12 schrieb Carl.D.Sorensen@gmail.com: > I'm wondering if it's possible to ...
7 years, 11 months ago (2011-07-05 13:27:19 UTC) #26
mike_apollinemike.com
On Jul 1, 2011, at 5:50 PM, Han-Wen Nienhuys wrote: > > On a tangent: ...
7 years, 11 months ago (2011-07-16 16:35:56 UTC) #27
mike_apollinemike.com
On Jul 1, 2011, at 5:50 PM, Han-Wen Nienhuys wrote: > > On a tangent: ...
7 years, 11 months ago (2011-07-16 16:36:04 UTC) #28
MikeSol
I've paired this down to a minimal implementation that only contains: 1) Init file stuff ...
7 years, 11 months ago (2011-07-17 18:22:39 UTC) #29
c_sorensen
On 7/17/11 12:22 PM, "mtsolo@gmail.com" <mtsolo@gmail.com> wrote: > I've paired this down to a minimal ...
7 years, 11 months ago (2011-07-17 19:26:43 UTC) #30
James.Lowe_datacore.com
Hello, ________________________________________ From: lilypond-devel-bounces+james.lowe=datacore.com@gnu.org [lilypond-devel-bounces+james.lowe=datacore.com@gnu.org] on behalf of mtsolo@gmail.com [mtsolo@gmail.com] Sent: 17 July 2011 19:22 ...
7 years, 11 months ago (2011-07-17 22:36:04 UTC) #31
MikeSol
After a discussion w/ Han-Wen, I've tried to implement this using a new GlissandoStem grob. ...
7 years, 11 months ago (2011-07-19 11:53:08 UTC) #32
hanwenn
http://codereview.appspot.com/4661061/diff/29001/lily/glissando-engraver.cc File lily/glissando-engraver.cc (right): http://codereview.appspot.com/4661061/diff/29001/lily/glissando-engraver.cc#newcode83 lily/glissando-engraver.cc:83: if (Glissando_stem::has_interface (stem)) acknowledge_glissando_stem ? http://codereview.appspot.com/4661061/diff/29001/lily/include/stem.hh File lily/include/stem.hh (right): ...
7 years, 11 months ago (2011-07-19 12:33:44 UTC) #33
MikeSol
Most of my comments below are Han-Wen specific, but I do have one request (also ...
7 years, 11 months ago (2011-07-19 13:42:06 UTC) #34
hanwenn
7 years, 11 months ago (2011-07-19 13:56:50 UTC) #35
hi there,

as you see I am really strict here; I hope you understand it is my duty as
maintainer to push back on anything invasive, so we can keep lilypond as simple
as possible; nothing personal.

http://codereview.appspot.com/4661061/diff/29001/lily/glissando-engraver.cc
File lily/glissando-engraver.cc (right):

http://codereview.appspot.com/4661061/diff/29001/lily/glissando-engraver.cc#n...
lily/glissando-engraver.cc:83: if (Glissando_stem::has_interface (stem))
On 2011/07/19 13:42:06, MikeSol wrote:
> On 2011/07/19 12:33:45, hanwenn wrote:
> > acknowledge_glissando_stem ?
> 
> So long as we can get the stems from the note columns, it seems that it'd be
> cleaner to get the glissando stems this way.

I don't think this is cleaner.  Acknowledge_glissando_stem is a much more direct
way of saying what you want.

http://codereview.appspot.com/4661061/diff/29001/lily/stem-engraver.cc
File lily/stem-engraver.cc (right):

http://codereview.appspot.com/4661061/diff/29001/lily/stem-engraver.cc#newcode67
lily/stem-engraver.cc:67: stem_ = make_item (to_boolean (get_property
("glissandoStem")) ? "GlissandoStem" : "Stem", gi.grob ()->self_scm ());
On 2011/07/19 13:42:06, MikeSol wrote:
> On 2011/07/19 12:33:45, hanwenn wrote:
> > this is borderline acceptable. Put a TODO here to make a separate glissando
> Note
> > Column engraver if the stem engraver needs to become more elaborate in the
> > future.
> > 
> > Probably, you could have a engraver create a GlissandoStem directly from a
> > glissando-note-event.
> 
> Doable - in general, I'm gonna need to figure out a good input syntax for this
> sorta thing.
> 
> Question: how would one modify the parser/lexer/whatever to create a new
symbol
> for glissando stems?  It'd work just like notes (a, b, c...), rests (r), or
the

Don't create syntax; it's hard and not worth the payoff.  Use a music function
instead that substitutes NoteEvents with GlissandoStemEvents

  \asGlissandoStems {  c4 c16 c8. }

http://codereview.appspot.com/4661061/diff/29001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4661061/diff/29001/lily/stem.cc#newcode1071
lily/stem.cc:1071: Glissando_stem::after_line_breaking (SCM smob)
On 2011/07/19 13:42:06, MikeSol wrote:
> On 2011/07/19 12:33:45, hanwenn wrote:
> > can you try to use property callbacks instead?
> > 
> > what does this do?
> 
> It repositions the note-heads to their appropriate place so that the glissando
> stem links up with them.
> 
> But why reposition noteheads, you ask, when they have no positive extent?  Why
> not dispense with this notehead business and just roll this into
> stem-begin-position?
> 
> Because I can imagine a case where people want noteheads printed (i.e. crosses

can you imagine or do you really need it now? If it's a future extension, drop
it for now and worry about it when someone asks for it.

Since the cross "notehead" has a continuous Y position (depending on the
glissando slope and whatnot), it is not a normal notehead. Much easier to have a
GlissandoHead, and have that adjust its position by reading stem-begin-position
of the glissando stem.

 
> to signify stopping a string momentarily) and want the glissando stem to pass
> through the notehead
> 
> The bulk of this can be farmed out to a property callback for something like
> "position-noteheads" - I'd be calling that callback, though, from
> after_line_breaking.

http://codereview.appspot.com/4661061/diff/29001/lily/stem.cc#newcode1168
lily/stem.cc:1168: me->set_property ("flag", Stem::calc_flag_proc);
On 2011/07/19 13:42:06, MikeSol wrote:
> On 2011/07/19 12:33:45, hanwenn wrote:
> > init in define-grobs.scm
> 
> The problem is that they take dummy-inits in define-grobs.scm that allow for
the
> spacing-spanner to do its thing.  This resets these properties so that the
> correct values can be calculated after the note-heads are moved above.

huh?

spacing spanner should not even see these stems.  You should remove
stem-interface from GlissandoStem. Alternatively, you could add
stem-spacing-interface to distinguish between a stem that affects spacing and
one that doesnt.

However, since the glissandostem is such an odd beast, I think it will be easier
to keep it completely separate.

http://codereview.appspot.com/4661061/diff/29001/lily/stem.cc#newcode1184
lily/stem.cc:1184: }
On 2011/07/19 13:42:06, MikeSol wrote:
> On 2011/07/19 12:33:45, hanwenn wrote:
> > why doesnt the engraver already arrange this ?
> 
> It does, but like the properties above, these properties need to be reset.

like above, figure a way to not have the problem in the first place, rather than
fixing it up afterwards.

http://codereview.appspot.com/4661061/diff/29001/lily/stem.cc#newcode1248
lily/stem.cc:1248: "Like stems, but for glissandi",
start out with an empty interface, and add properties until your regtest
compiles cleanly.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b