Enable more linear gradient fill cases, including animated linear gradients. #146

simeoncran · 2019-08-31T00:04:10Z

Linear gradient fills should work fine for common cases now.

…dients.

eliezerpMS · 2019-08-31T00:15:28Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+
+                var gradientStopKeyFrames = stops.KeyFrames.ToArray();
+
+                // Create the Composition stops and animated them.


animated [](start = 52, length = 8)

nit typo: animate #Closed

eliezerpMS · 2019-08-31T00:52:07Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

            }

            return result;
        }

+        static IEnumerable<KeyFrame<T>> ExtractKeyFramesFromGradientStopKeyFrames<T>(KeyFrame<Sequence<GradientStop>>[] stops, int stopIndex, Func<GradientStop, T> selector)


static IEnumerable<KeyFrame> ExtractKeyFramesFromGradientStopKeyFrames(KeyFrame<Sequence>[] stops, int stopIndex, Func<GradientStop, T> selector) [](start = 7, length = 166)

Probably make this line shorter #Closed

eliezerpMS · 2019-08-31T00:52:32Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                    var gradientStop = _c.CreateColorGradientStop();
+                    colorStops.Add(gradientStop);
+
+                    var colorKeyFrames = ExtractKeyFramesFromGradientStopKeyFrames(gradientStopKeyFrames, i, gs => MultiplyColorByOpacityPercent(MultiplyColorByOpacityPercent(gs.Color, gs.OpacityPercent), opacityPercentValue)).ToArray();


var colorKeyFrames = ExtractKeyFramesFromGradientStopKeyFrames(gradientStopKeyFrames, i, gs => MultiplyColorByOpacityPercent(MultiplyColorByOpacityPercent(gs.Color, gs.OpacityPercent), opacityPercentValue)).ToArray(); [](start = 20, length = 217)

should make this line not as long #Closed

eliezerpMS · 2019-08-31T00:54:33Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                    var gradientStop = _c.CreateColorGradientStop();
+                    colorStops.Add(gradientStop);
+
+                    var colorKeyFrames = ExtractKeyFramesFromGradientStopKeyFrames(gradientStopKeyFrames, i, gs => MultiplyColorByOpacityPercent(MultiplyColorByOpacityPercent(gs.Color, gs.OpacityPercent), opacityPercentValue)).ToArray();


MultiplyColorByOpacityPercent(MultiplyColorByOpacityPercent(gs.Color, gs.OpacityPercent), opacityPercentValue) [](start = 115, length = 110)

Make a function for the MultiplyColorByOpacityPercent passed to MultiplyColorByOpacityPercent perhaps along with a comment since the idea of propagating opacity in addition to the opacity on the gradient stops is something we should call out that we are supporting. #Closed

Not sure what you are asking for with the comment.

In reply to: 319705117 [](ancestors = 319705117)

Comment is no longer relevant with the new checks at the top of this function.

In reply to: 320396945 [](ancestors = 320396945,319705117)

eliezerpMS · 2019-08-31T01:04:33Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                    var color = MultiplyColorByOpacityPercent(MultiplyColorByOpacityPercent(stop.Color, stop.OpacityPercent), opacityPercentValue);
+                    if (color == null)
+                    {
+                        // We currently do not support gradients that contain separate opacity stops.


// We currently do not support gradients that contain separate opacity stops. [](start = 24, length = 77)

Suggestion: Maybe this comment can read "We currently do not support gradient stops that are opacity or color only"

From reviewing what MultiplyColorByOpacityPercent does and I assume that Lotties could have stops that are opacity but not color as well? #Closed

Updated. We do support color only. It's the opacity-only ones that we can't support (because we don't know what color to use). Eventually we will handle this by creating a ShapeVisual per Shape/Group and putting the opacity on the ShapeVisual

In reply to: 319705548 [](ancestors = 319705548)

* LottieGen reports stats to a TSV file instead of CSV. Added a more general stats reporting mechanism. * LottieGen: eliminate Verbose option and replace with much better stats file outputs. * CR feedback.

…dients.

…stops. Ignore opacity if it is specified for a gradient, rather than rendering nothing.

…nsistent with ShapeLayer.

…kit/Lottie-Windows into LinearGradientFill2

eliezerpMS · 2019-09-05T20:23:56Z

source/LottieData/ColorGradientStop.cs

+        }
+
+        public readonly Color Color;
+        public readonly double Offset;


public readonly double Offset; [](start = 8, length = 30)

I know we don't gain much but would it make sense to have a base class that is GradientStop and knows how to print the offset and then you just re-use that when doing the ToString #ByDesign

The only thing that's the same in the ToString is the @{Offset} part. The ColorGradientStop and OpacityGradientStop have no relationship with each other, i.e. we never want to treat them as just of type GradientStop.

In reply to: 321464452 [](ancestors = 321464452)

eliezerpMS · 2019-09-05T20:26:52Z

source/LottieData/LinearGradientFill.cs

            : base(in args, fillType, opacityPercent)
        {
            StartPoint = startPoint;
            EndPoint = endPoint;
-            GradientStops = gradientStops;
+            ColorStops = colorStops;


ColorStops = colorStops; [](start = 12, length = 24)

It is legal to not have any Color stops? (this should be totally legal in most art programs, but might be weird for our scenarios unless we use the gradient in masks) #ByDesign

Although I could also see that we want to just store the data and do WinComp related support checks in the translator in which case you can ignore this comment.

In reply to: 321465656 [](ancestors = 321465656)

If someone gives us a silly Lottie file, they'll get silly output. AFAICT there's nothing illegal about opacity only.

In reply to: 321468369 [](ancestors = 321468369,321465656)

eliezerpMS · 2019-09-05T20:33:52Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

            {
+                // We don't yet support animated opacity with LinearGradientFill.
+                _issues.GradientFillIsNotSupported();


GradientFillIsNotSupported [](start = 24, length = 26)

should this error message maybe be a bit more specific if the opacityPercent is not something indicated directly on the gradient. #WontFix

There are quite a few cases that we can't get quite right at the moment with gradients, so rather than trying to enumerate them all, I figure we can at least point out that something isn't right with gradients.
When it becomes clear what we definitely can or cannot support, then it might be worth making these more specific, but for now, to make progress, I'll keep it general.

In reply to: 321468727 [](ancestors = 321468727)

eliezerpMS

Enable more linear gradient fill cases, including animated linear gra…

b16ccfa

…dients.

simeoncran requested a review from eliezerpMS as a code owner August 31, 2019 00:04

eliezerpMS reviewed Aug 31, 2019

View reviewed changes

Simeon Cran added 7 commits September 3, 2019 10:44

Addressing CR feedback.

362cfb4

Better stats for LottieGen (#148)

0307288

* LottieGen reports stats to a TSV file instead of CSV. Added a more general stats reporting mechanism. * LottieGen: eliminate Verbose option and replace with much better stats file outputs. * CR feedback.

Enable more linear gradient fill cases, including animated linear gra…

d79ab79

…dients.

Addressing CR feedback.

ab276be

Refactor parsing of gradient stops to separate the color and opacity …

98258a9

…stops. Ignore opacity if it is specified for a gradient, rather than rendering nothing.

Atomize empty arrays. Renaming of the contents of ShapeGroup to be co…

5ba3bd8

…nsistent with ShapeLayer.

Merge branch 'LinearGradientFill2' of https://github.com/windows-tool…

f2d4dec

…kit/Lottie-Windows into LinearGradientFill2

simeoncran requested a review from sohchatt as a code owner September 5, 2019 01:24

simeoncran requested a review from eliezerpMS September 5, 2019 01:27

eliezerpMS reviewed Sep 5, 2019

View reviewed changes

eliezerpMS approved these changes Sep 5, 2019

View reviewed changes

simeoncran merged commit b305a19 into master Sep 5, 2019

delete-merged-branch bot deleted the LinearGradientFill2 branch September 5, 2019 20:45

michael-hawker added this to the 6.0 milestone Nov 13, 2019

sohchatt mentioned this pull request Nov 15, 2019

Linear and Radial Gradient Fills #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable more linear gradient fill cases, including animated linear gradients. #146

Enable more linear gradient fill cases, including animated linear gradients. #146

simeoncran commented Aug 31, 2019

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

simeoncran Sep 3, 2019

eliezerpMS Sep 5, 2019

eliezerpMS Aug 31, 2019 •

edited

simeoncran Sep 3, 2019

eliezerpMS Sep 5, 2019 •

edited by simeoncran

simeoncran Sep 5, 2019

eliezerpMS Sep 5, 2019 •

edited by simeoncran

eliezerpMS Sep 5, 2019

simeoncran Sep 5, 2019

eliezerpMS Sep 5, 2019 •

edited by simeoncran

simeoncran Sep 5, 2019

eliezerpMS left a comment


		var gradientStopKeyFrames = stops.KeyFrames.ToArray();

		// Create the Composition stops and animated them.

Enable more linear gradient fill cases, including animated linear gradients. #146

Enable more linear gradient fill cases, including animated linear gradients. #146

Conversation

simeoncran commented Aug 31, 2019

eliezerpMS Aug 31, 2019 • edited

Choose a reason for hiding this comment

eliezerpMS Aug 31, 2019 • edited

Choose a reason for hiding this comment

eliezerpMS Aug 31, 2019 • edited

Choose a reason for hiding this comment

eliezerpMS Aug 31, 2019 • edited

Choose a reason for hiding this comment

simeoncran Sep 3, 2019

Choose a reason for hiding this comment

eliezerpMS Sep 5, 2019

Choose a reason for hiding this comment

eliezerpMS Aug 31, 2019 • edited

Choose a reason for hiding this comment

simeoncran Sep 3, 2019

Choose a reason for hiding this comment

eliezerpMS Sep 5, 2019 • edited by simeoncran

Choose a reason for hiding this comment

simeoncran Sep 5, 2019

Choose a reason for hiding this comment

eliezerpMS Sep 5, 2019 • edited by simeoncran

Choose a reason for hiding this comment

eliezerpMS Sep 5, 2019

Choose a reason for hiding this comment

simeoncran Sep 5, 2019

Choose a reason for hiding this comment

eliezerpMS Sep 5, 2019 • edited by simeoncran

Choose a reason for hiding this comment

simeoncran Sep 5, 2019

Choose a reason for hiding this comment

eliezerpMS left a comment

Choose a reason for hiding this comment

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Aug 31, 2019 •

edited

eliezerpMS Sep 5, 2019 •

edited by simeoncran

eliezerpMS Sep 5, 2019 •

edited by simeoncran

eliezerpMS Sep 5, 2019 •

edited by simeoncran