Update piet-scene to depend on peniko #208

dfrg · 2022-11-22T19:56:39Z

This adds a new dependency on peniko, reintroduces kurbo for geometry (!), removes the now defunct types from piet-scene and updates all the test scenes to use the new types.

There is unfortunately a lot of churn here, but most of the changes should be relatively simple.

Depends on linebender/peniko#1

This adds a new dependency on peniko, reintroduces kurbo for geometry (!), removes the now defunct types from piet-scene and updates all the test scenes to use the new types.

DJMcNab · 2022-11-22T20:21:38Z

piet-scene is depended on by both the new and the old editions, to add to the context.

dfrg · 2022-11-22T20:33:34Z

piet-scene is depended on by both the new and the old editions, to add to the context.

Right, thanks. To expand on this a bit, piet-scene currently contains a set of types for defining rendering and composition state (brushes, blend modes, stroke/fill styles, etc) along with it's own geometry types (point, matrix). This lifts the former set of types to the peniko crate which now depends on kurbo for the latter set.

The goal of all this is to both reintroduce kurbo to piet-gpu and to provide a separate crate for these "styling" types that can be used across the ecosystem. At the very least, parley (text layout), the font enumeration library, and the oxidize glyph loader will make use of this.

also removes some annoying lingering tabs from blend.wgsl

dfrg · 2022-11-23T17:55:43Z

This is now ready to go. linebender/xilem#1 will need changes after this lands. Also depends on linebender/glazier#33 to sync kurbo versions.

raphlinus

Mostly looks good, but I want to understand empty clip paths.

raphlinus · 2022-11-23T19:26:36Z

piet-gpu/Cargo.toml

@@ -28,7 +28,6 @@ path = "../piet-gpu-types"

 [dependencies.piet-scene]
 path = "../piet-scene"
-features = ["kurbo"]


raphlinus · 2022-11-23T19:29:03Z

piet-scene/src/scene/builder.rs

 use smallvec::SmallVec;

 /// Builder for constructing a scene or scene fragment.
 pub struct SceneBuilder<'a> {
    scene: &'a mut SceneData,
-    layers: SmallVec<[BlendMode; 8]>,
+    layers: SmallVec<[(BlendMode, bool); 8]>,


I personally would find it useful to have a comment what the bool means.

raphlinus · 2022-11-23T19:31:08Z

piet-scene/src/scene/builder.rs

-        E: Iterator,
-        E::Item: Borrow<PathElement>,
-    {
+    fn encode_path(&mut self, shape: &impl Shape, is_fill: bool) -> bool {


This is another place where I'd like a comment explaining the bool result.

A further thought on this: when reading through kurbo code, I noticed that it treats a line/quad/curve at the start of a path as a move. This code currently assumes an implicit move to (0,0) in that case. Should I update this to be consistent with kurbo?

I'm slightly confused what you're asking. In BezPath builder methods, there's a debug assert against that (I consider it an invalid state). I think a reason to have an implicit move of (0, 0) is so you don't get an invalidly encoded path as output even when the input is wrong, but the exact value of what you get is not too important, people shouldn't be relying on it.

But perhaps I'm just misunderstanding.

It specifically noted for the Shape impl for slices here: https://github.com/linebender/kurbo/blob/fd839c25ea0c98576c7ce5789305822675a89938/src/bezpath.rs#L1177

The logic is implemented in the Segments iterator. FWIW, I think SVG does the same but I’d have to check the spec.

Ah yes, this behavior was changed in linebender/kurbo#98. That particular use case doesn't seem too important here, but probably having fewer different behaviors is better.

raphlinus · 2022-11-23T19:34:05Z

piet-scene/src/scene/builder.rs

+            self.begin_clip(blend);
+            self.layers.push((blend, true));
+        } else {
+            // When the layer has an empty path, record an entry to prevent


I think I'm confused what the semantics should be for an empty path. Putting blends aside, clipping to an empty path should suppress rendering of the children altogether (not super useful, but it should do the right thing). Here it seems it renders the children without a clip.

For blends, the path given should enclose the content, so an empty path with nonempty content is a client bug. I'm less concerned about correct semantics in that case.

I waffled on this as well but I agree with your reasoning here. I’ll encode a valid empty path here in the case of clips

raphlinus · 2022-11-23T19:36:54Z

piet-wgsl/shader/shared/blend.wgsl

-	return mix(
-		screen(cb, 2.0 * cs - 1.0),
-		cb * 2.0 * cs,
+    return mix(


Completely unrelated, but I think these should be select rather than mix, as the selector really is boolean. I have a draft patch with this change, simplification of vec3 where it's not needed, and fixes to make it more wgsl-analyzer friendly, but haven't pushed the patch because wgsl-analyzer still has some trouble.

Makes sense. This is mostly a direct port of the GLSL where ?: doesn’t work like that.

raphlinus · 2022-11-23T19:46:39Z

piet-gpu/src/samples.rs

        None,
-        make_diamond(1024.0, 100.0),
+        &&make_diamond(1024.0, 100.0)[..],


Double-ampersands not needed here.

Oops. Missed those when I was updating.

raphlinus · 2022-11-23T19:47:20Z

piet-gpu/src/samples.rs

        None,
-        &path,
+        &&path[..],


Apparently the double-ampersand is needed here, but it looks very strange. I didn't investigate too deeply whether there's a more ergonomic way to write this.

We can impl Shape for Vec<PathEl> in kurbo to fix this.

Hmmm. The way I've dealt with this in the path is use BezPath which is basically just a newtype for that, and I think I wanted to encourage people to use that rather than the raw vec. So I think the most idiomatic way to write this in this function is path = BezPath::new().

I don't want to rathole on this too much, but I do find that sample code often has a disproportionate impact, as people often use it as a starting point.

Fair enough! I believe it’s just a drop in replacement anyway.

raphlinus · 2022-11-23T20:13:53Z

Very happy for this to go in once my review comments are addressed. I'll be around to do an approve quickly.

dfrg · 2022-11-23T21:57:14Z

This should be ready now.

For the invalid layer shape case, I decided to encode an empty path regardless of whether it's a clip or blend. This makes sense since blends are also clips in the valid case.

In the future, I'd also like to consider moving the blend mode syncing between BeginClip/EndClip to the clip_leaf shader where path_ix is copied over. This would remove the need for accounting on the encoding side.

raphlinus

Looks good, thanks!

I think moving the blend mode syncing into clip_leaf makes sense but haven't thought it through in detail.

dfrg added 2 commits November 22, 2022 14:49

Update piet-scene to depend on peniko

f19dbdb

This adds a new dependency on peniko, reintroduces kurbo for geometry (!), removes the now defunct types from piet-scene and updates all the test scenes to use the new types.

prevent encoding empty paths for layers too

c1e91cb

update for improved kurbo/peniko ergonomics

fc1a6e9

also removes some annoying lingering tabs from blend.wgsl

dfrg mentioned this pull request Nov 23, 2022

Update for piet-scene changes linebender/xilem#5

Merged

raphlinus reviewed Nov 23, 2022

View reviewed changes

address review feedback

eccce74

raphlinus approved these changes Nov 23, 2022

View reviewed changes

dfrg merged commit 5dbeb99 into linebender:dev Nov 23, 2022

dfrg deleted the peniko branch November 23, 2022 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update piet-scene to depend on peniko #208

Update piet-scene to depend on peniko #208

dfrg commented Nov 22, 2022

DJMcNab commented Nov 22, 2022

dfrg commented Nov 22, 2022

dfrg commented Nov 23, 2022

raphlinus left a comment

raphlinus Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus Nov 23, 2022

dfrg Nov 23, 2022

raphlinus commented Nov 23, 2022

dfrg commented Nov 23, 2022

raphlinus left a comment

Update piet-scene to depend on peniko #208

Update piet-scene to depend on peniko #208

Conversation

dfrg commented Nov 22, 2022

DJMcNab commented Nov 22, 2022

dfrg commented Nov 22, 2022

dfrg commented Nov 23, 2022

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus commented Nov 23, 2022

dfrg commented Nov 23, 2022

raphlinus left a comment

Choose a reason for hiding this comment