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 to all types of clips #7477

Open
wants to merge 38 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.

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.
  • 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.
  • 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 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

I think I have fixed most of the bugs, however I don't understand how journalling works enough to fix the bug where the new clips don't disappear after undoing. I will continue to look into it, but I may need some help.

Also, I decided to remove the restriction on midi clips needing to be split on the bar, and instead implemented your suggestion on letting the left clip's length be longer and extend behind the right clip. So now they can be split at arbitrary positions!

@regulus79
Copy link
Contributor Author

regulus79 commented Sep 5, 2024

Ah, nevermind, it seems that using close() instead of remove() makes it work! I will have to look into why that happens.

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.

A quick review before I test it again

src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
src/gui/clips/PatternClipView.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
include/ClipView.h Show resolved Hide resolved
@messmerd
Copy link
Member

@regulus79 It seems that when I split a midi clip, the original singular clip still remains though it is invisible. You can hear it when the song plays since it plays twice as loud as before the clip was split, and if you clone the track, it becomes visible in the cloned track.

@regulus79
Copy link
Contributor Author

Okay that's an interesting bug... I think it may have to do with me using close() instead of remove() when removing the old clip. But that's odd, since it doesn't appear to happen for AutomationClips which also use close() (???)

After some testing, it appears that using remove() solves the issue. I also think I found the cause of the old problem when using remove(), which is that I was calling it after m_clip->getTrack()->restoreJournallingState();. Again, I'm not an expert in Journalling, but calling it before appears to fix the problem.

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.

Midi clip splitting seems to work fine now. I wasn't able to find any problems with it while testing.

Pattern clip splitting also seems fine.

However, for automation clips, if you make the split directly on a node with modified tangents or with an outValue, it doesn't split correctly.

Comment on lines +398 to +401
if (m_marker)
{
p.drawLine(m_markerPos, rect().bottom(), m_markerPos, rect().top());
}
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? I'm not sure why it's in the double click event

Comment on lines +677 to +680
if (m_marker)
{
p.drawLine(m_markerPos, rect().bottom(), m_markerPos, rect().top());
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines +162 to +165
if (m_marker)
{
p.drawLine(m_markerPos, rect().bottom(), m_markerPos, rect().top());
}
Copy link
Member

Choose a reason for hiding this comment

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

And here too. SampleClipView doesn't have this.

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.

4 participants