Song Editor - Pasting tco on first position

[zonkmachine]

If you try and paste something to the first bar in the Song Editor it ends up on the second bar. This is a glitch that has been introduced after lmms-1.2 .

Affected LMMS versions

master

[PhysSong]

Seems like a duplicate of #5393. If it's still not fixed on the latest master in #5401, @Spekular will have to investigate.

[zonkmachine]

Thanks! A duplicate it is. Closing.

[zonkmachine]

Sorry... Those were closed already. Sticking to this one then.

[Spekular]

Can't reproduce on my development copy on linux, should I pull an even newer copy to test?

[Spekular]

Pulled latest master, still can't reproduce. Steps? OS?

[zonkmachine]

He, he. A bit of a mystery then. I think I will have to do a bisect down to where it happens for me on master then.

OS?

I'm now on Ubuntu/Mate 20.10
Version 1.3.0-alpha.1.33+g4f74151 (Linux/x86_64, Qt 5.6.3, GCC 5.4.0 20160609).

Steps?

I notice now that this bug is only if you copy a pattern that starts on the first bar to a second instruments first bar so the steps to test replicate this is:

• Default project, clone the triple oscillator
• Add a note to the first instrument on the first bar
• Copy that tco and paste it on the second instrument, first bar

[IanCaio]

@zonkmachine That's a weird one. If you create the TCO on the second bar of the first track and copy/paste on the first bar of the second track it works fine. So it seems to be a requirement for the bug to happen that the TCO being copied is at the first bar..

[Spekular]

Ok, I know what's happening. Any time you copy from one bar to the same bar, the clip will be offset to avoid landing on itself. Looks like the logic doesn't account for the fact that you're pasting on another track.

[Spekular]

Unfortunately I'm lost in the new copy/paste code and can't find where the offset happens anymore. @IanCaio you did the clipboard updates, right?

[IanCaio]

Unfortunately I'm lost in the new copy/paste code and can't find where the offset happens anymore. @IanCaio you did the clipboard updates, right?

Yes, if I'm not mistaken the offset calculation starts here:

lmms/src/gui/widgets/TrackContentWidget.cpp

Lines 478 to 482 in 3d7d005

	TimePos offset = TimePos(tcoPos - grabbedTCOPos);
	// Users expect clips to "fall" backwards, so bias the offset
	offset = offset - TimePos::ticksPerBar() * snapSize / 2;
	// The offset is quantized (rather than the positions) to preserve fine adjustments
	offset = offset.quantize(snapSize);

And the problem might be here:

lmms/src/gui/widgets/TrackContentWidget.cpp

Line 511 in 3d7d005

if (offset == 0) { pos += shift; }

[Spekular]

PR opened :)

[zonkmachine]

On track 1, if I copy the object on bar 9 and paste it on the object on bar 5 I end up with the situation on track 2

[zonkmachine]

This left shift is decided in SongEditor::getSnapSize() and is relative to the Quantization setting. I don't think this is working well.

[IanCaio]

On track 1, if I copy the object on bar 9 and paste it on the object on bar 5 I end up with the situation on track 2

Just a note, I think that's unrelated to the first issue that Spekular submitted a fix to. I'm trying to figure out what it could be, probably some error while calculating the offset. Doing some debugging here.

[zonkmachine]

Just a note, I think that's unrelated to the first issue that Spekular submitted a fix to.

I think so too.

You right click for the context menu and then left click to choose paste. Is this right click/left click combination passed on to the pattern underneath as a delete command?

[IanCaio]

I think I found the issue:

lmms/src/gui/widgets/TrackContentWidget.cpp

Lines 479 to 482 in 3d7d005

	// Users expect clips to "fall" backwards, so bias the offset
	offset = offset - TimePos::ticksPerBar() * snapSize / 2;
	// The offset is quantized (rather than the positions) to preserve fine adjustments
	offset = offset.quantize(snapSize);

Line 480 subtracts half the snapsize from the offset. The idea is that if the snapsize is 1 bar for example, and you click on the second half of the bar, you'll still paste the TCO on that bar instead of the next one. The logic is as follows:

• If you clicked on the position 0 from the bar, subtracting half a bar will leave you on the half of the previous bar. Quantizing the position will snap up to the original bar.
• If you clicked between the position 0 from the bar and 1/2 of it, subtracting half a bar will leave you between the second half of the previous bar and 0. Quantizing will snap up to the original bar.
• If you clicked between 1/2 the bar and just before the next bar, subtracting half a bar will leave you between position 0 and just before 1/2 the bar. Quantizing will snap down to the original bar.

That works fine if the offset is positive, so clicking on paste in a TCO after the one you copied works fine. When the offset is negative on the other hand (the TCO is before the one being copied), TimePos::quantize will receive a negative value. Let's say we are pasting on a TCO exactly 1 bar behind the one copied:

• offset will be -192 ticks. Subtracting half the snapsize of 1 bar we have offset = -192 - (192/2), or offset = -288
• Then TimePos::quantize is called:

lmms/src/core/TimePos.cpp

Lines 66 to 78 in 3d7d005

	TimePos TimePos::quantize(float bars) const
	{
	//The intervals we should snap to, our new position should be a factor of this
	int interval = s_ticksPerBar * bars;
	//The lower position we could snap to
	int lowPos = m_ticks / interval;
	//Offset from the lower position
	int offset = m_ticks % interval;
	//1 if we should snap up, 0 if we shouldn't
	int snapUp = offset / (interval / 2);
	return (lowPos + snapUp) * interval;
	}

• interval is 192 ticks, lowPos is -288 / 192, or -1. offset (from quantize, not the previous offset) will be -288 % 192, or -96. Finally, snapUp will be -96 / (192 / 2), which is -1. What happens is that instead of snapping up we will end up snapping down because:
• (lowPos + snapUp) * interval will be (-1 + (-1)) * 192 or -384, which is the offset to the previous bar.

TimePos::quantize was probably originally thought to only work with positive values (as can be seen from the expected values of 1 or 0 on snapUp). When we use negative values, it works reversed. We don't notice it pasting on the timeline because we rarely click exactly on the position 0 of the bar, so snapUp is 0 and we don't notice that behavior of snapping down. Sorry if it all sounds confusing.

I'd still have to think what is a good fix for that.

[IanCaio]

I'm not sure changing TimePos::quantize is the way to go. We could make it so if offset == -(interval / 2) snapUp will be 0 with something like int snapUp = offset == (-interval/2) ? 0 : offset / (interval / 2);. That would probably fix the issue, because now negative positions only snap down when they are below half the interval. But honestly, quantize is just using the same rule for both the positive and negative sides:

Should we make an exception for the negative side and instead of snapping on the range (-1, -1/2] snap on the range (-1, -1/2)?

[IanCaio]

Posted a possible fix for the second issue. The first one will still happen (pasting on the same position in a separate track) because it's a separate fix.

Another noticed behavior which is more a misbehavior than a bug, but still something we should change later: If we copy a TCO, delete it and try to paste on the same position it was before it will snap up, because it will still consider we can't copy on the same position we had before, even though the TCO was removed.