Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix brush transforms #283

Merged
merged 7 commits into from
Feb 23, 2023
Merged

Fix brush transforms #283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
033870d
Fix brush transforms
dfrg Feb 23, 2023
6612b7a
From typo in comment
dfrg Feb 23, 2023
c65c19d
add additional test scene
dfrg Feb 23, 2023
3c15bff
Proper inverse of translation components
dfrg Feb 23, 2023
1f938e5
linear algebra refresher
dfrg Feb 23, 2023
659ab2f
simplify
dfrg Feb 23, 2023
f657b88
use matrix math!
dfrg Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shader/fine.wgsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

@armansito armansito Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the inverse of the translation components was incorrect. This seems to be working for all cases now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good to go :)

let ba = dot(xy_xformed, rad.c1);
let ca = rad.ra * dot(xy_xformed, xy_xformed);
let t = sqrt(ba * ba + ca) - ba - rad.roff;
Expand Down
8 changes: 7 additions & 1 deletion src/encoding/encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encodes it an -> encodes it and

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Fixed, ty.

/// returns true. Otherwise, encodes nothing and returns false.
pub fn encode_transform(&mut self, transform: Transform) -> bool {
if self.transforms.last() != Some(&transform) {
self.path_tags.push(PathTag::TRANSFORM);
self.transforms.push(transform);
true
} else {
false
}
}

Expand Down
26 changes: 14 additions & 12 deletions src/scene.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,14 @@ impl<'a> SceneBuilder<'a> {
});
if self.scene.encode_shape(shape, true) {
if let Some(brush_transform) = brush_transform {
self.scene
.encode_transform(Transform::from_kurbo(&(transform * brush_transform)));
self.scene.swap_last_path_tags();
self.scene.encode_brush(brush, 1.0);
} else {
self.scene.encode_brush(brush, 1.0);
if self
.scene
.encode_transform(Transform::from_kurbo(&(transform * brush_transform)))
{
self.scene.swap_last_path_tags();
}
}
self.scene.encode_brush(brush, 1.0);
}
}

Expand All @@ -164,13 +165,14 @@ impl<'a> SceneBuilder<'a> {
self.scene.encode_linewidth(style.width);
if self.scene.encode_shape(shape, false) {
if let Some(brush_transform) = brush_transform {
self.scene
.encode_transform(Transform::from_kurbo(&(transform * brush_transform)));
self.scene.swap_last_path_tags();
self.scene.encode_brush(brush, 1.0);
} else {
self.scene.encode_brush(brush, 1.0);
if self
.scene
.encode_transform(Transform::from_kurbo(&(transform * brush_transform)))
{
self.scene.swap_last_path_tags();
}
}
self.scene.encode_brush(brush, 1.0);
}
}

Expand Down