From 319dbb1c6caab2882ca1057b27d728a298749287 Mon Sep 17 00:00:00 2001 From: Ivan Matkov Date: Fri, 10 Mar 2023 12:22:32 +0100 Subject: [PATCH] Fix SkiaLayer transform (#422) * Fix SkiaLayer transform * Fixed cameraDistance min and conversation to pt * Fix unit tests * Revert hardcoded dpi like on android * Update golden images for rotation tests * Update compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaLayer.skiko.kt Co-authored-by: dima.avdeev <99798741+dima-avdeev-jb@users.noreply.github.com> * Update rotation tests * Use screenshots generated by CI --------- Co-authored-by: dima.avdeev <99798741+dima-avdeev-jb@users.noreply.github.com> --- .../compose/ui/platform/GraphicsLayerTest.kt | 153 ++++++++++++------ .../compose/ui/platform/SkiaLayer.skiko.kt | 59 +++---- .../compose/ui/platform/SkiaLayerTest.kt | 27 ++-- ...i_platform_GraphicsLayerTest_rotationX.png | Bin 158 -> 622 bytes ...platform_GraphicsLayerTest_rotationXYZ.png | Bin 0 -> 1397 bytes ...i_platform_GraphicsLayerTest_rotationY.png | Bin 141 -> 882 bytes ...i_platform_GraphicsLayerTest_rotationZ.png | Bin 214 -> 838 bytes 7 files changed, 154 insertions(+), 85 deletions(-) create mode 100644 golden/compose/ui/ui-desktop/platform/androidx_compose_ui_platform_GraphicsLayerTest_rotationXYZ.png diff --git a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/GraphicsLayerTest.kt b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/GraphicsLayerTest.kt index 3d1880884b4a3..cff8c0a4eda4e 100644 --- a/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/GraphicsLayerTest.kt +++ b/compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/platform/GraphicsLayerTest.kt @@ -21,14 +21,19 @@ import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.requiredSize import androidx.compose.foundation.shape.RoundedCornerShape +import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.Brush import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.TransformOrigin import androidx.compose.ui.graphics.graphicsLayer -import androidx.compose.ui.test.InternalTestApi +import androidx.compose.ui.isLinux import androidx.compose.ui.renderComposeScene +import androidx.compose.ui.test.InternalTestApi import androidx.compose.ui.test.junit4.DesktopScreenshotTestRule +import androidx.compose.ui.unit.DpSize import androidx.compose.ui.unit.dp +import org.junit.Assume.assumeTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -66,70 +71,122 @@ class GraphicsLayerTest { screenshotRule.write(snapshot) } - @Test - fun rotationZ() { - val snapshot = renderComposeScene(width = 40, height = 40) { - Box( - Modifier - .graphicsLayer( - translationX = 10f, - rotationZ = 90f, - scaleX = 2f, - scaleY = 0.5f, - transformOrigin = TransformOrigin(0f, 0f) - ) - .requiredSize(10f.dp, 10f.dp).background(Color.Red) + @Composable + fun testRotationBoxes( + rotationX: Float = 0f, + rotationY: Float = 0f, + rotationZ: Float = 0f + ) { + val size = DpSize(10.dp, 10.dp) + val backgroundBrush = + Brush.verticalGradient( + colors = listOf(Color.Red, Color.Blue) ) - Box( - Modifier - .graphicsLayer( - translationX = 10f, - translationY = 20f, - rotationZ = 45f + Box( + Modifier + .graphicsLayer( + translationX = 0f, + translationY = 0f, + rotationX = rotationX, + rotationY = rotationY, + rotationZ = rotationZ, ) - .requiredSize(10f.dp, 10f.dp).background(Color.Blue) + .requiredSize(size) + .background(brush = backgroundBrush) + ) + Box( + Modifier + .graphicsLayer( + translationX = 20f, + translationY = 0f, + rotationX = rotationX, + rotationY = rotationY, + rotationZ = rotationZ, + transformOrigin = TransformOrigin(0f, 0f), + ) + .requiredSize(size) + .background(brush = backgroundBrush) + ) + Box( + Modifier + .graphicsLayer( + translationX = 0f, + translationY = 20f, + rotationX = rotationX, + rotationY = rotationY, + rotationZ = rotationZ, + cameraDistance = 0.1f + ) + .requiredSize(size) + .background(brush = backgroundBrush) + ) + Box( + Modifier + .graphicsLayer( + translationX = 20f, + translationY = 20f, + rotationX = -rotationX, + rotationY = -rotationY, + rotationZ = -rotationZ, + cameraDistance = 0.1f + ) + .requiredSize(size) + .background(brush = backgroundBrush) + ) + + } + + @Test + fun rotationX() { + + // TODO Remove once approximate comparison will be available. The problem: there is a difference + // in antialiasing between platforms. The golden screenshot currently matches CI behaviour. + assumeTrue(isLinux) + + val snapshot = renderComposeScene(width = 40, height = 40) { + testRotationBoxes( + rotationX = 45f, ) } screenshotRule.write(snapshot) } @Test - fun rotationX() { + fun rotationY() { + + // TODO Remove once approximate comparison will be available. The problem: there is a difference + // in antialiasing between platforms. The golden screenshot currently matches CI behaviour. + assumeTrue(isLinux) + val snapshot = renderComposeScene(width = 40, height = 40) { - Box( - Modifier - .graphicsLayer(rotationX = 45f) - .requiredSize(10f.dp, 10f.dp).background(Color.Blue) + testRotationBoxes( + rotationY = 45f, ) - Box( - Modifier - .graphicsLayer( - translationX = 20f, - transformOrigin = TransformOrigin(0f, 0f), - rotationX = 45f - ) - .requiredSize(10f.dp, 10f.dp).background(Color.Blue) + } + screenshotRule.write(snapshot) + } + @Test + fun rotationZ() { + val snapshot = renderComposeScene(width = 40, height = 40) { + testRotationBoxes( + rotationZ = 45f, ) } screenshotRule.write(snapshot) } @Test - fun rotationY() { + fun rotationXYZ() { + + // TODO Remove once approximate comparison will be available. The problem: there is a difference + // in antialiasing between platforms. The golden screenshot currently matches CI behaviour. + assumeTrue(isLinux) + val snapshot = renderComposeScene(width = 40, height = 40) { - Box( - Modifier - .graphicsLayer(rotationY = 45f) - .requiredSize(10f.dp, 10f.dp).background(Color.Blue) - ) - Box( - Modifier - .graphicsLayer( - translationX = 20f, - transformOrigin = TransformOrigin(0f, 0f), - rotationY = 45f - ) - .requiredSize(10f.dp, 10f.dp).background(Color.Blue) + testRotationBoxes( + rotationX = 45f, + rotationY = 45f, + rotationZ = 45f, ) } screenshotRule.write(snapshot) diff --git a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaLayer.skiko.kt b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaLayer.skiko.kt index 8b65e5543ba69..4aec6c294cd73 100644 --- a/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaLayer.skiko.kt +++ b/compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaLayer.skiko.kt @@ -24,6 +24,7 @@ import androidx.compose.ui.geometry.toRect import androidx.compose.ui.graphics.Canvas import androidx.compose.ui.graphics.ClipOp import androidx.compose.ui.graphics.Color +import androidx.compose.ui.graphics.DefaultCameraDistance import androidx.compose.ui.graphics.DefaultShadowColor import androidx.compose.ui.graphics.Matrix import androidx.compose.ui.graphics.Outline @@ -40,13 +41,13 @@ import androidx.compose.ui.graphics.toArgb import androidx.compose.ui.graphics.toSkiaRRect import androidx.compose.ui.graphics.toSkiaRect import androidx.compose.ui.node.OwnedLayer -import androidx.compose.ui.node.InvokeOnCanvas import androidx.compose.ui.unit.Density import androidx.compose.ui.unit.IntOffset import androidx.compose.ui.unit.IntSize import androidx.compose.ui.unit.LayoutDirection import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.toSize +import kotlin.math.max import org.jetbrains.skia.ClipMode import org.jetbrains.skia.Picture import org.jetbrains.skia.PictureRecorder @@ -65,6 +66,12 @@ internal class SkiaLayer( OutlineCache(density, size, RectangleShape, LayoutDirection.Ltr) // Internal for testing internal val matrix = Matrix() + private val inverseMatrix: Matrix + get() = Matrix().apply { + setFrom(matrix) + invert() + } + private val pictureRecorder = PictureRecorder() private var picture: Picture? = null private var isDestroyed = false @@ -75,6 +82,7 @@ internal class SkiaLayer( private var rotationX: Float = 0f private var rotationY: Float = 0f private var rotationZ: Float = 0f + private var cameraDistance: Float = DefaultCameraDistance private var scaleX: Float = 1f private var scaleY: Float = 1f private var alpha: Float = 1f @@ -112,11 +120,19 @@ internal class SkiaLayer( } override fun mapOffset(point: Offset, inverse: Boolean): Offset { - return getMatrix(inverse).map(point) + return if (inverse) { + inverseMatrix + } else { + matrix + }.map(point) } override fun mapBounds(rect: MutableRect, inverse: Boolean) { - getMatrix(inverse).map(rect) + if (inverse) { + inverseMatrix + } else { + matrix + }.map(rect) } override fun isInLayer(position: Offset): Boolean { @@ -133,17 +149,6 @@ internal class SkiaLayer( return isInOutline(outlineCache.outline, x, y) } - private fun getMatrix(inverse: Boolean): Matrix { - return if (inverse) { - Matrix().apply { - setFrom(matrix) - invert() - } - } else { - matrix - } - } - override fun updateLayerProperties( scaleX: Float, scaleY: Float, @@ -170,6 +175,7 @@ internal class SkiaLayer( this.rotationX = rotationX this.rotationY = rotationY this.rotationZ = rotationZ + this.cameraDistance = max(cameraDistance, 0.001f) this.scaleX = scaleX this.scaleY = scaleY this.alpha = alpha @@ -186,27 +192,26 @@ internal class SkiaLayer( invalidate() } - // TODO(demin): support perspective projection for rotationX/rotationY (as in Android) - // TODO(njawad) Add camera distance leveraging Sk3DView along with rotationX/rotationY - // see https://cs.android.com/search?q=RenderProperties.cpp&sq= updateMatrix method - // for how 3d transformations along with camera distance are applied. b/173402019 private fun updateMatrix() { val pivotX = transformOrigin.pivotFractionX * size.width val pivotY = transformOrigin.pivotFractionY * size.height matrix.reset() + matrix.translate(x = -pivotX, y = -pivotY) matrix *= Matrix().apply { - translate(x = -pivotX, y = -pivotY) - } - matrix *= Matrix().apply { - translate(translationX, translationY) - rotateX(rotationX) - rotateY(rotationY) rotateZ(rotationZ) + rotateY(rotationY) + rotateX(rotationX) scale(scaleX, scaleY) } matrix *= Matrix().apply { - translate(x = pivotX, y = pivotY) + // the camera location is passed in inches, set in pt + val depth = cameraDistance * 72f + set(row = 2, column = 3, v = -1f / depth) + set(row = 2, column = 2, v = 0f) + } + matrix *= Matrix().apply { + translate(x = pivotX + translationX, y = pivotY + translationY) } } @@ -234,11 +239,11 @@ internal class SkiaLayer( } override fun transform(matrix: Matrix) { - matrix.timesAssign(getMatrix(inverse = false)) + matrix.timesAssign(this.matrix) } override fun inverseTransform(matrix: Matrix) { - matrix.timesAssign(getMatrix(inverse = true)) + matrix.timesAssign(inverseMatrix) } private fun performDrawLayer(canvas: Canvas, bounds: Rect) { diff --git a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/SkiaLayerTest.kt b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/SkiaLayerTest.kt index 66b58cb40a861..f5e45309c8dd1 100644 --- a/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/SkiaLayerTest.kt +++ b/compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/SkiaLayerTest.kt @@ -19,6 +19,7 @@ package androidx.compose.ui.platform import androidx.compose.foundation.shape.CircleShape import androidx.compose.ui.geometry.Offset import androidx.compose.ui.graphics.Color +import androidx.compose.ui.graphics.DefaultCameraDistance import androidx.compose.ui.graphics.DefaultShadowColor import androidx.compose.ui.graphics.RectangleShape import androidx.compose.ui.graphics.RenderEffect @@ -139,7 +140,8 @@ class SkiaLayerTest { layer.resize(IntSize(100, 10)) layer.updateProperties( rotationX = 45f, - transformOrigin = TransformOrigin(0f, 0f) + transformOrigin = TransformOrigin(0f, 0f), + cameraDistance = Float.MAX_VALUE ) val matrix = layer.matrix @@ -153,7 +155,8 @@ class SkiaLayerTest { layer.resize(IntSize(100, 10)) layer.updateProperties( rotationX = 45f, - transformOrigin = TransformOrigin(1f, 1f) + transformOrigin = TransformOrigin(1f, 1f), + cameraDistance = Float.MAX_VALUE ) val matrix = layer.matrix @@ -167,7 +170,8 @@ class SkiaLayerTest { layer.resize(IntSize(100, 10)) layer.updateProperties( rotationY = 45f, - transformOrigin = TransformOrigin(0f, 0f) + transformOrigin = TransformOrigin(0f, 0f), + cameraDistance = Float.MAX_VALUE ) val matrix = layer.matrix @@ -181,7 +185,8 @@ class SkiaLayerTest { layer.resize(IntSize(100, 10)) layer.updateProperties( rotationY = 45f, - transformOrigin = TransformOrigin(1f, 1f) + transformOrigin = TransformOrigin(1f, 1f), + cameraDistance = Float.MAX_VALUE ) val matrix = layer.matrix @@ -254,29 +259,31 @@ class SkiaLayerTest { translationX = 60f, translationY = 7f, rotationX = 45f, - transformOrigin = TransformOrigin(0f, 0f) + transformOrigin = TransformOrigin(0f, 0f), + cameraDistance = Float.MAX_VALUE ) val matrix = layer.matrix val y = (10 * cos45).roundToInt() - val translationY = (7 * cos45).roundToInt() + val translationY = 7 assertEquals(IntOffset(0 + 60, 0 + translationY), matrix.map(Offset(0f, 0f)).round()) assertEquals(IntOffset(100 + 60, y + translationY), matrix.map(Offset(100f, 10f)).round()) } @Test - fun translation_rotationY_left_top_origi() { + fun translation_rotationY_left_top_origin() { layer.resize(IntSize(100, 10)) layer.updateProperties( translationX = 60f, translationY = 7f, rotationY = 45f, - transformOrigin = TransformOrigin(0f, 0f) + transformOrigin = TransformOrigin(0f, 0f), + cameraDistance = Float.MAX_VALUE ) val matrix = layer.matrix val x = (100 * cos45).roundToInt() - val translationX = (60 * cos45).roundToInt() + val translationX = 60 assertEquals(IntOffset(0 + translationX, 0 + 7), matrix.map(Offset(0f, 0f)).round()) assertEquals(IntOffset(x + translationX, 10 + 7), matrix.map(Offset(100f, 10f)).round()) } @@ -383,7 +390,7 @@ class SkiaLayerTest { rotationX: Float = 0f, rotationY: Float = 0f, rotationZ: Float = 0f, - cameraDistance: Float = 0f, + cameraDistance: Float = DefaultCameraDistance, transformOrigin: TransformOrigin = TransformOrigin.Center, shape: Shape = RectangleShape, clip: Boolean = false, diff --git a/golden/compose/ui/ui-desktop/platform/androidx_compose_ui_platform_GraphicsLayerTest_rotationX.png b/golden/compose/ui/ui-desktop/platform/androidx_compose_ui_platform_GraphicsLayerTest_rotationX.png index 9e380f536035b335313e12368dcb5e1fdac284c3..e4564fde5026261c5174a3e2e8dfdbac172b8992 100644 GIT binary patch delta 582 zcmV-M0=fO30qz8lGk*dlNklA$tay2m-0y>CLTdA#spU6=h^y>t19aQCsdw8ECGZWP=~gjg`@K9Dq}(NbfCS01GH40tu3fUdj%b= zgvJZ~7JuQ$S*5zCk?b0yw*%yHL0*JgFCro$BI5t0g+158HX$8zZgZ$Mq1`ZG)k&`c zXM)rL*3;A{g@4XHSfGFapBNg6(SBo)R6pYd(`XZ$7-s9E`}q)f$Ekurp7b+TFvSd; zrS9he14F<_7pErh*>9p{rk(*8dd#2-M z#A)V$qsnkQP2w|tlD$UuVfSzVj0@&2RIYdxnr3nLK7ZE>4pv!{&cZ3oGQaER$qnwQ zJ`)|R{5-Q2Kkq6Fj|6ks%Q48~2VC=Pct7BIo~r@^J8_zsMW#KwK1VtAoz$9van}i9 zBS|A$A@8|l_WdcAwE+Q^R>*lS>OYu4==!l!1Mz+6{vjFO_7V_-6gSC~dKnW08DzWH zfQ=aKi9aodU0oi;G!YRI5fL5y24{(~ Ulr@FzGXMYp07*qoM6N<$f*GX~H2?qr delta 114 zcmaFIGLLbBaW+G!r;B4qMC;r02YDMDc$gifmoj%PWz(1@=EObm1E-vIe)iOe!n>R2 z+D1;j^4y@}Oj)E~{hW8Xn{Sj`d{}#5K7xBzwNcolbtwU#=DuZQ0E37pj5$j<7f^BxJ{0X{da<=loQy0)$6*KLh-KP|yYN{BW(%*(m5 zv5Bs&O>}K-qxg`EnXY@SWL$3I zxQXLo;E-UjXl_v62y60B7T5+;O7kn1d70qiYS+)Lf%2mzN$5F^8cN~$dx2z;m# z{4GofBUOU*Qnz1#zexS7D#5FTK$Rd70{aCltq@Sshp66NFak!pa+u$oxm?P`aX&X1 zs1kH#vAj_sAb|8;n9ygFriw73RsrjaAdg1~d^`C9Re}!{uPqq}SuD?n2@Myn#V{d; zaUChvWCZ&jm7qTP0*9zRBEVD;AV^mu1YRk`jlHfH@aW8Kr1KF1A5AgU>w#Sr0s`=k zO0atpaxjW-b+JY|rUNR$M1(*gFcZbMR)A6=pp=Q@-lr106ejd`m{3bUexIM{GsRm; z(I~!lm7o?Oa57A&wVOu2kLY_@EL+P3Ryc&>WO&hpcVJHnd&T})DjquW@gZD!mB6ZU z>L;(kiid{MtYeaNOuOg%yJj5%mW^N&{rE%=OH*fAXP<44`5wU*9TOS~8j9UY=8@Sp zm{(vPxh01gBQ=z#1W2>#F|WWJ<_W+Y=2fTJuz5B;<_#!dSq^hU4l`LSxi58--~g zGgxQ@T9TbJc6AR4pKN6!OjSF1MA6PeXt~8 z8XH4Y6UFE3p`q;z&F2JcpQCY85~nc91K!RFLnIRja*>m z5dm|V@~`8#Fa2}QbCaNk1T`4oG30G8@N#oeU=|B$Y$S0qfQRcW=4aN_=PltIB+mMN{Lc^IneybP zfOK9X|BZ&S*+7m`#Rzs1)XXDr92x~)&rb8H3?Pu+FIq`yg zS|KolwR0h1Mi@`Gv^?^=g`=3m+_|8#|Az3G&{7VP!b?x)$itB~!b1%apy@T6atTf(}lRWXZ!u3qztLiWDFA>0tAMk|C3#92Y3` z11|VH=YIT=_wrl=U_|pr;4wKFNVfY!6aRZkYD7~OTuNJv>wjhQWKsYh=wTpvs5#JA zr4t^|ojDea+|ggl@|^%4&^-r^r2zN2an98X1_TzKU!=^zS?8)t11{3Cfb$Nn3R(!b zYzGH)&%~?(sR0)ySvGmz!F+60$a4X!B%RN7VXs(DCz^Rxb?~Af^*p#LSf;##^V_x7 zTI>HL0Yl)MGJiLIb_qQ5efAR6?vR#OcyoXi3fvbiXR;EwH^Ckpa#h~bD+b)BD+63f zP`d)xJ|dVKQ-3zVm3_MMhFh}7b|IkhaGA16gn7ro|5KQJWx($j40mV3)>9v+7hq@o z5z{D)RUR%T5g`@<_O&MC-x9o^6zO*cOjiWkOi@n`aep6-X&NDxhD)fEP`pG@9xe@H z8pN1HOxQAfoNqhy?+T+Y4R~DjI7?OIHO6h(?EtcKN)HX9tBK2L&4 zghj}Z_RbXsU%=Z+fbY0@JA(_cW?VaBBWtxnKo6H1$2vrjYB&GpI`Fm%A1~RSaVbTxR=GGIjt{8}0Cn3@KXxn7`L8K#D`&je?Q)01+sl3wwte$X1nXH0-{ z3o_;5YM=}%ra^>Bh*cTzCEM5pm_#&)QKb4as^wmCDZnE(^aq@Q`8W)?vJzm2J`O$> zAtsR?h#R?KQV2Mtt%rVuNq~)yL!Zvxh}Iuf0e|0d^Tm+%UWfjUgFoYD=BOeqv`~4t zM3_w3B*bD&lOgS&6Y3AaSYe_zUXPH^1MDX*-h(D%!y;spY2MAkQKEl!r_TK|Ve4x! zo>vE(ft;rB=n#1xVm)>6dp3cFY!DZx=&~;^15qTqARnz*0CDTY&s+v_gAa0xH?qxM zaYZ#_6NDi5K`v=+DEDk8gR2}$kN3rZAeUaSg7g&0Zi`_V!2e2OfE1VfYpu1``T7@d WhHzZkk;TgZ0000MtV!rG9JsRFf!5jF>589F9S}Mq&EH$>x6Xq zNRg`8&&Mx)oiu(R&U5ZPIbcC^3%4#Je+u|R~6C@aDNk2)_JK&P0u$3 z-Gntg!B`3&%>^_#dC$Fs{Fy)ekg%&ro&Q6@TvL$w5vc+gCjelXBR{}&^GrX!-KCJJ zz}cw?YVRiR)_(!6^U*h$q}y6K(hjL_0==Yz|9P1^R~9Omw`I_zsf^*|*)dMdcR%el9O^b6p+ zvp9o8fQ!MoeS?nomY2Eu>;Shn&z__pE}mq)+hV{m%b5?bbL~mW;3i&OmmvY(H<6|H zB(n*rCn3^s<4I;-EjMs-Pm&amV0}P?$th;w_J2lLnYY+FXIFt*Nk07bi!-|?Ny}3{ z^xHH&gWIeE2Vejk4oQlg(YDA^`E+JbC<|_kQ40L6EhA;+WpIglyX#Dxit)NHi^`VTm+E~?Z|r#C&iCuo V;S>(mv#S|^z|+;wWt~$(696AbN3j3^