-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: master
Are you sure you want to change the base?
Conversation
…ying to remove notes from current clip
… new, removing the old.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now
@szeli1 mind reviewing this? |
I will review this soon |
There was a problem hiding this 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.
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
Co-authored-by: szeli1 <[email protected]>
There was a problem hiding this 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
Here are some things I noticed while testing it:
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. |
Co-authored-by: Dalton Messmer <[email protected]>
Co-authored-by: Dalton Messmer <[email protected]>
… normal splitting
Fixed. Now if the clip type does not support hard splitting, it defaults to soft splitting. |
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. |
@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(?) |
@regulus79 I tested yet again with an updated version of this PR and the bugs are gone, noice. |
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? |
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. |
In order to fix a bug where the right side of a split |
I have added the ability to toggle the auto resize state ( 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. |
…s, and edit Clip.h to make the code simpler
I have quickly reworked how the toggling of auto-resize works. Now the code resides in |
Makes the knife tool work for all types of clips, not just
SampleClip
s, 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, includingMidiClip
s andAutomationClip
s. 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
MidiClipView
s which deletes all notes outside of a resized clip's bounds.Changes
splitClip
function added toClipView
hardSplitClip
function added toClipView
. By default this function does nothing, but it can be implemented by a derived clip view class. Currently onlyMidiClipView
implements hard splitting.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 madeprotected
by messmerd's advice. Instead, outside classes must useClip::clone
to copy the clip.AutomationClipView
andMidiClipView
updated to support drawing resized clips.PianoRoll
andAutomationEditor
now have overlays to show where the clip bounds are.PianoRoll
limited to only play within the clip boundsm_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 toMidiClipView
as an option in the context menu, along with a function to operate on a vector of clips,bulkDiscardNotesOutOfBounds
.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 fromClipView
toMidiClipView
where it belongs.MidiClipView::mergeClips
updated to only merge visible notes.DetuningHelper
and its parentInlineAutomation
, which is used byNote
's copy constructor, instead of copying the detuning viasharedObject::ref
.Clip::updateLength
was added as a virtual method. Currently onlyAutomationClip
andMidiClip
implement it, but having it there makes the code much simpler.Bugs introduced
Bugs also in master
Old PR description
Changes
Most of the code is copied from
SampleClipView.cpp
'ssplitClip()
function, and then modified for each clip type.ClipView.cpp
to allow splitting more than justSampleClip
s, and to hold off on dragging clips that cannot be resized (MidiClip
s) when knife mode is enabled.PatternClip
s was trivial.AutomationClip
s andMidiClip
s was more involved, as becausesetStartTimeOffset()
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
InFixedAutomationClipView.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.BecauseFixedMidiClip
s 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.remove()
close()
at the end ofspltiClip()
forAutomationClipView.cpp
andMidiClipView.cpp
, so I hope that takes care of everything.