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

Deferred errors? #34

Closed
raphlinus opened this issue Jan 28, 2019 · 4 comments
Closed

Deferred errors? #34

raphlinus opened this issue Jan 28, 2019 · 4 comments

Comments

@raphlinus
Copy link
Contributor

This is branched off #32, particularly the complaint that too many methods have Result types.

I don't agree with the suggestion of just panicking, as I think that's extremely unfriendly in a GUI context - it seems to me that in most cases it's best to have incomplete display than bring down the whole app.

That said, I am wondering if there might be a better approach. I notice that both Cairo and Direct2D have a style where most drawing ops succeed, but the render target can go into an error state, which in Cairo is retrieved with a status call, and in Direct2D seems mostly to be retrieved at EndDraw. Part of this, I think, is to support pipelined operation, where the error might not occur until later.

I'm wondering whether it would be better to similarly defer errors for simple draw calls. My thinking is that it's still valuable to have them for resource creation (brush, image, text layout). Part of the reason this is coming to mind now is that I'm working out the signature for gradient creation, and thinking it would be best to have the error only on the final build call.

One benefit is less branchy control flow, which at the least will result in smaller code size. The main downside is that when there is an error, it might be a little harder to track down the exact source, but if there is a status check like Cairo, that can be added to isolate faults during debugging.

What do people think?

@RazrFalcon
Copy link

RazrFalcon commented Jan 28, 2019

I don't agree with the suggestion of just panicking

It's a horrible suggestion. But I've proposed it in the context of an alpha-quality build. Because the Unsupported error is a bit too "verbose".

As for error handling, resvg doesn't have errors at all. It simply renders as much as possible, ignoring "bad" branches. Failed to load/allocate an image - skip it.

So I personally don't care about errors at all, because there are no errors that I want to handle. I can't think of any. What could go so wrong, that we have to return an error?

For me, warnings (via log crate) + Option is the best solution.

@RazrFalcon
Copy link

As for a real-world example, let's say we want to draw a path without MoveTo.

We can:

  • skip it and print a warning.
  • start at 0x0 (and print a warning). But it's an unexpected behavior (even if it will be noted in the docs).
  • return an error. But it will become a nightmare because it's a very popular method and in 99.99% cases everything will be ok.
  • design an API that will not allow this in the first place. Like:
    let path = Path::new().add_subpath(SubPath::new(0, 0).line_to(0.0));

Same goes for the transform methods. cairo-rs will panic if you pass a 0, 0, 0, 0, 0, 0 transform before rendering (Status::InvalidMatrix).

@raphlinus
Copy link
Contributor Author

These are deep questions. How rigorous do clients need to be in validating inputs before passing them to back-ends? Is a zero-scale transform invalid? How about one with a determinant of 1e-17? Different back-ends will vary in their strictness, which makes testing harder. Things are also different if the graphics are being generated from the app or from possibly malicious sources. I think one thing we can all agree on, if you open a malformed PDF, you don't want your viewer to just panic.

It seems to me like the proposal in this issue is a compromise between what @RazrFalcon would like, which is silent swallowing of errors, and the "everything is a Result" style that's the current code-base. I feel that I want to pass errors from back-ends up to apps in some way. However, it's possible that just logging them and then silently proceeding is good enough for developers.

I'm happy to discuss these questions on this issue, but ultimately I'd like to drive consensus. Is this proposal an improvement over the status quo? Is there something else that would be even better? There are many potential use-cases for piet, so it would be great to hear from a diversity of people.

@RazrFalcon
Copy link

Different back-ends will vary in their strictness, which makes testing harder.

Looks like writing a 2D library from scratch is not such a bad idea. =)

However, it's possible that just logging them and then silently proceeding is good enough for developers.

For example, QPainter doesn't have errors at all. If something is wrong then it simply ignores that part. And it doesn't panic either. But it does support rendering onto pixmaps, opengl, SVG, printers, and PDF.

So panics must be forbidden and Result should be used when an error can be handled in any meaningful way. So, for example, if image loading is failed - I can handle it. If a transform is invalid then that the app bug. Nothing we can do.

raphlinus added a commit that referenced this issue Feb 4, 2019
Reduce the amount of error checking that's necessary.

Closes #34
@raphlinus raphlinus mentioned this issue Feb 4, 2019
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

2 participants