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

Calling BezPath::close_path infallibly adds a PathEl::ClosePath, leading to buggy behavior #213

Open
ctrlcctrlv opened this issue Jan 13, 2022 · 3 comments
Labels
enhancement New feature or request needs discussion This change requires some more discussion before we decide we definitely want it

Comments

@ctrlcctrlv
Copy link

kurbo/src/bezpath.rs

Lines 211 to 214 in b9247ed

/// Push a "close path" element onto the path.
pub fn close_path(&mut self) {
self.push(PathEl::ClosePath);
}

My conversion code is aware of the shortcoming and patches Kurbo's output:

https://github.com/MFEK/glifparser.rlib/blob/ee7f0e8786cac7328f27064a5575aeb1a6175006/src/outline/kurbo.rs#L176

I also test for it:

https://github.com/MFEK/glifparser.rlib/blob/ee7f0e8786cac7328f27064a5575aeb1a6175006/tests/kurbo_conv.rs#L12-L13

ctrlcctrlv added a commit to MFEK/glifparser.rlib that referenced this issue Jan 13, 2022
Seems stable. Works in MFEKglif. I'm done with Kurbo's path stuff until
some of the issues, like linebender/kurbo#211 especially but also
linebender/kurbo#213 and linebender/kurbo#214 are solved.
@cmyr
Copy link
Member

cmyr commented Jan 14, 2022

To clarify, the problem as I understand it is that you can have ClosePath elements without a matching MoveTo(?)

This is similar to #212: we do not ensure that paths the user constructs are well formed. I think there are merits to the simplicity of the current API, but this is something that could also be addressed by some kind of builder type that imposed additional constraints.

@derekdreery
Copy link
Collaborator

I could imagine some cfg[debug_assertions] code that emitted a compiler_warning if it found malformed paths.

@derekdreery derekdreery added enhancement New feature or request needs discussion This change requires some more discussion before we decide we definitely want it labels Mar 18, 2023
@ctrlcctrlv
Copy link
Author

Some of my own algorithms rely on infallibly closing, but since I handle #211 by basically doing a double-split, first on Close, and then on MoveTo, if I do not patch your output I end up with empty vectors containing nothing.

I must do it this way because MFEK's tools, including core library glifparser.rlib must receive all contours as Vec<Point>. An outline is then Vec<Vec<Point>>.

So first I take out the closed contours, where we see a MoveTo up to a Close but not including a MoveTo. Then the open ones are handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs discussion This change requires some more discussion before we decide we definitely want it
Projects
None yet
Development

No branches or pull requests

3 participants