Skip to content

Conversation

@Jan125
Copy link
Contributor

@Jan125 Jan125 commented Oct 8, 2025

Fixes #7528

LFO Controller phase is now updated properly on moving the playhead with the mouse.

Unsure whether this fix has far reaching consequences.

Fixes LMMS#7528

LFO Controller phase is now updated properly on moving the playhead with the mouse.

Unsure whether this fix has far reaching consequences.
Song.h:
-Formatting:
--Remove spaces (Leftover from copy-paste)

TimeLineWidget.cpp:
-Formatting:
--Remove unnecessary parentheses for inversion (Leftover from copy-paste)
--Unpack else
--Remove spaces (Leftover from copy-paste)
--Remove unused code

Thanks to szeli1 for pointing it out.
@Jan125 Jan125 force-pushed the lfocontrollermouseplayheadmovefix branch from 165f284 to 8e57977 Compare October 10, 2025 15:10
@Jan125 Jan125 requested a review from szeli1 October 10, 2025 15:53
All instances of setToTime replaced by setPlayPos.
Seemingly unused delta time in setPlayPos changed to absolute position.
Associated unused code removed.

Side effect:
Loop-jumping in the editor now also calculates LFO Controller phases correctly.
Export is unaffected as of yet.
(Intended design for smoother song flow vs. reproducability of song in editor?)
@Jan125
Copy link
Contributor Author

Jan125 commented Oct 11, 2025

As mentioned in that commit, exporting with loops is a case that handle differently.
Do we want to keep the current behaviour of loops in export not recalculating the state for smoother song flow, or
do we recalculate the state for reproducability?

m_elapsedTicks += ticksFromPlayMode - ticks;
m_elapsedMilliSeconds[static_cast<std::size_t>(playMode)] += TimePos::ticksToMilliseconds( ticks - ticksFromPlayMode, getTempo() );
m_elapsedTicks = ticks;
m_elapsedMilliSeconds[static_cast<std::size_t>(playMode)] = TimePos::ticksToMilliseconds( ticks , getTempo() );
Copy link
Member

Choose a reason for hiding this comment

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

It seems this change would mean tempo automation would incorrectly alter the millisecond counter? I believe @messmerd had concerns about it correctly handling tempo changes, which was one issue with #7454.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not seeing a change in behaviour here.
It has been handled arguably incorrectly before:
-Run song from the beginning: Get total time correctly.
-Run song from after tempo change: Time is calculated on that point as if the current tempo was always the tempo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I am now seeing is a regression in that the timer on top no longer updates when you move the playhead with the mouse when it's not playing, so I will be fixing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out I was looking in the wrong place regarding the previous comment.

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.

Update LFO Controller status on playback position change while playing

4 participants