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

Add a "reset" animation to AnimationPlayer #1597

Closed
reduz opened this issue Oct 2, 2020 · 28 comments
Closed

Add a "reset" animation to AnimationPlayer #1597

reduz opened this issue Oct 2, 2020 · 28 comments

Comments

@reduz
Copy link
Member

reduz commented Oct 2, 2020

Describe the project you are working on:
Godot
Describe the problem or limitation you are having in your project:

Users often complain that, despite the huge flexibility, a key limitation of AnimationPlayer, is the fact that once an animation is played, its initial values are lost. This is specially annoying for cinematics or UI animations because, when saving, the wrong state gets saved to disk. Additionally, if you have multiple animations that each modify a different state of the scene, resetting everything can be a very laborious task.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

To overcome this problem, the idea is to create a "reset" track. It will help restore default values when needed.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

When a new track is added to an animation, the UI will also ask you if you want to create a reset track for it. This is on by default.

image

If you do this, an additional new animation named RESET will be created and a key with the current value for this track will be added there (added at time 0, and the animation will have length zero), besides adding it to the currently edited animation:

image

Now that you have the RESET animation, you can do a couple of things:

  • The first and most obvious one is to have a reset button, that just applies the reset track and resets everything. It can be a button or an item in the EDIT menu, up to you guys to discuss it.
    image
    If not used as often, menu item may be better (we could start with this option, to avoid clutter, then move it to dedicated button if we see its actually needed)
  • AnimationPlayer can get an extra property in the inspector "Reset on Save", which means that when the scene is saved, before actually saving, the reset track is applied. This ensures tracks are always saved with the initial value.
  • As RESET is an animation, it can be very easy to add your custom tracks, call functions, or inspect and change default values.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
Is there a reason why this should be core and not an add-on in the asset library?:

No, this needs to be core, requires changes to the existing tooling.

@Sslaxx
Copy link

Sslaxx commented Oct 2, 2020

Is this intended to fix godotengine/godot#812 this issue?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2020

When a new track is added to an animation, the UI will also ask you if you want to create a reset track for it. This is on by default.

This dialog can be disabled. Does this mean by default the RESET track will be created without confirmation? (personally I wouldn't mind)

Also, it could be convenient to have an option to quickly (or auto?) update the value in RESET track, in case you want to change the default. I imagine someone might change the property and forget to update the default and lose their change, so we might want to prevent that. (and such problems aren't very uncommon, see #694 for example)

Is this intended to fix godotengine/godot#812 this issue?

Yes.

@reduz
Copy link
Member Author

reduz commented Oct 2, 2020

@KoBeWi if you disable it its up to you, we can add another global option "create reset tracks by default", which is what ticks the checkbox in the dialog too.

@jcostello
Copy link

@KoBeWi yes to the auto update the reset track

@RandomShaper
Copy link
Member

RandomShaper commented Oct 2, 2020

This can be really useful and also would integrate seamlessly in my workflow.

I usually have a zero-length rest animation, that I can go back to if things get messed up and, more importantly, I want my scenes to be saved in the reset/default state represented by it.

This will be even more powerful in a workflow like this if people set the RESET animation as autoplay.

Suggestion: reset before save also restores the old state after saving is done (should be easy to implement thanks to the code used for onion skinning).

@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2020

Suggestion: reset before save also restores the old state after saving is done (should be easy to implement thanks to the code used for onion skinning).

I think this is meant to only affect what is saved, so the scene state in the editor would not change anyways.

@eon-s
Copy link

eon-s commented Oct 2, 2020

What about making a snapshot only for the editor? because keeping sync of a "reset" track may be annoying and you will need a reset track for many states (making it very confusing).

A snapshot should be a set of values you want the editor keep on save, ignoring anything else modified by an animationplayer or potentially another similar tool.

@reduz
Copy link
Member Author

reduz commented Oct 2, 2020

@eon-s It will be marked as a special track to avoid confusion. Also as many mentioned, you still want the ability to quickly reset things from within the game and the fact its an animation means you can queue it between two of them. Many users already do this manually as part of their workflow.

@Itachi124 Additionaly to the point above, doing what you suggest means you have to create a separate editor UI that does exactly the same as the animation editor (you want to edit default values, add them, remove them, change their order, make sure if you rename a node the paths rename, preview the values, etc), which makes not much sense to do twice. Additionally, users often want to call functions when resetting, and using the same system allows you do it by adding a call track.

@Calinou
Copy link
Member

Calinou commented Oct 2, 2020

The reset track could probably be shown as [reset] to distinguish it better from other tracks. It could also be displayed always at the top of the list, and perhaps with a different color (if possible).

@RandomShaper
Copy link
Member

The reset track could probably be shown as [reset] to distinguish it better from other tracks. It could also be displayed always at the top of the list, and perhaps with a different color (if possible).

It could even display an icon (pretty much like in the case of autoplay). That way the animation wouldn't even need to have a specific name (albeit by default it's created as "reset" if no reset animation is already present).

@UnwarySage
Copy link

This pretty much describes my present manual process. I am in favor.

@Riteo
Copy link

Riteo commented Oct 3, 2020

It could even display an icon (pretty much like in the case of autoplay). That way the animation wouldn't even need to have a specific name (albeit by default it's created as "reset" if no reset animation is already present).

Oh, are you implying that any animation could be set as the reset? That sounds cool, but if anything like this should ever happen, I think that there should be a way to identify the animation from code, as it can be potentially useful for animation plugins.

@RandomShaper
Copy link
Member

It could even display an icon (pretty much like in the case of autoplay). That way the animation wouldn't even need to have a specific name (albeit by default it's created as "reset" if no reset animation is already present).

Oh, are you implying that any animation could be set as the reset? That sounds cool, but if anything like this should ever happen, I think that there should be a way to identify the animation from code, as it can be potentially useful for animation plugins.

AnimationPlayer.get_reset_animation() -> Animation (or null)

Well, this is just an idea. If anyone thinks that the ability to mark any (only one) animation as reset has downsides, please share your thoughts.

@RandomShaper
Copy link
Member

Brainstorm:

a) If you have multiple animation players in a scene and have the reset-on-save editor setting enabled, should it apply to all of them? I think it should.
b) Add a button (or anim editor menu item) that allows you to create any track as the current values of the current animation. That's both a way to create a reset track or any other animation you want to start exactly at the state of another at any point in time.

@Riteo
Copy link

Riteo commented Oct 3, 2020

a) If you have multiple animation players in a scene and have the reset-on-save editor setting enabled, should it apply to all of them? I think it should.

I think that this "reset-on-save" function should be implemented on its own, resetting to the values before selecting the animationplayer on its deselection.

@Eoin-ONeill-Yokai
Copy link

Couldn't we just have an extra track type that, when playback finishes or changes to an animation without the same track's path, will reset to desired "default value"?

Additionally, I find that working with multiple animations with many different "tracks" can become difficult because you have to manually search for every property you used in a previous animation. So when creating a new track for animation, will it use the reset track as a "base line" even when creating a new animation? In other words, could the Reset animation be treated as a kind of base class animation?

@reduz
Copy link
Member Author

reduz commented Oct 3, 2020

@Eoin-ONeill-Yokai , @Itachi124 The problem is that the most common use case desired here is having a default state for all tracks animated in all animations, not per animation. This is why there is no default value in the animation themselves (it would be redundant and prone to errors).

@DanielKinsman
Copy link

So reset on save should mean the source/diff shouldn't change at all right? That's the most annoying thing about this for me.

@RandomShaper
Copy link
Member

So reset on save should mean the source/diff shouldn't change at all right? That's the most annoying thing about this for me.

Yes, only the stuff that you've actually changed. I often find myself carefully resetting parts of scene files that changed because of having tweaked the value of a single key frame, since that made me seek to a certain point of some animation, which caused a lot of properties of lots of nodes to change.

In short, I feel your pain since I've been there and I can confirm this will get us rid of that.

@reduz
Copy link
Member Author

reduz commented Oct 4, 2020

@Itachi124 IMO that proposal is about something else completely unrelated, not a common use case. This is not about implementing a new feature because its cool, but about making it easier to do something users have been doing for a while now, which is having a way to reset the objects animated either by one or all animations, to their default state. Other features should be discussed in a separate proposal.

@reduz
Copy link
Member Author

reduz commented Oct 4, 2020

@Itachi124 Just to make it clear, there is no intention from my part in discussing any of the features you mention, and this is not the way these things are discussed. I suggest open a separate proposal, make a coherent presentation (fill the template) and try to gather interest to your ideas instead of derailing discussion on this one. If you manage to get enough interest from community, then core contributors will discuss the proposal and give you feedback.

@reduz
Copy link
Member Author

reduz commented Oct 4, 2020

@Itachi124 You have to understand that the way we do development is not really via speculation on what is better than what. Its just impossible to do know this beforehand. The reason we created the proposal system is to be able to see users propose things and see the interest and feedback that other users get. Otherwise its just impossible for us to be able to tell what is useful and what is not. Your opinion on this is as good (and subjective) as mine.

If you open a proposal and it gathers no interest (or it does but there is no consensus), be it from either you or mine, then there won't be any priority in having it implemented.

@reduz
Copy link
Member Author

reduz commented Oct 4, 2020

Before this system, users just opened issues and PRs and we core developers had no idea if those were something user community wanted, or whether it would be useful. We would spend afternoons speculating that and it would lead to nowhere. So, in the end, we tend to act to solve problems mostly when several users run into the same issue, and not when someone has a cool idea.

@Eoin-ONeill-Yokai
Copy link

@reduz The problem is that the most common use case desired here is having a default state for all tracks animated in all animations, not per animation.

That makes a lot of sense.

Though would a kind of animation "inheritance" still be off the table here? It would be nice in the case of this reset animation too, because you could have new animations inherit the reset and only override properties when an animation track is defined (or perhaps allow the user to disable inherited reset.) This would also mean that all animations would have their tracks update to reflect the desired "reset" behavior, even for channels they were previously uninterested in.

@slapin
Copy link

slapin commented Oct 8, 2020

For skeletons it would be more useful to add RESET/default animation instead which will set all the transforms to identity.
Because many animations won't contain initial state at all.

@slapin
Copy link

slapin commented Oct 8, 2020

@Itachi124 the proposal system is just a victim of the loop - to know you need something you have to have it;
if you don't have it you don't need it. The most technology proposals come from people who seen other engines and frameworks, and majority is just starting with simplest projects not knowing what they actually want and having no experience of what actually exists. That will always be the problem of demand-supply loop because it has no clearly defined goals.

@akien-mga
Copy link
Member

Implemented by godotengine/godot#43115.

@fire
Copy link
Member

fire commented Aug 3, 2021

Wanted to notify that my PR for a related feature to bake RESET tracks was merged godotengine/godot#51057.

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

No branches or pull requests