-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Clear ReferenceNode ParamNames before serializing each expression string to avoid collisions #4183
Clear ReferenceNode ParamNames before serializing each expression string to avoid collisions #4183
Conversation
Thanks arcadiogarcia for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
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 good! I guess there's a tiny bit of overhead due to the recursive reset for each call now, but (1) each call is very very short, (2) trees are generally very small anyway given that they're manually built for animations and (3) they're never created in hot-paths anyway, as one would just create an expression animation once and forget about it.
Thanks for the fix! 🙌
@arcadiogarcia I'm not sure what happened here. I merged your other PR #4189 and then resolved a conflict there, but the other half of c4f6009 seems to be missing now that uses the new function??? I think you had your two branches based off one another or something and had a remove commit in the other one, so now that's part of our history from the other merge and causing it to be missing in this one... Can you reset your fork's main, rebase your branch on top of it, but then only take the relevant commits to your ClearReference to fix the PR? |
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.
Invalid history because of history of #4189
25933c5
to
fa6a3ed
Compare
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
I just force pushed a new history for this branch that has a new commit with the same changes, should be good to go. |
Thanks @arcadiogarcia! Will double-check everything looks good post-merge and we should be good. |
Fixes
PR Type
What kind of change does this PR introduce?
This is a fix that prevents name collisions when nodes/subtrees are reused to build more than one expression string. It is not completely clear to me if reusing nodes/subtrees to build multiple expression strings is a explicitly supported scenario, but the docs don't warn against it and it is imo desirable to:
As far as I can tell, the underlying issue has already existed independently of whether we use the "InteractionTracker_1", guids or alphabetical identifiers, but it is now easier to hit since my previous PR #4112 makes the ids sequential.
What is the current behavior?
Currently, if you try to reuse a ReferenceNode across many animations, we only set the ParamName of each node once and we reuse the old name on future animations. Since ParamNames are only guaranteed to be unique for each call to EnsureReferenceInfo, this can result in two nodes having the same ParamName, e.g.
throws
This means that in practice it is dangerous to reuse references nodes as collisions might happen in a way that is hard to predict, depending on the tree of each animation and the order in which they start. E.g. in the previous example, the code runs fine if we swap the order of the StartAnimation calls.
What is the new behavior?
ClearReferenceInfo is always called before ToExpressionString, wiping the ParamName and _objRefList in each node in the tree. This has a small cost since it involves traversing the tree, but guarantees that we don't reuse stale data from previous calls.
An alternative solution would be to clear the data after/at the end of each ToExpressionString call, to guarantee that the nodes are always in a clean state.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information