Knife Tool for Sample Clips

[Spekular]

This PR builds off of #4973, so that one needs to be reviewed and merged first.

Needs to be updated with all the changes from #4973 before merge, somehow.

Mergeable once again!

Adds a knife tool for splitting sample clips, based on BaraMGB's work.

Screeshot shows, from left to right: Unquantized cut, quantized cut before releasing mouse, cut quantized to clip boundaries (in this case, two bars left of the clip's end)

TODO:

• Cursor for knife mode
• Undo correctly
• Short hang when cutting playing clip, according to @PhysSong. Still present in latest commit?

Usage:

• Select knife mode.
• Click and hold on a sample clip. A marker appears showing where the split will occur.
• The mouse can be dragged around. Split position is quantized to song editor Q by default
• Holding ctrl or alt disables quantization.
• Holding shift snaps the cut position to a quantized offset from the clip's left or right edge, whichever is closer to the cursor. This means at least one of the new clips after the cut will have a quantized length (both, if the original length is a multiple of the current snap size).
• On release, the clip will be split, unless the mouse was moved off to either side (or is exactly at the start/end position).

[Gabrielxd195]

friend, this also serves for tco or piano roll clips ?. is that this tool looks great.

[Spekular]

This currently only works for sample clips.

[Spekular]

I'd love it if git didn't completely destroy everything every time I tried to rebase. Fetched and pulled on snap branch, checked out knife branch, fetched and pulled again, "rebase spek SESnap", everything explodes.

[PhysSong]

I can help you with rebasing commits.

[Spekular]

It seems to have fixed itself once I fixed the merge "conflict" that there was an extra newline somewhere. For a while it was displaying changes as if I'd touched tens of files, even though the "changes" matched master exactly. I may have been trigger happy in my complaining, since last time was a huge timesink.

[PhysSong]

I tried rebasing commits. Surprisingly, there were only 2 commits after rebasing. If you want a clean branch, I can push the rebased one.

[Spekular]

Feel free to do so!

…

On Sat, Jul 27, 2019, 15:45 Hyunjin Song ***@***.***> wrote: I tried rebasing commits. Surprisingly, there were only 2 commits after rebasing. If you want a clean branch, I can push the rebased one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4998?email_source=notifications&email_token=ACEBLGRLI2XI5T7IHPJMOH3QBRGNZA5CNFSM4HQTK6X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD26LXQI#issuecomment-515685313>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACEBLGXJAN4BI6QMVSQFNCTQBRGNZANCNFSM4HQTK6XQ> .

[PhysSong]

Rebased. You can pull the branch with

git checkout Knife
git fetch spek Knife
git reset hard spek/Knife

[Spekular]

Great! Lots of refactoring coming soon, and a shift mode for this as well, that snaps the length of the new left/right clip. (EDIT: The issue description below this is incorrect.) Just one tiny issue to fix: Cuts end up one pixel to the left of where they should be at 200% zoom and less. Might have to do with the fact that the patterns are drawn one pixel to the left of the bar lines. One fix is to add a pixel to the calculations and see if it helps. Another is to do the quantization twice, once for drawing and once for cutting, but this results in essentially duplicate code. Therefore I'm trying the pixel solution first, even though it feels hacky.

[Spekular]

Fixing that "tiny issue" took a while. You'd think that the result from knifeMarkerPos( me ) could be converted back to a MidiTime and used for the actual cut itself, but for some reason the shift/default behavior ends up inverted if you do. Adding a boolean switch to flip the two still left some weirdness, so I moved the code that decides the position back into mouseReleaseEvent. After a ton of trial and error I think it's working, but I can't say I understand why it has to be the way it is.

[Spekular]

With the icon added, I don't think there's anything left to add to this. Ready for code review and testing!

[LostRobotMusic]

@Spekular If you right click while holding left click with the knife tool, the black marker line thing stays behind (even after switching tools). If you move the TCO with the black line left behind, then the mark will stay there permanently.

[Spekular]

Good catch. I can probably just add a call to disable the marker in the RMB handling, will check when I get the time.

…

On Sun, Jul 28, 2019, 22:52 Lost Robot ***@***.***> wrote: @Spekular <https://github.com/Spekular> If you right click while holding left click with the knife tool, the black marker line thing stays behind (even after switching tools). If you move the TCO with the black line left behind, then the mark will stay there permanently. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4998?email_source=notifications&email_token=ACEBLGSUF4TY3KCU3HZKGDTQBYBHZA5CNFSM4HQTK6X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD27GKOI#issuecomment-515794233>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACEBLGU3KSGGPSGMUTGV6OLQBYBHZANCNFSM4HQTK6XQ> .

[BaraMGB]

There are a lot of spaces in braces. As far as I know, we decided to use the current coding convention if we write new code. Perhaps you look into it. I think I have not barked at every single point.

[BaraMGB]

Let's remove the spaces inside the brackets in new code.

[Spekular]

I thought policy was to match surrounding code until an autoformatter fixes this stuff.

[PhysSong]

match surrounding code

setIsPlaying() doesn't have spaces in the parentheses, but setSampleBuffer() does. Neither way will match the surrounding code perfectly.

[Spekular]

The vast majority of the file uses spaces, but fair point.

[Spekular]

@PhysSong I can't reproduce the hang. Does it still occur on the latest commt?

[RebeccaDeField]

I have found that both the default and classic them use odd sizes for the cursor which are different from each other. I will start with the knife tool cursor icons tomorrow with 32x32 and we can scale down to 16x16 if it seems too large. Might want to update the default cursor to match so that the size doesn't significantly jump and if it is more widely supported that may be an overall improvement.

[RebeccaDeField]

A few options from what I have mocked up so far. The first one needs some final tweaking to get it just right.

I lean towards the razor icon because I am used to it from other programs. Scissors might look more obvious. Thoughts?

[Spekular]

Between these I prefer the razor as well, but in general I'm partial to a knife/scalpel metaphor. One of my concerns with scissors is their association with cut as in cut/copy/paste. I've added a screenshot to the PR description where you can see what the icon for the button looks like, apologies for not having this up before :(

[RebeccaDeField]

@Spekular no problem, my mistake for not checking the current icon.

One of my concerns with scissors is their association with cut as in cut/copy/paste.

Good point, I agree.

Between these I prefer the razor as well, but in general, I'm partial to a knife/scalpel metaphor.

I like this concept. I can refine the existing icon as well. Should the cursor match this icon, just in a 32x32 size w/ border?

[Spekular]

I can refine the existing icon as well.

Feel free to, here's the .svg file.

Should the cursor match this icon, just in a 32x32 size w/ border?

Matching the orientation might look odd; the button icon has to aim 45 degrees down and left to match the pencil, whereas cursors usually range between pointing up/left and pointing straight up. Of course, if it doesn't look weird or out of place with the other cursors, I suppose it's not an issue.

Otherwise I think it's probably a good idea for them to match. I assume you're referring to the black outline/stroke around most cursors when you say "border"? My first thought was a square frame around the icon so I'm double checking, haha.

[RebeccaDeField]

Matching the orientation might look odd; the button icon has to aim 45 degrees down and left to match the pencil, whereas cursors usually range between pointing up/left and pointing straight up. Of course, if it doesn't look weird or out of place with the other cursors, I suppose it's not an issue.

I think this is a great idea.

Otherwise I think it's probably a good idea for them to match. I assume you're referring to the black outline/stroke around most cursors when you say "border"?

Yep 👍

Okay, I have created a slightly tweaked version for hopefully added crispness (.5 aligned slanted shapes tend to stairstep better) and a cursor version. For the cursor, I took out the slant on the tip of the blade so that it's a little more precise and matching a regular cursor expectation. I can leave the icon version with the slant as it does help metaphorically. Or we can make this straight as well for consistency.

@Spekular

[Spekular]

Looks awesome! I'm a fan of straight cursor + slanted button, so let's go with that unless you disagree? Once I have .pngs for the cursor and tweaked button I'll try to get the cursor implemented as soon as I can. (I might wait for #5489, though)

[RebeccaDeField]

It's a little tricky to make these crisp due to the number of slanted lines. I'm attaching the best results for the icon I could get. Slanted lines should translate better at half pixel but I don't see much of a difference in this case.

knifeicons.zip

If the icon master himself (@Umcaruje) has time to look at this and notices any way to crisp it up any further...

But otherwise, let's just make sure to use a cairo export.

[PhysSong]

LMMS sometimes crashes on undoing cuts.

lmms/src/tracks/SampleTrack.cpp

Lines 646 to 651 in 5cee7b2

	for( int i = 0; i < numOfTCOs(); ++i )
	{
	TrackContentObject * tco = getTCO( i );
	SampleTCO * sTco = dynamic_cast<SampleTCO*>( tco );
	if( _start >= sTco->startPosition() && _start < sTco->endPosition() )

It seems like getTCO(i) sometimes return null due to some multithread-unsafe code, and we don't check the return value.

The hang was fixed by not using copy() and paste(), but it caused an issue. SampleTCO and SamplePlayHandle assumes that no two TCOs share the same SampleBuffer instance, but your code breaks the assumption now. This makes the first TCO doesn't stop playing when it should.
I think the best solution is to copy the sample buffer without reloading the audio file, but it looks quite tricky with the current design of it.

[Spekular]

It seems like getTCO(i) sometimes return null due to some multithread-unsafe code, and we don't check the return value

Does this multithread unsafeness need to be fixed in the areas I've touched, or elsewhere? Also, that loop never references i except to get TCOs, would replacing it with a range-based for loop help at all?

The hang was fixed by not using copy() and paste(), but it caused an issue. SampleTCO and SamplePlayHandle assumes that no two TCOs share the same SampleBuffer instance, but your code breaks the assumption now. This makes the first TCO doesn't stop playing when it should.
I think the best solution is to copy the sample buffer without reloading the audio file, but it looks quite tricky with the current design of it.

Ok, so how do we move forward? I believe I tried using a copy constructor for the SampleBuffer, but the compiler complained about it not existing even though a default one should exist.

[PhysSong]

Does this multithread unsafeness need to be fixed in the areas I've touched, or elsewhere?

I think Track::loadSettings() is deleting TCOs in a thread-unsafe manner, I'll fix that on master.

Ok, so how do we move forward? I believe I tried using a copy constructor, but the compiler complained about it not existing even though a default one should exist.

There's no default copy constructor because the copy constructor of QReadWriteLock is deleted. Even if there's one, you shouldn't use it because it won't copy the memory block for samples.
You may use saveSettings() and loadSettings() from SerializingObject, but that will reintroduce the short hang. I'm okay with it because it's better than having a weird bug.

[Spekular]

@PhysSong how does this look?

master...Spekular:ROT

Still seems to work from a quick test, the first TCO stops playing when it should. If this is good then I should also add a copy assignment to SampleBuffer and SampleTCO.

[PhysSong]

That looks fine, but I think this will conflict with #4994. I want to hear @Reflexe's opinion for the final decision, but I'd say go ahead for now.