Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check for extra rest duration #462

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kylewilk567
Copy link
Contributor

Patch to fix issue #421 . Checks the ending duration of what is sent to the client matches the PartOffset in the Score object. If there is a mismatch (as in the case with a trailing rest), extra duration is appended to the last note.

lastEvent := event
// Last event offset + duration must equal score.duration
currNoteEnd := int32(math.Round(lastEvent.Duration)) + int32(lastEvent.EventOffset())
partOffset := int32(score.CurrentParts[len(score.CurrentParts)-1].CurrentOffset)
Copy link
Member

@daveyarwood daveyarwood Nov 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think selecting the last Part in the score is potentially error-prone, as it might not be the Part that the note belongs to. I think lastEvent.Part is probably what we want here.

@daveyarwood
Copy link
Member

Thanks for the PR, Kyle! It looks good overall, apart from the thing I mentioned above.

I'll do some manual testing soon when I have time, and merge it if everything looks and sounds good.

@daveyarwood
Copy link
Member

daveyarwood commented Nov 24, 2022

This doesn't appear to fix the issue in #421, unfortunately :(

Here's what I did to test:

  • Run alda repl -s -p 12345 to start a REPL server on port 12345.
  • Check out this branch.
  • Run the following command twice in rapid succession:
client/bin/run repl \
  --client \
  --port 12345 \
  --message '{"op": "eval-and-play", "code": "percussion: o2 (tempo 240) c4 r4 c4 r4 | c4 r4 c4 r4 | c4 r4 c4 r4 | c4 r4 d4 r4"}'

The "hiccup" is still there where the final rest is skipped.

@kylewilk567
Copy link
Contributor Author

kylewilk567 commented Nov 25, 2022

I tested the code using the following commands in Windows Powershell and it works:
image

The r4 that occurs at the end of the first line is correctly handled. I wasn't able to replicate it using the procedure above since my Alda executable in my PCs PATH is the old version. Do you have any suggestions as to why our testing may have given different results?

@daveyarwood
Copy link
Member

@kylewilk567 Sorry for the late reply - I've been on vacation the last couple weeks.

This is interesting! I tried it the way you described, and I also heard the last rest being handled correctly. But I tried it again the way I described (running a REPL server in another terminal and using the CLI to send it messages), and the rest is still getting left out when I do it that way.

I wonder if there is some important difference in the way the messages are being sent to the REPL server when you do it via the CLI vs. doing it interactively in a REPL client session?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants