Adding first pieces needed in LottieToWinCompTranslator to enable image layer. #105

plaiMS · 2019-04-29T19:31:01Z

Changes protected under #define EnableImageLayer.

…lator.

simeoncran · 2019-04-29T21:54:29Z

// This code was generated by a tool.

This code is generated.... don't modify it directly. The source is in source\Issues. Add a new .md file there for LT0028.md, then run the GenerateISsuesClasses.cmd script which will regenerate this file. #Closed

Refers to: source/LottieToWinComp/TranslationIssues.cs:7 in dc91f49. [](commit_id = dc91f49, deletion_comment = False)

simeoncran

🕐

sohchatt · 2019-04-29T22:24:37Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                case Asset.AssetType.Image:
+                    var imageAsset = (ImageAsset)referencedLayersAsset;
+                    content.Size = new Sn.Vector2((float)imageAsset.Width, (float)imageAsset.Height);
+                    var brush = CreateColorBrush(LottieData.Color.FromArgb(1, 1, 0, 0));


maybe a comment that explains the intentions of using a colorbrush? #Closed

Comment should explain the intention of the brush. There should be comments in this method explaining why you're doing what you're doing - the brush is not the only weird thing in here.

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

What are the other weird things here?

In reply to: 279590537 [](ancestors = 279590537,279561955)

sohchatt · 2019-04-29T22:30:11Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

@@ -694,7 +737,16 @@ Visual TranslatePreCompLayerToVisual(TranslationContext context, PreCompLayer la
                    AddTranslatedLayersToContainerVisual(contentsNode, subContext, referencedLayers, $"{layer.Name}:{layerCollectionAsset.Id}");
                    break;
                case Asset.AssetType.Image:
+#if EnableImageLayer


Why does this need to be done twice? Shouldn't this case be handled by regular translation above? #Resolved

simeoncran · 2019-04-30T00:51:50Z

�[�c�o�m�m�e�n�t�]�:� �#� �(�t�e�x�t�:�I�m�a�g�e� �l�a�y�e�r�s� �i�s� �n�o�t� �s�u�p�p�o�r�t�e�d�)�

Remove the s or make it "Image layers are not supported". #Closed

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

simeoncran · 2019-04-30T00:52:53Z

�[�c�o�m�m�e�n�t�]�:� �#� �(�t�e�x�t�:�R�e�f�e�r�e�n�c�e�d�A�s�s�e�t�D�o�e�s�N�o�t�E�x�i�s�t�)�

This shouldn't be smoooshed together.
Also, do you think this should/could have have the asset reference id in the text? #Closed

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

simeoncran · 2019-04-30T00:54:34Z

�[�c�o�m�m�e�n�t�]�:� �#� �(�t�e�x�t�:�{�c�u�r�r�e�n�t�L�a�y�e�r�T�y�p�e�}� �r�e�f�e�r�e�n�c�e�d� �a�s�s�e�t� �{�a�s�s�e�t�R�e�f�I�d�}� �o�f� �t�y�p�e� �{�a�s�s�e�t�T�y�p�e�}� �w�h�i�c�h� �i�s� �i�n�v�a�l�i�d�.� �E�x�p�e�c�t�e�d� �a�s�s�e�t� �o�f� �t�y�p�e� �{�e�x�p�e�c�t�e�d�A�s�s�e�t�T�y�p�e�}�.�)�

...Expected an asset... #Closed

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

simeoncran · 2019-04-30T00:54:47Z

�#� �L�o�t�t�i�e�-�W�i�n�d�o�w�s� �W�a�r�n�i�n�g� �L�T�0�0�0�4�

Wrong number #Closed

Refers to: source/Issues/LT0029.md:9 in b43759e. [](commit_id = b43759e, deletion_comment = False)

simeoncran · 2019-04-30T00:59:20Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                            yield return shapeVisual;
+                            shapeVisual = null;
+                        }
+


I don't understand what this code is intended to achieve. It looks like you'd get the same result if you just added SpriteVisual to the list of cases in the previous case (case ContainerVisual: case ShapeVisual: case SpriteVisual:).
#Closed

Good call.

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

simeoncran · 2019-04-30T01:01:10Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

                }

                // Translation currently does not support having multiple paths for masks.
                // If possible users should combine masks when exporting to json.
                if (layer.Masks.Length > 1)
                {
-                    _unsupported.MultipleShapeMasks();
+                    _issues.MultipleShapeMasks();


MultipleShapeMasks [](start = 28, length = 18)

This is an unsupported case (as per the comments) so that name of the issue should be updated to reflect that. Should be done for all the issue types that are saying that things are unsupported. #Closed

Pretty much all of them are issues because they are unsupported. They might look silly if all issues say "NotSupported"?

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

simeoncran · 2019-04-30T01:02:45Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+            var referencedLayersAsset = _lc.Assets.GetAssetById(layer.RefId);
+            if (referencedLayersAsset == null)
+            {
+                _issues.ReferencedAssetDoesNotExist();


ReferencedAssetDoesNotExist [](start = 24, length = 27)

Because this is done twice now, it should be moved to a method on this class: GetAssetById(...) that either return the asset or reports the problem and returns null.
Can that method do the check for the asset type and report the invalid asset type as well? So: GetAssetById(Asset.AssetType, id).
#Resolved

I went with GetAssetById() without the type checking because I think that makes the code easier to follow.

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

I don't understand how it makes the code harder to follow. Maybe it's about choosing a better name for GetAssetById. Or maybe it's just a matter of using named parameters to make it clear from the caller: GetAssetById(id: layer.RefId, expectedAssetType: Asset.AssetType.Image);
But I do see that the Image and Precomp layers now have slightly different behavior when given the wrong type of asset, so we already have introduced a bug because we've violated DRY.

In reply to: 279607824 [](ancestors = 279607824,279590241)

simeoncran · 2019-04-30T17:05:42Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

@@ -703,6 +740,18 @@ Visual TranslatePreCompLayerToVisual(TranslationContext context, PreCompLayer la
            return result;
        }

+        internal Asset GetAssetById(string assetId)


I don't think needs to be internal. #Closed

simeoncran · 2019-04-30T17:06:07Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

@@ -682,7 +718,8 @@ Visual TranslatePreCompLayerToVisual(TranslationContext context, PreCompLayer la
 #endif

            // TODO - the animations produced inside a PreComp need to be time-mapped.
-            var referencedLayersAsset = _lc.Assets.GetAssetById(layer.RefId);
+            var referencedLayersAsset = GetAssetById(layer.RefId);
+
            switch (referencedLayersAsset.Type)


referencedLayersAsset [](start = 20, length = 21)

What if GetAssetById returns null? #Closed

simeoncran · 2019-04-30T17:07:00Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+            if (referencedAsset == null)
+            {
+                _issues.ReferencedAssetDoesNotExist(assetId);
+                return null;


return null; [](start = 15, length = 13)

This return is redundant, and unnecessarily violates the principle of avoiding multiple exit points in a method. #Closed

simeoncran · 2019-04-30T17:07:34Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+
+            var referencedLayersAsset = GetAssetById(layer.RefId);
+
+            switch (referencedLayersAsset.Type)


referencedLayersAsset [](start = 20, length = 21)

What is GetAssetById returned null? #Closed

simeoncran · 2019-04-30T17:09:06Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

+                    content.Brush = brush;
+                    break;
+                default:
+                    _issues.InvalidAssetReferenceFromCurrentLayer(layer.Type.ToString(), layer.RefId, referencedLayersAsset.Type.ToString(), Asset.AssetType.Image.ToString());


The word "Current" doesn't belong here. Current only makes sense in the context of this method. When people see the error they don't have a concept of "current". #Closed

simeoncran · 2019-04-30T17:10:42Z

source/LottieToWinComp/LottieToWinCompTranslator.cs

@@ -694,7 +731,7 @@ Visual TranslatePreCompLayerToVisual(TranslationContext context, PreCompLayer la
                    AddTranslatedLayersToContainerVisual(contentsNode, subContext, referencedLayers, $"{layer.Name}:{layerCollectionAsset.Id}");
                    break;
                case Asset.AssetType.Image:
-                    _unsupported.ImageAssets();
+                    _issues.InvalidAssetReferenceFromCurrentLayer(layer.Type.ToString(), layer.RefId, referencedLayersAsset.Type.ToString(), Asset.AssetType.LayerCollection.ToString());


InvalidAssetReferenceFromCurrentLayer [](start = 28, length = 37)

Move this into the default and remove the InvalidOperationException(). We don't need to test for Image here. #Closed

simeoncran · 2019-04-30T18:06:48Z

�#� �L�o�t�t�i�e�-�W�i�n�d�o�w�s� �W�a�r�n�i�n�g� �L�T�0�0�0�8�

This file is showing up in CodeFlow as a rename of LT0008.md. That means LT0008.md not longer exists. That's suspicious. I think you've done this by accident, and LT00028 is not a new error. #Closed

Refers to: source/Issues/LT0008.md:9 in b44898e. [](commit_id = b44898e, deletion_comment = True)

plaiMS · 2019-04-30T18:08:57Z

�#� �L�o�t�t�i�e�-�W�i�n�d�o�w�s� �W�a�r�n�i�n�g� �L�T�0�0�0�8�

LT0008 is the ImageAsset() warning that we said no longer needed and can remove. Somehow Codeflow is treating LT0028 as rename of the LT0008 but it's not.

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

Refers to: source/Issues/LT0008.md:9 in b44898e. [](commit_id = b44898e, deletion_comment = True)

simeoncran

plaiMS added 6 commits April 19, 2019 13:42

Prototype changes in TranslateImageLayer

80f4fb0

Adding the pieces plus some hacky for supporting image layer in trans…

cb37fe4

…lator.

Prototype changes in TranslateImageLayer

1777427

Adding the pieces plus some hacky for supporting image layer in trans…

ab862ed

…lator.

Merge branch 'EnableImageLayer' of https://github.com/windows-toolkit…

e4346d6

…/Lottie-Windows into EnableImageLayer

Disable #define EnableImageLayer

dc91f49

plaiMS requested review from eliezerpMS and simeoncran as code owners April 29, 2019 19:31

plaiMS requested a review from sohchatt April 29, 2019 19:31

simeoncran suggested changes Apr 29, 2019

View reviewed changes

sohchatt reviewed Apr 29, 2019

View reviewed changes

Add new issue types and update a few to make their names more clear.

b43759e

simeoncran reviewed Apr 30, 2019

View reviewed changes

Updated all translation issues to more accurately say not supported.

b44898e

simeoncran reviewed Apr 30, 2019

View reviewed changes

Move asset type checking to GetAssetById function.

f42d307

simeoncran approved these changes Apr 30, 2019

View reviewed changes

Merge branch 'master' into EnableImageLayer

480f6d4

plaiMS merged commit 96af58f into master Apr 30, 2019

delete-merged-branch bot deleted the EnableImageLayer branch April 30, 2019 20:18

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding first pieces needed in LottieToWinCompTranslator to enable image layer. #105

Adding first pieces needed in LottieToWinCompTranslator to enable image layer. #105

plaiMS commented Apr 29, 2019

simeoncran commented Apr 29, 2019 •

edited

simeoncran left a comment

sohchatt Apr 29, 2019 •

edited by simeoncran

simeoncran Apr 30, 2019

plaiMS Apr 30, 2019

sohchatt Apr 29, 2019 •

edited by plaiMS

simeoncran commented Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

plaiMS Apr 30, 2019

simeoncran Apr 30, 2019 •

edited

plaiMS Apr 30, 2019

simeoncran Apr 30, 2019 •

edited by plaiMS

plaiMS Apr 30, 2019

simeoncran Apr 30, 2019

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

plaiMS commented Apr 30, 2019

simeoncran left a comment


		var referencedLayersAsset = GetAssetById(layer.RefId);

		switch (referencedLayersAsset.Type)

Adding first pieces needed in LottieToWinCompTranslator to enable image layer. #105

Adding first pieces needed in LottieToWinCompTranslator to enable image layer. #105

Conversation

plaiMS commented Apr 29, 2019

simeoncran commented Apr 29, 2019 • edited

simeoncran left a comment

Choose a reason for hiding this comment

sohchatt Apr 29, 2019 • edited by simeoncran

Choose a reason for hiding this comment

simeoncran Apr 30, 2019

Choose a reason for hiding this comment

plaiMS Apr 30, 2019

Choose a reason for hiding this comment

sohchatt Apr 29, 2019 • edited by plaiMS

Choose a reason for hiding this comment

simeoncran commented Apr 30, 2019 • edited

simeoncran commented Apr 30, 2019 • edited

simeoncran commented Apr 30, 2019 • edited

simeoncran commented Apr 30, 2019 • edited

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

plaiMS Apr 30, 2019

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

plaiMS Apr 30, 2019

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited by plaiMS

Choose a reason for hiding this comment

plaiMS Apr 30, 2019

Choose a reason for hiding this comment

simeoncran Apr 30, 2019

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

simeoncran Apr 30, 2019 • edited

Choose a reason for hiding this comment

simeoncran commented Apr 30, 2019 • edited

plaiMS commented Apr 30, 2019

simeoncran left a comment

Choose a reason for hiding this comment

simeoncran commented Apr 29, 2019 •

edited

sohchatt Apr 29, 2019 •

edited by simeoncran

sohchatt Apr 29, 2019 •

edited by plaiMS

simeoncran commented Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited by plaiMS

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran Apr 30, 2019 •

edited

simeoncran commented Apr 30, 2019 •

edited