In short: LGTM. The rest of this comment is just continuing discussion on some of the points in your previous response.
On 2016/07/12 05:01:16, Dan Eble wrote:
> What are the main user-level justifications for increasing the > complexity of the Dynamic_performer
It also improves an example I saw elsewhere recently (I don't remember where, but I think it was yours), something like mf < ... < ... < ... < ... f. The volume no longer saturates early.
Sorry about this, that example was not really meant as something that I suggested as a problem that I'd wish to have fixed (the example was only there to show how a previous patch of yours to "add \!'s" to all dynamic span boundaries already improved the behavior from what it used to be) - and the new behavior is even better since it avoids saturating the volume altogether.
The regression tests show no differences.
I wasn't thinking about regression tests here, but rather about the existing input files that a user might have that could start behaving differently from what they used to (for example, that the volume level at the end of a sequence of crescendos followed by a sequence of descrendos, all with unspecified volumes, will now always return to the starting level regardless of the number of crescendo and descrendo spans). I'm not arguing for or against any particular heuristic of handling unspecified (de)crescendos, only that any changes in behavior might be seen differently by different users, and whatever heuristics there are should preferably be documented in a detailed enough level so that the intended behavior becomes clear. I'm of course speaking strictly from the mindset of a (likely a very small group of) users who have (due to the lack of reference documentation) gone through the trouble of trying to learn the rules about how LilyPond handles MIDI dynamics by studying the source code. While such users might be annoyed about the changes in the rules if there's nothing mentioned about the changes anywhere, they have only themselves to blame for relying on undocumented behavior in the first place... So I believe you can safely ignore my concerns about missing user-level documentation about the changes - I don't think there will be any users that are going to complain about the changes.
Well, the level of detail in the documentation should depend on the
use
case for MIDI output.
True - however, I'm not certain whether there's any clear consensus among the user/developer base about what the use case for LilyPond's MIDI output actually is, besides the obvious thing that in practice the MIDI backend is considered to be of secondary importance to the majority of people (at least to the core developers - since v2.14, which introduced the "velocity as MIDI volume" interpretation of volume, I think that the fixes and enhancements to the MIDI code have mostly come from outside developers).
I've been told very clearly that "It can serve as a proofhearing aid or a practice aid. It is not intended to serve as a substitute for a player when recording. For that, LilyPond produces sheet music fit for running through a human"
(http://lists.gnu.org/archive/html/lilypond-devel/2013-11/msg00194.html). I think this is just the personal opinion of one of the core developers. I can only symphathize with your sentiments in that discussion. In my opinion, if the above really were the only accepted use case for MIDI output, then I'm baffled about why the developers of the MIDI backend ever bothered with handling MIDI dynamics in the first place, and why, for example, the articulate.ly script (even though it was made by an outside developer) got accepted into the official releases. Funnily enough, had LilyPond not had any support for MIDI dynamics when I started using it, I'd probably have never got involved with LilyPond development at all myself, to try to work around or fix bugs where the MIDI output backend "tried too much (performance-wise)" and failed.
I don't think that these minor changes to MIDI output in cases where the user didn't care enough to specify a dynamic are much to brag about.
I think that removing the internal programming errors in MIDI dynamic handling is a good thing since these errors could have been a source of user confusion. Maybe this would be still worth mentioning? https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode101
lily/dynamic-performer.cc:101: Real max_target_vol) Assuming that context properties do not change in the course of the performance does not seem very lilypondish. It is true that I have not tried to handle the possibility rigorously, but I thought it better to write this interface this way. It's small potatoes compared to the rest.
OK, this was just a thought (possibly raised since the equalizer settings remaining the same was mentioned in the code comments, so I began to wonder how strongly this is actually assumed throughout the implementation - in the case where the implementation were known to stop behaving as intended if equalization settings change, making the interfaces too generic would bring little benefit and only make the code more difficult to understand).
> is it correct that the function just > overwrites the values of the queue's member variables
Correct, and I think handling a change in equalizer settings at arbitrary moments during crescendi would greatly complicate this performer.
Don't worry, I'm didn't mean to suggest that you need to still work on this. The question was just due to my own confusion about the names of the queue member variables (whether they were supposed to represent the maximum and minimum over all elements of the queue or not). https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode234
lily/dynamic-performer.cc:234: // less than the farther one. On 2016/07/09 18:16:50, ht wrote: > What is the purpose of computing two candidate values for > depart_vol in this function? Does this improve the behavior in > some corner cases (some regression tests maybe), for example, make > it less likely to hit the peak volume for depart_vol, or something > else? (Just curious to know.)
That was my intent. [...] Would it help to put this example in the comments?
Definitely, thanks for the explanation! With decisions like this, it would be good to have comments that describe the idea - since from just the code it could be difficult to judge whether the calculations need to be that way because of code correctness or for some other reasons. https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode381
lily/dynamic-performer.cc:381: { On 2016/07/09 18:16:50, ht wrote: > Could this case be combined with the code further below (possibly > simplifying the initialization of volume) since the if statement > directly following this block will be skipped anyway if the control > flow passes through this block?
This block is executed the first time only, but the if (!open_span_.dynamic_) below is executed whenever a (de)crescendo has ended.
Isn't the last "if" block executed also on the first time since the condition in the last "if" block and the "first time" case is the same? (That is, wouldn't "volume" be assigned the default volume in the last "if" block even without the "first time" case?) https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode382
lily/dynamic-performer.cc:382: // Idea: look_up_absolute_volume
(ly_symbol2scm
("mf")). On 2016/07/09 18:16:50, ht wrote: > Actually, the value 90.0/127.0 for > Audio_span_dynamic::DEFAULT_VOLUME is about 0.71, which lands > between mf (0.68) and f (0.75) in the default absolute-volume-alist.
I know. Did you mean to suggest that this idea is good or bad?
Neither :-), it was just about the "Idea" comment being inaccurate (since the default volume level is not "mf"). However, it will become correct if the default is changed to "mf" (on which we seem to agree in the Doc issue about MIDI dynamics).
If someone can point me to a good example of IM documentation, I will consider combining that with a future patch for another issue.
There don't seem to be many engravers or performers with that much IM documentation (the longest examples that I could find which appear to try to tell something about how an engraver works are lily/bar-number-engraver.cc, lily/keep-alive-together-engraver.cc and lily/paper-column-engraver.cc). Yes, I admit that I've never written such documentation myself... https://codereview.appspot.com/302910043/diff/40001/lily/staff-performer.cc
File lily/staff-performer.cc (right):
https://codereview.appspot.com/302910043/diff/40001/lily/staff-performer.cc#newcode352
lily/staff-performer.cc:352: } On 2016/07/09 18:16:50, ht wrote: > When pushing the changes, maybe the removal of this flag and the > resulting unnecessary classes could be done as a separate commit > (since the removal of the
It isn't the removal of this flag that makes the Audio_dynamic class unnecessary. The flag was unnecessary, period.
Yes, we're not in disagreement here.
If you'll excuse me, I've jumped through git's hoops enough in the past few weeks that I'd rather not split out such a trivial change (just removing the flag) into its own commit, but if someone felt strongly enough about it to second your request, I'd do it.
Just do what you think is best, I'm happy with whatever decision you make (and certainly didn't mean to upset you, and I'm sorry if I did). https://codereview.appspot.com/302910043/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel