-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Allow changing imported AnimationLibrary names in AnimationPlayer in the editor #67965
Conversation
Is this the right way? Changes to externally imported resources are not saved and their editing should be prohibited. @SaracenOne @reduz There are already several PRs to make them unique and editable instead, and I think those should be prioritized (but I do not remember if those worked with AnimationLibrary or not). |
This is not a property on AnimationLibrary, it's a property of the AnimationPlayer, as explained in the linked issue. It belongs to the scene and does get saved. |
I remember discussing with @reduz earlier that a scene might temporarily store key values, but the problem is that those changes are lost when the resource is re-imported. |
I think that's referring to keys used by libraries to reference animations, not keys used by players to reference libraries. I was modifying these keys before this change happened, and I've been editing |
Ah, I may have been a bit misleading, by "key" do you mean the animation library name in AnimationPlayer? It is indeed a property of AnimationPlayer. I had it confused with Key in Animation Track. |
Yeah, it saves as a dictionary with string keys and library values. |
AnimationPlayer should be temporarily read-only when imported from outside, but after the scene is Make Local, library renaming in AnimationPlayer should be allowed. |
I think this should depend on whether the scene is editable rather than the animation library. Should not be always editable. @SaracenOne may give some good feedback. |
In what situation is a scene not editable, and how do you check? |
So, I apologize for not looking over this sooner. Admittedly, the 'rules' around editable vs non-editable is a quite confusing, and it does require me to repeatedly test things to make sure they do indeed work as intended. Admittedly, a lot of my testing was based around animation libraries embeded into imported scenes rather than this workflow which I really learned about recently, and I think it's permissible to allow editing the names of the keys since the implicit result is, due to the fact that renaming the keys simply modifies the libraries property which is just key/value dictionary, it's not affecting the underlying libraries which have much stricter restrictions on what you can do with them. I did run into an issue though where upon modifying the [global] key in one of my test scenes which reference a library embedded in another, things still continued to work as expected, the library was referenced as embedded in another scene, but upon investigating, still seemed everything was referenced properly. However, upon reverting the scene, the external library reference got broken. I think it's related to another bug though which I've raised a potential fix for here. #82750 As such, since that bug seems to be out of the scope of this PR, I'm going to tentitively approve this for merging. Apologies again for being so overdue. |
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.
LGTM 👍
Happy PR anniversary 🎉 😅 Could you rebase the PR on latest |
f511547
to
1419eef
Compare
Rebased. |
Thanks! |
Fixes #65625. Doesn't make anything else editable.