Cleanup and generalization: get rid of Audio_column.

Cleanup and generalization: get rid of Audio_column.

The engraving columns exist to keep grobs aligned during layout, but
there's no layout step in audio, so Audio_column has no purpose. Each
Audio_item now knows its moment instead.

This means performers can generate Audio_items at times other than the
current moment. This makes more flexible MIDI output hooks possible —
in particular, ones that change note times, or generate additional
notes for ornaments.

Control_track_performer now creates its Audio_texts at the first
timestep rather than during initialization, so they'll have a valid
moment. Previously they were created at no timestep, but
Score_performer::acknowledge_audio_elements saw them after the first
timestep (even though they weren't part of that timestep) and added
them to the first Audio_column.

[Devon Schudy, 2013-12-16 04:21:01]

Removed an unrelated commit that was accidentally included.

[dak, 2013-12-20 16:46:19]

https://codereview.appspot.com/42600043/diff/20001/lily/audio-item.cc
File lily/audio-item.cc (right):

https://codereview.appspot.com/42600043/diff/20001/lily/audio-item.cc#newcode44
lily/audio-item.cc:44: return int (moment_to_ticks (when_));
Why cast to int here?  moment_to_ticks is already an int.

https://codereview.appspot.com/42600043/diff/20001/lily/audio-item.cc#newcode201
lily/audio-item.cc:201: Audio_tempo::Audio_tempo (Moment when, int per_minute_4)
per_minute_4 warrants a comment.  Yes, it was already missing one before you
started.

https://codereview.appspot.com/42600043/diff/20001/lily/control-track-perform...
File lily/control-track-performer.cc (right):

https://codereview.appspot.com/42600043/diff/20001/lily/control-track-perform...
lily/control-track-performer.cc:52:
Control_track_performer::start_translation_timestep ()
Ok, here is a filthy bad thing:  in issue 2392 I finally punted and gave up on
start_translation_timestep in implicitly created contexts to be reliably run.

That means that for implicitly created contexts from a Simple_music_iterator,
start_translation_timestep will only get run at the timestep _after_ creation.

If you think you can fix _that_, go ahead (you are obviously more into the Midi
code than I was, but that's more a problem of the iterator framework).

If not, this code will likely not do what you think it should do in all
circumstances.

https://codereview.appspot.com/42600043/diff/20001/lily/include/lily-proto.hh
File lily/include/lily-proto.hh (left):

https://codereview.appspot.com/42600043/diff/20001/lily/include/lily-proto.hh...
lily/include/lily-proto.hh:26: class Audio_column;
Good call: this is easily forgotten, and I recently cleaned out a few long gone
classes from here.

[Devon Schudy, 2013-12-21 02:58:05]

dak@gnu.org wrote:
> Ok, here is a filthy bad thing:  in issue 2392 I finally punted and gave
> up on start_translation_timestep in implicitly created contexts to be
> reliably run.
>
> That means that for implicitly created contexts from a
> Simple_music_iterator, start_translation_timestep will only get run at
> the timestep _after_ creation.

...and it crashes on \score { { | c4 } \midi {} }, for the same reason
as issue 2392.

> If you think you can fix _that_, go ahead

OK: Issue 3744 fixes that for all score-level translators by creating
the Score context before iteration starts. Creating it later was
dubious anyway, because Score_engraver or Score_performer is needed
for iteration to work. AFAICT it's never valid to explicitly create a
score context (and the notation manual says so); if that's not true,
then this isn't safe.

Other contexts can legitimately be created during iteration, so their
translators can still miss start_translation_timestep. That could be
prevented by calling start_translation_timestep in
Translator_group::create_child_translator, but that's not strictly
accurate since it's not actually the start of a timestep. It's less
surprising for newly created contexts than for Score, though.

[dak, 2013-12-21 05:59:17]

On 2013/12/21 02:58:05, Devon Schudy wrote:

> OK: Issue 3744 fixes that for all score-level translators by creating
> the Score context before iteration starts. Creating it later was
> dubious anyway, because Score_engraver or Score_performer is needed
> for iteration to work. AFAICT it's never valid to explicitly create a
> score context (and the notation manual says so); if that's not true,
> then this isn't safe.

Where does the manual say that?  It's not true.  Most particularly, it is
possible to do
\score {
  \new Score \with { .... }
  {
     ...
  }
}

The Japanese manual actually does that for removing Timing_translator.  While it
appears like the other manuals do this differently, I don't think that this
practice has been officially "forbidden" and I can see no reason do to so.

> Other contexts can legitimately be created during iteration, so their
> translators can still miss start_translation_timestep. That could be
> prevented by calling start_translation_timestep in
> Translator_group::create_child_translator, but that's not strictly
> accurate since it's not actually the start of a timestep.

Better late than never...  The problem is then to avoid calling
start_translation_timestep twice under any circumstances.

[Devon Schudy, 2013-12-21 21:29:13]

dak wrote:
> > AFAICT it's never valid to explicitly create a score context (and
> > the notation manual says so); if that's not true, then this isn't
> > safe.
>
> Where does the manual say that?  It's not true.

I was imagining it. :( Learning Manual 3.3.2 says “\new Score should
not be used” (but it also says “The Score context is usually left to
be created automatically”), and Notation Manual 5.1.1 says "A Score
context is instantiated implicitly when a \score {…} or \layout {…}
block is processed", but it never actually says it *can't* be
explicitly created.

[on doing start_translation_timestep for new contexts]
> Better late than never...  The problem is then to avoid calling
> start_translation_timestep twice under any circumstances.

It would only be called again if a context was created during
start_translation_timestep. I don't think that happens now, but
translators do a lot in start_translation_timestep, so it might
accidentally happen someday.

Doing start_translation_timestep in mid-timestep is unclean, though,
and may confuse translators that expect it to be called at the
beginning. It passes the tests, but I wouldn't be surprised if it
introduces bugs. I don't think it's an improvement.

Iteration has an awful lot of undocumented invariants about what's
called when and who listens to what. It would be nice to detect and
warn about violations.

Some possibilities to make iteration more maintainable:
 * Call start/stop_translation_timestep directly from
run_iterator_on_me, not through translators. Remove the Prepare and
OneTimeStep events.
 * Create contexts in advance by calling create_children before each timestep.
 * Warn when contexts are created in mid-timestep.
 * Figure out other context invariants and write them down.

[dak, 2013-12-21 21:43:15]

On 2013/12/21 21:29:13, Devon Schudy wrote:
> dak wrote:
>
> Doing start_translation_timestep in mid-timestep is unclean, though,
> and may confuse translators that expect it to be called at the
> beginning.

How would they know the difference?  There is nothing sent to an
implicit context before it is created.  If it did not miss any piece
of the action, where is the problem?

> It passes the tests, but I wouldn't be surprised if it
> introduces bugs. I don't think it's an improvement.

I don't see how we could do anything better.

> Some possibilities to make iteration more maintainable:
>  * Call start/stop_translation_timestep directly from
> run_iterator_on_me, not through translators. Remove the Prepare and
> OneTimeStep events.

Part of the reason to use events for about anything is the ability to
_record_ them and reply them, like the quote iterator does.

>  * Create contexts in advance by calling create_children before each timestep.
>  * Warn when contexts are created in mid-timestep.
>  * Figure out other context invariants and write them down.

I think I find the "start_translation_timestep will occur before
process_music" invariant pretty important, even if it is reduced to
"start_translation_timestep will occur before my own and any
subcontext's process_music", and not making a guarantee about _higher_
contexts process_music timing.

[pkx166h, 2013-12-22 00:03:13]

On 21/12/13 21:28, Devon Schudy wrote:
> dak wrote:
>>> AFAICT it's never valid to explicitly create a score context (and
>>> the notation manual says so); if that's not true, then this isn't
>>> safe.
>> Where does the manual say that?  It's not true.
> I was imagining it. :( Learning Manual 3.3.2 says “\new Score should
> not be used” (but it also says “The Score context is usually left to
> be created automatically”), and Notation Manual 5.1.1 says "A Score
> context is instantiated implicitly when a \score {…} or \layout {…}
> block is processed", but it never actually says it *can't* be
> explicitly created.
>
> [on doing start_translation_timestep for new contexts]
>> Better late than never...  The problem is then to avoid calling
>> start_translation_timestep twice under any circumstances.
> It would only be called again if a context was created during
> start_translation_timestep. I don't think that happens now, but
> translators do a lot in start_translation_timestep, so it might
> accidentally happen someday.
>
> Doing start_translation_timestep in mid-timestep is unclean, though,
> and may confuse translators that expect it to be called at the
> beginning. It passes the tests, but I wouldn't be surprised if it
> introduces bugs. I don't think it's an improvement.
>
> Iteration has an awful lot of undocumented invariants about what's
> called when and who listens to what. It would be nice to detect and
> warn about violations.
>
> Some possibilities to make iteration more maintainable:
>   * Call start/stop_translation_timestep directly from
> run_iterator_on_me, not through translators. Remove the Prepare and
> OneTimeStep events.
>   * Create contexts in advance by calling create_children before each
timestep.
>   * Warn when contexts are created in mid-timestep.
>   * Figure out other context invariants and write them down.
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
http://code.google.com/p/lilypond/issues/detail?id=1322

Has this anything to do with this conversation?

James

[dak, 2013-12-22 00:09:28]

James <pkx166h@gmail.com> writes:

> On 21/12/13 21:28, Devon Schudy wrote:
>> dak wrote:
>>>> AFAICT it's never valid to explicitly create a score context (and
>>>> the notation manual says so); if that's not true, then this isn't
>>>> safe.
>>> Where does the manual say that?  It's not true.
>> I was imagining it. :( Learning Manual 3.3.2 says “\new Score should
>> not be used” (but it also says “The Score context is usually left to
>> be created automatically”), and Notation Manual 5.1.1 says "A Score
>> context is instantiated implicitly when a \score {…} or \layout {…}
>> block is processed", but it never actually says it *can't* be
>> explicitly created.

> http://code.google.com/p/lilypond/issues/detail?id=1322
>
> Has this anything to do with this conversation?

Not really, apart from " - clean up the docs/regtests (and possibly
snippets), just like we did with \new Score."

But it would be more relevant to find out about the issue (if any)
concerned with the "just like we did with \new Score." bit.

-- 
David Kastrup

[Devon Schudy, 2013-12-23 00:09:02]

I wrote:
> I was imagining it. [...] it never actually says it *can't* be
> explicitly created.

It's in engraver-init.ly: “You cannot explicitly instantiate a
@code{Score} context”.

David Kastrup wrote:
> > Doing start_translation_timestep in mid-timestep is unclean, though,
> > and may confuse translators that expect it to be called at the
> > beginning.
>
> How would they know the difference?  There is nothing sent to an
> implicit context before it is created.  If it did not miss any piece
> of the action, where is the problem?

They could miss earlier events from that timestep — but if no
translators care about events from before their context was created,
that doesn't matter. If a translator announces something in
start_translation_timestep (is this allowed?), other translators would
hear it in mid-timestep.

> Part of the reason to use events for about anything is the ability to
> _record_ them and reply them, like the quote iterator does.

Does it replay (or even see) timestep boundaries?

> I think I find the "start_translation_timestep will occur before
> process_music" invariant pretty important

Yeah. The CG even says so, in the engraver tutorial:
“`start_translation_timestep ()' is called before any user information
enters the translators, i.e., no property operations (\set, \override,
etc.) or events have been processed yet.”