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.

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

Add splitting (and resizing) to all types of clips #7477

Open
wants to merge 112 commits into
base: master
Choose a base branch
from

Conversation

regulus79
Copy link
Contributor

@regulus79 regulus79 commented Aug 31, 2024

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.

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.

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

src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/PatternClipView.cpp Outdated Show resolved Hide resolved
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.

Looks good now

@Rossmaxx
Copy link
Contributor

@szeli1 mind reviewing this?

@szeli1
Copy link
Contributor

szeli1 commented Aug 31, 2024

@szeli1 mind reviewing this?

I will review this soon

Copy link
Contributor

@szeli1 szeli1 left a comment

Choose a reason for hiding this comment

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

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.

src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/ClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/ClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks before I test it

src/gui/clips/AutomationClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/PatternClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/PatternClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/PatternClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/SampleClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
@messmerd
Copy link
Member

messmerd commented Sep 4, 2024

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
Copy link
Contributor Author

Trying to hard cut an automation clip leaves the marker in place instead of doing a soft cut.

Fixed. Now if the clip type does not support hard splitting, it defaults to soft splitting.

@regulus79
Copy link
Contributor Author

At some point in time I accidentally broke hard splitting for midi clips which have been resized from the left, but it's fixed now.

@regulus79
Copy link
Contributor Author

regulus79 commented Feb 10, 2025

@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
Copy link

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

@regulus79
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants