Implement Tiny-Skia example #55

nicoburns · 2024-05-21T14:52:47Z

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

Output

Notes

This has the same issue as Implement simple example using swash to render to png #54 around emoji rendering not working / being implemented
There is an issue with the centers of closed letter forms incorrectly being filled
The code for this is IMO much nicer than the swash version (at the cost of the heavier tiny-skia dependency)
This example does not have hinting enabled

waywardmonkeys

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...

nicoburns · 2024-05-21T15:22:47Z

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 · 2024-05-21T19:54:14Z

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 · 2024-05-21T19:57:13Z

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

dfrg · 2024-05-21T21:44:36Z

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 · 2024-05-21T22:11:12Z

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.

examples/tiny-skia.rs

nicoburns · 2024-05-22T12:41:38Z

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 · 2024-05-22T13:17:33Z

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 · 2024-05-22T13:22:37Z

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

waywardmonkeys · 2024-05-22T13:30:45Z

.github/workflows/ci.yml

@@ -11,7 +11,9 @@ env:
  RUST_MIN_VER: "1.70"
  # List of packages that will be checked with the minimum supported Rust version.
  # This should be limited to packages that are intended for publishing.
-  RUST_MIN_VER_PKGS: "-p parley -p fontique"
+  PRIMARY_PKGS: "-p parley -p fontique"


You don't use PRIMARY_PKGS below, but you do use PUBIC_PKGS ...

Is the purpose of renaming this away from RUST_MIN_VER_PKGS to denote which are published and not?

We're running check/build/test/doc etc over parley and fontique crates with --features std and --features libm, neither of which work with the tiny_skia_render example crate because it doesn't have those features. So we can't use --workspace to run those checks. So I'm using this "primary packages" variable to do specify "parley and fontique".

examples/tiny_skia_render/Cargo.toml

examples/tiny_skia_render/src/main.rs

nicoburns · 2024-05-22T17:22:08Z

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

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.
This issue ([skrifa] CBDT/CBLC and SBIX bitmap color font access interfaces googlefonts/fontations#593) suggests that there is not currently any support for bitmap emoji formats in skrifa. Is that correct?

dfrg · 2024-05-22T17:31:11Z

@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.

This issue ([skrifa] CBDT/CBLC and SBIX bitmap color font access interfaces googlefonts/fontations#593) suggests that there is not currently any support for bitmap emoji formats in skrifa. Is that correct?

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 · 2024-05-22T18:38:31Z

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 :)

dfrg

I can’t really speak for the CI stuff (which seems fine?) but the code all LGTM. Thanks for taking this on!

src/style/brush.rs

xorgy

It's looking good to me now. I love the detailed comments in the actual example code.

Changes have been addressed. And we have too many PRs in flight. We can fix any outstanding issues in follow up PRs

With #55 we introduced a package that doesn't have the `std` feature. The CI was adjusted to cope with that. This PR here improves support for such packages in key ways: * Automatically support it using the `--ignore-unknown-features` flag. * Remove the manual listing of `PRIMARY_PKGS` and `EXAMPLE_PKGS` and restore usage of `--workspace` so that the CI will automatically pick up new packages. * Restores support for "primary packages" that don't need to follow MSRV.

nicoburns requested review from dfrg and xorgy May 21, 2024 14:52

waywardmonkeys reviewed May 21, 2024

View reviewed changes

nicoburns force-pushed the tiny-skia-example branch from 77fb6be to 49551e7 Compare May 21, 2024 15:29

nicoburns force-pushed the tiny-skia-example branch from 49551e7 to 67c8ab7 Compare May 21, 2024 22:14

dfrg reviewed May 21, 2024

View reviewed changes

examples/tiny-skia.rs Outdated Show resolved Hide resolved

dfrg reviewed May 21, 2024

View reviewed changes

examples/tiny-skia.rs Outdated Show resolved Hide resolved

nicoburns force-pushed the tiny-skia-example branch from 67c8ab7 to 877ac29 Compare May 21, 2024 22:33

DJMcNab mentioned this pull request May 22, 2024

add emoji family for emoji clusters #56

Merged

nicoburns force-pushed the tiny-skia-example branch 4 times, most recently from b9a9773 to 439b079 Compare May 22, 2024 11:52

nicoburns force-pushed the tiny-skia-example branch 3 times, most recently from 7d77eca to cd9d6a7 Compare May 22, 2024 13:35

waywardmonkeys previously requested changes May 22, 2024

View reviewed changes

waywardmonkeys requested a review from xStrom May 22, 2024 14:56

nicoburns mentioned this pull request May 22, 2024

Implement simple example using swash to render to png #54

Merged

dfrg reviewed May 22, 2024

View reviewed changes

examples/tiny_skia_render/src/main.rs Show resolved Hide resolved

nicoburns added 11 commits May 22, 2024 19:26

Implement Skrifa + Tiny-Skia example

7399af2

Make tiny_skia_render example it's own crate

35c4b23

Make tiny_skia_render output location invariant of CWD

0bb6e17

Ignore examples when checking with libm feature

ac6091c

CI fixes

684dcd5

Ci fixes linebender#2

379da7c

Move example main.rs to src directory

e352beb

Simplify background fill

68463da

Prevent example from being published

8c4649c

Remove unused import

71c4b15

Fix env var typo

09fbc75

nicoburns force-pushed the tiny-skia-example branch from c2cb2b6 to 09fbc75 Compare May 22, 2024 18:26

nicoburns added 3 commits May 22, 2024 19:30

Implement normalized coords

c440aed

Cargo update

6ee4fca

Add note that emoji rendering is not implemented

66a9aca

dfrg approved these changes May 22, 2024

View reviewed changes

src/style/brush.rs Show resolved Hide resolved

xorgy approved these changes May 23, 2024

View reviewed changes

nicoburns added this pull request to the merge queue May 23, 2024

Merged via the queue into linebender:main with commit 94c5d78 May 23, 2024
19 checks passed

nicoburns deleted the tiny-skia-example branch May 23, 2024 13:03

xStrom mentioned this pull request May 23, 2024

Add automatic CI support for packages without the std feature. #63

Merged

This was referenced May 23, 2024

Ignore this CI test. #64

Closed

Ignore this CI test, take two. #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Tiny-Skia example #55

Implement Tiny-Skia example #55

nicoburns commented May 21, 2024 •

edited

Loading

waywardmonkeys left a comment

nicoburns commented May 21, 2024 •

edited

Loading

xorgy commented May 21, 2024

xorgy commented May 21, 2024

dfrg commented May 21, 2024

nicoburns commented May 21, 2024

nicoburns commented May 22, 2024

waywardmonkeys commented May 22, 2024

waywardmonkeys commented May 22, 2024

waywardmonkeys May 22, 2024

nicoburns May 22, 2024 •

edited

Loading

nicoburns commented May 22, 2024

dfrg commented May 22, 2024 •

edited

Loading

nicoburns commented May 22, 2024

dfrg left a comment

xorgy left a comment

Implement Tiny-Skia example #55

Implement Tiny-Skia example #55

Conversation

nicoburns commented May 21, 2024 • edited Loading

Output

Notes

waywardmonkeys left a comment

Choose a reason for hiding this comment

nicoburns commented May 21, 2024 • edited Loading

xorgy commented May 21, 2024

xorgy commented May 21, 2024

dfrg commented May 21, 2024

nicoburns commented May 21, 2024

nicoburns commented May 22, 2024

waywardmonkeys commented May 22, 2024

waywardmonkeys commented May 22, 2024

waywardmonkeys May 22, 2024

Choose a reason for hiding this comment

nicoburns May 22, 2024 • edited Loading

Choose a reason for hiding this comment

nicoburns commented May 22, 2024

dfrg commented May 22, 2024 • edited Loading

nicoburns commented May 22, 2024

dfrg left a comment

Choose a reason for hiding this comment

xorgy left a comment

Choose a reason for hiding this comment

nicoburns commented May 21, 2024 •

edited

Loading

nicoburns commented May 21, 2024 •

edited

Loading

nicoburns May 22, 2024 •

edited

Loading

dfrg commented May 22, 2024 •

edited

Loading