black-renderer icon indicating copy to clipboard operation
black-renderer copied to clipboard

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

Open horasio opened this issue 3 years ago • 7 comments

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.

horasio avatar May 12 '21 14:05 horasio

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 (???).

justvanrossum avatar May 12 '21 14:05 justvanrossum

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

https://github.com/googlefonts/color-fonts/pull/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 avatar Apr 13 '22 15:04 drott

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 avatar Apr 20 '22 13:04 drott

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 avatar Apr 20 '22 16:04 drott

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 https://github.com/googlefonts/colr-gradients-spec/issues/361#issuecomment-1105250860

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.

drott avatar Apr 21 '22 14:04 drott

Also relevant: https://github.com/fonttools/fonttools/pull/2743

justvanrossum avatar Aug 15 '22 12:08 justvanrossum

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

image

drott avatar Aug 15 '22 12:08 drott