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

The fact BezPath allows "subpaths" is poorly documented, annoying for implementors, and contrary to common sense #211

Closed
ctrlcctrlv opened this issue Jan 13, 2022 · 5 comments

Comments

@ctrlcctrlv
Copy link

Since @MatthewBlanchard recommended the gradual switchover of MFEK/math.rlib and MFEK/glifparser.rlib to Kurbo, I have been working on making both libraries more amenable for such a change.

Overall, Kurbo's primitives are better than ours. Except for one—BezPath/PathEl.

The title pretty much sums up my thoughts, but if it's not enough, please view this file: https://github.com/MFEK/glifparser.rlib/blob/ee7f0e8786cac7328f27064a5575aeb1a6175006/src/outline/kurbo.rs

That's what it takes to split Kurbo paths safely. That's absurd. Please begin guaranteeing BezPath won't contain more than one contour—use Vec<BezPath> for that. It's true that a lot of that file stems from our (MFEK's) Point type having a prev/next paradigm and not a segment-based paradigm (i.e. it represents points how users think of them, not how computers do), but a significant amount of the frustration—actually, most of it—was from this lack of separation.

Even now, for reasons I can't quite work out, I can't round trip open paths. I admitted defeat:

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

/// Kurbo module — warning: only guaranteed to round trip closed contours!
@cmyr
Copy link
Member

cmyr commented Jan 14, 2022

Kurbo is intended for 2D drawing, and the vast majority of 2D drawing APIs are based on the API of PostScript. This uses the pattern of modeling a Bèzier path as a series of operations with a pen. The common operations are,

  • "move to": lift the pen, without drawing, and move it to a location (beginning a new subpath)
  • "line to": draw a straight line from the pen's previous location to some new location
  • "curve to": draw a cubic from the previous location to a new location
  • "close path": draw a line to the first point in the current subpath (the "move to" location) and close the subpath

there are some variations: postscript also has "arc" which adds a circle segment.

In any case, these APIs all support the pattern of having multiple "subpaths" in a given path, and kurbo follows this pattern. I also found this slightly counter intuitive at first. I do think there is a reasonable solution, though: since presumably you are controlling the conversion to and from kurbo, you can just personally maintain the invariant that your kurbo paths contain only a single contour. This is what runebender does (and this might help with the round-tripping business?)

https://github.com/linebender/runebender/blob/master/runebender-lib/src/cubic_path.rs#L58

some other examples of this pattern in common graphics libraries:

NSBezierPath
SkPath

@raphlinus
Copy link
Contributor

I agree with Colin here. What we're doing is standard. I see value in a type that has one closed path by construction, but it certainly wouldn't be acceptable to change the semantics of BezPath to be that type; it would break most of the existing use cases.

It's possible there could be a newtype around BezPath which is concretely the same type, but with the invariant of a single subpath. This can be done outside kurbo, or we could consider adding it to kurbo if there's a good case for it (ie we think there would be more than one user).

raphlinus added a commit that referenced this issue Jan 14, 2022
Clearly document the correctness invariants of BezPath and the fact that
it can represent multiple subpaths. Add debug asserts to catch construction
of invalid paths.

Note: the test_elements_to_segments_starts_on_quad formerly constructed
an invalid path and expected particular behavior. This is changed to
expect a panic on debug assert.

Responsive to #211
raphlinus added a commit that referenced this issue Jan 14, 2022
Clearly document the correctness invariants of BezPath and the fact that
it can represent multiple subpaths. Add debug asserts to catch construction
of invalid paths.

Note: the test_elements_to_segments_starts_on_quad formerly constructed
an invalid path and expected particular behavior. This is changed to
expect a panic on debug assert.

Responsive to #211
@richard-uk1
Copy link
Collaborator

I think discussion has finished here, so I'm closing the issue. Feel free to re-open if necessary.

@richard-uk1 richard-uk1 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2023
@ctrlcctrlv
Copy link
Author

No problem. We've stabilized this in glifparser.rlib and I understand now. Thanks for closing this out.

I fixed it by really giving @cmyr's comments on PostScript some thought and adding a trait called IntoPenOperations and a type to hold them.

Now my entire impl is:

impl<PD: PointData> IntoKurbo for Outline<PD> {
    fn into_kurbo_vec(self) -> Result<Vec<PathEl>, GlifParserError> {
        let ret = self
            .into_iter()
            .map(|c| c.into_kurbo_vec())
            .filter(|kv| kv.is_ok())
            .map(Result::unwrap)
            .flatten()
            .collect();

        Ok(ret)
    }
}

use crate::outline::conv::IntoPenOperations;
impl<PD: PointData> IntoKurbo for Contour<PD> {
    fn into_kurbo_vec(mut self) -> Result<Vec<PathEl>, GlifParserError> {
        let ret = self
            .into_pen_operations()?
            .into_iter()
            .map(|po| po.into())
            .collect();

        Ok(ret)
    }
}

And all the hard stuff is generalized so I get a lot more use out of it! :-)

@ctrlcctrlv
Copy link
Author

(Of course, https://github.com/MFEK/glifparser.rlib/blob/master/src/outline/conv/penops.rs is a nightmare, but it keeps our UI code simple not to do it the PostScript way, so is worth doing.)

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

No branches or pull requests

4 participants