Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add clips to tracks and tracks to track containers only after the constructor completes #7594

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft

Add clips to tracks and tracks to track containers only after the constructor completes #7594

wants to merge 34 commits into from

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Nov 23, 2024 •
edited
Loading

Currently, we add tracks and clips prematurely to there respective containers, i.e., they are added within the base class. This has seemed to have caused a number of problems involving calls to pure virtual functions (because the object wasn't fully created yet), to dynamic_cast failures and ultimately crashes. This PR fixes this by only adding a track or clip to their respective containers after the object has been fully constructed.

Rossmaxx
Rossmaxx reviewed Nov 23, 2024
View reviewed changes
include/AutomationTrack.h Outdated Show resolved Hide resolved
Rossmaxx
Rossmaxx reviewed Nov 23, 2024
View reviewed changes
Copy link
Contributor

@Rossmaxx Rossmaxx left a comment

Choose a reason for hiding this comment

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

A few thoughts. Another doubt I have is that, does this fix that crash with deleting a track while playing?

src/core/Track.cpp Outdated Show resolved Hide resolved
src/core/Track.cpp Outdated Show resolved Hide resolved
@sakertooth
Copy link
Contributor Author

A few thoughts. Another doubt I have is that, does this fix that crash with deleting a track while playing?

I believe crashes like those happen because of synchronization issues involving mutexes (which I believe should be fixed, albeit not ideally). What I am addressing here with this PR is the problem where we are adding clips to tracks and tracks to track containers (like the song editor) way too early. If I created a SampleTrack object for example, it would add itself to the song editor when its constructor hasn't completed yet. This has caused a number of bugs when processing the Song.

@sakertooth
Copy link
Contributor Author

sakertooth commented Nov 23, 2024 •
edited
Loading

A few thoughts. Another doubt I have is that, does this fix that crash with deleting a track while playing?

Wow, I discovered something. When tracks are deleted, only the base destructor is called (Track::~Track) and not the derived one (maybe something like SampleTrack::~SampleTrack for example) because the destructor isn't virtual. Destructors like SampleTrack::~SampleTrack remove all play handles associated with that track, but it may not be ran when deleting it at all. I'm just realizing how many issues need to be fixed here.

Wait, nevermind, whatever Track inherits have virtual destructors already. I do remember that we covered these cases with a clang-tidy PR awhile back actually.

@sakertooth sakertooth changed the title Improve the creational correctness of tracks and clips Improve ownership semantics involving tracks and clips Nov 23, 2024
@sakertooth sakertooth changed the title Improve ownership semantics involving tracks and clips Improve creational correctness involving tracks and clips Nov 23, 2024
@sakertooth sakertooth added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing and removed needs code review A functional code review is currently required for this PR labels Nov 23, 2024
Rossmaxx
Rossmaxx reviewed Nov 24, 2024
View reviewed changes
src/core/Track.cpp Outdated Show resolved Hide resolved
@sakertooth sakertooth marked this pull request as draft November 25, 2024 02:48
@sakertooth sakertooth force-pushed the creational-correctness-tracks-clips branch from 64b8f86 to 3c0c167 Compare November 25, 2024 04:40
@sakertooth
Copy link
Contributor Author

PR too big. Started over. This PR is only concerned with delaying the addition of clips to tracks and tracks to track containers after the constructor is complete.

@sakertooth sakertooth changed the title Improve creational correctness involving tracks and clips Add clips to tracks and tracks to track containers only after the constructor completes Nov 25, 2024
@sakertooth sakertooth marked this pull request as ready for review November 25, 2024 04:45
@sakertooth sakertooth marked this pull request as draft November 29, 2024 23:57
@sakertooth sakertooth force-pushed the creational-correctness-tracks-clips branch from f8d545b to f20d308 Compare December 8, 2024 09:56
@sakertooth sakertooth marked this pull request as ready for review December 8, 2024 21:21
@sakertooth sakertooth marked this pull request as draft February 2, 2025 18:47
@sakertooth
Remove concepts TODO comment
31043c4
@sakertooth
Remove calls to movePosition(0)
5eb1283 
Already called when constructing clips
@sakertooth
Move track dependent initialization to onAddedToTrack
719b57f
@sakertooth
Manage ownership of clips using std::unique_ptr's
7f5875a
@sakertooth
Make sure to add clip when created
8549bee
@sakertooth
Fix crash when deleting clips
b590ccf
@sakertooth
Simplify repeated actions of adding and creating a new clip
77aea52
@sakertooth
Rename addTrack to addNewTrack
9b78d49
@sakertooth
Manage ownership of tracks with std::unique_ptr's
59dca59
@sakertooth
Replace addSampleTrack, addAutomationTrack, and addPatternTrack with …
289b113 
…addNewTrack
@sakertooth
Prevent misuse by only allowing adding new clips to tracks
e094224
@sakertooth
Rename createTrack to addNewTrack
45ff4ae
@sakertooth
Remove destroyedClip signal
cd7041e
@sakertooth
Remvoe destroyedTrack signal
a53fd94
@sakertooth
Remove forward declartion of Clip
2496637
@sakertooth
Remove updateAfterTrackAdd
b6beb52
@sakertooth
Replace signal slot connection for updating pattern store when adding…
8f49a88 
… pattern tracks with observer function
@sakertooth
Make defining onAddedToTrack optional
4b2328d
@sakertooth
Fix segfaults when deleting clips and tracks
eb4fa7c 
If you simply erase the unique_ptr from the vector on removal, then when we reach the destructor of the derived track, the unique_ptr will be empty, and any attempt to read the array of tracks in the parent track container will result in a track being read that is nullptr. The same can be said for clips.

To fix this, we need to first make the unique_ptr release ownership of the object, remove the empty unique_ptr from the array, and then delete the object.
@sakertooth
Fix deadlocks when removing tracks
b59b242 
Removed m_tracksMutex as it was far too error prone, and we already have requestChangeInModel/doneChangeInModel (which should be the only lock used IMO, that way we only have to worry about removing one lock for real-time safety).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rossmaxx Rossmaxx Rossmaxx left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
needs testing This pull request needs more testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sakertooth @Rossmaxx