WindowsCommunityToolkit icon indicating copy to clipboard operation
WindowsCommunityToolkit copied to clipboard

Expression animation does not work after update the Microsoft.Toolkit.Uwp.UI.Animations nuget package

Open h82258652 opened this issue 2 years ago • 8 comments

Describe the bug

After update the Microsoft.Toolkit.Uwp.UI.Animations nuget package. The expression animation does not work.

Regression

7.0.2

Reproducible in sample app?

  • [ ] This bug can be reproduced in the sample app.

Steps to reproduce

I create a bug demo here: https://github.com/h82258652/ToolkitExpressionReferenceBugDemo The 7.0.2 version is working and the 7.1.2 version is not working.

Expected behavior

The 7.1.2 version should work like the 7.0.2 version.

Screenshots

No response

Windows Build Number

  • [ ] Windows 10 1809 (Build 17763)
  • [ ] Windows 10 1903 (Build 18362)
  • [ ] Windows 10 1909 (Build 18363)
  • [ ] Windows 10 2004 (Build 19041)
  • [ ] Windows 10 20H2 (Build 19042)
  • [ ] Windows 10 21H1 (Build 19043)
  • [X] Windows 11 21H2 (Build 22000)
  • [ ] Other (specify)

Other Windows Build number

No response

App minimum and target SDK version

  • [ ] Windows 10, version 1809 (Build 17763)
  • [ ] Windows 10, version 1903 (Build 18362)
  • [ ] Windows 10, version 1909 (Build 18363)
  • [ ] Windows 10, version 2004 (Build 19041)
  • [ ] Other (specify)

Other SDK version

No response

Visual Studio Version

2022

Visual Studio Build Number

4.8.04161

Device form factor

Desktop

Nuget packages

7.0.2 7.1.2

Additional context

No response

Help us help you

No.

h82258652 avatar Jul 06 '22 02:07 h82258652

Hello h82258652, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

ghost avatar Jul 06 '22 02:07 ghost

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost avatar Jul 21 '22 04:07 ghost

The link provided for the bug didn't work for me, Can you provide more info on the issue

LalithaNadimpalli avatar Jul 22 '22 22:07 LalithaNadimpalli

Oh, sorry. I forget to make it public. It should be works now. Could you try again? Thanks.

h82258652 avatar Jul 22 '22 23:07 h82258652

Thanks for supply the repro @h82258652.

After some investigation I managed to find the commit that introduced this issue: fa6a3ed9f4de06a275fb4f2046da04ce3aef592f.

I'll continue debugging this on Monday. @arcadiogarcia, this is code you added in #4183, any quick insights you can provide in the meantime?

Arlodotexe avatar Aug 27 '22 00:08 Arlodotexe

Haven't tried running the code yet, but I'm pretty sure that the issue is that my fix didn't take into consideration the pattern where CreateConstantVector3 (or some of the other equivalent methods) is used - creating a Vector3 with a ParamName predetermined at its constructor - so then when CreateExpressionAnimationFromNode invokes ClearReferenceInfo it clears the user-given property name and replaces it with a procedural id which does no longer match the id of the Vector3 parameter.

If we all agree that

  • The ParamName generated procedurally to refer to "anonymous" nodes shouldn't be reused across runs, and the best way to do that is to wipe them clean right before generating an expression string
  • We do want to preserve the ParamName for the nodes that where named by the user, not only to guarantee

then I would propose the following fix

  • We create another field under ExpressionNode to hold the ephemeral text that is actually used to refer to the node in the final expressionstring, called nodeName or something like that
  • ClearReferenceInfo wipes that new field (instead of ParamName)
  • ParamName is still set (and only set when) the user creates a named node (e.g. via CreateExpressionAnimationFromNode) explicitly
  • EnsureReferenceInfo populates nodeName using the same logic currently being used to populate ParamName, unless ParamName already has a value, in that case we just copy it

That way we still wipe the name of anonymous nodes, while preserving user-named parameters. Thoughts?

arcadiogarcia avatar Sep 05 '22 19:09 arcadiogarcia

@arcadiogarcia Thanks, I think I understand the issue and why it's happening now. I'll give the proposed fix a try and report back if I hit any issues.

Arlodotexe avatar Sep 06 '22 18:09 Arlodotexe

@Arlodotexe Cool! And sorry for the delay, I don't check my github notifications very often, feel free to ping me here or by other channels if I can be of help.

arcadiogarcia avatar Sep 06 '22 18:09 arcadiogarcia