Add splitting (and resizing) to all types of clips

[regulus79]

Makes the knife tool work for all types of clips, not just SampleClips, and adds the ability to resize all types of clips from either side.

Originally, this PR simply added a splitClip function to all the other clip types, which were often quite complex in order to properly divide the notes or automation nodes into the resulting clips. However, due to imperfections in automation clip splitting, and the desire for all clips to be resizable in the future, this PR has expanded to add resizing support to all clip types, including MidiClips and AutomationClips. This allows a more perfect form of clip splitting done via duplicating and resizing.

As a compromise to those who may desire a destructive splitting of midi clips, using shift+knife on midi clips will perform destructive splitting, while the default use of the knife tool will do normal, nondestructive splitting. Note: Shift+kinfe used to perform quantized splitting relative to the clip start/end, but I have combined that functionality with the default knife functionality, so even without holding shift, the split pos will still be quantized either globally or relative to the clip start/end, whichever is closest.

Also, to accommodate workflows which may desire it, a new rightclick option was added to MidiClipViews which deletes all notes outside of a resized clip's bounds.

Changes

• virtual splitClip function added to ClipView
• virtual hardSplitClip function added to ClipView. By default this function does nothing, but it can be implemented by a derived clip view class. Currently only MidiClipView implements hard splitting.
• To aid with clip copying necessary for splitting, copy constructors were added to Clip and all derived clip classes which lacked one. This makes sure things like color, name, muted, etc, are preserved when a clip is split. These copy constructors were made protected by messmerd's advice. Instead, outside classes must use Clip::clone to copy the clip.
• AutomationClipView and MidiClipView updated to support drawing resized clips.
  • PianoRoll and AutomationEditor now have overlays to show where the clip bounds are.
  • The playhead in the PianoRoll limited to only play within the clip bounds
• m_hasBeenResized added to all clips to tell when to use automatic resizing or not.
• NotePlayHandles will get cut off if they extend past then end of a midi clip. Likewise, if they start before the start of a midi clip and extend past the left bound, they will still play.
• discardNotesOutOfBounds was added to MidiClipView as an option in the context menu, along with a function to operate on a vector of clips, bulkDiscardNotesOutOfBounds.
• An option to change the status of m_hasBeenResized, "Enable/Disable Auto-Resize", was added to the context menus of all clip types, along with support for bulk operation on a selection of clips.
• mergeClips was finally moved from ClipView to MidiClipView where it belongs.
• MidiClipView::mergeClips updated to only merge visible notes.
• Notes with detuning patterns share the same detuning when splitted #5940 was fixed(?) in the process of adding midi clip splitting. A copy constructor was added to DetuningHelper and its parent InlineAutomation, which is used by Note's copy constructor, instead of copying the detuning via sharedObject::ref.
• Clip::updateLength was added as a virtual method. Currently only AutomationClip and MidiClip implement it, but having it there makes the code much simpler.
• Minor: The split line color is now modifiable via css.

Bugs introduced

• When performing destructive splitting of a midi clip right across a note with detuning, the part of the note which ends up in the right clip has its detuning incorrectly positioned. However, this is quite difficult to solve in a nice manner, and it may have to wait until automation clips are reworked. Therefore, I think it is still reasonable to merge this PR in the meantime.

Bugs also in master

• Zooming while holding down the knife tool causes the split marker line position to move around.
• Knife tool doesn't work on selections of clips.

Old PR description

Changes

Most of the code is copied from SampleClipView.cpp's splitClip() function, and then modified for each clip type.

• A couple changes had to be made to ClipView.cpp to allow splitting more than just SampleClips, and to hold off on dragging clips that cannot be resized (MidiClips) when knife mode is enabled.
• Splitting PatternClips was trivial.
• Splitting AutomationClips and MidiClips was more involved, as because setStartTimeOffset() appears to do nothing, it required copying the correct nodes over and offsetting them back by the length of the left clip. However, the original clip still had all the notes, which meant that if the user changed some of them, its length would snap back to full. I had difficulty finding a good way to delete certain notes via a loop, so I decided to instead spawn two new clips, one left and one right, populate them with notes, and delete the original clip.

Notes

• In AutomationClipView.cpp, for some reason when splitting an automation clip, the new clips sometimes sets themselves to record mode. I added a temporary fix for this by setting the recording mode to the mode of the original clip. Fixed
• Because MidiClips are only drawn in multiples of 1 bar, splitting them between bars led to buggy graphics. Because of this, I forced the split position to be a multiple of a bar. Fixed
• I am not an expert in how to have objects properly remove themselves without memory issues. I called remove()close() at the end of spltiClip() for AutomationClipView.cpp and MidiClipView.cpp, so I hope that takes care of everything.

[Rossmaxx]

Style review. Do look at fixing the bracket spacing by looking at the diff.

[Rossmaxx]

Looks good now

[Rossmaxx]

@szeli1 mind reviewing this?

[szeli1]

@szeli1 mind reviewing this?

I will review this soon

[szeli1]

Please make the requested changes for all cases.
You should update this line inside ClipView:

//! Return true iff the clip could be split. Currently only implemented for samples
virtual bool splitClip( const TimePos pos ){ return false; };

And lastly I think using the remove() method is fine.

[messmerd]

Some minor nitpicks before I test it

[messmerd]

Here are some things I noticed while testing it:

• In Knife mode, the knife mouse icon doesn't appear when hovering over midi clips, automation clips, and pattern clips like it does for sample clips.
• Undo works incorrectly for midi clip and automation clip splits - the old singular clip returns, but the two split clips also remain.
• The snapping size is different for midi clips - I can't split into anything smaller than a full measure. Nevermind, midi clips don't work like that
• Double-clicking in Knife mode performs the same action as double-clicking in Draw mode. This problem already existed for sample tracks, but now also exists for other types of tracks too. Double-clicking shouldn't do anything special in Knife mode.
• When splitting midi clips, if the clip is cut down the middle of a note, the clip size of the resulting left clip is too small and hides the note that passed through the cut point. If the two new clips are ordered correctly, you should be able to make the right clip display over top of the left clip without needing to touch the size of the left clip.

Unrelated bug: In the Piano Roll, the mouse icon when in Pitch Bend mode can disappear if you click on a note, then close the note's automation editor window.

Other than the issues above, everything seems to work fine. I'm pretty excited about this feature and hope we can get it merged soon.

[regulus79]

@CorruptVoidSoul I cannot reproduce any of the issues you are encountering. Would you mind maybe sharing a screen recording of the bugs happening? Oh also, what commit are you on? I made some changes to the undo for destructive splitting, and I know there were some merge conflicts so the CI builds weren't going for a while(?)

[CorruptVoidSoul]

@regulus79 I tested yet again with an updated version of this PR and the bugs are gone, noice.

[regulus79]

Apparently, merging midi clips was not quite working when the notes overlapped the clip bounds. I have fixed that in the most recent commit.

Related question: Currently, merging midi clips results in a midi clip which has not yet been resized, so it's length is quantized to the nearest bar. Is this what we want, or should it be changed so that the length of the resulting clip is the min/max of the bounds of the merged clips?

[bratpeki]

I think the length of it should be the length that covers the distance from the start of the first merged clip to the end of the last merged clip.

As for setting that output clip to an HBR clip, I think you should, simply because it's the more inclusive option of the two in regards to length. An auto-resizing clip could overlap with neighboring clips, so it makes sense to just make it a HBR clip.

[regulus79]

In order to fix a bug where the right side of a split SampleClip was not getting properly initialized, I copy-pasted all the setup connections and stuff from the main SampleClip constructor to the copy constructor. This feels wrong since a lot of code is being copied, so I would love to hear your thoughts on it.

[regulus79]

I have added the ability to toggle the auto resize state (m_hasBeenResized) on selections of clips.

I have also found some a minor bugs which I can't figure out how to fix. When doing "Clear notes out of bounds" or "Enable/Disable Auto-Resize" on a selection of clips, if you try to undo, the undo goes clip by clip (or track by track), instead of all at once. However, it appears that changing the color on a selection of clips in does this too, so it's probably not a big deal.

[regulus79]

I have quickly reworked how the toggling of auto-resize works. Now the code resides in ClipView.h and works for all clip types (I know not all clip types do anything with regards to m_hasBeenResized, but in the future they might) I also made Clip::updateLength a virtual function which is implemented by the derived clip classes in order to make the code much simpler without dynamic casting to check if the clip length needs to be updated. Again, not all clips implement this function, but they could in the future.

[bratpeki]

As this PR is being wrapped up, I'd like to post my findings:

MIDI clip cutting issue

Cutting across a note with per-note automation can be done three ways:

1. Don't cut
2. Do a soft cut
3. Do a hard cut, give both notes the same automation (Currently implemented)
   • There was an issue of linked automation, as I understand it, but that's been fixed
     • Somewhat related: Notes with detuning patterns share the same detuning when splitted #5940

BB clips

#7559 should be tested and merged after this.

Existing issues discovered in PR

If issues to these don't exist, I'll open them.

• TextBox for resizing pops up at the end of the clip, even when it's out of bounds
• The knife marker moves when zooming
• When a sample track is loaded into a sample clip, it's only as big as the clip was. This is good. However, a context menu option such as "Expand clip to full sample length" would be great!
• It would be nice if a knife tool applied to a clip a selection was previously made would invalidate the selection and make the cut
• "Esc" to disable a knife cut would be good.
• Gray clips don't show the name of the clip. This would be beneficial, as you could keep the name as a TODO note as to what the clip should be (eg. main melody, bassline, etc).

Future plans

• BB note view (Add Beat Preview to PatternClipView #7559) [Heavily recommended]
• Add auto-resize capabilities to sample and BB clips [Heavily recommended]
• Resizing clips in bulk [Optional]
• Make it so selected clips are all cut together [Optional]

[bratpeki]

Future plans updated, this PR is already big as-is!

[sakertooth]

Why? If a clip has been resized, that is only information that you have in the current moment and isn't something you should serialize.

[regulus79]

If someone resizes a long clip to only contain a small segment and then saves the project, I figured it should remember that the clip has been resized and not try to automatically resize the clip if the user edits the notes/nodes/whatever inside the clip.

[sakertooth]

This feels wrong to me. If someone resizes a long clip to only contain a small segment and then saves the project, then the clip should stay put. The user edits the clip within the bounds (regardless of inside the application or in the file), then the clip's length shouldn't change. If it was outside the bounds, then the clip should resize to reflect that change. Then again, it is confusing to talk about bounds because you mentioned this was about the clip being edited in general.

Why are we trying to prevent resizing the clip from within the project file if the clip is somehow changed?

[sakertooth]

The clip being resized is not a property that you should need to serialize. It's not something like storing the color of the clip, or its length, or type, but more like storing if the length of the clip was changed at some point. Storing that information feels unnecessary.

[regulus79]

If it was outside the bounds, then the clip should resize to reflect that change.

I see your point, maybe when you drag a note, it could only change the bounds if the user drags the note outside of the clip bounds. That might be a good idea. We did kind of consider it, but I felt like it was very easy for a user to accidentally drag a note a tiny bit outside of the clip bounds which would cause the bounds to unintentionally resize and the user would have to go back and resize the clip back.

but more like storing if the length of the clip was changed at some point.

That is true, but I think it might be ok since the user probably wants the behavior of the clip to remain the same between sessions, and this value has to be stored in order to do that. Maybe storing it serialized isn't ideal? I'm not sure.

[sakertooth]

but I felt like it was very easy for a user to accidentally drag a note a tiny bit outside of the clip bounds which would cause the bounds to unintentionally resize and the user would have to go back and resize the clip back.

For MIDI clips, master automatically resizes the clip and gives it enough space for the note that was out of bounds, and resizes back once the note is in the original bounds. The problem seems to be that because nondestructive splitting of MIDI clips was added, we need some way to resize the clip manually (and also additionally store state in the project file?)

I would've been fine with destructive splitting and making gradual improvements over time, but that besides the point. If we want nondestructive splitting in this case (which I can definitely see being beneficial), then the reasonable behavior seems to be that the length of clip is capped at the position of the last note, but can be resized to be made shorter, which would limit the number of notes shown in the clip view.

So the auto resizing behavior you mentioned is fine and probably expected IMO, even if it was on accident. That should be what happens. That functionality shouldn't really conflict with the need to manually make the clip shorter. Though it could be argued how much the clip resizes, that shouldn't be a necessary detail here.

[sakertooth]

Can't say I am a fan of the PR, though I do appreciate the work put into it. The changes just seem way too extensive for the problem in question, not to mention the scope creep with adding resizing to all clip types as well.

In my mind, splitting should've worked the same way as sample clips, with the two clips being completely separate. The destructive and nondestructive splitting semantics feel... unusual. The user just wants to split the clip. The way we do right now is what this PR would consider destructive splitting. It may not be perfect in all cases (especially automation clips), but any imperfections could've been fixed later if there was enough reason to be concerned.

Auto resizing originally meant that (specifically in the pattern editor it seems) its length was automatically handled. What does it even mean for the user to be able to auto resize a clip?

Also, the polls to see if users would prefer destructive and nondestructive splitting... weren't effective IMO. To me, it seemed more like users wouldn't mind either way and not a hard yes or no to one or the other.

I can't review this PR and say I feel comfortable with the changes.

[sakertooth]

Update: Thought about the problem some more, and I will say that duplicating the clip and then resizing it is a viable way to implement splitting. The other destructive way that removes the data from the split clip could've also worked, but has clear downsides, with the only upside I see being that there is possibly less data is being copied but moved to the new clip.

As a compromise to those who may desire a destructive splitting of midi clips

I can't think of a situation where destructive splitting would work that nondestructive splitting doesn't. IMO, we should choose one way to split clips, not both. We should stick with non destructive splitting since it seems to work in more cases. This will simplify not only the interface in the code (i.e., developers do not have to worry about the semantics of hard splitting), but also simplify the interface for the user (i.e., so they don't ask the question, "how do I want to split this clip?" when using the application, which is an odd question).

The auto resizing behavior should stay the same. What needs to change is that clips can manually be made shorter on top of the auto resizing functionality that already happens with MIDI clips for example. If we go with the nondestructive splitting approach (which seems like the main idea here), then first we need to allow for all clips to be resized and make sure everything works in that regard. Then we can add nondestructive splitting. Doing both at the same time may complicate things, though this is mostly my opinion in the interest of keeping PRs small, focused, and reviewable.

[sakertooth]

Non destructive splitting should also have the added benefit of working the same way regardless of the clip type, so there shouldn't be any need for virtual functions (though, I guess you would still need a virtual clone function yeah)