Add support for Effects without Win2d for Cpp/Cx #133

eliezerpMS · 2019-06-07T21:58:54Z

This change adds code to support allowing a developer to user Effects (which is necessary for Mattes and Masks) without the developer needing to take a dependency on Win2d.

… supported

plaiMS · 2019-06-13T17:35:12Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+        /// Write the CompositeEffect factory code.
+        /// </summary>
+        /// <param name="builder">A <see cref="CodeBuilder"/> used to create the code.</param>
+        /// <param name="compositeEffect">Composte effect object.</param>


Composte [](start = 42, length = 8)

typo #Closed

plaiMS · 2019-06-13T17:39:03Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

@@ -1849,6 +1874,11 @@ protected internal sealed class CodeGenInfo
            /// </summary>
            public bool UsesStreams { get; }

+            /// <summary>
+            /// Gets a value indicating whether the composition depends the composite effect used as a Windows.Graphics.Effects.IGraphicsEffect.


whether the composition depends the composite effect used as a Windows.Graphics.Effects.IGraphicsEffect. [](start = 40, length = 104)

I don't understand this sentence. Please rephrase. #Closed

This comment now points to the wrong line. I was referring to the comment "Gets a value indicating whether the composition depends on a composite effect used as a Windows.Graphics.Effects.IGraphicsEffect.." on line 1908. I don't get what it's trying to say.

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

plaiMS · 2019-06-13T17:39:58Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

@@ -2123,6 +2153,9 @@ internal string LongComment
            // Set to indicate that the node uses asset file(s).
            internal bool UsesAssetFile => Object is Wmd.LoadedImageSurface lis && lis.Type == Wmd.LoadedImageSurface.LoadedImageSurfaceType.FromUri;

+            // Value indicating whether the composition depends the composite effect used as a Windows.Graphics.Effects.IGraphicsEffect.


the composition depends the composite effect used as a Windows.Graphics.Effects.IGraphicsEffect [](start = 40, length = 95)

same sentence that needs rephrasing. #Closed

I don't see any changes for this sentence.

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

plaiMS · 2019-06-13T17:42:15Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

@@ -2123,6 +2153,9 @@ internal string LongComment
            // Set to indicate that the node uses asset file(s).
            internal bool UsesAssetFile => Object is Wmd.LoadedImageSurface lis && lis.Type == Wmd.LoadedImageSurface.LoadedImageSurfaceType.FromUri;

+            // Value indicating whether the composition depends the composite effect used as a Windows.Graphics.Effects.IGraphicsEffect.
+            internal bool UsesCompositeEffect => Object is CompositionEffectBrush && ((CompositionEffectBrush)Object).GetEffect().Type == Mgce.GraphicsEffectType.CompositeEffect;


Object is CompositionEffectBrush && ((CompositionEffectBrush)Object).GetEffect().Type == Mgce.GraphicsEffectType.CompositeEffect; [](start = 49, length = 129)

Can be written as :
Object is CompositionEffectBrush compositeEffectBrush && compositeEffectBrush.GetEffect().Type == Mgce.GraphicsEffectType.CompositeEffect;

which is more modern and consistent with the code above it. #Closed

plaiMS · 2019-06-13T17:45:09Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+                builder.WriteLine($"compositeEffect{Deref}Sources{Deref}{IListAdd}({New} CompositionEffectSourceParameter({String(source.Name)}));");
+            }
+
+            return $"compositeEffect";


$ [](start = 19, length = 1)

static string does not need the '$'. #Closed

plaiMS · 2019-06-13T17:51:33Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+            if (info.UsesCanvasEffects)
+            {
+                // Write the composite effect class that will allow the use
+                // of this effect without win2d


win2d [](start = 42, length = 5)

Missing a period at the end of comment. #Closed

plaiMS · 2019-06-13T17:55:05Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+
+            if (info.UsesCanvasEffects)
+            {
+                // Write the composite effect class that will allow the use


// [](start = 16, length = 3)

As we discussed offline last night, this PR will make the CX and C# code diverge. You should add a documentation on github about this. #Resolved

Ok. I will sync up with you offline about where to put documentation about this.

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

That'll be a question for Sohum :)

In reply to: 297456313 [](ancestors = 297456313,293504363)

plaiMS · 2019-06-13T17:56:26Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+        /// <inheritdoc/>
+        protected override string GenerateCompositeEffectFactory(CodeBuilder builder, Mgce.CompositeEffect compositeEffect)
+        {
+            builder.WriteLine($"ComPtr<CompositeEffect> compositeEffect(new CompositeEffect());");


$ [](start = 30, length = 1)

$ is not needed. #Closed

plaiMS · 2019-06-13T17:56:47Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+            {
+                builder.OpenScope();
+                builder.WriteLine($"auto sourceParameter = ref new CompositionEffectSourceParameter({String(source.Name)});");
+                builder.WriteLine($"compositeEffect->AddSource(reinterpret_cast<ABI::Windows::Graphics::Effects::IGraphicsEffectSource*>(sourceParameter));");


$ [](start = 34, length = 1)

$ is not needed. #Closed

plaiMS · 2019-06-13T17:57:09Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+                builder.CloseScope();
+            }
+
+            return $"reinterpret_cast<Windows::Graphics::Effects::IGraphicsEffect^>(compositeEffect.Get())";


$ [](start = 19, length = 1)

$ is not needed. #Closed

plaiMS · 2019-06-13T18:00:16Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+            switch (index)
+            {
+            case D2D1_COMPOSITE_PROP_MODE: return statics->CreateUInt32(m_mode, (IInspectable**)value);
+            default: return E_INVALIDARG;


indent. #Closed

plaiMS · 2019-06-13T18:04:21Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+    }
+
+private:
+    // Invokes a functor with the pointer to the property factory


factory [](start = 58, length = 7)

ends with '.' #Closed

plaiMS

simeoncran · 2019-06-13T19:28:54Z

source/WinCompData/Compositor.cs

@@ -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);


This isn't a method on Compositor AFAICT #Closed

Hmm I think I must not have done a merge yet. I branched this off of my previous change.

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

simeoncran

🕐

plaiMS

simeoncran · 2019-06-26T23:54:13Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

+        /// <param name="builder">A <see cref="CodeBuilder"/> used to create the code.</param>
+        /// <param name="compositeEffect">Composite effect object.</param>
+        /// <returns>string representation of the composite effect.</returns>
+        protected virtual string GenerateCompositeEffectFactory(CodeBuilder builder, Mgce.CompositeEffect compositeEffect)


GenerateCompositeEffectFactory [](start = 33, length = 30)

Because the code is very different between C# and CX then this method should be abstract and you should add an implementation into both. There should not be default implementations of things in this class that only apply to C#.
Also: this should not be called "Generate*" because it doesn't follow the pattern of the other "Generate*" methods. It maybe is a "Write*" method.
Also: move it with the other abstract methods.
Also: The comment doesn't seem right - it doesn't return the string representation of the composite effect. #Closed

simeoncran · 2019-06-26T23:58:50Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

-                    builder.WriteLine($"compositeEffect{Deref}Mode = {CanvasCompositeMode(compositeEffect.Mode)};");
+                    effectCreationString = GenerateCompositeEffectFactory(builder, (Mgce.CompositeEffect)effect);
+                    break;
+                default:


default [](start = 16, length = 7)

Do we ever generate code that produces other GraphicsEffectType? Looks like you're asserting that we don't... maybe needs a comment explaining that. #Closed

We could support more in the future.

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

simeoncran · 2019-06-27T00:03:33Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

@@ -1737,7 +1760,7 @@ string StrokeLineJoin(CompositionStrokeLineJoin value)
            }
        }

-        string CanvasCompositeMode(CanvasComposite value)
+        protected string CanvasCompositeMode(CanvasComposite value)


protected [](start = 8, length = 9)

Call me crazy, but... I'd prefer that we don't expose these helpers - they are only meant to be available inside this class. Clever conversion from a type to syntax should either be done in this class, or should be on the stringifier, or should be in the subclass. I want to keep the interface into this class as small as possible. For CX you should hold your nose, copy-paste, and replace the {ScopeResolve} with ::. #Closed

I will put in in stringifierBase and on the Istringifier.

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

simeoncran · 2019-06-27T00:04:26Z

source/UIData/CodeGen/InstantiatorGeneratorBase.cs

@@ -1803,7 +1826,8 @@ protected internal sealed class CodeGenInfo
                bool usesNamespaceWindowsUIXamlMedia,
                bool usesStreams,
                bool hasLoadedImageSurface,
-                IEnumerable<LoadedImageSurfaceNode> loadedImageSurfaceNodes)
+                IEnumerable<LoadedImageSurfaceNode> loadedImageSurfaceNodes,
+                bool usesCompositeEffect)


For reasons of obsessive consistency, move this up with the other bools. #Closed

simeoncran · 2019-06-27T00:07:38Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+    MaskInvert = 12,
+} CanvasComposite;
+
+class CompositeEffect WrlFinal :


WrlFinal [](start = 22, length = 8)

WrlFinal is cruft from a past era. Replace with "final" #Closed

simeoncran · 2019-06-27T00:09:35Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+@"
+#include <vector>
+
+typedef enum CanvasComposite : int


typedef [](start = 0, length = 7)

Shouldn't need to typedef. I realize this is all copy-paste, but if it's not too onerous we should avoid inheriting ugly. #Closed

simeoncran · 2019-06-27T00:18:43Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+        UINT index, 
+        ABI::Windows::Foundation::IPropertyValue ** value) override
+    {
+        return UsePropertyFactory([=](ABI::Windows::Foundation::IPropertyValueStatics* statics)


UsePropertyFactory [](start = 15, length = 18)

This seems way too clever for our use. Any reason to keep the template and lambda rather than just writing the code out directly? #Closed

simeoncran · 2019-06-27T00:21:08Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+    public ABI::Windows::Graphics::Effects::IGraphicsEffectD2D1Interop
+{
+public:
+    CompositeEffect() = default;


CompositeEffect() = default; [](start = 1, length = 31)

Is this necessary? #Closed

simeoncran · 2019-06-27T00:22:30Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+    MaskInvert = 12,
+} CanvasComposite;
+
+class CompositeEffect WrlFinal :


CompositeEffect [](start = 6, length = 15)

It would be good to have a small comment explaining the purpose of this code (which I think is that it's a partial implementation of various IGraphicsEffect* interfaces, sufficient for use by Composition.... right?) #Closed

simeoncran · 2019-06-27T00:24:10Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+        {
+            return E_INVALIDARG;
+        }
+


Consider tightening up some of this code - we really don't care if someone calls this with no UINT*, just don't deref the nullptr and return S_OK. #Closed

simeoncran · 2019-06-27T00:33:37Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+
+    ULONG m_cRef = 0;
+
+    CanvasComposite m_mode = CanvasComposite::SourceOver;


SourceOver [](start = 46, length = 10)

Why do we initialize to this explicit value rather than just the default value (which happens to be the same thing). I.e. why not just {} initialize all of the fields? #Closed

simeoncran · 2019-06-27T00:36:57Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+    IFACEMETHODIMP GetEffectId(GUID* id) override 
+    { 
+        // set CLSID_D2D1Composite value
+        *id = { 0x48fc9f51, 0xf6ac, 0x48f1, { 0x8b, 0x58,  0x3b,  0x28,  0xac,  0x46,  0xf7,  0x6d } };


Not checking for id == nullptr here. You should do what I suggest for GetSourceCount (i.e. just don't crash but return S_OK) #Closed

simeoncran · 2019-06-27T00:38:25Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+        ULONG * iidCount,
+        IID ** iids) override
+    {
+        *iidCount = 0;


null checks? #Closed

simeoncran · 2019-07-01T18:41:21Z

source/UIData/CodeGen/CSharpInstantiatorGenerator.cs

+                builder.WriteLine($"compositeEffect.Sources.Add(new CompositionEffectSourceParameter({String(source.Name)}));");
+            }
+
+            return "compositeEffect";


compositeEffect [](start = 20, length = 15)

This string exists in 4 separate places in this method. It should be defined once and reused rather than copying the string around. #Closed

simeoncran · 2019-07-01T18:45:43Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+    MaskInvert = 12,
+};
+
+// This class is a substitue for the Microsoft::Graphics::Canvas::Effects::CompositeEffect


substitue [](start = 19, length = 9)

typo #Closed

simeoncran · 2019-07-01T18:56:41Z

source/UIData/CodeGen/CxInstantiatorGenerator.cs

+            return S_OK;
+        }
+
+        return E_NOINTERFACE;


return [](start = 8, length = 6)

*ppvObject is not assigned in this case. I think the contract is specified in various places as out... which means it should get nullptr.
Anyhow, consider refactoring this to make it shorter and more efficient:
a. check whether ppvObject is nullptr, and if it isn't...
b. assign *ppvObject with nullptr
c. do the __uuidof checks, and when one succeeds assign the result of the cast this pointer. Use static_cast<...> syntax to be modern. Use "else if" to avoid the multiple GUID comparisons.
d. After all the if/else-ifs, if *ppvObject != null, AddRef() and return S_OK. Otherwise return E_NONTERFACE. #Closed

simeoncran

simeoncran

eliezerpMS added 7 commits April 30, 2019 13:08

Initial changes for multi shape mask and matte translation support

acccdc8

Merge branch 'master' into MultiMaskAndMatteTranslation

8c821ff

Add new tranlation issues and fix some issues after merge

3a3088a

Editing TranslateAndApplyMasks return value when VisualSurface is not…

c742927

… supported

Initial changes for getting composite effect to work without win2d

113deea

Merge branch 'master' into user/eliezerpms/compositeEffectWithoutWin2d

9e89e3e

Minor change and a fix before PR

5cb8bbb

eliezerpMS requested a review from simeoncran as a code owner June 7, 2019 21:58

eliezerpMS requested a review from plaiMS June 10, 2019 16:10

plaiMS reviewed Jun 13, 2019

View reviewed changes

plaiMS previously approved these changes Jun 13, 2019

View reviewed changes

simeoncran reviewed Jun 13, 2019

View reviewed changes

simeoncran suggested changes Jun 13, 2019

View reviewed changes

Merge branch 'master' into user/eliezerpms/compositeEffectWithoutWin2d

31254e9

eliezerpMS dismissed plaiMS’s stale review via 31254e9 June 26, 2019 01:34

eliezerpMS added 2 commits June 26, 2019 09:05

Addressing previous iteration comments

5cc7f17

Addressing previous iteration comments

89f8bbf

plaiMS previously approved these changes Jun 26, 2019

View reviewed changes

simeoncran reviewed Jun 26, 2019

View reviewed changes

simeoncran reviewed Jun 27, 2019

View reviewed changes

Addressing current iteration comments

1d36bd6

eliezerpMS dismissed plaiMS’s stale review via 1d36bd6 July 1, 2019 16:28

Fix minor missed issue from previous iteration

3ba5ac7

simeoncran reviewed Jul 1, 2019

View reviewed changes

Addressing previous iteration comments

d115e17

simeoncran approved these changes Jul 1, 2019

View reviewed changes

eliezerpMS merged commit 7cdbb40 into master Jul 1, 2019

delete-merged-branch bot deleted the user/eliezerpms/compositeEffectWithoutWin2d branch July 1, 2019 23:58

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


		ULONG m_cRef = 0;

		CanvasComposite m_mode = CanvasComposite::SourceOver;

Add support for Effects without Win2d for Cpp/Cx #133

Add support for Effects without Win2d for Cpp/Cx #133

Conversation

eliezerpMS commented Jun 7, 2019

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 26, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 26, 2019

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited by eliezerpMS

Choose a reason for hiding this comment

eliezerpMS Jun 26, 2019

Choose a reason for hiding this comment

plaiMS Jun 26, 2019

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 • edited

Choose a reason for hiding this comment

plaiMS left a comment

Choose a reason for hiding this comment

simeoncran Jun 13, 2019 • edited

Choose a reason for hiding this comment

eliezerpMS Jun 13, 2019

Choose a reason for hiding this comment

simeoncran left a comment

Choose a reason for hiding this comment

plaiMS left a comment

Choose a reason for hiding this comment

simeoncran Jun 26, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 26, 2019 • edited

Choose a reason for hiding this comment

eliezerpMS Jun 30, 2019

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

eliezerpMS Jun 27, 2019

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jun 27, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jul 1, 2019 • edited

Choose a reason for hiding this comment

simeoncran Jul 1, 2019 • edited

Choose a reason for hiding this comment

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 26, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited by eliezerpMS

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

plaiMS Jun 13, 2019 •

edited

simeoncran Jun 13, 2019 •

edited

simeoncran Jun 26, 2019 •

edited

simeoncran Jun 26, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jun 27, 2019 •

edited

simeoncran Jul 1, 2019 •

edited

simeoncran Jul 1, 2019 •

edited

simeoncran Jul 1, 2019 •

edited