Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sweep gradients: they were buggy, and the spec is changing #17

Open
horasio opened this issue May 12, 2021 · 7 comments
Open

Fix sweep gradients: they were buggy, and the spec is changing #17

horasio opened this issue May 12, 2021 · 7 comments

Comments

@horasio
Copy link
Contributor

horasio commented May 12, 2021

In particular, extend modes seem to behave differently, in the spec, for sweep gradient than for linear and radial ones.
For example, normalizing the colorLine seems unwated for sweeps, but more processing of the color linee be required, for simulating repeat, pad, mirror.
Solutions will be different for Skia on one hand, and the other backends on the other hand.

@justvanrossum
Copy link
Collaborator

Also: the Skia backend doesn't currently implement the sweep extend mode correctly: the extend mode should not extend before the start angle and after end angle (???).

@drott
Copy link

drott commented Apr 13, 2022

First of all, big thanks @justvanrossum for the report on Chromium/Skia using the incorrect start axis (vertical up instead of horizontal right). I filed the following issues after further investigation: https://bugs.chromium.org/p/skia/issues/detail?id=13208, https://bugs.chromium.org/p/skia/issues/detail?id=13209, https://bugs.chromium.org/p/skia/issues/detail?id=13210. So the start angle was wrong, repeat modes didn't work, the sector was cut, and sectors crossing 0 did not work.

To fix this, I increased test coverage and improved the Skia implementation.

In the following I try to break down some observations of what I am seeing in BlackRenderer to hopefully get to a common understanding and help with debugging issues in BlackRenderer for sweeps.

New Test Glyphs

googlefonts/color-fonts#98 contains additional test glyphs for sweep (now merged to main)

Skia Fix and Results

My fix CL for Skia is in:
https://skia-review.googlesource.com/c/skia/+/529460

With that I get the following, in our opinion correct results (rows are extend modes pad, reflect, repeat respectively). Please do comment if you think some interpretations/renderings are still wrong as that may well be possible.

image

BlackRenderer results

Extend mode pad:

$ blackrenderer ~/dev/skia/resources/fonts/more_samples-glyf_colr_1.ttf "a456789α" sweeps.png --font-size=100

The tests glyphs are:

_sample_sweep(-360, 0, "pad", "narrow", next(access_chars)),
_sample_sweep(0, 90, "pad", "narrow", next(access_chars)),
_sample_sweep(45, 90, "pad", "narrow", next(access_chars)),
_sample_sweep(247.5, 292.5, "pad", "narrow", next(access_chars)),
_sample_sweep(90, 270, "pad", "narrow", next(access_chars)),
_sample_sweep(-270, 270, "pad", "narrow", next(access_chars)),
_sample_sweep(-45, 45, "pad", "narrow", next(access_chars)),
_sample_sweep(315, 45, "pad", "narrow", next(access_chars)),

image

This shows:

  • Start color red is not drawn only from start angle, but from 0 degrees, but color stop 0 should be mapped to start angle.
  • Issues when start Angle is negative.
  • Issues when angle valus cross 0/360 degrees. (last three images)

Extend mode reflect

$ blackrenderer ~/dev/skia/resources/fonts/more_samples-glyf_colr_1.ttf "βγδεζηθι" sweeps_reflect.png --font-size=100

_sample_sweep(-360, 0, "reflect", "narrow", next(access_chars)),
_sample_sweep(0, 90, "reflect", "narrow", next(access_chars)),
_sample_sweep(45, 90, "reflect", "narrow", next(access_chars)),
_sample_sweep(247.5, 292.5, "reflect", "narrow", next(access_chars)),
_sample_sweep(90, 270, "reflect", "narrow", next(access_chars)),
_sample_sweep(-270, 270, "reflect", "narrow", next(access_chars)),
_sample_sweep(-45, 45, "reflect", "narrow", next(access_chars)),
_sample_sweep(315, 45, "reflect", "narrow", next(access_chars)),

image

Issues, as much as I can tell:

  • Start angle (in first picture) starts from vertical position, not from horizontal
  • The reflecting sector seems too small?

Extend mode repeat

$ blackrenderer ~/dev/skia/resources/fonts/more_samples-glyf_colr_1.ttf "κλμνξοπρ" sweeps_repeat.png --font-size=100

_sample_sweep(-360, 0, "repeat", "narrow", next(access_chars)),
_sample_sweep(0, 90, "repeat", "narrow", next(access_chars)),
_sample_sweep(45, 90, "repeat", "narrow", next(access_chars)),
_sample_sweep(247.5, 292.5, "repeat", "narrow", next(access_chars)),
_sample_sweep(90, 270, "repeat", "narrow", next(access_chars)),
_sample_sweep(-270, 270, "repeat", "narrow", next(access_chars)),
_sample_sweep(-45, 45, "repeat", "narrow", next(access_chars)),
_sample_sweep(315, 45, "repeat", "narrow", next(access_chars)),

image

Issues, as much as I can tell, similar to reflect:

  • Start angle (in first picture) starts from vertical position, not from horizontal
  • The reflecting sector seems too small?

@drott
Copy link

drott commented Apr 20, 2022

I think I have the reflect and repeat modes wrong and the observation that the

  • The reflecting sector seems too small?
    is incorrect.

For the

_sample_sweep(0, 90, "reflect", "narrow", next(access_chars)),

test shown in my second row, second image from the left, there's an implicit "pad" that fills the 0-90 degree angle, which is incorrect.

I'll update this report after fixing that.

@drott
Copy link

drott commented Apr 20, 2022

Also, contrary to what I said earlier:

image

This shows:

Start color red is not drawn only from start angle, but from 0 degrees, but color stop 0 should be mapped to start angle.

Starting from start angle 0, and padding with red seems correct and is not an issue.

@drott
Copy link

drott commented Apr 21, 2022

With WIP Skia CL #532057 we are getting closer but we still need some clarifications on what happens at the 0 / 360 degree boundary.

Extend mode pad:

Test glyphs

_sample_sweep(-360, 0, "pad", "narrow", next(access_chars)),
_sample_sweep(0, 90, "pad", "narrow", next(access_chars)),
_sample_sweep(45, 90, "pad", "narrow", next(access_chars)),
_sample_sweep(247.5, 292.5, "pad", "narrow", next(access_chars)),
_sample_sweep(90, 270, "pad", "narrow", next(access_chars)),
_sample_sweep(-270, 270, "pad", "narrow", next(access_chars)),
_sample_sweep(-45, 45, "pad", "narrow", next(access_chars)),
_sample_sweep(315, 45, "pad", "narrow", next(access_chars)),

$ blackrenderer ~/dev/skia/resources/fonts/more_samples-glyf_colr_1.ttf "a456789α" sweeps.png --font-size=100
image

Skia
image

  • _sample_sweep(-270, 270, "pad", "narrow", next(access_chars)), does not match in BlackRenderer and Skia, even though this glyph IMO should be equivalent to _sample_sweep(90, 270, "pad", "narrow", next(access_chars)),.
  • Also, not sure why we diverge on _sample_sweep(315, 45, "pad", "narrow", next(access_chars)),.
  • This result however is based on certain assumptions that are not 100% clear in the spec, see Clarifications for Sweep gradients needed googlefonts/colr-gradients-spec#361 (comment)

Extend mode reflect

Test glyphs

_sample_sweep(-360, 0, "reflect", "narrow", next(access_chars)),
_sample_sweep(0, 90, "reflect", "narrow", next(access_chars)),
_sample_sweep(45, 90, "reflect", "narrow", next(access_chars)),
_sample_sweep(247.5, 292.5, "reflect", "narrow", next(access_chars)),
_sample_sweep(90, 270, "reflect", "narrow", next(access_chars)),
_sample_sweep(-270, 270, "reflect", "narrow", next(access_chars)),
_sample_sweep(-45, 45, "reflect", "narrow", next(access_chars)),
_sample_sweep(315, 45, "reflect", "narrow", next(access_chars)),

$ blackrenderer ~/dev/skia/resources/fonts/more_samples-glyf_colr_1.ttf "βγδεζηθι" sweeps_reflect.png --font-size=100

image

Skia
image

Similarly quite well aligned, not sure what happens in BlackRenderer on the 6th and 8th glyph, which should IMO be equivalent to 5th and 7th respectively.

Extend mode repeat

Test glyphs

_sample_sweep(-360, 0, "repeat", "narrow", next(access_chars)),
_sample_sweep(0, 90, "repeat", "narrow", next(access_chars)),
_sample_sweep(45, 90, "repeat", "narrow", next(access_chars)),
_sample_sweep(247.5, 292.5, "repeat", "narrow", next(access_chars)),
_sample_sweep(90, 270, "repeat", "narrow", next(access_chars)),
_sample_sweep(-270, 270, "repeat", "narrow", next(access_chars)),
_sample_sweep(-45, 45, "repeat", "narrow", next(access_chars)),
_sample_sweep(315, 45, "repeat", "narrow", next(access_chars)),

$ blackrenderer ~/dev/skia/resources/fonts/more_samples-glyf_colr_1.ttf "κλμνξοπρ" sweeps_repeat.png --font-size=100

image

Skia
image

Similarly quite well aligned, not sure what happens in BlackRenderer on the 6th and 8th glyph, which should IMO be equivalent to 5th and 7th respectively.

@justvanrossum
Copy link
Collaborator

Also relevant: fonttools/fonttools#2743

@justvanrossum justvanrossum changed the title Make sure the implementations of sweep gradients adhere to the spec Fix sweep gradients: they were buggy, and the spec is changing Aug 15, 2022
@drott
Copy link

drott commented Aug 15, 2022

New, wider set of tests in googlefonts/color-fonts#119, Chrome rendering with https://skia-review.googlesource.com/c/skia/+/566416

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants