Skip to content

Fix Sequential Note Playback in Simple Tuplet Block #4699

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ganasekhar-gif
Copy link

@Ganasekhar-gif Ganasekhar-gif commented May 28, 2025

This PR addresses the issue #4633 with the timing and behavior of tuplet blocks, building upon my earlier PRs. After reviewing and testing further, I have restructured and cleaned up the implementation to ensure accurate note scheduling in tuplet patterns.

Debug Logs for Verification(for an 7 1/4 tuplet block):
bpmFactor: 2.6666666666666665
RhythmBlockPaletteBlocks.js:1009 noteBeatValue: 4
RhythmBlockPaletteBlocks.js:1010 beatValue: 0.09523809523809523
RhythmBlockPaletteBlocks.js:1012 Scheduling note 1/7 with delay: 0ms, duration: 4
RhythmBlockPaletteBlocks.js:1012 Scheduling note 2/7 with delay: 95.23809523809523ms, duration: 4
RhythmBlockPaletteBlocks.js:1012 Scheduling note 3/7 with delay: 190.47619047619045ms, duration: 4
RhythmBlockPaletteBlocks.js:1012 Scheduling note 4/7 with delay: 285.7142857142857ms, duration: 4
RhythmBlockPaletteBlocks.js:1012 Scheduling note 5/7 with delay: 380.9523809523809ms, duration: 4
RhythmBlockPaletteBlocks.js:1012 Scheduling note 6/7 with delay: 476.19047619047615ms, duration: 4
RhythmBlockPaletteBlocks.js:1012 Scheduling note 7/7 with delay: 571.4285714285714ms, duration: 4

These logs confirm that the delays are now calculated and scheduled at accurate intervals for each note in the tuplet block.

I would really appreciate it if you could take a look and let me know:

If these changes fully resolve the timing issue
If there are any additional improvements or refinements you'd suggest

Thank you again for your time and guidance throughout this process!

Copy link

✅ All Jest tests passed! This PR is ready to merge.

@Ganasekhar-gif
Copy link
Author

Hi @therealharshit,

Whenever you get a chance, could you please take a look at this PR and share your feedback?

@therealharshit
Copy link
Member

Why you keep on closing PRs and open new one for same issue, this is your 3rd PR for same issue.
This makes it difficult for me to track changes and review.

@Ganasekhar-gif
Copy link
Author

Apologies for the inconvenience caused. In my previous PR, I encountered unexpected changes to the package-lock.json file, and my attempts to resolve it didn’t work as expected. To keep things clean and include detailed logs, I decided to open a new PR.

I understand the importance of keeping changes in one PR and will make sure to avoid this in the future.
No rush—please take your time to review whenever convenient. Thank you!

@Ganasekhar-gif
Copy link
Author

Hi @therealharshit, friendly reminder to review this PR when you get a chance.
Let me know if there’s anything you’d like me to adjust. Thanks!

@therealharshit
Copy link
Member

Hi @therealharshit, friendly reminder to review this PR when you get a chance.
Let me know if there’s anything you’d like me to adjust. Thanks!

Give me some time, I'll review it soon.

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