Skip to content
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

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

Closed
1 of 14 tasks
h82258652 opened this issue Jul 6, 2022 · 8 comments · Fixed by #4756
Closed
1 of 14 tasks
Assignees
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 need more info 📌
Milestone

Comments

@h82258652
Copy link
Contributor

h82258652 commented Jul 6, 2022

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)
  • 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 h82258652 added the bug 🐛 An unexpected issue that highlights incorrect behavior label Jul 6, 2022
@ghost ghost added the needs triage 🔍 label Jul 6, 2022
@ghost
Copy link

ghost commented Jul 6, 2022

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 ghost added the needs attention 👋 label Jul 21, 2022
@ghost
Copy link

ghost commented Jul 21, 2022

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.

@LalithaNadimpalli
Copy link
Contributor

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

@h82258652
Copy link
Contributor Author

h82258652 commented Jul 22, 2022

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

@michael-hawker michael-hawker added this to the 7.1.3 milestone Aug 9, 2022
@Arlodotexe Arlodotexe self-assigned this Aug 26, 2022
@Arlodotexe
Copy link
Member

Arlodotexe commented Aug 27, 2022

Thanks for supply the repro @h82258652.

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

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

@arcadiogarcia
Copy link
Member

arcadiogarcia commented Sep 5, 2022

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?

@Arlodotexe
Copy link
Member

Arlodotexe commented Sep 6, 2022

@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.

@arcadiogarcia
Copy link
Member

@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.

@ghost ghost added the In-PR 🚀 label Sep 6, 2022
@ghost ghost added Completed 🔥 and removed In-PR 🚀 labels Oct 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 need more info 📌
Projects
Status: Done
5 participants