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

Allow changing imported AnimationLibrary names in AnimationPlayer in the editor #67965

Merged
merged 1 commit into from
Oct 28, 2023

Conversation

jtnicholl
Copy link
Contributor

Fixes #65625. Doesn't make anything else editable.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 29, 2022

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

@jtnicholl
Copy link
Contributor Author

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.

@TokageItLab
Copy link
Member

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.

@jtnicholl
Copy link
Contributor Author

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 .tscn files in a text editor as a workaround since, and reimporting hasn't reset anything.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 29, 2022

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.

@jtnicholl
Copy link
Contributor Author

Yeah, it saves as a dictionary with string keys and library values.

@TokageItLab TokageItLab changed the title Allow changing keys for imported AnimationLibraries in the editor Allow changing imported AnimationLibrary names in AnimationPlayer on the editor Oct 29, 2022
@TokageItLab
Copy link
Member

AnimationPlayer should be temporarily read-only when imported from outside, but after the scene is Make Local, library renaming in AnimationPlayer should be allowed.
In any case, I think that this PR needs to be reviewed by @reduz.

@TokageItLab TokageItLab requested a review from a team October 29, 2022 16:54
@jtnicholl jtnicholl changed the title Allow changing imported AnimationLibrary names in AnimationPlayer on the editor Allow changing imported AnimationLibrary names in AnimationPlayer in the editor Nov 22, 2022
@reduz
Copy link
Member

reduz commented Nov 28, 2022

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.

@jtnicholl
Copy link
Contributor Author

In what situation is a scene not editable, and how do you check?
Currently, imported scenes can be edited, the editor just displays some warnings that changes won't be saved.

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@SaracenOne SaracenOne self-assigned this Oct 27, 2023
@SaracenOne
Copy link
Member

SaracenOne commented Oct 27, 2023

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.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@akien-mga
Copy link
Member

Happy PR anniversary 🎉 😅

Could you rebase the PR on latest master? There's no conflict so it's not strictly speaking needed, but since the commit is so old it would be good to refresh it so it shows up close to its merge commit.

@jtnicholl
Copy link
Contributor Author

Rebased.

@akien-mga akien-mga merged commit 8bdf3c7 into godotengine:master Oct 28, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot change the key AnimationPlayer uses for an imported AnimationLibrary
6 participants