Fix brush transforms #283
Fix brush transforms #283
Conversation
This fixes an incorrect application of the inverse transform for radial gradients in fine. Also fixes an edge case in `SceneBuilder` where a brush transform is identical to the path transform leading to a corrupt encoding.
src/encoding/encoding.rs
Outdated
| @@ -120,10 +120,16 @@ impl Encoding { | |||
| } | |||
|
|
|||
| /// Encodes a transform. | |||
| pub fn encode_transform(&mut self, transform: Transform) { | |||
| /// | |||
| /// If the given transform is different from the current one, encodes it an | |||
There was a problem hiding this comment.
encodes it an -> encodes it and
shader/fine.wgsl
Outdated
| @@ -251,7 +251,7 @@ fn main( | |||
| for (var i = 0u; i < PIXELS_PER_THREAD; i += 1u) { | |||
| let my_xy = vec2(xy.x + f32(i), xy.y); | |||
| // TODO: can hoist y, but for now stick to the GLSL version | |||
| let xy_xformed = rad.matrx.xz * my_xy.x + rad.matrx.yw * my_xy.y - rad.xlat; | |||
| let xy_xformed = rad.matrx.xy * my_xy.x + rad.matrx.zw * my_xy.y - rad.xlat; | |||
There was a problem hiding this comment.
When I tested this change locally as is it didn't fix the broken rotation for me on the blend_grid test (you can reproduce it with the new Q/E controls that @DJMcNab added).
However I was able to fix it by reverting your change to fine.wgsl back to the original and applying @DJMcNab's suggestion on zulip to change shaders/draw_leaf.wgsl#153 to:
let inv_mat = inv_det * vec4(matrx.w, matrx.y, matrx.z, matrx.x);
If I combined your change to fine.wgsl with the change to draw_leaf.wgsl I was getting an even weirder rotation behavior.
There was a problem hiding this comment.
The fix in this PR comes from experimenting with radial gradient rotation in general.
I used this codepen as reference: https://codepen.io/dfrgy/pen/KKxMpWj and added a corresponding example to the brush transform test scene. Without these changes, the transform is very wonky.
The original inverse calculation is algebraically correct so I'm assuming the problem is elsewhere but I haven't been able to track down the source of the reversed rotation direction in the blend grid. I'll keep investigating.
There was a problem hiding this comment.
Looks like the inverse of the translation components was incorrect. This seems to be working for all cases now.
shader/draw_leaf.wgsl
Outdated
| @@ -151,8 +151,11 @@ fn main( | |||
| let r1 = bitcast<f32>(scene[dd + 6u]); | |||
| let inv_det = 1.0 / (matrx.x * matrx.w - matrx.y * matrx.z); | |||
| let inv_mat = inv_det * vec4(matrx.w, -matrx.y, -matrx.z, matrx.x); | |||
| var inv_tr = inv_mat.xz * translate.x + inv_mat.yw * translate.y; | |||
| inv_tr += p0; | |||
| var inv_tr = inv_det * vec2( | |||
There was a problem hiding this comment.
Just to confirm my understanding, the 2x2 matrix is stored in column-major order in the vector, correct?
A slightly shorter formulation that reuses inv_mat and built-in matrix multiplication could look like:
let inv_tr = mat2x2<f32>(inv_mat.xy, inv_mat.zw) * (-translate) - p0;
which has the added benefit of declaring inv_tr as immutable.
Feel free to push back if you disagree (or if there's a performance assumption that I'm missing here). Otherwise the PR overall lgtm.
There was a problem hiding this comment.
I think the lack of matrix types in the codebase is simply a tradition at this point. I like this formulation and we should probably do more of it moving forward. Updated!
|
Thank you for the back and forth! |
This fixes an incorrect application of the inverse transform for radial gradients in fine.
Also fixes an edge case in
SceneBuilderwhere supplying a brush transform that is identical to the path transform generates a corrupt encoding.Discovery of both done by @DJMcNab on this zulip thread.
Should fix #281