Skip to content

Fix Resized Samples getting Shifted after Tempo Change #7890

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 8 commits into
base: master
Choose a base branch
from

Conversation

regulus79
Copy link
Contributor

@regulus79 regulus79 commented May 14, 2025

If you change the tempo of a song, the contents of all sample clips get scaled forwards/backwards. However, they get scaled relative to the internal 0 time of the clip, not the start of the clip. This causes issues when dealing with resized or cut/sliced sample clips.

This PR fixes this issue by updating the startTimeOffset of the sample clip when the tempo changes.

Originally I was just multiplying the startTimeOffset by the ratio of the new tempo to the old tempo, but that resulted in precision errors due to integer division. Instead, I decided to store the oldstartTimeOffset in terms of frames and calculate the new startTimeOffset based on that. That way, even if the user drastically changes the bpm from 140 to 10 to 999 to 300 to 10 and back to 140, the startTimeOffset will still return to its correct value, since it's being calculated by the original frame offset.

Note

In order to store the old startTimeOffset in frames between tempo changes, I have it track with the startTimeOffset when resizing/splitting, but leaving it unaffected when changing the tempo. To do this, I overrode Clip::setStartTimeOffset in SampleClip to make it also update the frame offset. That way, resizing the clip by the user, or splitting the clip, or any other action which calls that function will update it correctly. But in the function which handles the tempo change, I still use Clip::startTimeOffset.

Before

2025-05-13.17-08-58.mp4

After

2025-05-13.17-09-38.mp4

@AW1534
Copy link
Member

AW1534 commented May 14, 2025

looks good (haven't tested yet, to be clear). will this break older projects?

@regulus79
Copy link
Contributor Author

As far as I know it shouldn't. This PR doesn't change how samples clips are saved or loaded, and the start frame offset should always be synced with the start time offset, so the only change in behavior should happen when changing the tempo.

@firiox
Copy link

firiox commented May 17, 2025

Ok It works properly. Congrats? just kidding nice job

Copy link
Contributor

@allejok96 allejok96 left a comment

Choose a reason for hiding this comment

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

Looks like a nice and clean solution. Tested and works good except two minor issues.

Copy link
Contributor

@allejok96 allejok96 left a comment

Choose a reason for hiding this comment

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

Looks fine. My goodness you are fast :)

}


void SampleClip::tempoChanged()
{
Clip::setStartTimeOffset(std::round(1.0f * m_startFrameOffset / Engine::framesPerTick()));
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to call Clip's implementation of setStartTimeOffset rather than this class's?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because SampleClip's implementation also handles setting the initial frame offset, which should only be set when the user manually resizes the clip. If it's an indirect change like due to changing tempo, then we only want to update the start offset, not the initial frame offset, so I call Clip::setStartTimeOffset.

Co-authored-by: Dalton Messmer <[email protected]>
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.

5 participants