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

Make move_to less strict #303

Open
raphlinus opened this issue Sep 22, 2023 · 3 comments
Open

Make move_to less strict #303

raphlinus opened this issue Sep 22, 2023 · 3 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@raphlinus
Copy link
Contributor

Currently after a close_path, the only valid path building operation on a BezPath is a move_to. This is stricter than most existing standards, including PDF and SVG. In most other worlds, you can continue using line_to and friends, and the implicit current point is the same as the start/end point of the previous subpath.

This is a semver breaking change, and would be good to get in 0.10. In addition to being friendlier, it enables a more efficient encoding into Vello paths - you can get the effect by clearing the SUBPATH_END_BIT set by the previous close_path.

The work involves removing a number of checks, changing the documentation, and updating tests to reflect the new behavior. The core of it is changing is_open_subpath() tests to !is_empty().

We're not at this time proposing making the initial move_to optional. It's been proposed to make it default to (0, 0), but that has its own set of problems, not least of which is that translation of a path by pointwise translation of all control points breaks.

@dfrg
Copy link
Contributor

dfrg commented Dec 14, 2023

#320 correctly addresses this and I was mistaken about the segments iterator which already has this behavior.

There is an additional question of what Segments should do when a path does not start with a move element. Current behavior is to panic if the path starts with a close element or use the end point of the non-move element as the start point. For reference, SVG simply renders nothing for a path that does not start with a move. It seems reasonable for the segments iterator to yield nothing in this case.

While exploring this, I also noticed that BezPath::from_svg panics if the path doesn't start with a move. This should probably result in an error instead since we're already returning a Result.

@platlas
Copy link
Collaborator

platlas commented Jan 15, 2024

While exploring this, I also noticed that BezPath::from_svg panics if the path doesn't start with a move. This should probably result in an error instead since we're already returning a Result.

This could be done for example by putting this check:

if path.elements().is_empty() {
    return Err(SvgParseError::UninitializedPath)
}

inside condition:

if c != b'm' && c != b'M' {

But I have here new variant for SvgParseError - would it be breaking change?

@dfrg
Copy link
Contributor

dfrg commented Jan 16, 2024

It would be a breaking change but I believe we already have a few of those in main so now is probably a good time to add another :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants