Fixes for AnimatedIcon (#424)

* Nudge the markers forward by 1/100 of a frame to stop flashing.

Due to floating point imprecision, a marker value may refer to the previous frame, which causes flashing for AnimatedIcon.

This change is a test to see if it helps the problem. If it does, it needs a little more tweaking so that the frame numbers in the comments don't look whacky.

* Remove the decimal place from the frame number. Frame numbers from Lottie are always integer values.

* Remove accidentally added "using".

* Fix bug in visibility optimization.

This was causing some things to be visible when they were not supposed to be.
While I was there I refactored the the types that are used to calculate visibility to make the code a bit easier to understand.

source/LottieMetadata/Duration.cs

@@ -16,7 +16,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.LottieMetadata
 #endif
     readonly struct Duration
     {
-        internal Duration(double frames, double fps)
+        public Duration(double frames, double fps)
         {
             if (frames < 0 || fps < 0)
             {
@@ -27,7 +27,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.LottieMetadata
             FPS = fps;
         }
 
-        internal Duration(double frames, Duration other)
+        public Duration(double frames, Duration other)
         {
             Frames = frames;
             FPS = other.FPS;

source/LottieMetadata/Marker.cs

@@ -12,7 +12,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.LottieMetadata
 #endif
     readonly struct Marker
     {
-        internal Marker(string name, Frame frame, Duration duration)
+        public Marker(string name, Frame frame, Duration duration)
         {
             Name = name;
             Frame = frame;

source/UIData/Tools/GraphCompactor.cs

@@ -709,79 +709,6 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
             }
         }
 
-        static VisibilityDescription ComposeVisibilities(in VisibilityDescription a, in VisibilityDescription b)
-        {
-            if (a.Sequence.Length == 0)
-            {
-                return b;
-            }
-
-            if (b.Sequence.Length == 0)
-            {
-                return a;
-            }
-
-            if (a.Duration != b.Duration)
-            {
-                throw new InvalidOperationException();
-            }
-
-            if (a.Sequence.SequenceEqual(b.Sequence))
-            {
-                // They're identical.
-                return a;
-            }
-
-            // Combine and optimize the 2 sequences.
-            var composedSequence = ComposeVisibilitySequences(a.Sequence, b.Sequence).ToArray();
-
-            return new VisibilityDescription(a.Duration, composedSequence);
-        }
-
-        // Composes 2 visibility sequence.
-        static IEnumerable<(bool isVisible, float progress)> ComposeVisibilitySequences(
-                                                                (bool isVisible, float progress)[] a,
-                                                                (bool isVisible, float progress)[] b)
-        {
-            var currentVisibility = false;
-            var currentProgress = 0F;
-
-            var ai = 0;
-            var bi = 0;
-
-            while (ai < a.Length || bi < b.Length)
-            {
-                var cura = ai < a.Length ? a[ai] : (a[a.Length - 1].isVisible, progress: float.MaxValue);
-                var curb = bi < b.Length ? b[bi] : (b[b.Length - 1].isVisible, progress: float.MaxValue);
-
-                // Is the visibility changing?
-                if ((cura.isVisible & curb.isVisible) != currentVisibility)
-                {
-                    yield return (currentVisibility, currentProgress);
-                    currentVisibility = !currentVisibility;
-                }
-
-                if (cura.progress == curb.progress)
-                {
-                    currentProgress = cura.progress;
-                    ai++;
-                    bi++;
-                }
-                else if (cura.progress < curb.progress)
-                {
-                    currentProgress = cura.progress;
-                    ai++;
-                }
-                else
-                {
-                    currentProgress = curb.progress;
-                    bi++;
-                }
-            }
-
-            yield return (currentVisibility, currentProgress);
-        }
-
         // Find ContainerVisuals that have a single ShapeVisual child with orthongonal properties and
         // push the properties down to the ShapeVisual.
         static void PushPropertiesDownToShapeVisual(ObjectGraph<Node> graph)
@@ -875,7 +802,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
         {
             var toVisibility = GetVisiblityAnimationDescription(to);
 
-            var compositeVisibility = ComposeVisibilities(fromVisibility, toVisibility);
+            var compositeVisibility = VisibilityDescription.Compose(fromVisibility, toVisibility);
 
             if (compositeVisibility.Sequence.Length > 0)
             {
@@ -883,10 +810,10 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
                 var c = new Compositor();
                 var animation = c.CreateBooleanKeyFrameAnimation();
                 animation.Duration = compositeVisibility.Duration;
-                if (compositeVisibility.Sequence[0].progress == 0)
+                if (compositeVisibility.Sequence[0].Progress == 0)
                 {
                     // Set the initial visiblity.
-                    to.IsVisible = compositeVisibility.Sequence[0].isVisible;
+                    to.IsVisible = compositeVisibility.Sequence[0].IsVisible;
                 }
 
                 foreach (var (isVisible, progress) in compositeVisibility.Sequence)
@@ -919,14 +846,14 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
 
             if (animator is null)
             {
-                return new VisibilityDescription(TimeSpan.Zero, Array.Empty<(bool, float)>());
+                return new VisibilityDescription(TimeSpan.Zero, Array.Empty<VisibilityAtProgress>());
             }
 
             var visibilityAnimation = (BooleanKeyFrameAnimation)animator.Animation;
 
             return new VisibilityDescription(visibilityAnimation.Duration, GetDescription().ToArray());
 
-            IEnumerable<(bool isVisible, float progress)> GetDescription()
+            IEnumerable<VisibilityAtProgress> GetDescription()
             {
                 if (animator is null)
                 {
@@ -947,11 +874,11 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
                         if (kf.Progress != 0 && visual.IsVisible == false)
                         {
                             // Output an initial keyframe.
-                            yield return (false, 0);
+                            yield return new VisibilityAtProgress(false, 0);
                         }
                     }
 
-                    yield return (kf.Value, kf.Progress);
+                    yield return new VisibilityAtProgress(kf.Value, kf.Progress);
                 }
             }
         }
@@ -980,7 +907,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
 
             return new VisibilityDescription(scaleAnimation.Duration, GetDescription().ToArray());
 
-            IEnumerable<(bool isVisible, float progress)> GetDescription()
+            IEnumerable<VisibilityAtProgress> GetDescription()
             {
                 foreach (KeyFrameAnimation<Vector2, Expr.Vector2>.ValueKeyFrame kf in scaleAnimation.KeyFrames)
                 {
@@ -1004,11 +931,11 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
                         // add a non-visible state at 0.
                         if (kf.Progress != 0 && shape.Scale == Vector2.Zero)
                         {
-                            yield return (false, 0);
+                            yield return new VisibilityAtProgress(false, 0);
                         }
                     }
 
-                    yield return (kf.Value == Vector2.One, kf.Progress);
+                    yield return new VisibilityAtProgress(kf.Value == Vector2.One, kf.Progress);
                 }
             }
         }
@@ -1513,16 +1440,6 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
         static CompositionObject.Animator? TryGetAnimatorByPropertyName(CompositionObject obj, string name) =>
             obj.Animators.Where(anim => anim.AnimatedProperty == name).FirstOrDefault();
 
-        readonly struct VisibilityDescription
-        {
-            internal VisibilityDescription(TimeSpan duration, (bool isVisible, float progress)[] sequence)
-                => (Duration, Sequence) = (duration, sequence);
-
-            internal TimeSpan Duration { get; }
-
-            internal (bool isVisible, float progress)[] Sequence { get; }
-        }
-
         sealed class Node : Graph.Node<Node>
         {
             internal CompositionObject? Parent { get; set; }

source/UIData/Tools/VisibilityAtProgress.cs

@@ -0,0 +1,102 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#nullable enable
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
+namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
+{
+    readonly struct VisibilityAtProgress
+    {
+        internal VisibilityAtProgress(bool isVisible, float progress)
+            => (IsVisible, Progress) = (isVisible, progress);
+
+        internal bool IsVisible { get; }
+
+        internal float Progress { get; }
+
+        internal void Deconstruct(out bool isVisible, out float progress)
+        {
+            isVisible = IsVisible;
+            progress = Progress;
+        }
+
+        // Composes 2 lists of VisibilityAtProgress by ANDing them together.
+        // The resulting sequence always has a value at progress 0. There will
+        // be no consective items with the same visibility (i.e. each item describes
+        // a change of visibility state). The visibility will be visibile when
+        // both a AND b are visible, and invisible if either a OR b is invisible.
+        internal static IEnumerable<VisibilityAtProgress> Compose(
+            VisibilityAtProgress[] a,
+            VisibilityAtProgress[] b)
+        {
+            // Get the visibilities in order, with any redundant visibilities removed.
+            var items = SanitizeSequence(a).Concat(SanitizeSequence(b)).OrderBy(v => v.Progress).ThenBy(v => !v.IsVisible).ToArray();
+
+            // The output is visible any time both a and b are visible at the same time.
+            var visibilityCounter = 0;
+
+            var has0ProgressBeenOutput = false;
+
+            foreach (var item in items)
+            {
+                visibilityCounter += item.IsVisible ? 1 : -1;
+                if (visibilityCounter == 2)
+                {
+                    // Both a and b are now visible.
+                    if (!has0ProgressBeenOutput)
+                    {
+                        // If we haven't output a value for progress 0, and the current
+                        // item isn't for 0, output a progress 0 value now.
+                        if (item.Progress != 0)
+                        {
+                            yield return new VisibilityAtProgress(false, 0);
+                        }
+
+                        has0ProgressBeenOutput = true;
+                    }
+
+                    yield return new VisibilityAtProgress(true, item.Progress);
+                }
+                else if (visibilityCounter == 1 && !item.IsVisible)
+                {
+                    // We were visible, but now we're not.
+                    yield return new VisibilityAtProgress(false, item.Progress);
+                }
+            }
+        }
+
+        // Orders the items in the sequence by Progress, and removes any redundant items.
+        static IEnumerable<VisibilityAtProgress> SanitizeSequence(IEnumerable<VisibilityAtProgress> sequence)
+        {
+            // Sequences start implicitly invisible.
+            var previousVisibility = false;
+
+            foreach (var item in sequence.GroupBy(v => v.Progress).OrderBy(v => v.Key))
+            {
+                // Do not allow multiple visibilities at the same progress. It should
+                // never happen, and if it did it's not clear what it means.
+                var group = item.ToArray();
+                if (group.Length > 1)
+                {
+                    throw new ArgumentException();
+                }
+
+                var visibility = group[0];
+
+                // Ignore any repeats of the same visibility state.
+                if (previousVisibility != visibility.IsVisible)
+                {
+                    yield return visibility;
+                    previousVisibility = visibility.IsVisible;
+                }
+            }
+        }
+
+        public override string ToString() => $"{(IsVisible ? "Visible" : "Invisible")} @ {Progress}";
+    }
+}

source/UIData/Tools/VisibilityDescription.cs

@@ -0,0 +1,65 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#nullable enable
+
+using System;
+using System.Linq;
+
+namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.Tools
+{
+    readonly struct VisibilityDescription
+    {
+        internal VisibilityDescription(TimeSpan duration, VisibilityAtProgress[] sequence)
+            => (Duration, Sequence) = (duration, sequence);
+
+        /// <summary>
+        /// The time over which the <see cref="VisibilityDescription"/> is valid.
+        /// </summary>
+        internal TimeSpan Duration { get; }
+
+        /// <summary>
+        /// The sequence of visibilites, ordered by progress. Initial visibility
+        /// is "not visible". Each entry in the sequence represents a change to
+        /// a different visibility.
+        /// </summary>
+        internal VisibilityAtProgress[] Sequence { get; }
+
+        /// <summary>
+        /// Composes the given <see cref="VisibilityDescription"/>s by ANDing them
+        /// together.
+        /// </summary>
+        /// <returns>A composed <see cref="VisibilityDescription"/>.</returns>
+        internal static VisibilityDescription Compose(
+            in VisibilityDescription a,
+            in VisibilityDescription b)
+        {
+            if (a.Sequence.Length == 0)
+            {
+                return b;
+            }
+
+            if (b.Sequence.Length == 0)
+            {
+                return a;
+            }
+
+            if (a.Duration != b.Duration)
+            {
+                throw new InvalidOperationException();
+            }
+
+            if (a.Sequence.SequenceEqual(b.Sequence))
+            {
+                // They're identical.
+                return a;
+            }
+
+            // Combine the 2 sequences.
+            var composedSequence = VisibilityAtProgress.Compose(a.Sequence, b.Sequence).ToArray();
+
+            return new VisibilityDescription(a.Duration, composedSequence);
+        }
+    }
+}

source/UIData/UIData.projitems

@@ -16,5 +16,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Tools\PropertyValueOptimizer.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Tools\PropertyId.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Tools\Stats.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Tools\VisibilityAtProgress.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Tools\VisibilityDescription.cs" />
   </ItemGroup>
 </Project>

source/UIDataCodeGen/CodeGen/MarkerInfo.cs

@@ -48,6 +48,10 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.CodeGen
 
         internal static IEnumerable<MarkerInfo> GetMarkerInfos(IEnumerable<Marker> markers)
         {
+            // Nudge the markers to prevent them from referring to the previous frame
+            // as a result of floating point imprecision.
+            markers = NudgeMarkersUp(markers);
+
             // Ensure the names are valid and distinct.
             var nameMap = markers.ToDictionary(m => m, m => SanitizeMarkerName(m.Name));
             EnsureNamesAreDistinct(nameMap);
@@ -64,6 +68,27 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.CodeGen
             }
         }
 
+        // Adjust the frame numbers up by 1/100 of a frame. This is done
+        // to nudge the marker past any errors caused by floating point imprecision.
+        // It's always better to nudge a marker forward than to allow it to be
+        // rounded down, because even a small amount of rounding down can result in
+        // the previous frame being shown, whereas rounding up won't show the next
+        // frame until the error is as big as a whole frame.
+        static IEnumerable<Marker> NudgeMarkersUp(IEnumerable<Marker> markers)
+        {
+            foreach (var marker in markers)
+            {
+                var adjustedFrame = marker.Frame;
+
+                if (adjustedFrame.Number > 0)
+                {
+                    adjustedFrame += new Duration(0.01, marker.Duration);
+                }
+
+                yield return new Marker(marker.Name, adjustedFrame, marker.Duration);
+            }
+        }
+
         // Returns the given name with non-printing and newline characters removed.
         // If the result is empty returns "anonymous".
         static string SanitizeMarkerName(string markerName)

source/UIDataCodeGen/CodeGen/Tables/LottieMarkersMonospaceTableFormatter.cs

@@ -42,7 +42,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Lottie.UIData.CodeGen.Tables
                 select (Row)new Row.ColumnData(
                     ColumnData.Create(m.Name, TextAlignment.Left),
                     ColumnData.Create(m.StartConstant, TextAlignment.Left),
-                    ColumnData.Create(m.StartFrame),
+                    ColumnData.Create((int)m.StartFrame),
                     ColumnData.Create(m.StartTime.TotalMilliseconds),
                     ColumnData.Create(stringifier.Float(m.StartProgress), TextAlignment.Left)
                 );