Skip to content

Fix Vector Layer scale+shear transforms

Stuffins requested to merge stuffins/krita:remove-shearOrigin into master

This is one possible fix to an issue I found where Vector layers that have been both scaled and sheared with the free transform tool get the wrong transform

This fixes KisTransformWorker::transform and also removes KisTransformWorker::*shearOrigin that was added in 91bff15b. It's not necessary to have two translations there and seems to be adding confusing complexity. After removing it, I can't reproduce the original bug that that commit fixed.

In master, shearOrigin is used in the following ways:

  • by providing 0,0
  • by providing a shearOrigin, and then pushing another origin through the transform to find a translation to compensate, which works regardless of the original shearOrigin
  • by using shearOrigin without compensation. As far as I can tell, this was done in exactly one place: KisImage::shearImpl when resizeImage is false, in the UI it's Layer->Transform->Shear Layer... and Layer->Transform All Layers->Shear All Layers...

After removing shearOrigin and changing KisImage::shearImpl to compensate for the motion of the origin like other places do, it seems to work just as before.

Within KisTransformWorker itself, the use of shearOrigin was a bit odd, since it was used conditionally only if shear was present, but also rounded. With this commit it's effectively as if shearOrigin is always 0,0 now, and this is where I'm not 100% sure that behavior won't change slightly, but the rounding before must have prevented any kind of simpleTranslation + subpixel shearOrigin shenanigans so it's probably fine.

This commit also fixes KisTransformWorker::transform, since it disagreed with the actual transform done by runPartial. In master it's

return TS.inverted() * S * TS * SC * R * T;

and now it's just

return SC * S * R * T;

Simpler and with SC and S in the order that reflects the transform that actually happens, as far as I can tell. Which follows the same conventions as transforms used in other places, like KisTransformUtils::MatricesPack and KisAlgebra2D::DecomposedMatrix.

(Reading runPartial is a little confusing, because it looks like the order is shear happens, then scale happens farther down. But if shear does happen it will actually do shear and scale at the same time (with parameters that work out to SC * S, not S * SC), then set scale to 1 so it isn't applied again below)

Fixing transform is what fixes Vector layers transformed by both scale and shear at the same time. Which before had a correct bounding box and preview in the transform tool, but would end up with an incorrect transformation matrix on application.

Also when updating KisTransformUtils::createTransformWorker I removed its out parameter, since it was never used.

The only issue I've noticed is libs-ui-kis_shape_layer_test will inconsistently hang (until timed out after 5 minutes) on KisImage::waitForDone. Sometimes it works, sometimes it doesn't. So I guess it's either a race condition or maybe some change here is randomly causing it, which would be worrying.

An alternative approach would be to leave shearOrigin alone and just fix KisTransformWorker::transform, and while it would be a much smaller change I think it would make that function even more confusing.

Test Plan

Check that Vector layers now transform correctly when using both scale and shear. Make sure Shear layer/all layers works like before (that is, the shear should pivot around the center of their collective bounding box). Double-check Shear Image works without incorrect translation off-canvas. Test free transform with various settings for paint layers to make sure nothing there changed.

Formalities Checklist

  • I confirmed this builds.
  • I confirmed Krita ran and the relevant functions work.
  • I tested the relevant unit tests and can confirm they are not broken. (If not possible, don't hesitate to ask for help!)

libs-ui-kis_shape_layer_test sometimes hangs on KisImage::waitForDone

  • I made sure my commits build individually and have good descriptions as per KDE guidelines.
  • I made sure my code conforms to the standards set in the HACKING file.
  • I can confirm the code is licensed and attributed appropriately, and that unattributed code is mine, as per KDE Licensing Policy.

Merge request reports