Refactor to move positionChanged signal to Timeline #7454

regulus79 · 2024-08-14T23:47:40Z

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

Moved definition of PlayPos to its own file, PlayPos.h.
The positionChanged signal and updatePosition slot were removed from TimeLineWidget and moved to Timeline.
The connections to Timeline::positionChanged were made into Qt::QueuedConnections to prevent errors when the audio thread changes the PlayPos which would immediately cause gui code to run if it were a direct connection.
The pos() method and PlayPos m_pos were removed from TimeLineWidget; PlayPos m_pos was added to Timeline, along with getter and setter methods which emit positionChanged.
The constructor for TimeLineWidget no longer requires a PlayPos reference.
Since each TimeLineWidget stores a reference to its Timeline, a new method was added, model(), for other classes to access it.
Removed array of PlayPoses in Song. Now each Timeline holds their respective PlayPos.
Song's methods for getPlayPos were changed to return the PlayPos of the respective Timeline. The non-const versions of the methods were removed because Timeline does not expose its PlayPos 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->model()->getPlayPos(), and calls to m_timeline->pos().setTicks(ticks) were changed to m_timeline->model()->setTicks(ticks).

Minor note: The members m_timeline or m_timeLine in places like PianoRoll or SongEditor should honestly really be renamed something like m_timeLineWidget, as they are not Timeline objects.

Old PR Description

This pull request fixes #7351 by changing the PianoRoll's connection from `m_timeline` to `&Engine::getSong()->getTimeline(Song::PlayMode::Song)`, and additionally adding the signal `positionChanged` to the `Timeline` class.

The new signal is necessary because getTimeline() returns a Timeline, which does not have a positionChanged signal, while m_timeline is a TimeLineWidget which does.

This pr is marked as a draft, since after fixing the bug I found another one: When pressing stop after record-play, the piano roll play head does not go back to the start, but instead stops at its current position. Edit: I have decided to leave fixing that bug to a future pr, as it appears that it existed prior to the regression. Instead, this pr simply fixes the pianoroll playhead movement.

messmerd · 2024-08-31T17:28:44Z

src/gui/editors/PianoRoll.cpp

@@ -290,7 +290,7 @@ PianoRoll::PianoRoll() :
 			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), SIGNAL(positionChanged(const lmms::TimePos&)), this, SLOT(updatePositionAccompany(const lmms::TimePos&)));


Suggested change

connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), SIGNAL(positionChanged(const lmms::TimePos&)), this, SLOT(updatePositionAccompany(const lmms::TimePos&)));

connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), &Timeline::positionChanged, this, &PianoRoll::updatePositionAccompany);

We're trying to avoid using the SIGNAL and SLOT macros in new code

Actually, there may be a better approach than adding a signal to Timeline... Let me think this over some more

I don't really like how Timeline and TimeLineWidget have implemented their signals and slots for changes in position. I think ideally Timeline should be the model and only it should emit any positionChanged signals, while TimeLineWidget should be the view and its updatePosition slot should not emit any signals. The way it is currently, the TimeLineWidget is trying to do the job of both a model and a view, and that complicates things.

@DomClark Do you agree with my assessment? It looks to me like a proper solution to this bug may require rethinking how the timeline position signals/slots are implemented.

I have now updated to PR to refactor everything in order to move the positionChanged signal from TimeLineWidget to Timeline.

Co-authored-by: Dalton Messmer <[email protected]>

Rossmaxx · 2024-10-02T15:46:39Z

Fix looks simple. @messmerd did this fix your issue?

…itting signals

messmerd

When I ran it, I saw this output in the console:

QObject::connect: Cannot queue arguments of type 'lmms::TimePos'
(Make sure 'lmms::TimePos' is registered using qRegisterMetaType().)

include/TimeLineWidget.h

include/Timeline.h

messmerd · 2024-12-12T04:19:24Z

src/core/Song.cpp

@@ -121,6 +121,7 @@ Song::Song() :

 	for (auto& scale : m_scales) {scale = std::make_shared<Scale>();}
 	for (auto& keymap : m_keymaps) {keymap = std::make_shared<Keymap>();}
+	for (auto& timeline : m_timelines) { connect(this, &Song::bufferProcessed, &timeline, &Timeline::updatePosition, Qt::QueuedConnection); }


This passes the Song's bufferProcessed signal to Timeline's updatePosition slot which does nothing except emit a positionChanged signal containing the play position which editors connect to for updating their position.

It seems to me that Timeline::updatePosition should not exist. When the setters of Timeline are called and its position changes, they would emit the positionChanged signal. Then any views connected to that signal would update as needed.

Maybe it would be a good idea to sketch out a tree diagram of the signal/slot connections between the different objects - might help with understanding and simplifying this.

I agree, I think the bufferProcessed signal and updatePosition slot can probably be removed.

Except that, when I remove them and instead rely on setTicks(ticks) to emit the signal, the processNextBuffer function in Song does not update the playhead position, so it stays still during playback. I believe this may be due to processNextBuffer being on a different thread?

But then it somehow fixes itself when I specify Qt::DirectConnection in all the connect()s. I'm not sure why.

include/PlayPos.h

src/core/Song.cpp

src/gui/editors/AutomationEditor.cpp

src/gui/editors/PianoRoll.cpp

Co-authored-by: Dalton Messmer <[email protected]>

regulus79 · 2024-12-12T20:51:24Z

The QObject::connect: Cannot queue arguments of type 'lmms::TimePos' warnings appear to have gone away after I changed the connections to Qt::DirectConnection. However, I do not know if this will impact the performance of the audio thread, as it may cause the gui to be updated and interrupt the buffer processing(?) However, using Qt::QueuedConnection seems to fail to actually recieve the signals, and I'm not sure why.

…signal

regulus79 · 2024-12-15T03:58:10Z

I found a solution to the warnings by simply removing the parameter from the signal and instead having each editor query the current position of the timeline themselves. The connections are all now Qt::QueuedConnection.

sakertooth · 2024-12-15T16:32:47Z

Here's a sketch of how I think this functionality should work (for my own understanding and possibly anybody elses). I'll try to complete the review at some point.

regulus79 · 2024-12-16T00:23:52Z

Okay, this is difficult.

Originally, I had it so that Timeline emitted Timeline::positionChanged whenever Timeline::setPlayPos or Timeline::setTicks was called. Timeline::positionChanged was then connected to all the editors with a Qt::QueuedConnection. That worked well, but sakertooth experienced segmentation faults which I could not reproduce. Additionally, the audio thread causing the Timeline to send out signals to the gui thread whenever a buffer was processed didn't seem like a great idea. They were all queued connections, so theoretically it should be fine, but I don't know.

So then sakertooth had the idea of routing all the connections to the gui editors through TimeLineWidget, which would be passing along the signals from Timeline. This could have been an improvement by reducing the number of Qt::QueuedConnections, but it does not solve the issue of emitting signals across threads (which...may have been the cause of the segfaults? I do not know, I cannot reproduce them).

Another potential solution which sakertooth suggested is using std::atomic. This is a good idea to prevent race conditions, such as if the song is playing while the user drags on the timeline. However, removing all the positionChanged signals, while it simplifies the threading, means that we would have to reintroduce the QTimer to the TimeLineWidgets in order to update them. This is suboptimal. It is possible, and if it solves the segfaults, maybe it's the way to go. But it's not good.

What should be done?

sakertooth · 2024-12-16T03:35:25Z

So then sakertooth had the idea of routing all the connections to the gui editors through TimeLineWidget, which would be passing along the signals from Timeline. This could have been an improvement by reducing the number of Qt::QueuedConnections, but it does not solve the issue of emitting signals across threads (which...may have been the cause of the segfaults? I do not know, I cannot reproduce them).

Coming up with sound designs is probably my biggest weakness. The idea I mentioned could work, but as more time passes I continue to fail to see a critical issue using queued connections that emit the positionChanged signal directly from Timeline. That being said, this is what we are currently doing already I believe, so I'll just work on completing my review since the refactor is working in a way that makes the most sense already.

sakertooth · 2024-12-15T13:53:18Z

include/PlayPos.h

+/*
+ * PlayPos.h - declaration of class PlayPos, a TimePos with additional features useful for Timelines
+ *


I would suggest removing PlayPos and using TimePos directly in Timeline. This class mostly is just a container for data and does not do anything special really.

TimePos can be refactored to store frames as its smallest unit of time (or just store the frame position maybe? do we even need a class just to store the frame position within the tick?). The member functions should then be changed accordingly to account for that as necessary. This might be okay in a separate PR if you feel its out of scope, but I think its minor enough that it shouldn't matter too much to do it in here. Up to you.

Then for the jumped variable, from what I can tell, this just lets us know if the timeline didn't change through time progressively but instead jumped in time. What would it mean for the jumped variable to be true even when the timeline is not jumping? This should be a parameter for the positionChanged signal. You might have to refactor the VST sync controller code for its invocation to be event-based and not executed within Song::processNextBuffer though.

I'm not sure TimePos can easily be refactored to store frames as its smallest unit of time, unless we allow for fractional frames. If the number of frames in a bar is not a whole number, then it will have to be rounded. That's fine, but if the bpm is changed, then every single TimePos will have to be updated at once so that the number of frames matches the correct tick amount. I feel like a better long-term solution would be to allow float tick values, but that's just me.

But I agree, the jumped variable can probably be combined with the positionChanged signal.

What about just storing the frame position directly and still removing PlayPos?

I have removed PlayPos. Now each Timeline stores it's current frame offset and jumped state. I will see if I can get jumped to be passed in the positionChanged signal instead.

include/Song.h

include/TimeLineWidget.h

src/core/Song.cpp

sakertooth · 2024-12-15T14:19:34Z

src/gui/editors/AutomationEditor.cpp

@@ -273,23 +270,24 @@ void AutomationEditor::keyPressEvent(QKeyEvent * ke )
 			break;

 		case Qt::Key_Left:
-			if( ( m_timeLine->pos() -= 16 ) < 0 )
+			if(m_timeLine->model()->getTicks() - 16 > 0)


Suggested change

if(m_timeLine->model()->getTicks() - 16 > 0)

if ((m_timeLine->model()->getTicks() -= 16) < 0)

Timeline does not currently implement the -= operator. I suppose I could add it.

sakertooth · 2024-12-15T14:23:19Z

src/gui/editors/AutomationEditor.cpp

 			ke->accept();
 			break;

 		case Qt::Key_Right:
-			m_timeLine->pos() += 16;
-			m_timeLine->updatePosition();
+			m_timeLine->model()->setTicks(m_timeLine->model()->getTicks() + 16);


I think it's fine to leave it as is?

Suggested change

m_timeLine->model()->setTicks(m_timeLine->model()->getTicks() + 16);

m_timeLine->model() += 16;

Are you sure it makes sense to += an integer to a Timeline*? I get that it would just add to the internal TimePos, but it just seems weird to add a number to a timeline object. It would definitely make the code simpler in this situation, but idk.

src/gui/editors/PianoRoll.cpp

src/gui/editors/AutomationEditor.cpp

Co-authored-by: saker <[email protected]>

regulus79 · 2025-01-17T18:21:08Z

Partially following sakertooth's suggestion, I have removed PlayPos.h from the PR. Now each Timeline stores it's own TimePos along with a frame offset and jumped variable.

I have also removed m_elapsedMilliseconds, m_elapsedBars, and m_elapsedTicks from Song.

I could not find any code referencing m_elapsedBars or m_elapsedTicks in the entire codebase (outside of the code to update them), so removing those shouldn't cause any issues.

m_elapsedMilliseconds is used in displaying the current time LCD at the top. The current time can easily be calculated using the current frame, so removing this should also be fine in most cases, however, if you change the tempo mid-song, it will not display the correct time.

sakertooth also suggested getting rid of the jumped variable and instead passing it along with the positionChanged signal. I am still thinking of how that would be implemented, since Song would somehow have to store the jumped state when it receives the signal so that it can operate on it during the next buffer(?)

Add positionChanged signal to Timeline and alter PianoRoll connection

07c47b5

regulus79 marked this pull request as ready for review August 25, 2024 16:29

messmerd reviewed Aug 31, 2024

View reviewed changes

Remove SLOT and SIGNAL macros

241eedb

Co-authored-by: Dalton Messmer <[email protected]>

regulus79 added 5 commits December 7, 2024 20:40

Merge branch 'master' into fix-record-play-pianoroll-new

4efe2b8

Initial changes

4e4b8d8

Cleaning up

e8f5ddb

Add getters and setters, do not expose PlayPos for writing without em…

840d991

…itting signals

Cleaning up again

3032b04

regulus79 changed the title ~~Fix PianoRoll playhead not moving during Record-Play~~ Refactor to move positionChanged signal to Timeline Dec 11, 2024

messmerd reviewed Dec 12, 2024

View reviewed changes

regulus79 and others added 9 commits December 12, 2024 10:46

Update include/Timeline.h

e22f925

Co-authored-by: Dalton Messmer <[email protected]>

Update include/PlayPos.h

f66b9b9

Co-authored-by: Dalton Messmer <[email protected]>

Change timeline() to model() in TimeLineWidget.h

83be137

Co-authored-by: Dalton Messmer <[email protected]>

Rename timeline() to model() in editors

6467cf6

Forgot to change one occurance

5d2d367

Remove bufferProcessed signal and updatePosition slot

4adfe5a

Add return

375a6b0

Specify Qt::DirectConnection

b80cb42

Remove TODO

a9e4c0c

Change DirectConnection to QueuedConnection and remove argument from …

af1e1e2

…signal

sakertooth reviewed Dec 16, 2024

View reviewed changes

regulus79 and others added 2 commits December 16, 2024 11:24

Fix formatting in Song.cpp

6ef9b4e

Co-authored-by: saker <[email protected]>

Fix more formatting in Song.cpp

f29ad42

Co-authored-by: saker <[email protected]>

regulus79 and others added 6 commits December 16, 2024 11:25

Update src/core/Song.cpp

1b3c761

Co-authored-by: saker <[email protected]>

Pass Timeline to TimeLineWIdget as a pointer

2a50a8c

Replace accidentally-removed connects

010d649

Update src/gui/editors/PianoRoll.cpp

4ce6eba

Co-authored-by: saker <[email protected]>

Forgot to add

8f7484c

Remove PlayPos and put frame count and jumped in Timeline

2e7234f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to move positionChanged signal to Timeline #7454

Refactor to move positionChanged signal to Timeline #7454

regulus79 commented Aug 14, 2024 •

edited

Loading

messmerd Aug 31, 2024

messmerd Aug 31, 2024

messmerd Aug 31, 2024

messmerd Aug 31, 2024 •

edited

Loading

regulus79 Dec 11, 2024

Rossmaxx commented Oct 2, 2024

messmerd left a comment

messmerd Dec 12, 2024

regulus79 Dec 12, 2024

regulus79 commented Dec 12, 2024

regulus79 commented Dec 15, 2024 •

edited

Loading

sakertooth commented Dec 15, 2024

regulus79 commented Dec 16, 2024

sakertooth commented Dec 16, 2024

sakertooth Dec 15, 2024 •

edited

Loading

regulus79 Dec 16, 2024

sakertooth Dec 16, 2024

regulus79 Jan 17, 2025

sakertooth Dec 15, 2024 •

edited

Loading

regulus79 Dec 16, 2024 •

edited

Loading

sakertooth Dec 15, 2024

regulus79 Jan 17, 2025

regulus79 commented Jan 17, 2025

	connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), SIGNAL(positionChanged(const lmms::TimePos&)), this, SLOT(updatePositionAccompany(const lmms::TimePos&)));
	connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), &Timeline::positionChanged, this, &PianoRoll::updatePositionAccompany);

	if(m_timeLine->model()->getTicks() - 16 > 0)
	if ((m_timeLine->model()->getTicks() -= 16) < 0)

	m_timeLine->model()->setTicks(m_timeLine->model()->getTicks() + 16);
	m_timeLine->model() += 16;

Refactor to move positionChanged signal to Timeline #7454

Are you sure you want to change the base?

Refactor to move positionChanged signal to Timeline #7454

Conversation

regulus79 commented Aug 14, 2024 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

messmerd Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rossmaxx commented Oct 2, 2024

messmerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regulus79 commented Dec 12, 2024

regulus79 commented Dec 15, 2024 • edited Loading

sakertooth commented Dec 15, 2024

regulus79 commented Dec 16, 2024

sakertooth commented Dec 16, 2024

sakertooth Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakertooth Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

regulus79 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regulus79 commented Jan 17, 2025

regulus79 commented Aug 14, 2024 •

edited

Loading

messmerd Aug 31, 2024 •

edited

Loading

regulus79 commented Dec 15, 2024 •

edited

Loading

sakertooth Dec 15, 2024 •

edited

Loading

sakertooth Dec 15, 2024 •

edited

Loading

regulus79 Dec 16, 2024 •

edited

Loading