-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multi-shape mask and matte translation #106
Conversation
source/Lottie/Instantiator.cs
Outdated
@@ -879,26 +879,23 @@ Wc.CompositionEffectBrush GetCompositionEffectBrush(Wd.CompositionEffectBrush ob | |||
return result; | |||
} | |||
|
|||
var compositeEffect = new Mgce.CompositeEffect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put this back to no longer have the switch statements. The issues was that we need to call CreateEffectFactory after populating the compositeEffect. I would be ok with making this function have 2 switch statements instead. One for before the CreateEffectFactory for the composite effect initialization and one for the compositeEffectBrush initialization #Closed
!mask.Opacity.IsAnimated && | ||
mask.Mode == Mask.MaskMode.Additive && | ||
layer.Masks.Length == 1) | ||
#if AllowVisualSurface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot that we by default assumed that we would run on an OS that supports geometric clip and so the "masks are not supported" issue is not used. I will put this back in here in the event "layer.Masks.Any()" and we do not allow for VisualSurface #Closed
CompositionSurfaceBrush CreateSurfaceBrush(ICompositionSurface surface) | ||
{ | ||
return _c.CreateSurfaceBrush(surface); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added and now in master. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
source/WinCompData/Compositor.cs
Outdated
@@ -66,5 +66,7 @@ sealed class Compositor | |||
public CompositionSurfaceBrush CreateSurfaceBrush(ICompositionSurface surface) => new CompositionSurfaceBrush(surface); | |||
|
|||
public CompositionEffectFactory CreateEffectFactory(Mgce.GraphicsEffectBase effect) => new CompositionEffectFactory(effect); | |||
|
|||
public CompositionEffectBrush CreateEffectBrush(Mgce.GraphicsEffectBase effect) => new CompositionEffectBrush(effect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be a method on the real compositor. Am I missing something? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why I added this. I will remove it and use CreateEffectFactory
In reply to: 283918629 [](ancestors = 283918629)
@@ -19,6 +19,14 @@ | |||
// property without going through a controller. Enable this to prevent flashes. | |||
#define ControllersSynchronizationWorkaround | |||
|
|||
// define POST_RS5_SDK if using an SDK that is for a release | |||
// after RS5 | |||
#if POST_RS5_SDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POST_RS5_SDK [](start = 4, length = 12)
The 10.0.18362.0 SDK is out now. Do we need this #define any more? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline I just added a github issue. We should look into switching the build system and default SDK over to the newest release. However, we also probably need to be able to surface issues when a user tries to run on an older Windows build.
In reply to: 283919302 [](ancestors = 283919302)
@@ -222,13 +232,41 @@ void Translate() | |||
} | |||
|
|||
// Takes a list of Visuals and Shapes and returns a list of Visuals. | |||
IEnumerable<Visual> VisualsAndShapesToVisuals(TranslationContext context, IEnumerable<CompositionObject> items) | |||
IEnumerable<Visual> VisualsAndShapesToVisuals(TranslationContext context, IEnumerable<(CompositionObject translatedLayer, Layer layer)> items) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to ShapeOrVisual to make the intention a bit clearer. Will need updating in the LINQ query that creates this list. #Closed
@@ -222,13 +232,41 @@ void Translate() | |||
} | |||
|
|||
// Takes a list of Visuals and Shapes and returns a list of Visuals. | |||
IEnumerable<Visual> VisualsAndShapesToVisuals(TranslationContext context, IEnumerable<CompositionObject> items) | |||
IEnumerable<Visual> VisualsAndShapesToVisuals(TranslationContext context, IEnumerable<(CompositionObject translatedLayer, Layer layer)> items) | |||
{ | |||
ShapeVisual shapeVisual = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is really hard for me to read - I don't understand all the magic. It was bad enough before, but at least the rule was simple - share a ShapeVisual if you can. But now I don't know how to explain this code. Can it be refactored or commented to make it clearer?
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline. I will just spit this function and have another function that does the matte work
In reply to: 283922427 [](ancestors = 283922427)
@@ -268,121 +334,229 @@ IEnumerable<Visual> VisualsAndShapesToVisuals(TranslationContext context, IEnume | |||
} | |||
} | |||
|
|||
void TranslateAndApplyMask(TranslationContext context, Layer layer, Visual visualForMask) | |||
// Helper for shape trees that need a mask applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End comments with a period. #Closed
@@ -268,121 +334,229 @@ IEnumerable<Visual> VisualsAndShapesToVisuals(TranslationContext context, IEnume | |||
} | |||
} | |||
|
|||
void TranslateAndApplyMask(TranslationContext context, Layer layer, Visual visualForMask) | |||
// Helper for shape trees that need a mask applied | |||
Visual TranslateAndApplyMasksOnShapeTree( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method needs a lot more explanation. #Closed
return TranslateAndApplyMasks(context, layer, contentShapeVisual); | ||
} | ||
|
||
Mask.MaskMode TranslateAndAddMaskPaths( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs comments.
Does it translate layer and put it in maskContainerShape? Is this a layer that was previously translated and is being translated again? #Closed
|
||
// The mask geometry needs to be colored with something so that it can be used | ||
// as a mask. | ||
maskSpriteShape.FillBrush = CreateColorBrush(LottieData.Color.Black); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black [](start = 90, length = 5)
Is this color used later on? If so, can it be given a name so it's obvious how they're related? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This color is not used later on. I just need a color that is not transparent.
In reply to: 283925078 [](ancestors = 283925078)
ApplyPathKeyFrameAnimation(context, geometry, SolidColorFill.PathFillType.EvenOdd, compositionPathGeometry, "Path", "Path", null); | ||
} | ||
|
||
if (maskMode == Mask.MaskMode.None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can change on each iteration. currentMaskMode? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layer layer, | ||
Visual visualToBeMasked) | ||
{ | ||
if (layer.Masks != null && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
layer.Masks != [](start = 16, length = 14)
Can we give Layer a default empty list of masks so we never have to check it for null? #Closed
ContainerVisual rootContainerVisual) | ||
Visual matteLayer, | ||
Visual mattedLayer, | ||
bool invert = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false [](start = 26, length = 5)
Avoid optional parameters on private methods. It requires the reader at the callsite to have to guess at what the default behavior is. #Closed
shapeVisual.Size = Vector2(context.Width, context.Height); | ||
// Calculate the context size which we will use as the size of the images we want to use | ||
// for the matte content and the content to be matted. | ||
var contextSize = Vector2(context.Width, context.Height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ector2(context.Width, context.Height); [](start = 31, length = 38)
I've noticed this code a few times. It should be put into the TranslationContext so it doesn't get repeated. #Closed
matteLayer, | ||
mattedLayer, | ||
contextSize, | ||
Sn.Vector2.Zero, invert ? CanvasComposite.DestinationOut : CanvasComposite.DestinationIn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invert ? CanvasComposite.DestinationOut : CanvasComposite.DestinationI [](start = 41, length = 70)
The final argument should be on a separate line, to be consistent with the other arguments. #Closed
Sn.Vector2.Zero, invert ? CanvasComposite.DestinationOut : CanvasComposite.DestinationIn); | ||
} | ||
|
||
SpriteVisual CompositeVisuals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a comment explaining the intention. #Closed
Sn.Vector2 offset, | ||
CanvasComposite compositeMode) | ||
{ | ||
// There is a bug where if the root visual is set as a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug [](start = 26, length = 3)
Bug in Composition? Make that clear. Is there any chance the bug will be fixed one day? If so you should mention which versions are known to have the problem. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't make any promises on when this bug will be fixed but I can call out when it was first introduced.
In reply to: 283929978 [](ancestors = 283929978)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
|
||
if (mask.Inverted) | ||
// NOTE: we assume here that the items appear in reverse order from how they appear in the original Lottie file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we assume here that the items a [](start = 21, length = 31)
Seeing as the code in this file determines that, I'd make a stronger statement - they are in that order. And explain what that order means, e.g. the matte layer is always before the matted layer (or whatever it is). #Closed
|
||
// ShapeVisual clips to its size | ||
#if NoClipping | ||
shapeVisual.Size = Vector2(float.MaxValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note - we have a helper that does this now (create the shape visual, adds the shape, and set the size) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline it looks like the helper is not there.
In reply to: 287538866 [](ancestors = 287538866)
{ | ||
_issues.MaskWithUnsupportedMode(mask.Mode.ToString()); | ||
// The layer to be matted is expected to come first. It is also expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected [](start = 49, length = 8)
Be strong! It's not just expected, it DOES come first. We're not even checking for it, right? #Resolved
{ | ||
_issues.MultipleShapeMasksIsNotSupported(); | ||
yield return item.translatedLayer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment on each of the yield returns explaining what they're returning. That would help me understand the difference between returning "visual" and "item.translatedLayer". I think I understand what the compositedMatteVisual is. #Closed
case CompositionObjectType.CompositionSpriteShape: | ||
if (layerIsMattedLayer || | ||
mattedVisual != null) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment explaining (I think) that this layer is involved in matteing, but it is a Shape so it must be converted to a Visual. #Closed
|
||
var contentShapeVisual = CreateShapeVisual(); | ||
contentShapeVisual.Size = contextSize; | ||
contentShapeVisual.Shapes.Add(containerShapeToMask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the new helper for this. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline it looks like the helper is not there.
In reply to: 287539420 [](ancestors = 287539420)
foreach (var mask in layer.Masks) | ||
{ | ||
var currentMaskMode = mask.Mode; | ||
bool maskCanBeTranslated = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool [](start = 20, length = 4)
var
#Closed
if (mask.Inverted) | ||
{ | ||
_issues.MaskWithInvertIsNotSupported(); | ||
maskCanBeTranslated = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skCanBeTranslated = false; [](start = 26, length = 26)
I'm not sure I follow the logic... is the idea that you will ignore this mask but translate all the others? Why not just replace the "maskCanBeTranslated = false" with a "continue" (and a comment). #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3602,6 +3816,8 @@ void Describe(IDescribable obj, string longDescription, string shortDescription | |||
|
|||
static Sn.Vector3 Vector3(double x, double y, double z) => new Sn.Vector3((float)x, (float)y, (float)z); | |||
|
|||
static Sn.Vector3 Vector3(float x) => new Sn.Vector3((float)x, (float)x, (float)x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
float [](start = 34, length = 5)
I couldn't see where this is called. Assuming it's useful, can the parameter be a double? If it needs to be a float then the body of the method doesn't need the casts. #Closed
var sourceIntermediateParent = CreateContainerVisual(); | ||
sourceIntermediateParent.Children.Add(source); | ||
|
||
var destinationIntermediateParent = CreateContainerVisual(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it help future readers to have some ASCII art here that explains how the visuals and the effects are constructed? #Resolved
// 3) A CompositeEffect is created which will be used to do the combining of the contents of the CompositionVisualSurfaceBrushes. | ||
// 4) A CompositionEffectBrush linked to the CompositeEffect is created so that the CompositeEffect can be rendered. | ||
// 5) A SpriteVisual is created and the CompositionEffectBrush is assigned to it so that the CompositionEffectBrush contents are | ||
// actually rendered as part of the visual tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do ASCII art I suspect these explanations aren't needed. I think ASCII art would be quicker to grok. #Resolved
if (layer.Masks.Any()) | ||
{ | ||
// Translate the mask paths | ||
var compositionPathGeometryMasks = new List<(Mask mask, CompositionPathGeometry geometry)>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used? I'm not seeing it. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I guess I just forgot to delete it. I will delete this.
In reply to: 288813296 [](ancestors = 288813296)
maskShapeVisual.Size = contextSize; | ||
maskShapeVisual.Shapes.Add(containerShapeMaskRootNode); | ||
|
||
var maskMode = TranslateAndAddMaskPaths(context, layer, containerShapeMaskContentNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maskMode [](start = 20, length = 8)
It feels awkward to be smuggling the success/failure flag into the maskMode. Why not make this bool TryTranslateAndAddMaskPaths(context, layer, containerShapeMaskContentMode, out maskMode) so that it's explicit. #Resolved
TranslationContext context, | ||
CompositionContainerShape containerShapeToMask) | ||
{ | ||
var contextSize = context.Size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var contextSize = context.Size; [](start = 11, length = 32)
Why not just set the .Size = context.Size; rather than storing it then setting it? #Pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
@@ -114,7 +114,7 @@ RunResult Run() | |||
#if DO_NOT_PROCESS_IN_PARALLEL | |||
foreach (var (file, relativePath) in matchingInputFiles) | |||
{ | |||
if (!LottieFileProcessor.ProcessFile(_options, _reporter, file, Path.Combine(outputFolder, relativePath))) | |||
if (!LottieFileProcessor.ProcessFile(_options, _reporter, file, System.IO.Path.Combine(outputFolder, relativePath))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.IO. [](start = 76, length = 10)
I don't see any other change that made this necessary? If anything it looks like we can remove the qualification from the other System.IO.Path usages. #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gives me: error CS0104: 'Path' is an ambiguous reference between 'Microsoft.Toolkit.Uwp.UI.Lottie.LottieData.Path' and 'System.IO.Path'
In reply to: 288814420 [](ancestors = 288814420)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change adds the translation logic for supporting masks with multiple shapes and mattes. Both of these use the VisualSurface. The single shape mask case that used GeometryClip has been removed.