Issue 4936: look up "mf" for default initial volume

Issue 4936: look up "mf" for default initial volume

When the Dynamic_performer begins processing, if no dynamic is
specified, act as if "mf" were specified.

Changes in note velocity from 90 to 86 in many MIDI regression tests
are expected.

[ht, 2016-08-04 21:23:12]

LGTM except for a small question (still) about the process_music logic.

Also, I noticed the following definition in scm/midi.scm:

;; 90 == 90/127 == 0.71 is supposed to be the default value
;; urg: we should set this at start of track
(define-public dynamic-default-volume 0.71)

I wonder whether this (possibly obsolete, "git grep" doesn't show it being
currently referenced from anywhere, certainly not from Dynamic_performer)
definition could (or should) be removed as part of this change?

[ht, 2016-08-04 21:23:25]

https://codereview.appspot.com/308890043/diff/1/lily/dynamic-performer.cc
File lily/dynamic-performer.cc (right):

https://codereview.appspot.com/308890043/diff/1/lily/dynamic-performer.cc#new...
lily/dynamic-performer.cc:412: volume = equalize_volume
(Audio_span_dynamic::DEFAULT_VOLUME);
I keep wondering whether this assignment is intended to be kept identical to the
way "volume" is initialized above in the "first time only" case (before this
patch, the behavior would be the same, but now it's different), and why the
"first time" case needs to be handled separately since the control flow will end
up in the if block starting at line 405 even in that case...

[Carl, 2016-08-04 21:30:58]

On 2016/08/04 21:23:12, ht wrote:
> LGTM except for a small question (still) about the process_music logic.
> 
> Also, I noticed the following definition in scm/midi.scm:
> 
> ;; 90 == 90/127 == 0.71 is supposed to be the default value
> ;; urg: we should set this at start of track
> (define-public dynamic-default-volume 0.71)
> 

It would seem to me that the Dynamic_performer should use the scheme variable
dynamic-default-volume as the default, rather than a hard-coded lookup of mf.

In my opinion, we should not have any constants that a user might want to change
hard-coded in C++, because then the user can't change them.

I'd also be delighted to have the default value of dynamic-default-volume be set
to the lookup of mf in scm/midi.scm

[Dan Eble, 2016-08-04 22:52:11]

On 2016/08/04 21:30:58, Carl wrote:
> On 2016/08/04 21:23:12, ht wrote:
> > LGTM except for a small question (still) about the process_music logic.
> > 
> > Also, I noticed the following definition in scm/midi.scm:
> > 
> > ;; 90 == 90/127 == 0.71 is supposed to be the default value
> > ;; urg: we should set this at start of track
> > (define-public dynamic-default-volume 0.71)

I'd be happy to remove that.

> It would seem to me that the Dynamic_performer should use the scheme variable
> dynamic-default-volume as the default, rather than a hard-coded lookup of mf.

Overkill.  A user who wants a specific dynamic should code it into the music.

Even overlooking that, it would be unlike Lilypond to define such a thing
globally rather than as a context property.

> In my opinion, we should not have any constants that a user might want to
change
> hard-coded in C++, because then the user can't change them.

I don't see how to escape having a C++ default for something like this:
    robust_scm2double (svolume, Audio_span_dynamic::DEFAULT_VOLUME);
Please describe your preferred alternative more specifically.

[Dan Eble, 2016-08-04 23:07:59]

https://codereview.appspot.com/308890043/diff/1/lily/dynamic-performer.cc
File lily/dynamic-performer.cc (right):

https://codereview.appspot.com/308890043/diff/1/lily/dynamic-performer.cc#new...
lily/dynamic-performer.cc:412: volume = equalize_volume
(Audio_span_dynamic::DEFAULT_VOLUME);
On 2016/08/04 21:23:25, ht wrote:
> [...] why the
> "first time" case needs to be handled separately since the control flow will
end
> up in the if block starting at line 405 even in that case...

"volume" is passed as a parameter to two functions between the "first time"
block and here.

[Dan Eble, 2016-08-04 23:30:17]

On 2016/08/04 21:23:12, ht wrote:
> LGTM except for a small question (still) about the process_music logic.
> 
> Also, I noticed the following definition in scm/midi.scm:
> 
> ;; 90 == 90/127 == 0.71 is supposed to be the default value
> ;; urg: we should set this at start of track
> (define-public dynamic-default-volume 0.71)
> 
> I wonder whether this (possibly obsolete, "git grep" doesn't show it being
> currently referenced from anywhere, certainly not from Dynamic_performer)
> definition could (or should) be removed as part of this change?

I just said I would be happy to delete this, but now I'm wondering, even if it
doesn't break Lilypond per se, is removing this going to break user code?  If
it's "public" that means people might be using it, right?

[ht, 2016-08-06 10:43:17]

On 2016/08/04 23:30:17, Dan Eble wrote:
> On 2016/08/04 21:23:12, ht wrote:
> > (define-public dynamic-default-volume 0.71)
> > 
> > I wonder whether this (possibly obsolete, "git grep" doesn't show it being
> > currently referenced from anywhere, certainly not from Dynamic_performer)
> > definition could (or should) be removed as part of this change?
> 
> I just said I would be happy to delete this, but now I'm wondering, even if it
> doesn't break Lilypond per se, is removing this going to break user code?  If
> it's "public" that means people might be using it, right?

This may of course be true; maybe this is just another case of it being
impossible to remove any of the old and undocumented functionality (even though
it's useless from the LilyPond point of view) just in case someone may have
started to depend on it...

In this particular case, the definition has been in actual use (in
lily/dynamic-performer.cc) only in the year 2000 between LilyPond 1.3.31, for
which the definition was added in commit
4990a43304a74ffe431951798e9d674101f09278, and LilyPond 1.3.93, where the only
reference to the Scheme definition was removed (commit
f988425624a6f6d1a48aea0ac0c1c84ff0857e56).  "git log -Sdynamic-default-volume"
doesn't show any new references having been introduced since.  Considering that
the variable doesn't have any behavioral meaning in LilyPond, the only place
where I could imagine any direct use for the variable is in the definition of a
custom absolute volume function... so I'd still be in favor of removing the
definition to remove an unnecessary point of confusion for understanding the
source code.

To better cater for Carl's suggestions, there could be the possibility (after
removing this unused definition) of extending the default absolute volume
function with a new "default" entry and use this special key instead of "mf" to
look up the default volume in Dynamic_performer (making it possible to customize
the default initial volume independently of the other predefined dynamics via a
custom absolute volume function).  However, having any kind of Scheme definition
for the default initial volume feels like overkill to me as well - as Dan wrote,
users who wish to use a custom initial dynamic level in a Voice can always do so
by specifying the dynamic explicitly (which is usually necessary anyway at the
beginning of each Voice to protect against any changes in the MIDI dynamic level
which are not visible in the typeset output).

[ht, 2016-08-06 11:15:35]

https://codereview.appspot.com/308890043/diff/1/lily/dynamic-performer.cc
File lily/dynamic-performer.cc (right):

https://codereview.appspot.com/308890043/diff/1/lily/dynamic-performer.cc#new...
lily/dynamic-performer.cc:412: volume = equalize_volume
(Audio_span_dynamic::DEFAULT_VOLUME);
On 2016/08/04 23:07:59, Dan Eble wrote:
> "volume" is passed as a parameter to two functions between the "first time"
> block and here.

Do you mean the 'finish_queued_spans' function?  Isn't it the case that
'this->depart_queue_.spans_' is empty when 'this->process_music' gets called the
first time, so 'finish_queued_spans' would report a programming error if it
actually got called (so volume must still be -1 when reaching line 411 in this
case)?  (Even if I'm wrong with this reasoning, I think the question of whether
the default volume lookup at lines 388-389 and 412 should always work the same
way, and if not, why, remains valid.)

[Dan Eble, 2016-08-06 21:38:21]

On 2016/08/06 11:15:35, ht wrote:
> Do you mean the 'finish_queued_spans' function?  Isn't it the case that
> 'this->depart_queue_.spans_' is empty when 'this->process_music' gets called
the
> first time, so 'finish_queued_spans' would report a programming error if it
> actually got called (so volume must still be -1 when reaching line 411 in this
> case)?  (Even if I'm wrong with this reasoning, I think the question of
whether
> the default volume lookup at lines 388-389 and 412 should always work the same
> way, and if not, why, remains valid.)

I'm not sure how to say this without sounding rude—I want to remain on good
terms—but what do you think of satisfying your curiosity by trying it yourself? 
There are things I would rather do than rewrite this section of working code. 
My goal was to initialize the dynamic to "mf" and I did that.

[ht, 2016-08-07 18:40:54]

On 2016/08/06 21:38:21, Dan Eble wrote:
> I'm not sure how to say this without sounding rude—I want to remain on good
> terms—but what do you think of satisfying your curiosity by trying it
yourself? 
> There are things I would rather do than rewrite this section of working code. 
> My goal was to initialize the dynamic to "mf" and I did that.

Sure.  I really appreciate all the improvements you've done to Dynamic_performer
lately, and never meant to dispute any of the ideas with my questions (which
came just out of curiosity from my part, to try and improve my own confidence
about understanding the new code in places where it took for me the most time to
comprehend the actual effects of the logic in different cases).  I hope that you
can accept my apology if it's me who's already started to sound rude with the
insistent questions about working code; I will stop now.

[dak, 2016-08-10 16:56:56]

Sigh.  Dan has suggested I might want to provide feedback here.  Now I'm not
really much into the Midi code to be honest, so this more or less boils down to
consulting a gut feeling and/or general policy.

The main thing this boils down to here is that Heikki feels that Dan changed one
occurence of code and not a parallel one, and Dan is not really sure whether the
second code path should even get encountered.

I'm with Heikki here (if I understand the argument correctly) that it does not
seem to make sense to leave the second code path different from the first: that
way lies confusion for the next person encountering the code, and additional
time will get spent analysing what may be dead code.

Heikki seems more confident about what code might be dead here but Dan is
currently proposing a patch.

Can we get to some version of the code where the code paths supposed to be
equivalent (is there agreement about that?) actually looks the same?

If so, this would be a good starter for Heikki to eventually propose a cleanup
that would result in a removal of the dead code or keeping it but adding a
programming error.  Something like that.

This sounds like work either party has not bargained on doing.  But it would
likely be easier to get it done now that people have taken a bit of a look than
at a time when the person puzzling over it has no indication about why stuff
looks different.

[Dan Eble, 2016-08-10 22:17:40]

David, do you have advice on the direction to take regarding the comments on
this thing in midi.scm?

        (define-public dynamic-default-volume 0.71)

To me it seems unnecessary (the user should specify a dynamic if a specific
dynamic is desired) and the wrong place for a default anyway.  It should be in a
context if it needs to be anywhere, right?  Thanks.

[c_sorensen, 2016-08-10 23:05:23]

On 8/10/16 4:17 PM, "nine.fierce.ballads@gmail.com"
<nine.fierce.ballads@gmail.com> wrote:

>David, do you have advice on the direction to take regarding the
>comments on this thing in midi.scm?
>
>         (define-public dynamic-default-volume 0.71)
>
>To me it seems unnecessary (the user should specify a dynamic if a
>specific dynamic is desired)

I'm not David, but I'd like to describe my use case.

When writing congregational hymns, dynamics are very seldom included.

But I may wish to generate practice midi from my engraving (I regularly do
it).  It would be nice to be able to define my own default midi volume that
would apply.

>and the wrong place for a default anyway.
>It should be in a context if it needs to be anywhere, right?

What context would it be defined in?  the midi performer can exist in a
variety of contexts.

I suppose that different contexts might have different default midi
volumes.

So I guess the default midi volume might be defined in the Score context,
and then any lower contexts could override the default midi volume.  And
of course, you are right that this should then become a documented context
property.

So at any rate, I think the public definition should be eliminated from
midi.scm, regardless of whether or not somebody has used it.

If eliminating causes them problems, they can add that definition to their
ly files.

Thanks,

Carl

[dak, 2016-08-11 10:28:43]

On 2016/08/10 23:05:23, c_sorensen wrote:
> 
> On 8/10/16 4:17 PM, mailto:"nine.fierce.ballads@gmail.com"
> <mailto:nine.fierce.ballads@gmail.com> wrote:
> 
> >David, do you have advice on the direction to take regarding the
> >comments on this thing in midi.scm?
> >
> >         (define-public dynamic-default-volume 0.71)
> >
> >To me it seems unnecessary (the user should specify a dynamic if a
> >specific dynamic is desired)
> 
> I'm not David, but I'd like to describe my use case.
> 
> When writing congregational hymns, dynamics are very seldom included.
> 
> But I may wish to generate practice midi from my engraving (I regularly do
> it).  It would be nice to be able to define my own default midi volume that
> would apply.
> 
> >and the wrong place for a default anyway.
> >It should be in a context if it needs to be anywhere, right?
> 
> What context would it be defined in?  the midi performer can exist in a
> variety of contexts.
> 
> I suppose that different contexts might have different default midi
> volumes.
> 
> So I guess the default midi volume might be defined in the Score context,
> and then any lower contexts could override the default midi volume.  And
> of course, you are right that this should then become a documented context
> property.
> 
> So at any rate, I think the public definition should be eliminated from
> midi.scm, regardless of whether or not somebody has used it.

If somebody has used it, it would have had no effect.  I think it is saner to
remove it so that the code trying to set it actually has a chance to trigger an
error (which usually will be dealt with by removing the ineffectual statement).

With regard to your use case: anything that cannot be dealt with by using the
equivalent of
\score {
  { c'-\hide\ppp }
  \layout {}
  \midi { }
}

\score {
  { g'-\hide\fff }
  \layout {}
  \midi { }
}

Maybe we should also provide the equivalent \loudness "ppp" or so that only
affects Midi (and can stand on its own rather than as an articulation and thus
may also be used in a \midi block)?  At any rate, this does not appear to affect
Dan's patch.  Removing dynamic-default-volume makes separate sense, and since
this patch deals with default volumes, wrapping its removal into this series
would be ok as well, but it should be a separate commit anyway since it is a
separate identifiable isolated change.

> If eliminating causes them problems, they can add that definition to their
> ly files.

Well, at some point of time adding mystic incantations that do nothing at all
stops making a lot of sense.  But again: that's not really the topic of this
issue/review.

[Dan Eble, 2016-08-11 12:04:02]

On 2016/08/10 23:05:23, c_sorensen wrote:
> 
> When writing congregational hymns, dynamics are very seldom included.
> 
> But I may wish to generate practice midi from my engraving (I regularly do
> it).  It would be nice to be able to define my own default midi volume that
> would apply.

This is a frequent use case for me too.  I've always handled it by removing the
dynamic engraver.  Then I specify dynamics anywhere I please without having them
appear in the printed score.

[Dan Eble, 2016-08-12 21:21:33]

James put this issue back in "review" thinking that there might be more than
needs to be discussed, but my understanding is that none of the reviewers has
any significant concern about pushing this as it is.

Is that correct?

[dak, 2016-08-12 22:04:52]

On 2016/08/12 21:21:33, Dan Eble wrote:
> James put this issue back in "review" thinking that there might be more than
> needs to be discussed, but my understanding is that none of the reviewers has
> any significant concern about pushing this as it is.
> 
> Is that correct?

I repeat:

Can we get to some version of the code where the code paths supposed to be
equivalent (is there agreement about that?) actually looks the same?

If so, this would be a good starter for Heikki to eventually propose a cleanup
that would result in a removal of the dead code or keeping it but adding a
programming error.  Something like that.

This sounds like work either party has not bargained on doing.  But it would
likely be easier to get it done now that people have taken a bit of a look than
at a time when the person puzzling over it has no indication about why stuff
looks different.

[c_sorensen, 2016-08-12 22:38:15]

On 8/12/16 3:21 PM, "nine.fierce.ballads@gmail.com"
<nine.fierce.ballads@gmail.com> wrote:

>James put this issue back in "review" thinking that there might be more
>than needs to be discussed, but my understanding is that none of the
>reviewers has any significant concern about pushing this as it is.
>
>Is that correct?

It is for me.

Carl

[ht, 2016-08-13 10:33:40]

On 2016/08/12 22:04:52, dak wrote:
> I repeat:
> 
> Can we get to some version of the code where the code paths supposed to be
> equivalent (is there agreement about that?) actually looks the same?

I think the simplest options here are to (1) make the same changes in both code
paths for consistency, or (2) add a comment explaining why "volume" should be
initialized to the level of the "mf" dynamic in one of the cases, but not in the
other (if there's a specific reason for it).

> If so, this would be a good starter for Heikki to eventually propose a cleanup
> that would result in a removal of the dead code or keeping it but adding a
> programming error.  Something like that.
> This sounds like work either party has not bargained on doing.

When this piece of code was introduced in Issue 4048, the code paths did not
behave exactly the same way, so I asked about this during the review
(https://codereview.appspot.com/302910043/patch/40001/50012), wondering whether
the code paths could be combined if the logic is actually supposed to be
identical (precisely to keep the code more maintainable in this case by removing
any opportunities to forget updating both code paths the same way if changes are
needed in one of them), although I didn't propose any concrete rewrite
suggestions there.  As a result, the logic in both code paths got unified in the
final patchset for Issue 4048 (into the state before the current patch), and
since the code looked (in my view) to be in a working state afterwards, I did
not push any further with the cleanup suggestions since my last comment in that
review ended the review discussion.

I can certainly try to propose a clean-up patch based on my own understanding
about the code, however since the discussion so far seems to suggest that I
could simply be missing a trivial critical point about the intended behavior,
I'd rather not put effort in preparing a patch that will only end up making
things worse.

In any case, I think it's best to handle the cleanup as a separate issue so that
this one can be closed.

[Dan Eble, 2016-08-15 23:09:12]

I've marked the ticket as "needs work".  Motivation has been lacking lately.  I
expect to return to this after some time (a week? a month?) but if someone else
would like to take over this issue before then, I won't be offended.