Classlib: Fix Pfindur bug where the last event could lose its Rest status #5113
Conversation
jamshark70
added
bug
| @@ -433,7 +434,9 @@ Pfindur : FilterPattern { | |||
| if (nextElapsed.roundUp(tolerance) >= localdur) { | |||
| // must always copy an event before altering it. | |||
| // fix delta time and yield to play the event. | |||
| inevent = inevent.copy.put(\delta, localdur - elapsed).yield; | |||
| remaining = localdur - elapsed; | |||
| if(inevent.isRest) { remaining = Rest(remaining) }; | |||
There was a problem hiding this comment.
IIUC, with this current version, if delta isn't a rest, and inevent.isRest is true for another reason, outgoing event's delta is still a rest (i don't know if this is okay or not but it feels a bit messy). would it be better here to check delta.isRest? that would also be more efficient -- for any event that isn't a rest, with the line above we'll cycle through all of the key/value pairs every time.
am i right in thinking that inevent.isRest is true and delta isn't a rest, and we make a copy of inevent with a different delta, then inevent should still be a rest?
There was a problem hiding this comment.
would it be better here to check delta.isRest?
The variable delta is always a number, because the event-delta primitive unwraps Rests and rejects non-numeric types. So delta.isRest is always false.
I could change it to inevent[\delta].isRest.
outgoing event's delta is still a rest (i don't know if this is okay or not but it feels a bit messy.
That's true. I didn't think it was especially harmful because (degree: Rest(0), delta: 1) and (degree: Rest(0), delta: Rest(1)) are both rests, so there's no change in outward behavior. But we don't know what the user is doing with anEvent[\delta] so you're right that it would be better to make it more precise.
am i right in thinking that inevent.isRest is true and delta isn't a rest, and we make a copy of inevent with a different delta, then inevent should still be a rest?
Yes. The bug occurs here if delta is the only Rest inside the event.
(Btw, aside, this is one of those places in the pattern library where the meanings of event and inevent are reversed. inevent should come from the argument or the previous yield and event should come from the stream. I never understood how this inverted convention got started but this isn't the only embedInStream method that makes this mistake.)
There was a problem hiding this comment.
The variable delta is always a number, because the event-delta primitive unwraps Rests and rejects non-numeric types. So delta.isRest is always false.
I could change it to inevent[\delta].isRest.
ah okay, i didn't realize Event:-delta is not just a member access, but that makes sense. yeah, i think limiting it that way would be better.
(Btw, aside, this is one of those places in the pattern library where the meanings of event and inevent are reversed. inevent should come from the argument or the previous yield and event should come from the stream. I never understood how this inverted convention got started but this isn't the only embedInStream method that makes this mistake.)
possibly someone misinterpreting inevent as meaning 'the event we are in' rather than 'the input/incoming event' (is that the intended meaning?).
mossheim
added
to-cherry-pick
Purpose and Motivation
When Pfindur reaches the end of its duration, it replaces the original event with a copy, where
deltahas been shortened so that the total duration matches thePfindurlimit.But if
event[\delta]is a Rest, thenlocaldur - elapsedis not a rest -- so the event loses itsisReststatus and may play sound, even though the user pattern specified a Rest.(It is valid for a pattern to specify timing using
\delta, and it's valid for rests to be specified in a\deltastream -- so, Pfindur should respect that.)Types of changes
To-do list