godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.48k stars 21.08k forks source link

AnimatedSprite's `autoplay` and `animation` are both stored in scene #77223

Open KoBeWi opened 1 year ago

KoBeWi commented 1 year ago

Godot version

4.1 dev2

System information

Windows 10 x64

Issue description

When you set autoplay of AnimatedSprite, the animation property can still be stored in the scene. In worst case, they point to the same animation. This is redundant. If an autoplay is defined, the animation property has no reason to be serialized, because it's overwritten right at the start.

Another issue is that if you don't have default animation in your SpriteFrames, trying to revert the animation with the revert arrow image results in error Animation 'default' doesn't exist.. The default is set, but gets overwritten by the first animation in the list once you restart the scene.

https://github.com/godotengine/godot/assets/2223172/38829798-6f8f-4e75-97d9-c3708aae507c

Solution: don't store animation in scene if autoplay exists. Not sure what to do about the error. I think property_get_revert() can be used to point to an existing animation.

Steps to reproduce

  1. Add AnimatedSprite2D
  2. Add SpriteFrames
  3. Add animation and rename default to something else
  4. Set autoplay to one of the animations
  5. Save and reload the scene
  6. Save the scene again
  7. Check the tscn file
  8. Both autoplay and animation are stored

Minimal reproduction project

animated_sprite_2d.tscn.txt

TokageItLab commented 1 year ago

The failure to revert is probably a bug, but the autoplay property needs to be saved separately from the animation property.

autoplay property is used to specify the animation that will automatically play when a saved scene is instantiated and called from another scene. It must be defined for each Node, so it must be serialized and stored in the AnimatedSprite side, not in the SpriteFrames which is a Resource. Also, AnimationPlayer should have the same implementation.

Well, it may indeed be redundant to save an animation that would be overwritten immediately after calling a saved scene if there is an autoplay, but I think it is not a bug that it is not empty.

KoBeWi commented 1 year ago

Both autoplay and animation are properties of AnimatedSprite. animation is not needed if autoplay exists.

I think it is not a bug that it is not empty.

Probably not strictly a bug, but we have means to skip it conditionally, so it can be implemented.

TokageItLab commented 1 year ago

If we always force animation to empty when autoplay is present, we will not be able to specify an alternate animation when autoplay is set to empty before playing autoplay animation.

I believe there is nothing wrong with current autoplay behavior for serializing. So I think the problem is simply that you cannot set an empty animation to animation arbitrarily when there is more than one animation.

KoBeWi commented 1 year ago

we will not be able to specify an alternate animation when autoplay is set to empty

I don't really understand. I'm not saying it should be forced to empty, it should simply be not saved to scene when autoplay is non-empty.

EDIT: btw another thing - why is autoplay a String not StringName?

TokageItLab commented 1 year ago

Doesn't not storing the animation in the scene mean that if it is called from another scene, the animation will be empty until the autoplay is set to animation?

If autoplay is empty, AnimatedSprite should display the stopped frame without playing back what is set in animation. So the animation property set separately from autoplay has a meaning as to what should be displayed if autoplay becomes empty. Well, it is probably a corner case to load a scene with autoplay set and set empty to autoplay before adding it to the scene.

btw another thing - why is autoplay a String not StringName?

I remember following the AnimationPlayer implementation.

dalexeev commented 1 year ago

animation is the animation that is selected in the editor (and that is displayed if autoplay is not set). autoplay is an animation that starts automatically. And they can be different. Otherwise, the selected animation will be reset if autoplay is set each time the scene is reopened.

Mickeon commented 1 year ago

I have been in this same situation for a while and I am glad there's a discussion for it.

No. Normally, animation should simply not be stored inside a PackedScene. It's prone to changes when testing, and redundant by defeating the whole purpose of having an autoplay animation. Every minute test may or may not tell git that something has changed, whereas it practically didn't.

If anything, it should be only stored if an autoplay animation is not defined.

And something similar could be said for frame and frame_progress when testing in the editor (if it hadn't already been addressed). Currently though they are stored, sometimes. It's inconsistent.