Fixes bug with pasting of TCOs (#5840)

[IanCaio]

Fixes second issue of #5840 . I'd like feedback on the approach, I didn't want to create an exception on the behavior of TimePos::quantize since it's consistent, it just happens that we are working with negative values and expected them to be quantized the same way as positive ones. This makes a minor change to the quantize and moves most of the fix to the pasteSelection method.

TimePos::quantize works for negative values, but ends up snapping the TCO to the opposite direction. This is because the snapping down happens in the direction of the origin, which is left for positive values and right for negative values. The snapping up (when we are halfway through the interval) also happens in the opposite direction, away from the origin or right for positive values and left for negative ones:

That wasn't accounted for in the pasteSelection method and we ended up with wrong positions when pasting before the TCO(s) we copied. This PR attempts a fix by doing the following:

• Changing TimePos::quantize to have a boolean parameter that will define whether it snaps the position up if we are halfway through the interval or not (this removes the need to use that trick of subtracting TimePos::ticksPerBar() * snapSize / 2 from the offset to try to "bias" it.
• Checking if we need quantizing at all by checking if the offset falls outsize the snap grid.
• If we do need quantizing and the offset is negative we subtract one snap, since the quantization will move the TCO to the opposite end (to the right). Then we call quantize.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.52%2Bged45d65-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.52%2Bged45d65e3-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.52%2Bged45d65e3-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://11626-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.52%2Bged45d65-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11626?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://11627-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.52%2Bged45d65e3-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11627?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11628-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.52%2Bged45d65e3-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11628?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/goef72vby99x24n5/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36831754"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/rbiosgxowofrwylt/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36831754"}]}, "commit_sha": "971b14c83d8da141003eb71aa93ca219ba951f83"}

[Spekular]

As the original author: If (-0.9).quantize(1) doesn't give -1 then the quantize function isn't working as intended, and I think that should be fixed rather than introducing workarounds or a snap parameter.

When the latter is needed one could just write pos -= (pos % interval), which is no less readable than pos quantize(1, false).

[IanCaio]

As the original author: If (-0.9).quantize(1) doesn't give -1 then the quantize function isn't working as intended, and I think that should be fixed rather than introducing workarounds or a snap parameter.

When the latter is needed one could just write pos -= (pos % interval), which is no less readable than pos quantize(1, false).

(-0.9).quantize(1) will give -1. The issue is that (-0.5).quantize(1) will give -1 while (0.5).quantize(1) will give 1, meaning that one is snapping down and the other one is snapping up. This breaks the biasing code because it assumes that an offset of 1/2 the interval will snap up, but the quantization of negative values works like a mirror from the positive quantization.

I tried to fix it without changing the logic of TimePos::quantize because I don't think it's necessarily wrong, and added the snap parameter because I thought it could be useful to avoid the biasing trick. But as I mentioned on the issue another solution could be to account for that -1/2 interval situation and forcing it to snap up instead of down for negative values too.

[Spekular]

Huh. Assuming (-0.1).quantize(1) also works as intended and gives 0, I'm not sure I see how the logic breaks. Either way perhaps it's better to just replace the bias + quantize code with a floor towards negative then, since really that's the intended behavior.

[IanCaio]

Situation 1:

snapSize is 1 bar (192 ticks). offset is 3 bars (576 ticks). Bias subtracts half a bar from it, now offset is 480 ticks. TimePos::quantize will realize that the position is 2 bars and a half, snapUp will be one, so bar 3 will be returned. All fine.

Situation 2:

snapSize is 1 bar (192 ticks). offset is -3 bars (-576 ticks). Bias subtracts half a bar from it, now offset is -672 ticks. TimePos::quantize will realize that the position is -3 bars and a half, snapUp will be minus one, so bar -4 will be returned. The snapUp is working as intended but in a mirrored way.

The illustration on the PR message illustrates it well, the quantization works fine. It's just that when we are exactly in the middle of the interval, the logic is that we snap to the next one. Problem is the next one on the positive side is to the right and the next one on the negative side is to the left. The biasing code depends on that behavior and assumes that it will snap to the right. That's why I wanted to keep the quantize logic kind of intact, it doesn't look wrong to me (the snap parameter is more of an convenience, but I can remove it, specially because if you don't look from the illustration perspective the snap = false behavior for negative values will look weird).

The alternative I see is having int snapUp = offset == - (interval / 2) ? 0 : offset / (interval / 2); on TimePos::quantize which will just change the behavior from (-1/2x).quantize(x) to snap to the right, independent of whether it's negative or positive.

[IanCaio]

@Spekular I used the alternative approach I mentioned in the last commit. I think it might be better since it's requires less change to the code. The only drawback, which in practice is not a problem at all, is that quantize behavior is not entirely symmetrical (when we are halfway through the interval we always snap to the right). After writing it I think I prefer this over the old one.

[PhysSong]

What should the following calls return in an ideal implementation?

• (-0.9).quantize(1)
• (-0.5).quantize(1)
• (-0.1).quantize(1)
• (+0.1).quantize(1)
• (+0.5).quantize(1)
• (+0.9).quantize(1)

[IanCaio]

What should the following calls return in an ideal implementation?

I think it always should snap to the closes quantized distance even on negative values (like the timeline extended backwards). It actually already does that on master. The last commit just changes the snapping direction when we have (-x/2).quantize(x) so it snaps to the right.

• (-0.9).quantize(1) -> -1 (On master: -1)
• (-0.5).quantize(1) -> 0 (On master: -1) *
• (-0.1).quantize(1) -> 0 (On master: 0)
• (+0.1).quantize(1) -> 0 (On master: 0)
• (+0.5).quantize(1) -> 1 (On master: 1)
• (+0.9).quantize(1) -> 1 (On master: 1)

*The paste method assumed this would snap to the right instead of to the "next" quantized position, which walking to the negative side is to the left

[Spekular]

What should the following calls return in an ideal implementation? [...]

I think the behavior as of the latest commit is good. Negative inputs don't make any sense for positions, so this should only affect deltas, like moving or resizing. In those cases I can't see any downsides to the new behavior, and it should fix the issue found in #5840.

[PhysSong]

Merge?