RazrFalcon / tiny-skia

A tiny Skia subset ported to Rust
BSD 3-Clause "New" or "Revised" License
1.05k stars 67 forks source link

stroker: Add `LineJoin::MiterClip` #91

Closed torokati44 closed 1 year ago

torokati44 commented 1 year ago

Implements #82.

miter_joiner and miter_clip_joiner now only delegate to the newly extracted miter_joiner_inner, passing false and true to it as miter_clip, respectively.

Inside miter_joiner_inner, do_blunt got renamed to do_blunt_or_clipped, and if miter_clip is true, it does a bit more than before - calculates and adds to the outer path builder the extended and clipped corner points. As a result, do_miter now no longer delegates to do_blunt, instead the original contents of do_blunt got incorporated into it. While a bit more verbose, I think it might be a little easier to read this way.

To reduce the amount of deviation from the ideal outline, the clipped corners are calculated by translating the original (blunt) corners along the tangents of the joined lines. The amount of translation needed is called x, and computed like this:

(Illustration and math credit to my girlfriend.)

See this test image showcasing joins with different line widths, angles, and miter limits:

LineJoin::Miter: image

LineJoin::MiterClip: image

torokati44 commented 1 year ago

I'm not entirely sure yet whether the nearly 0°, 90°, and 180° cases are handled correctly...

RazrFalcon commented 1 year ago

Wow! That's really cool. And you even have an illustration! Thanks a lot for the patch. I will publish a new release tomorrow.

torokati44 commented 1 year ago

Whoa, that was quick! 😅 Thanks!

RazrFalcon commented 1 year ago

I see minor differences in the miter rendering. Is it expected? I would prefer that miter would not be affected by this change.

Example:

fn main() {
    let mut paint = Paint::default();
    paint.set_color_rgba8(0, 255, 0, 255);

    let path = {
        let mut pb = PathBuilder::new();
        pb.move_to(40.0, 155.0);
        pb.cubic_to(95.0, 140.0, 100.0, 105.0, 100.0, 40.0);
        pb.cubic_to(100.0, 105.0, 105.0, 140.0, 160.0, 160.0);
        pb.finish().unwrap()
    };

    let mut pixmap = Pixmap::new(300, 300).unwrap();
    // Scale is important here.
    pixmap.stroke_path(&path, &paint, &Stroke::default(), Transform::from_scale(1.5, 1.5), None);
    pixmap.save_png("image.png").unwrap();
}

Old -> New

old new

torokati44 commented 1 year ago

Yeah, sorry about that, it definitely wasn't intentional. Indeed it was the Nearly180 case that got buggy, as suggested above... :sweat_smile: I'll send a fix soon!

torokati44 commented 1 year ago

I've adjusted your test a bit so the line is thicker, there is both an AngleType::Sharp variant as well as AngleType::Nearly180, and the line where the pivot points are is marked: https://gist.github.com/torokati44/8e61f8553bb1c405d30f93740812db29

This is what it looks like on current master, with my regression: image

The one on the bottom left is the regression you mentioned, and there is a division by zero on the bottom right.

With my upcoming fix: image

Both the Miter regression is gone, and the divisons by zero are avoided.

torokati44 commented 1 year ago

At the moment, a miter limit smaller than 1.0 will revert both Miter and MiterClip to Bevel. This is visible on the second row in my test case above.

While in general it doesn't make sense to have a miter limit smaller than 1.0, since with shallow angles, it would move the corner points "backward", which could interfere with any neighboring joins if they are too close.

Perhaps the limit should instead depend on the join angle...? So that with these really sharp angles, one could use a miter limit smaller than 1.0 - perhaps right down to sin(angle/2)?

RazrFalcon commented 1 year ago

Why MiterClip interfere with Miter to begin with? I would assume the addition of MiterClip would not affect the existing code.

As for the math, this is a Skia port. I have no idea how it works.

torokati44 commented 1 year ago

Since so much of the basic setup code, and some of the edge cases, are shared between Miter and MiterClip, I didn't want to duplicate 150 lines of code to then just add 30 lines and change 5 in the middle. Instead, I added that 30 lines in the middle of the existing miter joiner, done only conditionally.

RazrFalcon commented 1 year ago

I understand that. But it still affected Miter. Was it just a bug or an intentional change to the how Miter works?

torokati44 commented 1 year ago

Was it just a bug or an intentional change to the how Miter works?

I intended to keep Miter exactly as it was, I just made a mistake. Which I now fixed in #92.

RazrFalcon commented 1 year ago

Great! That's what I wanted to hear. Now resvg tests are passing as well.