Better error types #516

DasLixou · 2024-03-09T17:06:02Z

Using custom thiserror errors instead of Box<dyn Error>, closes #285

This removed the use of Result as a return type for many functions, including RenderContext::new(). This "could" fail when no backend feature is activated for wgpu, but because we don't vendor them, this should never happen. (also there is no failable version of this function)

I also had to make a RendererError which I totally hate, but the problem here is that without the wgpu-profiler feature this function never fails, so should I remove the Result and just unwrap for the GpuProfiler?

There are still many unwraps, which may(?) still be handled correctly with errors, but for most of them I'm not sure. They are in many functions which had Result return types but still, unwrap was called, so was it intention?

simbleau

I think this needs to be redone with a high-level VelloError, and only have variants which apply to helping a developer. Simply saying "channel was closed" in an error message is only confusing, for example, and doesn't address how this came about or how to fix it, or an event a developer cares about.

src/lib.rs

src/shaders.rs

src/util.rs

src/wgpu_engine.rs

src/lib.rs

src/util.rs

DasLixou · 2024-03-10T10:58:12Z

src/wgpu_engine.rs

-                                .get_gpu_buf(proxy.id)
-                                .ok_or("buffer for indirect dispatch not in map")?;
+                            let buf = self.bind_map.get_gpu_buf(proxy.id).ok_or(
+                                VelloError::UnavailableBufferUsed(proxy.name, "indirect dispatch"),


I made "indirect dispatch" and "download" strings here. Should I make an enum for those two so it is easier to match on the error?

I think it's fine, personally. Unless I hear a reason why a developer would need to differentiate between those cases.

I have a broader question - is there ever a reason we wouldn't find this buffer? Couldn't we just panic here? Theoretically, if our code is sound, would this bind map would always be found?

DasLixou · 2024-03-10T11:01:11Z

I would like to also make the CPU variant non-panic but there's no output of the function saying that it isn't available, just that it's in wrong format.
https://github.com/linebender/vello/blob/main/src/wgpu_engine.rs#L512

src/lib.rs

src/util.rs

src/shaders.rs

simbleau

Another preliminary pass. It looks better.

simbleau · 2024-04-12T22:50:58Z

src/lib.rs

+    /// Error when using a buffer inside a recording while it's not available.
+    /// Check if you have created it and not freed before its last usage.
+    #[cfg(feature = "wgpu")]
+    #[error("Buffer '{0}' is not available but used for {1}")]
+    UnavailableBufferUsed(&'static str, &'static str),


Should this only exist if the profiler feature is active, as the docs suggest?

Where is that written in the docs?

"/// Error when using a buffer inside a recording"

Aren't the recordings only captured when the wgpu-profiler feature is active? Genuinely not sure, need some guidance.

If it was unclear, I'm asking whether or not this should be

#[cfg(all(feature = "wgpu", feature = "wgpu-profiler"))]

src/lib.rs

src/util.rs

simbleau · 2024-04-12T22:57:34Z

src/wgpu_engine.rs

@@ -553,7 +552,7 @@ impl WgpuEngine {
                    let src_buf = self
                        .bind_map
                        .get_gpu_buf(proxy.id)
-                        .ok_or("buffer not in map")?;
+                        .ok_or(VelloError::UnavailableBufferUsed(proxy.name, "download"))?;


This error doesn't feel meaningful... is there a way for the developer to react if this is triggered? I don't think so... this error would only happen if we (as Vello maintainers) aren't handling the buffers correctly. In such case, I'd accept a panic here instead with a safety comment.

This is a question about where our abstraction boundaries lie. Concretely, there are effectively two parts:

The recording abstraction

Connecting that abstraction to the Vello shaders

As seen in #416, the recording can theoretically be used for additional functionality by end-users.
Our current API here is very much research quality, and so. I think it's likely that we'll want to have slightly more fine-grained error types for this in future (i.e. a different error at the recording/higher-level Vello levels), but this is fine for this PR.

waywardmonkeys · 2024-05-13T13:47:30Z

@DasLixou How far away from ready is this? Any chance it could be done for Vello 0.2?

DasLixou · 2024-05-13T13:53:54Z

I would have to look over this, will probably do a rebase, but IIRC nothing really blocked this, only some not-100%-sure decisions

DJMcNab

Sorry for not reviewing this earlier - some initial comments, although I've not done a full review

src/lib.rs

DasLixou · 2024-05-13T14:20:07Z

Sometimes merge conflicts are just hilarious 😂 (at least they're easy)

DasLixou · 2024-05-13T14:36:57Z

That should be it

DasLixou · 2024-05-13T14:37:31Z

Also just checked that everything runs fine

simbleau · 2024-05-13T17:42:58Z

I have many comments that are unresolved still

DJMcNab

Once this is rebased/updated with main we can land this. We aren't committing to any stability guarantees, so I think it's perfectly reasonable to land this as an improvement.

This doesn't really change the API we expect our users to use, as seen in with_winit not needing to be updated.

src/lib.rs

simbleau · 2024-05-19T16:58:17Z

This doesn't really change the API we expect our users to use, as seen in with_winit not needing to be updated.

That's because the example panics or discards most errors. Currently, that's not giving me the confidence I want yet.

I think approving this is a bit hasty as-is. It's definitely an improvement, but the error types will have a big effect on the crate going forward.

I'd prefer if we were specific with the feature flags on error types, whether VelloError should be non_exhaustive, and (personal opinion) get rid of the specialized Result type we've made, and generally address the comments that are withstanding so there is less uncertainty.

src/lib.rs

simbleau · 2024-05-21T16:47:25Z

Agree with Chad's comment above, but glad to see all the improvements in such a short turnaround. Nice work @DasLixou !

Approving

Addressed

xStrom

We want to get this in for Vello 0.2.

I resolved the merge conflict and addressed Chad's reasonable nit. I will merge this based on existing approvals. Additional changes are welcome as follow-up PRs.

Thank you for all your work @DasLixou.

simbleau requested changes Mar 9, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/shaders.rs Show resolved Hide resolved

src/util.rs Outdated Show resolved Hide resolved

src/wgpu_engine.rs Outdated Show resolved Hide resolved

DasLixou commented Mar 10, 2024

View reviewed changes

src/lib.rs Show resolved Hide resolved

DasLixou commented Mar 10, 2024

View reviewed changes

src/util.rs Outdated Show resolved Hide resolved

DasLixou commented Mar 10, 2024

View reviewed changes

DasLixou requested a review from simbleau March 10, 2024 10:59

Philipp-M reviewed Mar 10, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

DJMcNab mentioned this pull request Mar 12, 2024

Resurrect pico_svg and use it to replace vello_svg in the examples. #518

Merged

DasLixou mentioned this pull request Mar 29, 2024

Remove unsafety in ResourceId creation #534

Merged

DJMcNab mentioned this pull request Apr 11, 2024

RenderContext::new() needn't return Result. #547

Merged

waywardmonkeys previously requested changes Apr 12, 2024

View reviewed changes

src/util.rs Outdated Show resolved Hide resolved

src/shaders.rs Show resolved Hide resolved

simbleau requested changes Apr 12, 2024

View reviewed changes

waywardmonkeys added this to the Vello 0.2 release milestone May 3, 2024

waywardmonkeys mentioned this pull request May 3, 2024

add_shader on the engine doesn't return a failure. #564

Merged

DJMcNab reviewed May 13, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

DasLixou force-pushed the better-error-types branch from 9ca0e2e to bcd5ea1 Compare May 13, 2024 14:29

DasLixou requested a review from DJMcNab May 13, 2024 14:36

DJMcNab approved these changes May 19, 2024

View reviewed changes

simbleau reviewed May 19, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

simbleau reviewed May 19, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

DasLixou added 3 commits May 20, 2024 09:47

Better error types

22acb7a

Add Default for RenderContext

973cf69

Fix imports for non-wgpu-feature

9ce0072

DasLixou added 11 commits May 20, 2024 09:47

clippy

a4df4b6

One big VelloError

5d8d726

don't put thiserror::Error behind feature flag

4a625c8

make VResult allow dead code

7cf372c

rename VResult to Result

d4c78e0

Update Cargo.lock

30353a1

better docs

c743fb2

Rename to NoCompatibleDevice

f51e0c9

make fmt happy

07c3e4c

cleanup

44c6d2e

fix clippy

67139bd

DasLixou force-pushed the better-error-types branch from d2b6479 to 67139bd Compare May 20, 2024 07:48

Make VelloError non-exhaustive

f21cefb

dfrg reviewed May 20, 2024

View reviewed changes

src/lib.rs Show resolved Hide resolved

simbleau approved these changes May 21, 2024

View reviewed changes

xStrom added 3 commits May 30, 2024 18:18

Merge branch 'main' into better-error-types

364ee55

Fix broken Cargo.lock.

8922d81

Rename VelloError to Error.

f1a7281

xStrom approved these changes May 30, 2024

View reviewed changes

xStrom added this pull request to the merge queue May 30, 2024

Merged via the queue into linebender:main with commit 3842b3d May 30, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error types #516

Better error types #516

DasLixou commented Mar 9, 2024

simbleau left a comment

DasLixou Mar 10, 2024

simbleau May 19, 2024 •

edited

Loading

DasLixou commented Mar 10, 2024

simbleau left a comment

simbleau Apr 12, 2024

DasLixou Apr 13, 2024

simbleau Apr 14, 2024 •

edited

Loading

simbleau May 19, 2024 •

edited

Loading

simbleau Apr 12, 2024

DJMcNab May 19, 2024

waywardmonkeys commented May 13, 2024

DasLixou commented May 13, 2024

DJMcNab left a comment

DasLixou commented May 13, 2024

DasLixou commented May 13, 2024

DasLixou commented May 13, 2024

simbleau commented May 13, 2024

DJMcNab left a comment

simbleau commented May 19, 2024

simbleau commented May 21, 2024

xStrom left a comment

Better error types #516

Better error types #516

Conversation

DasLixou commented Mar 9, 2024

simbleau left a comment

Choose a reason for hiding this comment

DasLixou Mar 10, 2024

Choose a reason for hiding this comment

simbleau May 19, 2024 • edited Loading

Choose a reason for hiding this comment

DasLixou commented Mar 10, 2024

simbleau left a comment

Choose a reason for hiding this comment

simbleau Apr 12, 2024

Choose a reason for hiding this comment

DasLixou Apr 13, 2024

Choose a reason for hiding this comment

simbleau Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

simbleau May 19, 2024 • edited Loading

Choose a reason for hiding this comment

simbleau Apr 12, 2024

Choose a reason for hiding this comment

DJMcNab May 19, 2024

Choose a reason for hiding this comment

waywardmonkeys commented May 13, 2024

DasLixou commented May 13, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

DasLixou commented May 13, 2024

DasLixou commented May 13, 2024

DasLixou commented May 13, 2024

simbleau commented May 13, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

simbleau commented May 19, 2024

simbleau commented May 21, 2024

xStrom left a comment

Choose a reason for hiding this comment

simbleau May 19, 2024 •

edited

Loading

simbleau Apr 14, 2024 •

edited

Loading

simbleau May 19, 2024 •

edited

Loading