Don't copy DetuningHelper in Note copy constructor

[regulus79]

TLDR This PR reverts some changes in #7477 and reworks how note detuning copying works so as not to perform a clip duplication and allocation by default in the constructors.

---

In #7477, I reworked how Notes are copied to make their detuning automation clip be deep-copied. This fixed some issues where copying a detuned note or splitting a clip with detuned notes would cause the notes to be linked to each other, where editing the detuning of one would edit the detuning of another.

However, this meant that Note's constructors now always performed an allocation and interacted with ProjectJournal::allocID, as @PhysSong pointed out in #7884. Normally, this might be okay if it were only called for gui operations. However, it turns out that it's also called in NotePlayHandle's constructor, which is problematic, as that is (afaik?) on the audio thread.

Ideally, we could maybe have two kinds of constructors, one which deep copies the automation clip, and one which just takes in the pointer. The former would be used for gui operations which need to actually make a new note, and the latter would be for temporary note copies which are not going to be modified, like in NotePlayHandle.

My current implementation simply makes Note's constructors take in a pointer for the DetuningHelper, and they don't do any kind of deep copy. Then, I added a Note::clone function which, analogous to Clip::clone, makes a full copy of the note and detuning. Then, I replaced all necessary occurrences of Note(*note) which needed to make deep copies to use note->clone() instead. This way the allocation only occurs when explicitly necessary.

I'm not 100% satisfied with my implementation. The way there are two ways to copy a note seems odd, and I removed the unique_ptr functionality, which might not be great. I would love to hear your opinions. But, I've tested it for about an hour on the project from #7884, and it hasn't crashed yet, so I think it works.

[regulus79]

I just realized I forgot about deleting the DetuningHelper and who owns the pointer, now that notes can be shallow-copied and detuning clips be shared.

So, I decided to make the DetuningHelper stored as a shared_ptr.

[regulus79]

At the request of @Rossmaxx, I have removed shared_object.h, as it is no longer used in the codebase.

[Rossmaxx]

Thanks for applying my suggestion. The code looks good. Not approving as I didn't test.

I feel like you missed something while removing shared object usage.

You need to delete the inheritance of shared_object in either SerializingObject or DetuningHelper

[regulus79]

You need to delete the inheritance of shared_object in either SerializingObject or DetuningHelper

That was already done for DetuningHelper in #7477. As far as I can tell, I have removed all references to it.

[Rossmaxx]

Didn't check the other PR. Now that you did, I checked it. Good job

[messmerd]

Why are you using shared pointers again rather than unique pointers? Would that reintroduce the bug where copying clips doesn't do a deep copy of per-note automations?

[regulus79]

Normally yes, but I have added a new clone function which performs a deep copy, and that one is used when adding notes to midi clips, so it doesn't reintroduce the bug.

Honestly I could have done it either way, either making the constructor by default do a deep copy while having another function do a shallow copy, or how I have it right now, where the constructor does a shallow copy, while the clone function does a deep copy. Or maybe I could add an extra parameter to the constructor to determine whether it does a deep copy?

The main thing is that NotePlayHandle derives from Note, so it has to use a Note constructor in its initialization. But we don't want to do a deep copy when creating a NotePlayHandle, since that was causing crashes.

[messmerd]

I really don't like the use of std::shared_ptr over std::unique_ptr, but it's more important to fix this bug than refactor this. And it isn't worse than the situation before 7477.

A refactor should probably try to remove the dynamic allocations altogether. DetuningHelper and NotePlayHandle's buffer are allocated on the heap each time a new note plays from a NotePlayHandle-based instrument, despite having a (flawed) memory pool implementation in use when allocating the NotePlayHandle objects. I will probably attempt this after #7459 is merged since the problem is related to pin connector support for NotePlayHandle-based instruments.

[regulus79]

Would it be better if NotePlayHandle didn't inherit from Note, but instead maybe stored a pointer to its note? That way it wouldn't have to do any copying of any DetuningHelper, deep or shallow.

[messmerd]

Would it be better if NotePlayHandle didn't inherit from Note, but instead maybe stored a pointer to its note? That way it wouldn't have to do any copying of any DetuningHelper, deep or shallow.

I think it would be better as a data member of NotePlayHandle, though looking at the way it is used (by searching for calls to NotePlayHandleManager::acquire), it appears the note is frequently a temporary which is passed to the acquire method, so if you stored a pointer to it, it would become dangling.

The exception is InstrumentTrack::play where the Note is stored somewhere else rather than being created on the spot. This is probably the reason why shared ownership was used.

[regulus79]

it appears the note is frequently a temporary which is passed to the acquire method, so if you stored a pointer to it, it would become dangling.

Maybe if we store it as a shared_ptr member variable in NotePlayHandle? That way if the Note is just temporary, it will be deleted when the NotePlayHandle is deleted, and if it's not temporary, it will only be deleted when it's ready to be deleted?

[messmerd]

Maybe if we store it as a shared_ptr member variable in NotePlayHandle? That way if the Note is just temporary, it will be deleted when the NotePlayHandle is deleted, and if it's not temporary, it will only be deleted when it's ready to be deleted?

No that wouldn't work because Note is on the stack not the heap. You can't assign a temporary to shared_ptr - you'd need to create a copy on the heap.

[AW1534]

Tested with the included project from #7884, works fine. Gonna keep it on loop for a while, but before the PR, it would crash at the EXACT same position, and it's been about 20 loops now.

[messmerd]

Haven't tested it, but it looks good to me code-wise. Further refactoring could be done in a later PR.