Enable image layer with external and embedded image assets. #111

plaiMS · 2019-05-10T22:23:42Z

No description provided.

…ce, and CS code generator for embedded LoadImageSurface with some hacks.

…st of expected files in LottieViewer and LottieGen in case of external image file.

…ents.

simeoncran · 2019-05-14T00:39:03Z

LottieGen/LottieFileProcessor.cs

-                    (float)_lottieComposition.Height,
-                    _lottieComposition.Duration,
-                    _options.DisableCodeGenOptimizer);
+        CSharpInstantiatorGenerator.CreateFactoryCode(


Now that we are not using the return value for the code, please have it return a bool and rename the method to TryCreateFactoryCode #Closed

Actually... let's add a class for the result and shove everything into that class

CSharpGeneratedCode

CxGeneratedCode
Return null on failure.
In the CSharp one have properties: CsText, ReferencedAssetPaths
In the Cx one have properties: CxText, HeaderText, ReferencedAssetPaths

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

I realized that I can get the asset file list from _lottieComposition, which means I don't need to modify CreateFactoryCode at all. Should I revert the function back to its original form and not create the new class?

In reply to: 283991046 [](ancestors = 283991046,283587851)

As discussed, changed to return Tuple instead of adding a new class.

In reply to: 284484652 [](ancestors = 284484652,283991046,283587851)

simeoncran · 2019-05-14T00:39:24Z

LottieGen/LottieFileProcessor.cs

+                (float)_lottieComposition.Height,
+                _lottieComposition.Duration,
+                out var csText,
+                out var infoText,


Put the outs last in the parameter list. #Closed

The disableFieldOptimization parameter is optional and must be the last parameter.

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

Should it be optional? ANSWER: no!

In reply to: 283956130 [](ancestors = 283956130,283587899)

simeoncran · 2019-05-14T00:41:06Z

LottieGen/LottieFileProcessor.cs


        if (result)
        {
            _reporter.WriteInfo($"C# code for class {_className} written to {outputFilePath}");
+            _reporter.WriteInfo(infoText);


infoText [](start = 32, length = 8)

The info text should be created by LottieFileProcessor from information given to it by the CSharpInstantiatorGenerator. The CSharpInstantiatorGenerator should not be creating messages for the user. #Closed

And it should be in the result object from the Cs/Cx-InstantiatorGenerator. And inside LottieGen (i.e. here!) you should turn it into a nice message.

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

simeoncran · 2019-05-14T00:42:26Z

source/Lottie/LottieVisualDiagnostics.cs

@@ -90,17 +90,22 @@ public string GenerateWinCompXml()
            return WinCompData.Tools.CompositionObjectXmlSerializer.ToXml(RootVisual).ToString();
        }

-        public string GenerateCSharpCode()
+        public void GenerateCSharpCode(out string csText)


public void GenerateCSharpCode(out string csText) [](start = 7, length = 50)

Why is the signature being changed? #Closed

simeoncran · 2019-05-14T00:44:05Z

source/Lottie/Instantiator.cs

+                case Wmd.LoadedImageSurface.LoadedImageSurfaceLoadType.FromUri:
+                    result = null;
+                    break;
+            }


} [](start = 11, length = 2)

Add a default case the throws. #Closed

simeoncran · 2019-05-14T00:44:39Z

source/Lottie/Instantiator.cs

+                    result = Wm.LoadedImageSurface.StartLoadFromStream(bytes.AsBuffer().AsStream().AsRandomAccessStream());
+                    break;
+                case Wmd.LoadedImageSurface.LoadedImageSurfaceLoadType.FromUri:
+                    result = null;


result = null; [](start = 20, length = 14)

Missing implementation? #Closed

simeoncran · 2019-05-14T00:45:23Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

@@ -3,7 +3,7 @@
 // See the LICENSE file in the project root for more information.

 // Enable use of Image Layer
-//#define EnableImageLayer


I think this #define can die now, right? #Closed

simeoncran · 2019-05-14T00:46:31Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                    imageAssetHeight = externalImageAsset.Height;
+                    _issues.ExternalImageFilesExpected($"{externalImageAsset.Path}{externalImageAsset.FileName}");
+                    break;
+            }


Add a default case that throws. Then get rid of the unnecessary initialization of variables that are initialized in the non-throwing cases. #Closed

simeoncran · 2019-05-14T00:47:00Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

-            var surface = LoadedImageSurface.StartLoadFromStream(embeddedImageAsset.Bytes);
-            var imageBrush = CreateSurfaceBrush(surface);
+            LoadedImageSurface surface = null;
+            CompositionSurfaceBrush imageBrush = null;


imageBrush [](start = 36, length = 10)

Declare at first use rather than up here. #Closed

simeoncran · 2019-05-14T00:49:54Z

source/UIData/CodeGen/Canonicalizer.cs

+                var grouping =
+                    from item in items
+                    let obj = item.obj
+                    let animators = obj.Animators.ToArray()


ToArray [](start = 50, length = 7)

Is there any case where we would could canonicalize with an animation? If we'll never want to canonicalize with an animation, don't covert to an array, just use "where !animators.Any()" #Closed

simeoncran · 2019-05-14T00:50:35Z

source/UIData/CodeGen/Canonicalizer.cs

+                            new
+                            {
+                                obj.FilePath,
+                            }


Do you need to create this object, or can you just use obj.FilePath directly? #Closed

simeoncran · 2019-05-14T00:51:51Z

source/UIData/CodeGen/CodeBuilder.cs

@@ -234,6 +235,49 @@ static string GetLine(string text, int maxLineLength, out string remainder)
            return text.Substring(0, breakAt);
        }

+        internal void BytesToLiteral(byte[] bytes, int maximumColumns = 100)


= 100 [](start = 69, length = 6)

Not clear to me why we should have a default value, or why it would be 100. #Closed

My point was really that the CodeBuilder should define the default policy. So it shouldn't have a default value.
Also, default values for parameters are usually a bit suspicious

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

Comment is wrong now... talks about the default.
Also, I think the name should be something like WriteLiteralBytes - to be consistent with the rest of the methods on this class.
And move it up with the rest of the Write* methods.

In reply to: 284905129 [](ancestors = 284905129,283589741)

simeoncran · 2019-05-14T00:52:20Z

source/UIData/CodeGen/CodeBuilder.cs

+        {
+            var bytesLines = BytesToBytesList(bytes, maximumColumns - 1 - IndentSize).ToArray();
+
+            int i;


int i [](start = 11, length = 6)

Please add a comment explaining the intention of this code... the pattern is a bit unusual. #Closed

simeoncran · 2019-05-14T00:52:44Z

source/UIData/CodeGen/CodeBuilder.cs

+            WriteLine(bytesLines[i]);
+        }
+
+        internal IEnumerable<string> BytesToBytesList(byte[] bytes, int maximumWidth)


internal [](start = 8, length = 8)

Can this be private? Also can it be static? #Closed

Don't mark it with "private" - members are private by default and the rest of the code never includes the accessibility modifier unless it is different from the default.

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

simeoncran · 2019-05-14T00:55:14Z

source/UIData/CodeGen/CSharpInstantiatorGenerator.cs

+            }
+        }
+
+        static void BytesToCSharpLiteral(CodeBuilder builder, byte[] bytes, int maximumColumns = 100)


s [](start = 7, length = 2)

This code is also in CodeBuilder. Can it be shared? #Closed

This code shouldn't be here. Removed.

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

simeoncran · 2019-05-14T17:04:59Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

@@ -130,17 +140,31 @@ namespace AnimatedVisuals
            builder.WriteLine("using namespace Windows::UI::Composition;");
            builder.WriteLine("using namespace Windows::Graphics;");
            builder.WriteLine("using namespace Microsoft::WRL;");
+            if (info.UsesLoadedImageSurfaceFromStream || info.UsesLoadedImageSurfaceFromUri)


Can/should we break these flags up so that they refer to the thing that the code needs rather than the thing that is in the code? E.g. UsesWindowsUiXamlMedia
Think about what happens when there's some other feature that we add then the decoding of the flags will get awkward. #Closed

UsesNamespaceWindowsUIXamlMedia
UsesNamespaceWindowsStorageStreams
UsesNamespacePlatform

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

simeoncran · 2019-05-14T17:06:29Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+            switch (obj.LoadType)
+            {
+                case LoadedImageSurface.LoadedImageSurfaceLoadType.FromStream:
+                    builder.WriteLine("InMemoryRandomAccessStream ^ stream = ref new InMemoryRandomAccessStream();");


[](start = 65, length = 1)

No space before the hat. But anyhow, prefer auto in this case. No need for the full name. #Closed

simeoncran · 2019-05-14T17:10:13Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+
+            public virtual string Static => "static";
+
+            public virtual string ExternalImageFileUri(string className, string filePath) => $"ms-appx:///Assets/{className}/{filePath}";


The name doesn't seem right - it's an asset, not necessarily an image file. And it's not external. #Closed

simeoncran · 2019-05-14T17:11:20Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+            }
+        }
+
+        protected override void WriteBytesField(CodeBuilder builder, string fieldName)


Is it necessary to create a virtual method for this? I'm hoping the base class can express this concept. #Closed

simeoncran · 2019-05-14T17:12:36Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+            }
+        }
+
+        protected virtual void WriteBytesField(CodeBuilder builder, string fieldName)


Needs comments, especially so because it is now part of the protected interface for the class. #Closed

simeoncran · 2019-05-14T17:13:51Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+        {
+            bool bytesWritten = false;
+
+            foreach (var node in nodes)


foreach [](start = 12, length = 7)

Rather than filtering inside a foreach, just do the foreach over a filtered list of nodes. #Closed

simeoncran · 2019-05-14T17:14:30Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+            Wmd.LoadedImageSurface obj,
+            string fieldName);
+
+        void WriteBytesFields(CodeBuilder builder, ObjectData[] nodes)


This seems weird to pass in all of the nodes, or are they not all the nodes? #Closed

simeoncran · 2019-05-16T21:45:21Z

source/UIData/CodeGen/Canonicalizer.cs

+                            select grouped;
+                        CanonicalizeGrouping(groupingExternal);
+                        break;
+                }


} [](start = 14, length = 3)

Needs a default case. #Closed

simeoncran · 2019-05-17T18:34:22Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+        /// <summary>
+        /// Call this to get a list of the asset files referenced by the generated code.
+        /// </summary>
+        /// <returns>List of asset files.</returns>


Needs a little more info - are these paths, or just file names? Is the generated code going to load from that path, or is it relative to the assets directory in a UWP, etc. #Closed

They're uris. Please return them as an IEnumerable and call it GetAssetsList()

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

simeoncran · 2019-05-17T18:36:11Z

source/Lottie/Instantiator.cs

+                    result = Wm.LoadedImageSurface.StartLoadFromStream(bytes.AsBuffer().AsStream().AsRandomAccessStream());
+                    break;
+                case Wmd.LoadedImageSurface.LoadedImageSurfaceType.FromUri:
+                    // Loading external image asset is not supported yet and it is captured on the issue list.


captured on the issue list [](start = 83, length = 26)

I don't understand this comment. Is it referring to the list of issues in GitHub? If it's that we output an issue, it might need a bit of explaining about where that is. #Closed

simeoncran · 2019-05-17T18:36:49Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

-            var embeddedImageAsset = (EmbeddedImageAsset)imageAsset;
-            var surface = LoadedImageSurface.StartLoadFromStream(embeddedImageAsset.Bytes);
+            LoadedImageSurface surface;
+            double imageAssetWidth = imageAsset.Width;


double [](start = 12, length = 6)

Use var. #Closed

simeoncran · 2019-05-17T18:38:10Z

source/WinUIXamlMediaData/LoadedImageSurface.cs

    {
-        readonly byte[] _bytes;
+        protected LoadedImageSurface()


Can this be private protected? Otherwise other assemblies can call it. #Closed

simeoncran · 2019-05-17T18:43:21Z

�

LT0004.md and LT0030.md - can you update them to UTF8 seeing as you're changing them? #Closed

Refers to: source/Issues/LT0004.md:12 in b205280. [](commit_id = b205280, deletion_comment = False)

simeoncran · 2019-05-17T18:46:50Z

�[�c�o�m�m�e�n�t�]�:� �#� �(�t�e�x�t�:�E�x�t�e�r�n�a�l� �i�m�a�g�e� �f�i�l�e� �e�x�p�e�c�t�e�d� �a�t� �{�f�i�l�e�P�a�t�h�}�)�

In what regard are these "External"? They're just files aren't they? #Closed

Refers to: source/Issues/LT0030.md:5 in b205280. [](commit_id = b205280, deletion_comment = False)

simeoncran · 2019-05-17T18:47:11Z

source/UIData/CodeGen/Canonicalizer.cs

@@ -468,6 +527,30 @@ into grouped
                CanonicalizeGrouping(grouping);
            }

+            void CanonicalizeLoadedImageSurface(LoadedImageSurface.LoadedImageSurfaceType type)
+            {
+                // Canonicalize LoadeImageSurfaces.


Typo
#Closed

simeoncran · 2019-05-17T18:52:10Z

source/UIData/CodeGen/CppStringifier.cs

        public string FailFastWrapper(string value) => $"FFHR({value})";

+        public string Const(string value) => $"const {value}";


There is a Const(...) method that is exactly the same in InstantiatorGeneratorBase. Seeing as const is the same in C# and CX, can this be on the StringifierBase as a non-virtual? #Closed

simeoncran · 2019-05-17T18:53:04Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

@@ -1883,6 +2004,10 @@ protected internal abstract class StringifierBase : IStringifier
            public abstract string Vector3(Vector3 value);

            public string Hex(int value) => $"0x{value.ToString("X2")}";
+
+            public virtual string Static => "static";


Does this need to be virtual? I would expect that it is the same in all languages. #Closed

simeoncran · 2019-05-17T21:10:44Z

I think this should be "required". ImageFileRequired. Saying "expected" implies that we're reporting the issue because the expectation was not met. It might be worth explaining what this means in the Remarks section because people are going to see this reported as an issue in LottieViewer, be confused, and go to this page to find out what's going on. #Closed

Refers to: source/Issues/LT0030.md:3 in c131919. [](commit_id = c131919, deletion_comment = False)

simeoncran

simeoncran

simeoncran

…oj to \dll and \source.

…aking it internal on context since the layer property on context is always null.

simeoncran

simeoncran

plaiMS added 3 commits May 3, 2019 16:50

Adding canonicalizer for CompositionSurfaceBrush and LoadedImageSurfa…

10ceedd

…ce, and CS code generator for embedded LoadImageSurface with some hacks.

Adding support for external image file and embedded images. Output li…

45d537f

…st of expected files in LottieViewer and LottieGen in case of external image file.

CX codegen for embedded images. Code cleanup. Issues message improvem…

9f387c9

…ents.

plaiMS requested review from eliezerpMS, simeoncran and sohchatt as code owners May 10, 2019 22:23

plaiMS and others added 2 commits May 10, 2019 15:24

Merge branch 'master' into EnableImageLayer

fcfdd23

Fix line spacing in codegen

964dbbd

simeoncran reviewed May 14, 2019

View reviewed changes

simeoncran reviewed May 16, 2019

View reviewed changes

More code improvements per PR feedback.

b205280

simeoncran reviewed May 17, 2019

View reviewed changes

Fix several comments plus other small changes per PR feedback.

c131919

Change asset list to Uri data type. Added remark for LT0030.

84beaee

simeoncran self-assigned this May 20, 2019

simeoncran previously approved these changes May 20, 2019

View reviewed changes

Merge branch 'master' into EnableImageLayer

0f111f8

plaiMS dismissed simeoncran’s stale review via 0f111f8 May 20, 2019 17:07

simeoncran approved these changes May 20, 2019

View reviewed changes

simeoncran previously approved these changes May 20, 2019

View reviewed changes

Fix merge conflict with blendMode PR. Re-add WinStorageStreamsData pr…

84c8e94

…oj to \dll and \source.

plaiMS dismissed simeoncran’s stale review via 84c8e94 May 20, 2019 18:17

Pass layer in as parameter to Translate*Shape* functions instead of m…

51ea64b

…aking it internal on context since the layer property on context is always null.

simeoncran approved these changes May 20, 2019

View reviewed changes

plaiMS merged commit 916aee0 into master May 20, 2019

delete-merged-branch bot deleted the EnableImageLayer branch May 20, 2019 20:51

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

sohchatt mentioned this pull request Nov 15, 2019

Add support for Image Layer (Codegen) #118

Closed

tarasvakulka mentioned this pull request Nov 13, 2020

Windows support [C#] lottie-react-native/lottie-react-native#700

Merged


		public virtual string Static => "static";

		public virtual string ExternalImageFileUri(string className, string filePath) => $"ms-appx:///Assets/{className}/{filePath}";

		public string FailFastWrapper(string value) => $"FFHR({value})";

		public string Const(string value) => $"const {value}";

Enable image layer with external and embedded image assets. #111

Enable image layer with external and embedded image assets. #111

Conversation

plaiMS commented May 10, 2019

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 16, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran commented May 17, 2019 • edited

simeoncran commented May 17, 2019 • edited

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 17, 2019 • edited

Choose a reason for hiding this comment

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 14, 2019 •

edited

simeoncran May 16, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran commented May 17, 2019 •

edited

simeoncran commented May 17, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran May 17, 2019 •

edited

simeoncran commented May 17, 2019 •

edited