255: Implicit Voice Group Closing

[bbqbaron]

I don't have much Alda knowledge, so I've probably missed some use cases. However, this at least tackles the specific problem cases in issue 255. It makes two changes that are kind of related, but wouldn't strictly have to be part of the same PR if one of them doesn't check out.

1. Use lookahead to allow an unclosed voice group to parse as part of an event sequence. This addresses the parse error caused by piano: [V1: c d e V2: e f g]*2.

2. Unconditionally close any :event-sequence (including variable defs) with an :end-voice-group. This allows foo = V1: c d e V2: e f g to be multiplied by foo*2. From a design standpoint, though, I'm not sure if you want this behavior so broadly. The comments on the issue implied that maybe you do, but please correct me!

The issue here seems to be that voice groups do not implicitly end at the end of a variable definition, which can cause unexpected behavior.

Right now the :end-voice-group is unconditional, even if, for instance, the sequence already ends with :end-voice-group or doesn't end with :voice-group at all! I wanted to get the change out there for discussion before optimizing.

I wrote some tests to cover my limited knowledge. Let me know what you think!

[daveyarwood]

Thanks for the PR, this looks great overall! I'm impressed 😀

One concern I have with this approach is that we might not necessarily want to do an end-voice-group at the end of every event sequence. I have a feeling this would prematurely end a voice group if one of the voices contains an event sequence, e.g.

piano:
  V1: [c8 d e f g]*2
  V2: c8 c c c c

Maybe the right thing to do here is to add an implicit end-voice-group event to the end of every set-variable event? end-voice-group should be idempotent, i.e. it shouldn't break anything if there isn't a voice group to end.

It would be good to have a few more test cases covering different combinations of voice groups and event sequences. They're kind of confusing in that voice groups can contain event sequences, but event sequences can also contain voice groups. I'm not exactly sure what the expected behavior should be if you tried to do something like this:

V1: [
  c d e f g
  V1: a b
  V2: f g
]

V2: g g g g

(If that last example gets too complicated, we can always handle it as a separate issue... I have a feeling it might take some work to handle cases like that properly!)

[bbqbaron]

So, I tried closing voice groups at the end of a set-variable, but it doesn't fix voice group repetition in other contexts, like the original piano: [V1: a b c V2: d e f] * 2, which still plays once. I think that without ending VGs, chained-together voices get resolved as a single event after expansion by repeat:

EDIT: That last example is from master, sorry, not the transitory code that was using set-variable to trigger closure. EDIT 2: Deleted it because it didn't really illustrate anything; the repeated piano event sequence is sufficient : |.

So, new prospective change: what if the start of any voice group implicitly ends voice groups? The comments in events suggest that this shouldn't always happen, but it otherwise makes a certain intuitive sense. I was wondering if you could provide a small snippet of that expected voice-group-continuation behavior so I could make sure it works/write a test if possible.

I also naturally removed the unconditional voice group closure at the end of any sequence.

Totally happy to write some more tests around nested voices and event sequences; I'll add those before merge, once we have a good behavior nailed down.

I do agree with the suggestion, though, that the problem of nesting voices might be better handled as a separate change. It strikes me as weird to even ask a voice to itself somehow play voices; maybe that should be a parse error?

[bbqbaron]

Was turning it over in my head a bit more. I thought, okay, what if we just end groups between any elements of a repeat? We can't, of course, in case there's a repeat inside a voice. Well, what if we just somehow...end groups that appear in the preceding element? You don't really want to do that, because the parser tracks voices by number, and it seems like you could create some pretty strange behaviors by accident.

It definitely seems soluble; the problem may be my understanding how voice resolution is intended to work. For instance, what should this do?

foo = V3: a b c
piano: V1: a b c V2: d e f foo V3: a b c

How many beats should the resulting playback last? As of this branch, it's 9 (V1 & V2 together, then V3, then V3 again). As of master, it's 6 (V1, V2, and V3, with V3 having six total notes). Is the resolution of foo supposed to happen before parse, or is its V3 some kind of scoped value such that it and any outer V3 don't automatically unify?

[daveyarwood]

So, new prospective change: what if the start of any voice group implicitly ends voice groups? The comments in events suggest that this shouldn't always happen, but it otherwise makes a certain intuitive sense. I was wondering if you could provide a small snippet of that expected voice-group-continuation behavior so I could make sure it works/write a test if possible.

I made a gist showing an example of this in an Alda REPL and doing the same thing in a Clojure REPL in the alda.lisp namespace. In both cases, we build up the score incrementally with these steps:

1. Start a score.
2. Add a piano part.
3. Add a voice group event, containing voice 1, containing the notes C, D, E.
4. Add voice 2, containing the notes E, F. G.

The way we have things implemented now, it is important that the voice group created in step 3, does not implicitly end itself, so that step 4 can add a second voice to the same voice group.

I would be open to re-thinking this a little... I wonder if it might be better to keep the voice group concept internal, so that any new voices get added to the voice group automatically? i.e, step 3 would become "Add voice 1, containing the notes C, D, E." A voice group would be implicitly created the first time a voice is encountered in a score, and the group would remain open until either:

• the "voice zero" event V0:
• the start of a new instrument part

It definitely seems soluble; the problem may be my understanding how voice resolution is intended to work. For instance, what should this do?

foo = V3: a b c
piano: V1: a b c V2: d e f foo V3: a b c

How many beats should the resulting playback last? As of this branch, it's 9 (V1 & V2 together, then V3, then V3 again). As of master, it's 6 (V1, V2, and V3, with V3 having six total notes). Is the resolution of foo supposed to happen before parse, or is its V3 some kind of scoped value such that it and any outer V3 don't automatically unify?

After thinking about this a little more, I don't think we should implicitly do end-voice-group at the end of a variable definition. When you use a variable, it should feel like a simple text expansion. In other words, your example above should be equivalent to:

piano: V1: a b c V2: d e f V3: a b c V3: a b c

which, in turn, is equivalent to:

piano: V1: a b c V2: d e f V3: a b c a b c

Of course, we aren't really text-expanding. We're parsing the events in a definition and storing them in the :env key in the score, then retrieving the events and applying them whenever we encounter the get-variable event. I think foo = V3: a b c probably parses that event as (voice-group (voice 3 (note (pitch :a)) (note (pitch :b)) (note (pitch :c)))), which could lead to unexpected behavior because the voice-group event blows away any existing :voice-instruments state. It would probably be better if it didn't do that, to work better with variables like in the example above.

I do agree with the suggestion, though, that the problem of nesting voices might be better handled as a separate change. It strikes me as weird to even ask a voice to itself somehow play voices; maybe that should be a parse error?

That's what I'm thinking. It seems like something we shouldn't support because it doesn't really make sense to start a new voice "inside of" a voice. Usually when the parser encounters a V2: (or any voice number) and it's already in a voice group, it considers that the start of a new voice, not a new voice group. But if you're inside of an event sequence inside of a voice, the voice can't be considered "over" until the event sequence is over, so we should enforce that.

Thanks for all your work on this, I really appreciate it! 🍻

[bbqbaron]

No problem! The explanation makes it much clearer, thank you. It does seem like we should go basically the opposite direction and not wipe voice instrument state except on part transition or when explicitly told to. So, I removed the reset of voice-instruments.

It makes me a little nervous because I figured it was intended for something, but none of the tests failed after I did it. I tried to at least cover the bases by manually testing:

• the use cases from the original issue
• your gist
• my piano: [V1: ...] example

...and some random toying around based on my new understanding of voice behavior. It all seems to check out, so it's possible that that called was just outdated?

Also added a couple more tests explicitly asserting empty or non-empty voice instruments at various transitions.

[bbqbaron]

Actually, interesting corollary effect: Does voice-group have no significance at all now? If it's just parsing a series of voices, couldn't the grammar just parse each voice, since it seems like the event just handles them individually anyway?

[daveyarwood]

It's a nice surprise that we actually don't seem to need the "voice group initialization" stuff in the voice-group event at all. I was thinking that we at least needed the (assoc :voice-instruments {}) part, but I guess the voice event takes care of that when (update-in [:voice-instruments number] #(if % % instruments)) gets called with a score that has nil as the value of :voice-instruments:

boot.user=> (update-in {:foo nil} [:foo :bar] #(if % % :baz))
{:foo {:bar :baz}}
boot.user=> (update-in *1 [:foo :bar] #(if % % :baz))
{:foo {:bar :baz}}

Thankfully, Clojure is a well-designed language and already does the right thing! Nicely done, Clojure!

Actually, interesting corollary effect: Does voice-group have no significance at all now? If it's just parsing a series of voices, couldn't the grammar just parse each voice, since it seems like the event just handles them individually anyway?

I had that same thought! It seems like we've moved towards voice groups being an implicit thing, so we don't technically need the voice-group (a.k.a. voices) event anymore. I'd be in favor of getting rid of the voice-group event and simplifying the parser so that it just parses voice events. Perhaps that could be a separate PR?

I'm going to check out your branch and do some testing to make sure this change doesn't break anything. If all looks good, I think we can merge it!

[daveyarwood]

Just tested this by running tests, experimenting in the REPL, and playing all of the example scores. Everything looks/sounds good! 👍