Notes with detuning patterns share the same detuning when splitted

[IanCaio]

Bug Summary

Reported by @SeleDreams . When using the Piano Roll knife tool in a note that has a detuning pattern, the newly created note will share the same detuning pattern as the previous note (as in changing the detuning in one will also change it equally in the other). It's probably desirable that each note has it's own independent detuning pattern, even if it's initially the same.

This issue is being opened as a reminder only, the reason behind it is known: The copy-constructor of Note adds the detuning of the copied note as a shared object. I didn't dive into changing the copy-constructor to make a deep copy of the detuning because the shared copy is done so explicitly that I'm thinking there might be a reason for it that I'm unaware of. I'll investigate first, and if I realize that the copy-constructor isn't used anywhere else that might require the detuning info to be shared the issue could be fixed by just making another instance of the detuning pattern for the copied note.

Another approach to fix it if the copy-constructor can't be messed with is not using it at all in the note split, but instead creating a new Note object and then just copying the information we need to it (position, length, velocity, key, etc).

Steps to reproduce

Split a note with detuning information.

Expected behavior

Each note has its own detuning pattern, even if they are initially equal.

Actual behavior

Notes share the same detuning pattern.

Screenshot

Affected LMMS versions

master branch (After merge of #5845)

[IanCaio]

Just confirmed the behavior is also the same when creating new notes by clicking a note with detuning using the "Shift + Drag". Same reason apparently (Copy constructor being used for new notes sharing the same detuning pattern object):

lmms/src/gui/editors/PianoRoll.cpp

Lines 1864 to 1871 in 00ac4f5

	// if they're holding shift, copy all selected notes
	if( ! is_new_note && me->modifiers() & Qt::ShiftModifier )
	{
	for (Note *note: selectedNotes)
	{
	Note *newNote = m_pattern->addNote(*note, false);
	newNote->setSelected(false);
	}

It did make me realize that Pattern.cpp uses the copy constructor on the addNote method:

lmms/src/tracks/Pattern.cpp

Lines 208 to 210 in 00ac4f5

	Note * Pattern::addNote( const Note & _new_note, const bool _quant_pos )
	{
	Note * new_note = new Note( _new_note );

I don't think it would be a problem for Pattern::addNote that the copy-constructor made a deep copy of the detuning pattern though. Just a note that this might have been where it was intended that the detuning object was shared.

[SeleDreams]

i think that the copy constructor should also make a copy of the detuning rather than just using a shared instance Le dim. 7 mars 2021 à 22:55, IanCaio <[email protected]> a écrit :

…

Just confirmed the behavior is also the same when creating new notes by clicking a note with detuning using the "Shift + Drag". Same reason apparently (Copy constructor being used for new notes sharing the same detuning pattern object): https://github.com/LMMS/lmms/blob/00ac4f58f50d67b79ad805ec11f1fb507962b2b6/src/gui/editors/PianoRoll.cpp#L1864-L1871 It did make me realize that *Pattern.cpp* uses the copy constructor on the addNote method: https://github.com/LMMS/lmms/blob/00ac4f58f50d67b79ad805ec11f1fb507962b2b6/src/tracks/Pattern.cpp#L208-L210 I don't think it would be a problem for Pattern::addNote that the copy-constructor made a deep copy of the detuning pattern though. Just a note that this might have been where it was intended that the detuning object was shared. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5940 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4UF4JXWZQLZ7MEUOF37CDTCPY65ANCNFSM4YYITZOQ> .

[IanCaio]

i think that the copy constructor should also make a copy of the detuning rather than just using a shared instance

I'm initially inclined to think the same. I just think it's worth to investigate it a little because it's not an implicit thing (as it would be if we had m_detuning(note.m_detuning)) but instead very explicit in the copy-constructor:

lmms/src/core/Note.cpp

Lines 76 to 79 in 00ac4f5

	if( note.m_detuning )
	{
	m_detuning = sharedObject::ref( note.m_detuning );
	}

So it could be this is some legacy thing left behind, but it could also be that some part of LMMS is expecting this to be shared. Think that's very unlikely, but just double-checking.

[SeleDreams]

something else that could be done would be to make it shared until the note gets edited by the user in this case it would separate to not affect the other note, a kind of ram saving feature

[Spekular]

something else that could be done would be to make it shared until the note gets edited by the user in this case it would separate to not affect the other note, a kind of ram saving feature

Copy on write. Unless it turns out that copying every time leads to RAM issues I don't think it's worth the added complexity, so let's make a build that copies and stress test it (I volunteer if no-one else gets to it before me). A quick test with duplicating automation clips implies that it could be non-negligible, but detune automations could be more lightweight.

[Spekular]

Ok, so my implementation is questionable, but here are some very crude test results:

16 notes stacked vertically * 16 notes per bar * 4 bars per clip * 18 clips = 18,432 notes
Two unique note detune automations, each with three points

No sharing, creating from scratch:

• Initial: < 50MiB
• Creation: 1.0GiB (crept up to 1.2 later)
• Deletion: 4.2GiB (stayed here even when I reloaded my default template)

No sharing, loading project:

• Initial: 34.4MiB
• Project Load: 370.7MiB
• Deletion: 3.9GiB

Master, creating from scratch:

• Initial: 35.4MiB
• Creation: 708.6MiB
• Deletion: 4.2GiB (stayed here even when I reloaded my default template)

Master, loading project:

• Initial: 34.2MiB
• Project Load: 366.1MiB
• Deletion: 3.8GiB

Sidenote: It kind of seems like selecting all the clips and deleting them raises memory use more than deleting them one at a time...

[Spekular]

A few notes:

• Using several clips because they're easier to clone was a mistake, clips don't share detune data even in master. I'll redo the test
• After saving and reloading a project, all note detune info is separate.

[Spekular]

Ok, this time I had 64 bars full of 16th notes stacked 16 high, so 16,384 notes all inside one clip. Still two detune patterns with 3 nodes each.

No sharing:

• Initial: 34.2MiB
• Created: 581.7MiB
• Deleted: 854.2MiB
• LMMS closed + reopened: 34.4MiB
• Project loaded: 328.8MiB
• Deleted: 497.6MiB

Sharing/current behavior:

• Initial: 34.6MiB
• Created: 390.6MiB
• Deleted: 582.1MiB
• LMMS closed + reopened: 34.5MiB
• Project loaded: 327.7MiB
• Deleted: 498.1MiB

This is testing with my very questionable implementation, so it's entirely possible that the extra RAM usage during note creation is due to an error on my part. Unless I'm missing something, it seems to me that RAM usage isn't a convincing reason to share the detune patterns.

[Spekular]

Here's my implementation for test purposes: https://github.com/Spekular/lmms/tree/noteDetune

If its decided that DetuneHelper shouldn't be shared, it'd probably be much better to remove the sharedObject stuff than to work around it like I did.

[regulus79]

Have we decided to go ahead with this?

[regulus79]

Could anyone explain how you all were planning to do this? I quickly implemented it in #7477 by adding a copy constructor to DetuningHelper and its parent InlineAutomation which just copies the FloatModel value, min, max, and step, and then copies the automation clip, clears the connected objects, and adds itself as the connected object. This doesn't feel very elegant to me, so I would be quite curious how you all wanted to fix this issue.