linebender / parley

Rich text layout library
Apache License 2.0
209 stars 24 forks source link

Implement Tiny-Skia example #55

Closed nicoburns closed 4 months ago

nicoburns commented 4 months ago

This example uses skrifa and tiny-skia rather than swash and image as in #54.

Output

tiny_skia_render

Notes

nicoburns commented 4 months ago

Maybe these should have their own Cargo.toml so that these things don't all end up being dev dependencies and polluting stuff. That might be better if this repo were using the multi-crate layout...

Yeah, that makes sense. ~I believe you can have example be their own crate and have them still work as examples if you set things up correctly (reference them from the Cargo.toml)~ (EDIT: maybe not. You can certainly have them in a non-standard location but perhaps not as their own crates. It's probably worth making them their own crates anyway).

xorgy commented 4 months ago

Perhaps understandably, I am partial to the example which renders plain text correctly.. but I agree this looks simpler. I also second Bruce's feedback.

xorgy commented 4 months ago

Do you think rendering errors are related to a different default fill rule or something similarly easy to change?

dfrg commented 4 months ago

Do you think rendering errors are related to a different default fill rule or something similarly easy to change?

Yes, the fill rule should be non-zero. Looks like it’s set to even-odd here.

nicoburns commented 4 months ago

Do you think rendering errors are related to a different default fill rule or something similarly easy to change?

Yes, the fill rule should be non-zero. Looks like it’s set to even-odd here.

It is EvenOdd. Tiny-Skia has FillRule::EvenOdd and FillRule::Winding (https://docs.rs/tiny-skia/latest/tiny_skia/enum.FillRule.html), but both seem to produce the same result.

nicoburns commented 4 months ago

Don't really understand these CI failures. The example crate compiles fine if specifically targeted with cargo run -p tiny_skia_render or if cargo run is run from it's directory but with cargo test --workspace it fails to find the tiny_skia dependency. Possible cargo bug?

I'm thinking we ought to land this without making it into it's own crate, and we can do the workspace changes in a followup?

waywardmonkeys commented 4 months ago

The reason is that it is getting compiled 2 ways: One via the example and one via parley picking it up as an example.

That won't happen if you put main.rs in a src directory.

waywardmonkeys commented 4 months ago

@nicoburns You'll need to remove a couple of lines from your example's Cargo.toml as well.

nicoburns commented 4 months ago

@dfrg What's the current situation regarding rendering emoji with skrifa?

dfrg commented 4 months ago

@dfrg What's the current situation regarding rendering emoji with skrifa?

Emoji is going to be a heavy lift in this example.

  • I can see the ColorPainter trait which seems to be for COLRv1 (and presumably applies to COLRv0?). Although it doesn't look that easy to implement on top of tiny-skia. I think we'd need to do the transforms and clipping ourselves? And I can't find any example implementation anywhere.

Yes, this covers COLRv1 and v0. The mapping is not difficult but is going to be very tedious. This is implemented in Skia both here: https://github.com/google/skia/blob/907372c4cceee9ab7cf30c8043eba9afd837681e/src/ports/fontations/src/ffi.rs#L148 and here: https://github.com/google/skia/blob/907372c4cceee9ab7cf30c8043eba9afd837681e/src/ports/SkTypeface_fontations.cpp#L1158

Also, tiny-skia is missing some functionality (full two-point conical gradients, sweep gradients and a couple blend modes, I think). We'll probably need to add this functionality ourselves.

Correct. We have low-level support in read-fonts and for Skia, the additional logic was implemented directly there because we never settled on an API for skrifa. Most of that bitmap logic is here: https://github.com/google/skia/blob/907372c4cceee9ab7cf30c8043eba9afd837681e/src/ports/fontations/src/ffi.rs#L1037

edit: I don't think we should block this PR on emoji support

nicoburns commented 4 months ago

edit: I don't think we should block this PR on emoji support

I am inclined to agree. And given that, I believe this PR may now be ready :)