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.

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

#35 Implement miter_limit_join #192

Merged
merged 1 commit into from
Oct 28, 2017
Merged

Conversation

o0Ignition0o
Copy link
Contributor

@o0Ignition0o o0Ignition0o commented Oct 21, 2017

This is Working on a function signature, before miter_limit_join_implementation (and tests).
I guess I should remove the magic number 4, to use an enum or something instead.

This should handle #35 some day x)

@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Oct 21, 2017

Hey @nical , feel free to tell me if:

  • There's a better way to represent the svg specification than using consts
  • Panicking during the obj construction is fine by you, or if i should set default value instead ( I don't really understand if it's the StrokeBuilder's duty to handle exceptions as described in https://svgwg.org/svg2-draft/implnote.html#ErrorProcessing )
  • I m allowed to link to the official svg spec in the comments ?

Thaks a lot :D

@o0Ignition0o o0Ignition0o force-pushed the miter_limit_join branch 3 times, most recently from ea09287 to c96c44f Compare October 21, 2017 12:29
@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Oct 21, 2017

else if join_type == LineJoin::Miter && normal.square_length() > limit * limit { // Per SVG spec: If the stroke-miterlimit is exceeded, the line join // falls back to bevel. join_type = LineJoin::Bevel } Ok it looks like I m' reimplementing something you already have ^^' I'll refactor my code to use your version of "stroke-miterlimit is exceeded"

@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Oct 22, 2017

I guess I'm almost there, still need to handle a weird glitch on the top left corner:
No idea whether it's related to #166 (comment) or not :/
[Edit] : After a couple of tryouts, it looks like I'm responsible for the glitch. Will try to fix it asap :)
[Edit 2] : I'm going a bit crazy about this issue, I think I need help x)
Here's what I get with StrokeOptions::tolerance(0.022).with_line_join(LineJoin::MiterClip).with_miter_limit(2.0)
image
And here's what I get with StrokeOptions::tolerance(0.022).with_line_join(LineJoin::MiterClip).with_miter_limit(2.0).dont_apply_line_width()
image

In all screenshots I build the following example path :

pub fn build_test_path<Builder: SvgBuilder>(path: &mut Builder) {; path.move_to(point(0.0, 0.0)); path.relative_line_to(vec2(10.0, 10.0)); path.relative_line_to(vec2(-6.0, -10.0)); path.close(); }
Any thoughts ? :)

BTW I'd love to add some tests to it, but I dont' really know If it's relevant, and how to proceed ^^'

advancement: self.length,
side: front_side,
}
);
Copy link
Owner

Choose a reason for hiding this comment

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

nit: let's use block indentation here, like this:

        let v = add_vertex!(
            self,
            Vertex {
                position: self.current,
                normal: normal,
                advancement: self.length,
                side: front_side,
            }
        );

back_vertex: VertexId,
normal: Vec2,
) -> (VertexId, VertexId) {
if self.miter_limit_is_exceeded(normal) {
Copy link
Owner

Choose a reason for hiding this comment

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

nit: really a minor thing but I tend to prefer when the complex code is out of the nested blocks, for example:

if condition {
    // complicated stuff
    return ...
}
return simple_stuff()

could be implemented like:

if !condition {
    return simple_stuff()
}
// complicated stuff
return ...

It helps with keeping the cyclomatic complexity low (what a scary word!) and and in my experience the code looks a tad less intimidating that way.

@nical
Copy link
Owner

nical commented Oct 22, 2017

BTW I'd love to add some tests to it, but I dont' really know If it's relevant, and how to proceed ^^'

Yeah, I have put a lot of focus on the fill tessellator so the latter has a pretty great test coverage but it would be good to start thinking about what kind of testing would be useful for the stroke tessellator.
I think that's more of a general thing, to look into outside the scope of this PR.

@o0Ignition0o
Copy link
Contributor Author

I have to do an other pass checking for unused variables + silly stuff that could be optimized away, but I feel like it should work fine :)

@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Oct 28, 2017

There might still be something wrong though, since I don't really understand the "neg_if_right" part of the bevel implementation, so I did not use it (and I might mix up start_vertex and end_vertex i guess?)

@o0Ignition0o
Copy link
Contributor Author

This should be it ! :)
image

@o0Ignition0o o0Ignition0o changed the title [WIP] #35 Implement miter_limit_join #35 Implement miter_limit_join Oct 28, 2017
Copy link
Owner

@nical nical left a comment

Choose a reason for hiding this comment

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

Looks good!

@nical nical merged commit 0dea3f6 into nical:master Oct 28, 2017
(v, v)
self.tessellate_miter_join(
front_side,
normal
Copy link
Owner

Choose a reason for hiding this comment

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

Oops actually there was a bug here: this should be front_normal instead of normal, I'll land a quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants