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

added improved flattening method for paths #618

Closed
wants to merge 1 commit into from

Conversation

BrownBear2
Copy link

This flattening method will only flatten non-linear path segments.
This prevents 90 degree angles between linear segments being messed up.

@lehni
Copy link
Member

lehni commented Mar 6, 2015

This certainly makes sense. I'd probably add it through an option parameter to the original method. But could you provide an example of a path where that angle mess up happens?

@techninja
Copy link

I second the inclusion of this! Very awesome indeed. Why add extra points to already linear path sections if it's they're already "flattened"?? The angle messup can happen with any angle being approximated via imperfect flattening.

Original 3 point linear:
selection_001

After path.flatten (150):
selection_002

@lehni
Copy link
Member

lehni commented Feb 9, 2016

There are plenty of scenarios where the current behavior is what you want, as a simple way to iterate at regular intervals over any type of path, e.g. to apply an effect to it. But I can see the need to offer the option to do this. I'll add it through the mentioned options parameter.

@BrownBear2
Copy link
Author

My recommendation would be to change the function as shown here and move the functionality you want into a new function called something like quantize, rasterize or equidistantSplit

@lehni
Copy link
Member

lehni commented Feb 9, 2016

That will break backwards compatibility and is hence to be avoided. Also, #rasterize() already exists and does something completely different (rendering the item into a Raster). There is no need to split functionality.

@lehni
Copy link
Member

lehni commented Feb 9, 2016

Also, your implementation doesn't actually use PathIterator, but should be using it. PathIterator is there to be able to efficiently step through a path in length-offsets as opposed to curve-time, which is non-linear, meaning you end up with steps of different lengths based on the tension in your curve. To just use curves, you have direct access through path.curves, btw.

I have a pretty clear idea how to extend the method, so I don't need further work on this PR.

@lehni
Copy link
Member

lehni commented Feb 9, 2016

If we are going to change names, then I think what really deserves the name flatten() is something like this that takes tangential error into account: http://antigrain.com/research/adaptive_bezier/

@lehni
Copy link
Member

lehni commented Feb 9, 2016

Another implementation we might want to look into is C#'s GraphicsPath.Flatten. Mono has an implementation based on code from Sodipodi's libnr that is in the pulbic domain:
https://github.com/mono/libgdiplus/blob/79ae6cdb90163ba5eb601f8691ea9b7558d25371/src/graphics-path.c#L1485

@lehni
Copy link
Member

lehni commented Feb 9, 2016

@BrownBear2 I was wrong about path.curves giving access to the same curves as iterator.curves, btw. It's years ago that I wrote PathIterator, I see now what you're doing. But I think you should either only use PathIterator's subdivision with the specified maximum error, or your own, not both combined. if you reduce the allowed error in the PathIterator constructor, you automatically get more subdivisions, which is pretty much what the above code from Sodipodi is also doing. I need to compare the specific conditions by which subdivisions happen to be certain, but I think I agree that flatten(flatness) should be introduced, with flatness controlling the maximum error.

@lehni lehni closed this in 9e8fcee Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants