Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Support for TwoPointConical gradients #5275

Merged
merged 1 commit into from
May 16, 2018

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented May 16, 2018

Engine side fix for flutter/flutter#17106

This patch updates Gradient.radial to support an optional focal Offset and radius. If these are provided, it the call will instantiate an SkTwoPointConicalGradient instead of plain old SkRadialGradient.

Skia fiddle here: https://fiddle.skia.org/c/adc33d57c035fcb8bcbd4c3ad6381c13
Interesting/relevant reading for Skia end: https://skia.org/dev/design/conical

I initially wanted to name this something like Gradient.conical (similar to Skia's naming convention), but I'm afraid that will cause confusion for a lot of users who google "conical gradients" and find stuff about planned support for what Skia calls Sweep gradients under that name (in fact, I got a bit muddled myself before realizing that the terms are not being used identically by Skia and the proposed CSS standard/top Google search results). The tiling mode documentation should not need to be changed for this.

My primary use case for this is to enable better support of SVG's <radialGradient> element, but this is not SVG specific.

There'd be a similar update in Flutter SDK to support this in ui.Gradient.radial over there.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Hixie for comments on Dart code.

Float64List matrix4
Float64List matrix4,
Offset focal,
double focalRadius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh man, we should have used named arguments. this has become sad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah :( but that'd be a a breaking change. I suppose it could be another method and deprecate this one....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could do another method, but as you point out in the commit message, that's got its own problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just do a good job of the framework wrapper, then this particular method won't matter much.

@Hixie
Copy link
Contributor

Hixie commented May 16, 2018

LGTM

@aam
Copy link
Member

aam commented May 17, 2018

This has broken flutter tests(once this lands in flutter/flutter).

╰─➤  ../../bin/flutter  --local-engine host_debug_unopt test test/painting/gradient_test.dart
00:05 +0: loading $FH/flutter/packages/flutter/test/painting/gradient_test.dart                     Shell: Do not know weight for: TeXGyreSchola (BoldItalic) 
00:06 +5 -1: RadialGradient with AlignmentDirectional [E]                                                                                  
  Expected: return normally
    Actual: <Closure: () => Shader>
     Which: threw ?:<'dart:ui/painting.dart': Failed assertion: line 2330 pos 14: 'center != Offset.zero || focal != Offset.zero': is not true.>
  
  package:test                                        expect
  package:flutter_test/src/widget_tester.dart 147:16  expect
  test/painting/gradient_test.dart 176:5              main.<fn>
  
00:06 +10 -1: Some tests failed.   

@dnfield
Copy link
Contributor Author

dnfield commented May 17, 2018

Urfh. Will fix but looks like it should be reverted til then. The assertion should also pass if focal is null.

aam added a commit to aam/engine that referenced this pull request May 17, 2018
This reverts commit 919e8c2 as it
breaks flutter gradient_test.
aam added a commit that referenced this pull request May 17, 2018
This reverts commit 919e8c2 as it
breaks flutter gradient_test.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants