From 0fe7d6f679ad120c77da6dc12ed7192b3a48b6ee Mon Sep 17 00:00:00 2001 From: regulus79 <117475203+regulus79@users.noreply.github.com> Date: Wed, 17 Dec 2025 06:31:36 -0500 Subject: [PATCH] Refactor to move positionChanged signal to Timeline (#7454) Previously, this PR simply added a new signal to the Timeline class and had it emitted by the TimeLineWidget class in order to solve #7351. However, as messmerd pointed out in his review comments, that was quite a hacky solution, and ideally the positionChanged signal would be solely managed by the backend Timeline class, while the frontend TimeLineWidget class would connect to those signals, instead of the other way around. This PR is no longer a simple bugfix, but instead a refactoring of the Timeline/TimeLineWidget signal/slots. Changes - The positionChanged signal and updatePosition slot were removed from TimeLineWidget and moved to Timeline. - Removed PlayPos, and instead store the timeline position in a TimePos along with a separate frame offset counter variable. The functions to set the ticks/timepos in Timeline emit the positionChanged signal. (Also, may emit positionJumped signal if the ticks were forcefully set--see below) - The pos() method and PlayPos m_pos were removed from TimeLineWidget; - The constructor for TimeLineWidget no longer requires a PlayPos reference. - Since each TimeLineWidget stores a reference to its Timeline, a new method was added, timeline(), for other classes to access it. - Removed array of PlayPos'es in Song. Now each Timeline holds their respective TimePos. - Song's methods for getPlayPos were changed to return the TimePos of the respective Timeline. The non-const versions of the methods were removed because Timeline does not expose its TimePos to write. - All of the places where Timelines are used were updated, along with their calls to access/modify the PlayPos. For example, occurrences of m_timeline->pos() were replaced with m_timeline->timeline()->pos(), and calls to m_timeline->pos().setTicks(ticks) were changed to m_timeline->timeline()->setTicks(ticks). - ALSO: Removed m_elapsedMilliseconds, m_elapsedBars, and m_elapsedTicks from Song. The elapsed milliseconds is now handled individually by each Timeline. - NEW: The m_jumped variable has been removed from Timeline. Now jumped events emit Timeline::positionJumped automatically whenever the ticks are forcefully set. This means it is no longer necessary to call timeline->setJumped(true) or timeline->setFrameOffset(0) whenever the playhead position is forcefully set. Many places in the codebase were already missing these calls, so this may fix some unknown bugs. --------- Co-authored-by: Dalton Messmer Co-authored-by: saker Co-authored-by: Alex --- include/AutomationEditor.h | 3 +- include/PianoRoll.h | 8 +- include/Song.h | 88 ++++---------------- include/SongEditor.h | 2 +- include/TimeLineWidget.h | 18 ++-- include/Timeline.h | 37 +++++++++ src/core/LfoController.cpp | 4 +- src/core/SampleClip.cpp | 8 +- src/core/Song.cpp | 90 +++++++------------- src/gui/editors/AutomationEditor.cpp | 24 ++---- src/gui/editors/PatternEditor.cpp | 6 +- src/gui/editors/PianoRoll.cpp | 41 +++++---- src/gui/editors/SongEditor.cpp | 42 ++++++---- src/gui/editors/TimeLineWidget.cpp | 31 ++----- tests/CMakeLists.txt | 1 + tests/src/core/TimelineTest.cpp | 119 +++++++++++++++++++++++++++ 16 files changed, 289 insertions(+), 233 deletions(-) create mode 100644 tests/src/core/TimelineTest.cpp diff --git a/include/AutomationEditor.h b/include/AutomationEditor.h index fe1380802..247292f6d 100644 --- a/include/AutomationEditor.h +++ b/include/AutomationEditor.h @@ -157,7 +157,7 @@ protected slots: void setProgressionType(int type); void setTension(); - void updatePosition( const lmms::TimePos & t ); + void updatePosition(); void zoomingXChanged(); void zoomingYChanged(); @@ -298,7 +298,6 @@ private: signals: void currentClipChanged(); - void positionChanged( const lmms::TimePos & ); } ; diff --git a/include/PianoRoll.h b/include/PianoRoll.h index b92b65234..7bb15b879 100644 --- a/include/PianoRoll.h +++ b/include/PianoRoll.h @@ -221,8 +221,8 @@ protected slots: void pasteNotes(); bool deleteSelectedNotes(); - void updatePosition(const lmms::TimePos & t ); - void updatePositionAccompany(const lmms::TimePos & t ); + void updatePosition(); + void updatePositionAccompany(); void updatePositionStepRecording(const lmms::TimePos & t ); void zoomingChanged(); @@ -327,6 +327,7 @@ private: void cancelStrumAction(); void updateScrollbars(); + void updatePositionLinePos(); void updatePositionLineHeight(); QList getAllOctavesForKey( int keyToMirror ) const; @@ -530,9 +531,6 @@ private: QBrush m_blackKeyActiveBackground; QBrush m_blackKeyInactiveBackground; QBrush m_blackKeyDisabledBackground; - -signals: - void positionChanged( const lmms::TimePos & ); } ; diff --git a/include/Song.h b/include/Song.h index 2d321115f..9bc400dc4 100644 --- a/include/Song.h +++ b/include/Song.h @@ -100,36 +100,6 @@ public: bool hasErrors(); QString errorSummary(); - class PlayPos : public TimePos - { - public: - PlayPos( const int abs = 0 ) : - TimePos( abs ), - m_currentFrame( 0.0f ) - { - } - inline void setCurrentFrame( const float f ) - { - m_currentFrame = f; - } - inline float currentFrame() const - { - return m_currentFrame; - } - inline void setJumped( const bool jumped ) - { - m_jumped = jumped; - } - inline bool jumped() const - { - return m_jumped; - } - - private: - float m_currentFrame; - bool m_jumped; - }; - void processNextBuffer(); inline int getLoadingTrackCount() const @@ -142,31 +112,11 @@ public: return getMilliseconds(m_playMode); } + //! Returns the elapsed milliseconds since the start of the song + //! This function attempts to give the correct value even despite mid-song tempo changes inline int getMilliseconds(PlayMode playMode) const { - return m_elapsedMilliSeconds[static_cast(playMode)]; - } - - inline void setToTime(TimePos const & pos) - { - setToTime(pos, m_playMode); - } - - inline void setToTime(TimePos const & pos, PlayMode playMode) - { - m_elapsedMilliSeconds[static_cast(playMode)] = pos.getTimeInMilliseconds(getTempo()); - getPlayPos(playMode).setTicks(pos.getTicks()); - } - - inline void setToTimeByTicks(tick_t ticks) - { - setToTimeByTicks(ticks, m_playMode); - } - - inline void setToTimeByTicks(tick_t ticks, PlayMode playMode) - { - m_elapsedMilliSeconds[static_cast(playMode)] = TimePos::ticksToMilliseconds(ticks, getTempo()); - getPlayPos(playMode).setTicks(ticks); + return 1000 * getTimeline(playMode).getElapsedSeconds(); } inline int getBars() const @@ -254,21 +204,22 @@ public: return m_playMode; } - inline PlayPos & getPlayPos( PlayMode pm ) + const TimePos& getPlayPos(PlayMode pm) const { - return m_playPos[static_cast(pm)]; + return getTimeline(pm).pos(); } - inline const PlayPos & getPlayPos( PlayMode pm ) const - { - return m_playPos[static_cast(pm)]; - } - inline PlayPos & getPlayPos() + const TimePos& getPlayPos() const { return getPlayPos(m_playMode); } - inline const PlayPos & getPlayPos() const + + void setPlayPos(tick_t ticks, PlayMode playMode) { - return getPlayPos(m_playMode); + getTimeline(playMode).setTicks(ticks); + } + void setPlayPos(tick_t ticks) + { + setPlayPos(ticks, m_playMode); } auto getTimeline(PlayMode mode) -> Timeline& { return m_timelines[static_cast(mode)]; } @@ -430,12 +381,9 @@ private: inline f_cnt_t currentFrame() const { - return getPlayPos(m_playMode).getTicks() * Engine::framesPerTick() + - getPlayPos(m_playMode).currentFrame(); + return getTimeline(m_playMode).ticks() * Engine::framesPerTick() + getTimeline(m_playMode).frameOffset(); } - void setPlayPos( tick_t ticks, PlayMode playMode ); - void saveControllerStates( QDomDocument & doc, QDomElement & element ); void restoreControllerStates( const QDomElement & element ); @@ -489,16 +437,11 @@ private: std::array m_timelines; PlayMode m_playMode; - PlayPos m_playPos[PlayModeCount]; bar_t m_length; const MidiClip* m_midiClipToPlay; bool m_loopMidiClip; - double m_elapsedMilliSeconds[PlayModeCount]; - tick_t m_elapsedTicks; - bar_t m_elapsedBars; - VstSyncController m_vstSyncController; int m_loopRenderCount; @@ -523,13 +466,12 @@ private: signals: void projectLoaded(); void playbackStateChanged(); - void playbackPositionChanged(); + void playbackPositionJumped(); void lengthChanged( int bars ); void tempoChanged( lmms::bpm_t newBPM ); void timeSignatureChanged( int oldTicksPerBar, int ticksPerBar ); void controllerAdded( lmms::Controller * ); void controllerRemoved( lmms::Controller * ); - void updateSampleTracks(); void stopped(); void modified(); void projectFileNameChanged(); diff --git a/include/SongEditor.h b/include/SongEditor.h index 4d535f1c9..932d026be 100644 --- a/include/SongEditor.h +++ b/include/SongEditor.h @@ -89,7 +89,7 @@ public slots: void setEditModeSelect(); void toggleProportionalSnap(); - void updatePosition( const lmms::TimePos & t ); + void updatePosition(); void updatePositionLine(); void selectAllClips( bool select ); diff --git a/include/TimeLineWidget.h b/include/TimeLineWidget.h index 6568ff57d..d7ec88da5 100644 --- a/include/TimeLineWidget.h +++ b/include/TimeLineWidget.h @@ -80,8 +80,8 @@ public: Disabled }; - TimeLineWidget(int xoff, int yoff, float ppb, Song::PlayPos& pos, Timeline& timeline, - const TimePos& begin, Song::PlayMode mode, QWidget* parent); + TimeLineWidget(int xoff, int yoff, float ppb, Timeline& timeline, + const TimePos& begin, QWidget* parent); ~TimeLineWidget() override; inline QColor const & getBarLineColor() const { return m_barLineColor; } @@ -133,11 +133,6 @@ public: m_cursorSelectRight = QCursor{m_cursorSelectRight.pixmap(), s.width(), s.height()}; } - inline Song::PlayPos & pos() - { - return( m_pos ); - } - static AutoScrollState defaultAutoScrollState(); AutoScrollState autoScroll() const { return m_autoScroll; } void setAutoScroll(AutoScrollState state) { m_autoScroll = state; } @@ -158,6 +153,11 @@ public: m_ppb / TimePos::ticksPerBar() ); } + Timeline* timeline() + { + return m_timeline; + } + bool isRecording() const { return m_isRecording; } void setRecording(bool recording) { m_isRecording = recording; } @@ -165,12 +165,10 @@ public: void setPlayheadVisible(bool visible) { m_isPlayheadVisible = visible; } signals: - void positionChanged(const lmms::TimePos& postion); void regionSelectedFromPixels( int, int ); void selectionFinished(); public slots: - void updatePosition(); void setSnapSize( const float snapSize ) { m_snapSize = snapSize; @@ -225,11 +223,9 @@ private: int m_xOffset; float m_ppb; float m_snapSize = 1.f; - Song::PlayPos & m_pos; Timeline* m_timeline; // Leftmost position visible in parent editor const TimePos & m_begin; - const Song::PlayMode m_mode; AutoScrollState m_autoScroll = defaultAutoScrollState(); diff --git a/include/Timeline.h b/include/Timeline.h index dc2d293c4..2e624b346 100644 --- a/include/Timeline.h +++ b/include/Timeline.h @@ -26,6 +26,8 @@ #include +#include "AudioEngine.h" +#include "Engine.h" #include "JournallingObject.h" #include "TimePos.h" @@ -43,6 +45,32 @@ public: KeepPosition }; + auto pos() const -> const TimePos& { return m_pos; } + + auto ticks() const -> tick_t { return m_pos.getTicks(); } + + //! Forcefully sets the current ticks, resets the frame offset, and sets the elapsed seconds based on the global position (ignoring potential mid-song tempo changes) + //! This function will emit the `positionJumped` signal to allow other widgets to update accordingly + void setTicks(tick_t ticks) + { + m_pos.setTicks(ticks); + m_frameOffset = 0; + m_elapsedSeconds = ticks * Engine::framesPerTick() / Engine::audioEngine()->outputSampleRate(); + emit positionJumped(); + emit positionChanged(); + } + + //! Advances the current timeline position by a certain number of ticks, in addition to updating the elapsed time based on the current tempo. + void incrementTicks(tick_t increment) + { + m_pos.setTicks(ticks() + increment); + m_elapsedSeconds += increment * Engine::framesPerTick() / Engine::audioEngine()->outputSampleRate(); + emit positionChanged(); + } + + auto frameOffset() const -> f_cnt_t { return m_frameOffset; } + void setFrameOffset(const f_cnt_t frame) { m_frameOffset = frame; } + auto loopBegin() const -> TimePos { return m_loopBegin; } auto loopEnd() const -> TimePos { return m_loopEnd; } auto loopEnabled() const -> bool { return m_loopEnabled; } @@ -58,11 +86,15 @@ public: void setPlayStartPosition(TimePos position) { m_playStartPosition = position; } void setStopBehaviour(StopBehaviour behaviour); + auto getElapsedSeconds() const -> double { return m_elapsedSeconds + frameOffset() / Engine::audioEngine()->outputSampleRate(); } + auto nodeName() const -> QString override { return "timeline"; } signals: void loopEnabledChanged(bool enabled); void stopBehaviourChanged(lmms::Timeline::StopBehaviour behaviour); + void positionChanged(); + void positionJumped(); protected: void saveSettings(QDomDocument& doc, QDomElement& element) override; @@ -72,6 +104,11 @@ private: TimePos m_loopBegin = TimePos{0}; TimePos m_loopEnd = TimePos{DefaultTicksPerBar}; bool m_loopEnabled = false; + TimePos m_pos = TimePos{0}; + + f_cnt_t m_frameOffset = 0; + + double m_elapsedSeconds = 0; StopBehaviour m_stopBehaviour = StopBehaviour::BackToStart; TimePos m_playStartPosition = TimePos{-1}; diff --git a/src/core/LfoController.cpp b/src/core/LfoController.cpp index dcdb59b7f..17290a5d3 100644 --- a/src/core/LfoController.cpp +++ b/src/core/LfoController.cpp @@ -66,8 +66,8 @@ LfoController::LfoController( Model * _parent ) : connect( Engine::getSong(), SIGNAL(playbackStateChanged()), this, SLOT(updatePhase())); - connect( Engine::getSong(), SIGNAL(playbackPositionChanged()), - this, SLOT(updatePhase())); + connect(Engine::getSong(), &Song::playbackPositionJumped, + this, &LfoController::updatePhase); updateDuration(); } diff --git a/src/core/SampleClip.cpp b/src/core/SampleClip.cpp index 0a1c13940..9e0092b11 100644 --- a/src/core/SampleClip.cpp +++ b/src/core/SampleClip.cpp @@ -56,8 +56,8 @@ SampleClip::SampleClip(Track* _track, Sample sample, bool isPlaying) connect( Engine::getSong(), SIGNAL(playbackStateChanged()), this, SLOT(playbackPositionChanged()), Qt::DirectConnection ); //care about loops and jumps - connect( Engine::getSong(), SIGNAL(updateSampleTracks()), - this, SLOT(playbackPositionChanged()), Qt::DirectConnection ); + connect(Engine::getSong(), &Song::playbackPositionJumped, + this, &SampleClip::playbackPositionChanged, Qt::DirectConnection); //care about mute Clips connect( this, SIGNAL(dataChanged()), this, SLOT(playbackPositionChanged())); //care about mute track @@ -94,8 +94,8 @@ SampleClip::SampleClip(const SampleClip& orig) : connect( Engine::getSong(), SIGNAL(playbackStateChanged()), this, SLOT(playbackPositionChanged()), Qt::DirectConnection ); //care about loops and jumps - connect( Engine::getSong(), SIGNAL(updateSampleTracks()), - this, SLOT(playbackPositionChanged()), Qt::DirectConnection ); + connect(Engine::getSong(), &Song::playbackPositionJumped, + this, &SampleClip::playbackPositionChanged, Qt::DirectConnection); //care about mute Clips connect( this, SIGNAL(dataChanged()), this, SLOT(playbackPositionChanged())); //care about mute track diff --git a/src/core/Song.cpp b/src/core/Song.cpp index 306647b6b..f9cf716c7 100644 --- a/src/core/Song.cpp +++ b/src/core/Song.cpp @@ -92,13 +92,10 @@ Song::Song() : m_length( 0 ), m_midiClipToPlay( nullptr ), m_loopMidiClip( false ), - m_elapsedTicks( 0 ), - m_elapsedBars( 0 ), m_loopRenderCount(1), m_loopRenderRemaining(1), m_oldAutomatedValues() { - for (double& millisecondsElapsed : m_elapsedMilliSeconds) { millisecondsElapsed = 0; } connect( &m_tempoModel, SIGNAL(dataChanged()), this, SLOT(setTempo()), Qt::DirectConnection ); connect( &m_tempoModel, SIGNAL(dataUnchanged()), @@ -120,6 +117,19 @@ Song::Song() : for (auto& scale : m_scales) {scale = std::make_shared();} for (auto& keymap : m_keymaps) {keymap = std::make_shared();} + + // Aggregate the `positionJumped` signals from all the timelines into a single `playbackPositionJumped` signal for other objects (sample tracks, LFOs, etc) to use. + for (auto& timeline : m_timelines) + { + connect(&timeline, &Timeline::positionJumped, this, [this](){ + // Only emit the signal when the song is actually playing + // This prevents LFOs from changing phase when the user drags the timeline while paused + if (isPlaying()) { emit playbackPositionJumped(); } + }, Qt::DirectConnection); + } + + // Inform VST plugins if the user moved the play head + connect(this, &Song::playbackPositionJumped, this, [this](){ m_vstSyncController.setPlaybackJumped(true); }, Qt::DirectConnection); } @@ -193,8 +203,6 @@ void Song::savePlayStartPosition() void Song::processNextBuffer() { - m_vstSyncController.setPlaybackJumped(false); - // If nothing is playing, there is nothing to do if (!m_playing) { return; } @@ -234,6 +242,8 @@ void Song::processNextBuffer() return; } + auto& timeline = getTimeline(); + // If the playback position is outside of the range [begin, end), move it to // begin and inform interested parties. // Returns true if the playback position was moved, else false. @@ -241,28 +251,17 @@ void Song::processNextBuffer() { if (getPlayPos() < begin || getPlayPos() >= end) { - setToTime(begin); - m_vstSyncController.setPlaybackJumped(true); - emit updateSampleTracks(); + getTimeline().setTicks(begin.getTicks()); return true; } return false; }; - const auto& timeline = getTimeline(); const auto loopEnabled = !m_exporting && timeline.loopEnabled(); // Ensure playback begins within the loop if it is enabled if (loopEnabled) { enforceLoop(timeline.loopBegin(), timeline.loopEnd()); } - // Inform VST plugins and sample tracks if the user moved the play head - if (getPlayPos().jumped()) - { - m_vstSyncController.setPlaybackJumped(true); - emit updateSampleTracks(); - getPlayPos().setJumped(false); - } - const auto framesPerTick = Engine::framesPerTick(); const auto framesPerPeriod = Engine::audioEngine()->framesPerPeriod(); @@ -270,16 +269,16 @@ void Song::processNextBuffer() while (frameOffsetInPeriod < framesPerPeriod) { - auto frameOffsetInTick = getPlayPos().currentFrame(); + auto frameOffsetInTick = timeline.frameOffset(); // If a whole tick has elapsed, update the frame and tick count, and check any loops if (frameOffsetInTick >= framesPerTick) { // Transfer any whole ticks from the frame count to the tick count const auto elapsedTicks = static_cast(frameOffsetInTick / framesPerTick); - getPlayPos().setTicks(getPlayPos().getTicks() + elapsedTicks); frameOffsetInTick -= elapsedTicks * framesPerTick; - getPlayPos().setCurrentFrame(frameOffsetInTick); + timeline.incrementTicks(elapsedTicks); + timeline.setFrameOffset(frameOffsetInTick); // If we are playing a pattern track, or a MIDI clip with no loop enabled, // loop back to the beginning when we reach the end @@ -323,7 +322,7 @@ void Song::processNextBuffer() // This must be done after we've corrected the frame/tick count, // but before actually playing any frames. m_vstSyncController.setAbsolutePosition(getPlayPos().getTicks() - + getPlayPos().currentFrame() / static_cast(framesPerTick)); + + timeline.frameOffset() / static_cast(framesPerTick)); m_vstSyncController.update(); } @@ -342,11 +341,11 @@ void Song::processNextBuffer() // Update frame counters frameOffsetInPeriod += framesToPlay; frameOffsetInTick += framesToPlay; - getPlayPos().setCurrentFrame(frameOffsetInTick); - m_elapsedMilliSeconds[static_cast(m_playMode)] += TimePos::ticksToMilliseconds(framesToPlay / framesPerTick, getTempo()); - m_elapsedBars = getPlayPos(PlayMode::Song).getBar(); - m_elapsedTicks = (getPlayPos(PlayMode::Song).getTicks() % ticksPerBar()) / 48; + timeline.setFrameOffset(frameOffsetInTick); } + + // Reset the jumped state after processing this buffer, since presumably it has now been handled. + m_vstSyncController.setPlaybackJumped(false); } @@ -603,26 +602,6 @@ void Song::updateLength() -void Song::setPlayPos( tick_t ticks, PlayMode playMode ) -{ - tick_t ticksFromPlayMode = getPlayPos(playMode).getTicks(); - m_elapsedTicks += ticksFromPlayMode - ticks; - m_elapsedMilliSeconds[static_cast(playMode)] += TimePos::ticksToMilliseconds( ticks - ticksFromPlayMode, getTempo() ); - getPlayPos(playMode).setTicks( ticks ); - getPlayPos(playMode).setCurrentFrame( 0.0f ); - getPlayPos(playMode).setJumped( true ); - -// send a signal if playposition changes during playback - if( isPlaying() ) - { - emit playbackPositionChanged(); - emit updateSampleTracks(); - } -} - - - - void Song::togglePause() { // Pause/unpause only works when something is actually playing @@ -674,20 +653,18 @@ void Song::stop() case Timeline::StopBehaviour::BackToZero: if (m_playMode == PlayMode::MidiClip) { - getPlayPos().setTicks(std::max(0, -m_midiClipToPlay->startTimeOffset())); + timeline.setTicks(std::max(0, -m_midiClipToPlay->startTimeOffset())); } else { - getPlayPos().setTicks(0); + timeline.setTicks(0); } - m_elapsedMilliSeconds[static_cast(m_playMode)] = 0; break; case Timeline::StopBehaviour::BackToStart: if (timeline.playStartPosition() >= 0) { - getPlayPos().setTicks(timeline.playStartPosition().getTicks()); - setToTime(timeline.playStartPosition()); + timeline.setTicks(timeline.playStartPosition().getTicks()); timeline.setPlayStartPosition(-1); } @@ -697,15 +674,12 @@ void Song::stop() break; } - m_elapsedMilliSeconds[static_cast(PlayMode::None)] = m_elapsedMilliSeconds[static_cast(m_playMode)]; - getPlayPos(PlayMode::None).setTicks(getPlayPos().getTicks()); - - getPlayPos().setCurrentFrame( 0 ); + getTimeline(PlayMode::None).setTicks(getPlayPos().getTicks()); m_vstSyncController.setPlaybackState( m_exporting ); m_vstSyncController.setAbsolutePosition( getPlayPos().getTicks() - + getPlayPos().currentFrame() + + timeline.frameOffset() / (double) Engine::framesPerTick() ); // remove all note-play-handles that are active @@ -738,14 +712,14 @@ void Song::startExport() m_exporting = true; updateLength(); - const auto& timeline = getTimeline(PlayMode::Song); + auto& timeline = getTimeline(PlayMode::Song); if (m_renderBetweenMarkers) { m_exportSongBegin = m_exportLoopBegin = timeline.loopBegin(); m_exportSongEnd = m_exportLoopEnd = timeline.loopEnd(); - getPlayPos(PlayMode::Song).setTicks(timeline.loopBegin().getTicks()); + timeline.setTicks(timeline.loopBegin().getTicks()); } else { @@ -768,7 +742,7 @@ void Song::startExport() ? timeline.loopEnd() : TimePos{0}; - getPlayPos(PlayMode::Song).setTicks( 0 ); + getTimeline(PlayMode::Song).setTicks(0); } m_exportEffectiveLength = (m_exportLoopBegin - m_exportSongBegin) + (m_exportLoopEnd - m_exportLoopBegin) diff --git a/src/gui/editors/AutomationEditor.cpp b/src/gui/editors/AutomationEditor.cpp index c847e69ce..9b1b53c7e 100644 --- a/src/gui/editors/AutomationEditor.cpp +++ b/src/gui/editors/AutomationEditor.cpp @@ -127,13 +127,10 @@ AutomationEditor::AutomationEditor() : // add time-line m_timeLine = new TimeLineWidget(VALUES_WIDTH, 0, m_ppb, - Engine::getSong()->getPlayPos(Song::PlayMode::AutomationClip), Engine::getSong()->getTimeline(Song::PlayMode::AutomationClip), - m_currentPosition, Song::PlayMode::AutomationClip, this + m_currentPosition, this ); - connect(this, &AutomationEditor::positionChanged, m_timeLine, &TimeLineWidget::updatePosition); - connect( m_timeLine, SIGNAL( positionChanged( const lmms::TimePos& ) ), - this, SLOT( updatePosition( const lmms::TimePos& ) ) ); + connect(m_timeLine->timeline(), &Timeline::positionChanged, this, &AutomationEditor::updatePosition); // init scrollbars m_leftRightScroll = new QScrollBar( Qt::Horizontal, this ); @@ -269,23 +266,17 @@ void AutomationEditor::keyPressEvent(QKeyEvent * ke ) break; case Qt::Key_Left: - if( ( m_timeLine->pos() -= 16 ) < 0 ) - { - m_timeLine->pos().setTicks( 0 ); - } - m_timeLine->updatePosition(); + m_timeLine->timeline()->setTicks(std::max(0, m_timeLine->timeline()->ticks() - 16)); ke->accept(); break; case Qt::Key_Right: - m_timeLine->pos() += 16; - m_timeLine->updatePosition(); + m_timeLine->timeline()->setTicks(m_timeLine->timeline()->ticks() + 16); ke->accept(); break; case Qt::Key_Home: - m_timeLine->pos().setTicks( 0 ); - m_timeLine->updatePosition(); + m_timeLine->timeline()->setTicks(0); ke->accept(); break; @@ -1719,7 +1710,7 @@ void AutomationEditor::stop() void AutomationEditor::horScrolled(int new_pos ) { m_currentPosition = new_pos; - emit positionChanged( m_currentPosition ); + m_timeLine->update(); update(); } @@ -1788,8 +1779,9 @@ void AutomationEditor::setTension() -void AutomationEditor::updatePosition(const TimePos & t ) +void AutomationEditor::updatePosition() { + const TimePos& t = m_timeLine->timeline()->pos(); if( ( Engine::getSong()->isPlaying() && Engine::getSong()->playMode() == Song::PlayMode::AutomationClip ) || diff --git a/src/gui/editors/PatternEditor.cpp b/src/gui/editors/PatternEditor.cpp index 8cea58a82..919393857 100644 --- a/src/gui/editors/PatternEditor.cpp +++ b/src/gui/editors/PatternEditor.cpp @@ -57,11 +57,10 @@ PatternEditor::PatternEditor(PatternStore* ps) : setModel(ps); m_timeLine = new TimeLineWidget(m_trackHeadWidth, 32, pixelsPerBar(), - Engine::getSong()->getPlayPos(Song::PlayMode::Pattern), Engine::getSong()->getTimeline(Song::PlayMode::Pattern), - m_currentPosition, Song::PlayMode::Pattern, this + m_currentPosition, this ); - connect(m_timeLine, &TimeLineWidget::positionChanged, this, &PatternEditor::updatePosition); + connect(m_timeLine->timeline(), &Timeline::positionChanged, this, &PatternEditor::updatePosition); static_cast(layout())->insertWidget(0, m_timeLine); connect(m_ps, &PatternStore::trackUpdated, @@ -199,7 +198,6 @@ void PatternEditor::updatePosition() { trackView->update(); } - emit positionChanged( m_currentPosition ); } void PatternEditor::updatePixelsPerBar() diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp index 149bbc36a..4e9e384e4 100644 --- a/src/gui/editors/PianoRoll.cpp +++ b/src/gui/editors/PianoRoll.cpp @@ -276,23 +276,22 @@ PianoRoll::PianoRoll() : // add time-line m_timeLine = new TimeLineWidget(m_whiteKeyWidth, 0, m_ppb, - Engine::getSong()->getPlayPos(Song::PlayMode::MidiClip), Engine::getSong()->getTimeline(Song::PlayMode::MidiClip), - m_currentPosition, Song::PlayMode::MidiClip, this + m_currentPosition, this ); - connect(this, &PianoRoll::positionChanged, m_timeLine, &TimeLineWidget::updatePosition); - connect( m_timeLine, SIGNAL( positionChanged( const lmms::TimePos& ) ), - this, SLOT( updatePosition( const lmms::TimePos& ) ) ); + connect(m_timeLine->timeline(), &Timeline::positionChanged, this, &PianoRoll::updatePosition); // white position line follows timeline marker m_positionLine = new PositionLine(this, Song::PlayMode::MidiClip); + connect(Engine::getSong(), &Song::playbackStateChanged, m_positionLine, qOverload<>(&QWidget::update)); + //update timeline when in step-recording mode connect( &m_stepRecorderWidget, SIGNAL( positionChanged( const lmms::TimePos& ) ), this, SLOT( updatePositionStepRecording( const lmms::TimePos& ) ) ); // update timeline when in record-accompany mode - connect(m_timeLine, &TimeLineWidget::positionChanged, this, &PianoRoll::updatePositionAccompany); + connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), &Timeline::positionChanged, this, &PianoRoll::updatePositionAccompany); // TODO /* connect( engine::getSong()->getPlayPos( Song::PlayMode::Pattern ).m_timeLine, SIGNAL( positionChanged( const lmms::TimePos& ) ), @@ -882,8 +881,8 @@ void PianoRoll::setCurrentMidiClip( MidiClip* newMidiClip ) } // Make sure the playhead position isn't out of the clip bounds. - Engine::getSong()->getPlayPos(Song::PlayMode::MidiClip).setTicks(std::clamp( - Engine::getSong()->getPlayPos(Song::PlayMode::MidiClip).getTicks(), + m_timeLine->timeline()->setTicks(std::clamp( + m_timeLine->timeline()->ticks(), std::max(0, -m_midiClip->startTimeOffset()), m_midiClip->length() - m_midiClip->startTimeOffset() )); @@ -1456,8 +1455,7 @@ void PianoRoll::keyPressEvent(QKeyEvent* ke) break; case Qt::Key_Home: - m_timeLine->pos().setTicks( 0 ); - m_timeLine->updatePosition(); + m_timeLine->timeline()->setTicks(0); ke->accept(); break; @@ -4316,7 +4314,8 @@ void PianoRoll::horScrolled(int new_pos ) { m_currentPosition = new_pos; m_stepRecorderWidget.setCurrentPosition(m_currentPosition); - emit positionChanged( m_currentPosition ); + m_timeLine->update(); + updatePositionLinePos(); update(); } @@ -4589,7 +4588,7 @@ void PianoRoll::pasteNotes() // create the note Note cur_note; cur_note.restoreState( list.item( i ).toElement() ); - cur_note.setPos( cur_note.pos() + Note::quantized( m_timeLine->pos(), quantization() ) ); + cur_note.setPos(cur_note.pos() + Note::quantized(m_timeLine->timeline()->pos(), quantization())); // select it cur_note.setSelected( true ); @@ -4655,8 +4654,9 @@ void PianoRoll::autoScroll( const TimePos & t ) -void PianoRoll::updatePosition(const TimePos & t) +void PianoRoll::updatePosition() { + const TimePos& t = m_timeLine->timeline()->pos(); if ((Engine::getSong()->isPlaying() && Engine::getSong()->playMode() == Song::PlayMode::MidiClip && m_timeLine->autoScroll() != TimeLineWidget::AutoScrollState::Disabled @@ -4664,10 +4664,15 @@ void PianoRoll::updatePosition(const TimePos & t) { autoScroll(t); } + updatePositionLinePos(); +} + +void PianoRoll::updatePositionLinePos() +{ // ticks relative to m_currentPosition // < 0 = outside viewport left // > width = outside viewport right - const int pos = (static_cast(m_timeLine->pos()) - m_currentPosition) * m_ppb / TimePos::ticksPerBar(); + const int pos = (static_cast(m_timeLine->timeline()->pos()) - m_currentPosition) * m_ppb / TimePos::ticksPerBar(); // if pos is within visible range, show it if (hasValidMidiClip() && pos >= 0 && pos <= width() - m_whiteKeyWidth) { @@ -4690,8 +4695,9 @@ void PianoRoll::updatePositionLineHeight() -void PianoRoll::updatePositionAccompany( const TimePos & t ) +void PianoRoll::updatePositionAccompany() { + const TimePos& t = Engine::getSong()->getPlayPos(Song::PlayMode::Song); Song * s = Engine::getSong(); if( m_recording && hasValidMidiClip() && @@ -4700,11 +4706,11 @@ void PianoRoll::updatePositionAccompany( const TimePos & t ) TimePos pos = t; if (s->playMode() != Song::PlayMode::Pattern) { - pos -= m_midiClip->startPosition(); + pos -= m_midiClip->startPosition() + m_midiClip->startTimeOffset(); } if( (int) pos > 0 ) { - s->getPlayPos( Song::PlayMode::MidiClip ).setTicks( pos ); + m_timeLine->timeline()->setTicks(pos); autoScroll( pos ); } } @@ -4729,6 +4735,7 @@ void PianoRoll::zoomingChanged() m_timeLine->setPixelsPerBar( m_ppb ); m_stepRecorderWidget.setPixelsPerBar( m_ppb ); m_positionLine->zoomChange(m_zoomLevels[m_zoomingModel.value()]); + updatePositionLinePos(); update(); } diff --git a/src/gui/editors/SongEditor.cpp b/src/gui/editors/SongEditor.cpp index 88706bfe5..55400cd3f 100644 --- a/src/gui/editors/SongEditor.cpp +++ b/src/gui/editors/SongEditor.cpp @@ -96,13 +96,11 @@ SongEditor::SongEditor( Song * song ) : { // Set up timeline m_timeLine = new TimeLineWidget(m_trackHeadWidth, 32, pixelsPerBar(), - m_song->getPlayPos(Song::PlayMode::Song), m_song->getTimeline(Song::PlayMode::Song), - m_currentPosition, Song::PlayMode::Song, this + m_currentPosition, this ); - connect(this, &TrackContainerView::positionChanged, m_timeLine, &TimeLineWidget::updatePosition); - connect( m_timeLine, SIGNAL( positionChanged( const lmms::TimePos& ) ), - this, SLOT( updatePosition( const lmms::TimePos& ) ) ); + connect(this, &TrackContainerView::positionChanged, m_timeLine, qOverload<>(&QWidget::update)); + connect(m_timeLine->timeline(), &Timeline::positionChanged, this, &SongEditor::updatePosition); connect( m_timeLine, SIGNAL(regionSelectedFromPixels(int,int)), this, SLOT(selectRegionFromPixels(int,int))); connect( m_timeLine, SIGNAL(selectionFinished()), @@ -120,7 +118,10 @@ SongEditor::SongEditor( Song * song ) : // When zoom changes, update position line // But we must convert pixels per bar to a zoom factor where 1.0 is 100% connect(this, &SongEditor::pixelsPerBarChanged, m_positionLine, - [this]() { m_positionLine->zoomChange(pixelsPerBar() / float(DEFAULT_PIXELS_PER_BAR)); }); + [this]() { + m_positionLine->zoomChange(pixelsPerBar() / float(DEFAULT_PIXELS_PER_BAR)); + updatePositionLine(); + }); // Ensure loop markers snap to same increments as clips. Zoom & proportional // snap changes are handled in zoomingChanged() and toggleProportionalSnap() @@ -329,6 +330,7 @@ void SongEditor::scrolled( int new_pos ) { update(); emit positionChanged(m_currentPosition = TimePos(new_pos)); + updatePositionLine(); } @@ -472,7 +474,7 @@ void SongEditor::keyPressEvent( QKeyEvent * ke ) } else if( ke->key() == Qt::Key_Left ) { - tick_t t = m_song->currentTick() - TimePos::ticksPerBar(); + tick_t t = m_song->getPlayPos(Song::PlayMode::Song).getTicks() - TimePos::ticksPerBar(); if( t >= 0 ) { m_song->setPlayPos( t, Song::PlayMode::Song ); @@ -480,7 +482,7 @@ void SongEditor::keyPressEvent( QKeyEvent * ke ) } else if( ke->key() == Qt::Key_Right ) { - tick_t t = m_song->currentTick() + TimePos::ticksPerBar(); + tick_t t = m_song->getPlayPos(Song::PlayMode::Song).getTicks() + TimePos::ticksPerBar(); if( t < MaxSongLength ) { m_song->setPlayPos( t, Song::PlayMode::Song ); @@ -760,8 +762,9 @@ static inline void animateScroll( QScrollBar *scrollBar, int newVal, bool smooth -void SongEditor::updatePosition( const TimePos & t ) +void SongEditor::updatePosition() { + const TimePos& t = m_timeLine->timeline()->pos(); const bool compactTrackButtons = ConfigManager::inst()->value("ui", "compacttrackbuttons").toInt(); const auto widgetWidth = compactTrackButtons ? DEFAULT_SETTINGS_WIDGET_WIDTH_COMPACT : DEFAULT_SETTINGS_WIDGET_WIDTH; const auto trackOpWidth = compactTrackButtons ? TRACK_OP_WIDTH_COMPACT : TRACK_OP_WIDTH; @@ -789,7 +792,18 @@ void SongEditor::updatePosition( const TimePos & t ) m_scrollBack = false; } - const int x = m_timeLine->markerX(t); + updatePositionLine(); +} + + + + +void SongEditor::updatePositionLine() +{ + const bool compactTrackButtons = ConfigManager::inst()->value("ui", "compacttrackbuttons").toInt(); + const auto widgetWidth = compactTrackButtons ? DEFAULT_SETTINGS_WIDGET_WIDTH_COMPACT : DEFAULT_SETTINGS_WIDGET_WIDTH; + const auto trackOpWidth = compactTrackButtons ? TRACK_OP_WIDTH_COMPACT : TRACK_OP_WIDTH; + const int x = m_timeLine->markerX(m_timeLine->timeline()->pos()); if( x >= trackOpWidth + widgetWidth -1 ) { m_positionLine->show(); @@ -800,14 +814,6 @@ void SongEditor::updatePosition( const TimePos & t ) m_positionLine->hide(); } - updatePositionLine(); -} - - - - -void SongEditor::updatePositionLine() -{ m_positionLine->setFixedHeight(totalHeightOfTracks()); } diff --git a/src/gui/editors/TimeLineWidget.cpp b/src/gui/editors/TimeLineWidget.cpp index 9c7ae5178..cc1105a52 100644 --- a/src/gui/editors/TimeLineWidget.cpp +++ b/src/gui/editors/TimeLineWidget.cpp @@ -46,25 +46,21 @@ namespace constexpr int MIN_BAR_LABEL_DISTANCE = 35; } -TimeLineWidget::TimeLineWidget(const int xoff, const int yoff, const float ppb, Song::PlayPos& pos, Timeline& timeline, - const TimePos& begin, Song::PlayMode mode, QWidget* parent) : +TimeLineWidget::TimeLineWidget(const int xoff, const int yoff, const float ppb, Timeline& timeline, + const TimePos& begin, QWidget* parent) : QWidget{parent}, m_xOffset{xoff}, m_ppb{ppb}, - m_pos{pos}, m_timeline{&timeline}, - m_begin{begin}, - m_mode{mode} + m_begin{begin} { move( 0, yoff ); setMouseTracking(true); - auto updateTimer = new QTimer(this); - connect(updateTimer, &QTimer::timeout, this, &TimeLineWidget::updatePosition); - updateTimer->start( 1000 / 60 ); // 60 fps connect( Engine::getSong(), SIGNAL(timeSignatureChanged(int,int)), this, SLOT(update())); + connect(m_timeline, &Timeline::positionChanged, this, qOverload<>(&QWidget::update)); } @@ -129,12 +125,6 @@ void TimeLineWidget::addToolButtons( QToolBar * _tool_bar ) _tool_bar->addWidget( behaviourAtStop ); } -void TimeLineWidget::updatePosition() -{ - emit positionChanged(m_pos); - update(); -} - void TimeLineWidget::toggleAutoScroll( int _n ) { m_autoScroll = static_cast( _n ); @@ -223,12 +213,12 @@ void TimeLineWidget::paintEvent( QPaintEvent * ) const QPixmap& marker = !m_isRecording ? m_posMarkerPixmap : m_recordingPosMarkerPixmap; // Only draw the position marker if the position line is in view - if (m_isPlayheadVisible && markerX(m_pos) >= m_xOffset && markerX(m_pos) < width() - marker.width() / 2) + if (m_isPlayheadVisible && markerX(m_timeline->pos()) >= m_xOffset && markerX(m_timeline->pos()) < width() - marker.width() / 2) { // Let the position marker extrude to the left p.setClipping(false); p.setOpacity(0.6); - p.drawPixmap(markerX(m_pos) - (marker.width() / 2), + p.drawPixmap(markerX(m_timeline->pos()) - (marker.width() / 2), height() - marker.height(), marker); } } @@ -341,16 +331,13 @@ void TimeLineWidget::mouseMoveEvent( QMouseEvent* event ) switch( m_action ) { case Action::MovePositionMarker: - m_pos.setTicks(timeAtCursor.getTicks()); - Engine::getSong()->setToTime(timeAtCursor, m_mode); + m_timeline->setTicks(timeAtCursor.getTicks()); if (!( Engine::getSong()->isPlaying())) { //Song::PlayMode::None is used when nothing is being played. - Engine::getSong()->setToTime(timeAtCursor, Song::PlayMode::None); + Engine::getSong()->getTimeline(Song::PlayMode::None).setTicks(timeAtCursor.getTicks()); } - m_pos.setCurrentFrame( 0 ); - m_pos.setJumped( true ); - updatePosition(); + update(); break; case Action::MoveLoopBegin: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7d7b499af..6e4df8854 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -9,6 +9,7 @@ set(LMMS_TESTS src/core/MathTest.cpp src/core/ProjectVersionTest.cpp src/core/RelativePathsTest.cpp + src/core/TimelineTest.cpp src/tracks/AutomationTrackTest.cpp ) diff --git a/tests/src/core/TimelineTest.cpp b/tests/src/core/TimelineTest.cpp new file mode 100644 index 000000000..cee778f70 --- /dev/null +++ b/tests/src/core/TimelineTest.cpp @@ -0,0 +1,119 @@ +/* + * TimelineTest.cpp + * + * Copyright (c) 2025 Keratin + * + * This file is part of LMMS - https://lmms.io + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program (see COPYING); if not, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + */ + + +#include +#include "Timeline.h" + +#include "Song.h" + +class TimelineTest : public QObject +{ + Q_OBJECT +public: + bool positionChangedReceived; + bool positionJumpedReceived; + void resetReceived() + { + positionChangedReceived = false; + positionJumpedReceived = false; + } + +private slots: + void onPositionChanged() { positionChangedReceived = true; } + void onPositionJumped() { positionJumpedReceived = true; } + +private slots: + + void initTestCase() + { + using namespace lmms; + Engine::init(true); + } + + void cleanupTestCase() + { + using namespace lmms; + Engine::destroy(); + } + + void JumpedTests() + { + using namespace lmms; + + Timeline timeline = Timeline(); + connect(&timeline, &Timeline::positionChanged, this, &TimelineTest::onPositionChanged); + connect(&timeline, &Timeline::positionJumped, this, &TimelineTest::onPositionJumped); + + // By default, setting the ticks is treated as a forceful jump + resetReceived(); + timeline.setTicks(10); + QCOMPARE(timeline.ticks(), 10); + QVERIFY(positionChangedReceived); + QVERIFY(positionJumpedReceived); + + // However, using incrementTicks will not emit positionJumped. + resetReceived(); + timeline.incrementTicks(10); + QCOMPARE(timeline.ticks(), 20); + QVERIFY(positionChangedReceived); + QVERIFY(!positionJumpedReceived); + } + + void ElapsedTimeTests() + { + using namespace lmms; + + Timeline timeline = Timeline(); + connect(&timeline, &Timeline::positionChanged, this, &TimelineTest::onPositionChanged); + connect(&timeline, &Timeline::positionJumped, this, &TimelineTest::onPositionJumped); + + // Forecefully setting the ticks to 0 should reset the elapsed time + timeline.setTicks(0); + QCOMPARE(timeline.getElapsedSeconds(), 0); + + // Setting the ticks to a nonzero value should reset the elapsed time to that tick's time based on the current tempo + Engine::getSong()->setTempo(240); + double secondsPerTick = 60.0f / Engine::getSong()->getTempo() * 4 / DefaultTicksPerBar; + timeline.setTicks(10); + double initialElapsedSeconds = timeline.getElapsedSeconds(); + QCOMPARE(static_cast(timeline.getElapsedSeconds() * 1000), static_cast(10 * secondsPerTick * 1000)); // Rouding to milliseconds to prevent double comparison issues + + // Changing the tempo and then non-forcefully incrementing the ticks will increase the elapsed time based on the new tempo + Engine::getSong()->setTempo(60); + secondsPerTick = 60.0f / Engine::getSong()->getTempo() * 4 / DefaultTicksPerBar; + timeline.incrementTicks(5); + QCOMPARE(static_cast(timeline.getElapsedSeconds() * 1000), static_cast((initialElapsedSeconds + 5 * secondsPerTick) * 1000)); + + // Forcefully setting the ticks (such as dragging the playhead with the mouse) will reset the elapsed time based on the global position and current tempo + Engine::getSong()->setTempo(180); + secondsPerTick = 60.0f / Engine::getSong()->getTempo() * 4 / DefaultTicksPerBar; + timeline.setTicks(25); + QCOMPARE(static_cast(timeline.getElapsedSeconds() * 1000), static_cast(25 * secondsPerTick * 1000)); + } + +}; + +QTEST_GUILESS_MAIN(TimelineTest) +#include "TimelineTest.moc"