MFEK / math.rlib

A library which supplies mathematics and algorithms for manipulating beziers.
Apache License 2.0
7 stars 4 forks source link

slope answer #20

Closed prajwalprabhu closed 2 years ago

ctrlcctrlv commented 2 years ago

Wow, this looks like it was a lot of work. Thanks Prajwal.

Did you patch MFEKpathops by chance? I see you added a test. It's 3am here in NJ right now but I'll take a closer look tomorrow, but if you could go ahead and patch MFEKpathops that'd be the easiest way for me/others to use/test this.

Looking over the functions, especially the core callers, that'd be MFEKmath::simplify::simplify, I do note that it looks like you're only removing points and not adjusting the handles on the remaining points to match, I'm not sure how that will work in practice. Did you try this out on the sample files? I could be wrong, I might just not understand it given how large of a changeset this and #19 are.

prajwalprabhu commented 2 years ago

I think it will help to remove the colinear points.It is not the complete solution. So in pathop I sould add it as a different command or under the BOOLEAN only

ctrlcctrlv commented 2 years ago

Also I note that the main public API, pub fn simplify(point: Vec<Vec<Point<()>>>) has a very strange API. It seems to take in Vec<Vec<Point()>> by value (instead of the convenience type Outline<PD>, and it just drops it right out of scope at the end. I really don't know how that'd work in practice, it sems like it would do the simplify but just discard the data. You call solution(i) on every contour but this function wouldn't, as far as I can tell, be usable. Also, there doesn't seem to be a way to set accuracy.

So in pathop I sould add it as a different command or under the BOOLEAN only

New command. Call it SIMPLIFY. :)

prajwalprabhu commented 2 years ago

Even the quick hull as I mentioned it may be useful in your TODO

ctrlcctrlv commented 2 years ago

Thank you, I'll catch up with you tomorrow, I can tell a lot of this is useful, and won't take that much cleanup to merge. We'll work on it in the days ahead. Thanks for your prompt work.

ctrlcctrlv commented 2 years ago

Oh by the way, before I log off for the night, if you want to know how to adjust handles, the API is a little wonky (my fault), you replace the Point<_> struct elements a and b, which contain Handle enumerations. If you set both handles to Colocated you should also set PointType to Line. You could take a look at fixup.rs (https://github.com/MFEK/math.rlib/blob/main/src/fixup.rs), which implements a similar function in FontForge, eventually I want to add more complicated fixup functions because even splines not produced by us can have all sorts of odd decisions, (two colocated handles but still marked curve, etc.). If you want to add/modify a control point you use Handle::At(x, y)…I'm pretty impressed w/this so far given you're a student.

ctrlcctrlv commented 2 years ago

In future do try to add traits for this stuff, similar to the Fixup trait in fixup.rs, and use the convenience types Contour<PD> and Outline<PD>. Even though they are the same to the compiler as Vec<Vec<Point<_>> and Vec<Point<_>> it's easier to read if you use the convenience types.

I know you haven't done much Rust so it's all good, generally in Rust you'd define a public trait on those types instead of having a pub fn, that makes it easier to call from other files / more clear what's going on. I know it's a work in progress, cheers :)

prajwalprabhu commented 2 years ago

I'll work on it .Thank you for the feedback .Ya I have worked bit less in rust specific concepts.Ill develop it soon.