New scene/resource API #168
Conversation
This commit implements actual rendering for the new piet-scene crate. In particular, it adds a new EncodedSceneRef type in piet_gpu::encoder to store references to the raw stream data. This is used as a proxy for both PietGpuRenderContext and piet_scene::scene::Scene to feed data into the renderer. Much of this is still hacky for expedience and because the intention is to be a transitional state that maintains both interfaces until we can move to the new scene API and add a wrapper for the traditional piet API. The general structure and relationships between these crates need further discussion.
This exposes a new uniform API for generating scene fragments for glyph outlines.
This allows rendering from raw stream data and may be temporary depending on the crate structure after the traditional piet api is removed.
First cut at a public C api that supports glyph rendering on Metal targets.
Changes the return mutability for pgpu_glyph_provider_get()
This should fix channel swizzling on M1 and restore functionality on Intel
raphlinus
left a comment
raphlinus
left a comment
There was a problem hiding this comment.
Overall looks good to me, and quite ambitious the way it subsumes so much of the encoding task. I have some suggestions for improvements, but none blocking - I'd be happy to build future stuff on top of this, and continue to iterate quality-of-life features such as kurbo conversions.
| @@ -0,0 +1,140 @@ | |||
| #include <cstdarg> | |||
There was a problem hiding this comment.
I think this file is auto-generated? It's actually a bit hard to tell as some of the comments look hand-written. In any case, would it be possible to add a line at or near the top explaining what's going on?
There was a problem hiding this comment.
This is auto-generated. I'll configure the generator to add a comment at the top marking it as such.
piet-scene/Cargo.toml
Outdated
| pinot = "0.1.5" | ||
| moscato = "0.1.2" | ||
|
|
||
| # remove these and move demo to another directory |
There was a problem hiding this comment.
Should this comment be labeled "TODO"?
There was a problem hiding this comment.
Oops. Good catch. I'm happy to just remove the binary altogether along with its dependencies. I believe we discussed adding a more structured example framework to the project and it seems like "soon" might be a good time to do that.
piet-scene/src/brush/color.rs
Outdated
|
|
||
| pub fn rgb<C: Into<f64>>(r: C, g: C, b: C) -> Self { | ||
| Self::rgb8( | ||
| (r.into() / 255.0) as u8, |
There was a problem hiding this comment.
It might be worth looking at the piet sources for this. This conversion neither rounds nor clamps to (0..=255) so I don't think it's quite ideal. Given that it's low level, it might not be important surfacing the float versions here.
There was a problem hiding this comment.
Thanks for not pointing out that all of these divisions should be multiplications anyway :) Clearly I never made use of the float versions. I'll go ahead and drop them.
piet-scene/src/brush/color.rs
Outdated
| } | ||
|
|
||
| pub fn to_premul_u32(self) -> u32 { | ||
| let a = self.a as f64 / 255.0; |
There was a problem hiding this comment.
I usually write this as a multiplication by (1.0 / 255.0), as this generates divsd (checked in godbolt). Also I think it would be better with rounding.
There was a problem hiding this comment.
Always nice to facilitate better codegen. Will make both of these changes.
| @@ -0,0 +1,225 @@ | |||
| // Copyright 2022 The piet-gpu authors. | |||
There was a problem hiding this comment.
Some of this code seems pretty familiar. That's ok, but maybe it should say something like "based in part on kurbo."
Similarly, a pretty common pattern would be to have kurbo as an optional dependency, and have conversions in place. I'm not sure that should be done this round, perhaps in future. I think the Rust traits for approximate conversions (f64->f32 and friends) were still a bit controversial last time I took a serious look at this, not sure if that's been settled by now.
There was a problem hiding this comment.
I did pull most of the Affine functions from kurbo. I'll add an attribution.
Happy to prep another PR adding the optional dependency along with the conversions and changing some signatures to take impl Into<T> for the appropriate types.
* Change pgpu-render header file generator to add a comment noting that the file is auto-generated * Remove bin target and associated dependencies from piet-scene crate * Remove FP constructors from the Color type * Better codegen and rounding in Color::to_premul_u32() * Add kurbo attribution for piet_scene::geometry module
2 participants
This is a rough but functioning cut of a new scene API supporting both retained, composable fragments and the initial foundation for a resource framework. In addition, this includes a C API that is (for now) designed specifically for rendering glyphs on Metal.
There are no significant changes to existing code, but the PR is large and will definitely require some (perhaps not so) fine tuning before merging.