Sync tempo widget BPM changes to engine in real time#5793
Sync tempo widget BPM changes to engine in real time#5793Ady0333 wants to merge 5 commits intosugarlabs:masterfrom
Conversation
|
✅ All Jest tests passed! This PR is ready to merge. |
1 similar comment
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender and @vanshika2720, the pr is ready please review it once. |
|
@Ady0333 please pull the latest master, rebase it into your branch, and push again. |
|
✅ All Jest tests passed! This PR is ready to merge. |
vanshika2720
left a comment
vanshika2720
left a comment
There was a problem hiding this comment.
@walterbender I’ve reviewed the changes — the real-time BPM sync is correctly implemented and scoped. I don’t see any blocking issues from my side. Looks good for approval.
@vanshika2720 Thanks!! |
|
Please remove the unrelated changes |
Yes sir sure!!! |
Ady0333
force-pushed
the
fix/tempo-widget-live-bpm
branch
from
9751244 to
14c3bce
Compare
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@Ady0333 Hi overall this pr looks good |
|
Everything seems good but I don't know why we are seeing the Lighthouse errors. |
Apologize for this. I will fix them . |
|
✅ All Jest tests passed! This PR is ready to merge. |
1 similar comment
|
✅ All Jest tests passed! This PR is ready to merge. |
Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
Update the global location comment in tempo.js to point to js/logoconstants.js instead of js/activity.js where TONEBPM is actually defined. Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
Resolve npm ci failures by updating the lock file to match the current package.json dependencies. This fixes CI failures in GitHub Actions. Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
Ady0333
force-pushed
the
fix/tempo-widget-live-bpm
branch
from
bd6a6b0 to
5b96945
Compare
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
The lock file was accidentally deleted in the previous commit. This restores it so that npm ci can run successfully in CI workflows. Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
4 participants
Summary
This PR fixes a real-time desynchronization between the Tempo widget and the playback engine. When BPM was changed using the Tempo widget (slider, buttons, or input),
_updateBPM()only updated the block’s visual value on the canvas. The engine continued using the old BPM until the program was stopped and run again.What changed
After updating the block value,
_updateBPM()now also propagates the BPM to the engine:setmasterbpm/setmasterbpm2: updatesSinger.masterBPMandSinger.defaultBPMFactorsetbpm3/setbpm2: updates the top value of each running turtle’ssinger.bpmstackExample:
Singer.masterBPM = bpmValue;
Singer.defaultBPMFactor = TONEBPM / bpmValue;
Why this change was needed
The engine computes note timing from:
TONEBPM / (tur.singer.bpm.length > 0 ? last(tur.singer.bpm) : Singer.masterBPM);
Since
_updateBPM()never updated these engine values, the metronome ticked at the new tempo while notes continued playing at the old one. This broke the expected real-time behavior of the Tempo widget.Scope
js/widgets/tempo.jsVerification