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 animation playback preview to scene import settings #76367

Merged

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Apr 23, 2023

This QoL enhancement will add the option to preview animations in the import settings window (to preview animations without performing extra actions such placing the models to a scene):

import_animation

Attached demo project:
TestModel.zip

Bugsquad edit:

@Ansraer
Copy link
Contributor

Ansraer commented Apr 27, 2023

Not sure how hard it would be to implement, but could you add an option to show the bones when rendering an animation?

I am considering to import all my animations from seperate gltf files, so I wouldn't have a mesh for preview purposes during the import.

@jeronimo-schreyer
Copy link
Contributor

Not sure how hard it would be to implement, but could you add an option to show the bones when rendering an animation?

I am considering to import all my animations from seperate gltf files, so I wouldn't have a mesh for preview purposes during the import.

godotengine/godot-proposals#6801

@TokageItLab
Copy link
Member

TokageItLab commented May 25, 2023

I compared #76367 and #76381, both have good and not good points.

#76367 has a seek bar. It is good that you can check the playback position with it. However, the negative is that the loop option is not linked to the import option for the preview. Since users may complain after importing that it is not in the same state as the preview, they should be linked.

image

#76381 does not have a seek bar, but it is preferable that the loop option for preview be linked to the import option. However, I recommend #76367 for animation because #76367 is more valuable for QOL since it has a seek bar.

BTW, the light settings for preview in #76381 are useful. So it is better to omit the animation feature and brush up #76381 for the preview environment option.

image

@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from 13df5fa to 32df433 Compare May 25, 2023 10:56
@Chaosus
Copy link
Member Author

Chaosus commented May 25, 2023

@TokageItLab I've updated this PR, and now when you click on animation in the list - it will auto-switch the loop button to enabled state (when loop mode is enabled in the animation) and will use play_backward if ping-pong is enabled at the animation end.

I don't think I need to add something more complex like default lightning or bone rendering (which is done by #76818)

@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from 32df433 to 5e8181e Compare May 25, 2023 11:25
@TokageItLab
Copy link
Member

TokageItLab commented May 25, 2023

@Chaosus I understand that those are not necessary for this PR. But I think they can be implemented separately from this PR.

Below are the current problems with this PR:

The loop button in the GUI seems to do nothing, so please get rid of it. Or make sure it is correctly linked to the import option.

image

Also, for ping-pong, it should play to the end, then reverse, and then play forward when it gets to the beginning again. Please refer to the behavior of AnimationPlayer or #76381 for more example on this.

1.mp4

I am also concerned about the presence of gaps at the beginning and end of the seek bar. I don't know if it is a drawing issue or if it is not actually processing the animation, but can it be remedied?

@jeronimo-schreyer
Copy link
Contributor

I compared #76367 and #76381, both have good and not good points.

#76367 has a seek bar. It is good that you can check the playback position with it. However, the negative is that the loop option is not linked to the import option for the preview. Since users may complain after importing that it is not in the same state as the preview, they should be linked.

image

#76381 does not have a seek bar, but it is preferable that the loop option for preview be linked to the import option. However, I recommend #76367 for animation because #76367 is more valuable for QOL since it has a seek bar.

BTW, the light settings for preview in #76381 are useful. So it is better to omit the animation feature and brush up #76381 for the preview environment option.

image

sorry, the light settings video sample got mixed up with #76140 but they're a different PR

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 12, 2023
@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from 5e8181e to 2e36c12 Compare June 12, 2023 14:34
@Chaosus
Copy link
Member Author

Chaosus commented Jun 12, 2023

The loop button in the GUI seems to do nothing, so please get rid of it. Or make sure it is correctly linked to the import option.

OK, I deleted it, it was working - but only after you press the play button manually.

Also, for ping-pong, it should play to the end, then reverse, and then play forward when it gets to the beginning again.

It's already being working, as you said. Isn't it?

I am also concerned about the presence of gaps at the beginning and end of the seek bar.

Not sure if I understand how to fix this...

@TokageItLab
Copy link
Member

Also, for ping-pong, it should play to the end, then reverse, and then play forward when it gets to the beginning again.

It's already being working, as you said. Isn't it?

No. It is always looped in the backward.

@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from 2e36c12 to b0e36b8 Compare June 12, 2023 16:14
@Chaosus
Copy link
Member Author

Chaosus commented Jun 12, 2023

No. It is always looped in the backward.

OK, Fixed, now. Check again.

@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from b0e36b8 to 9bb5e42 Compare June 12, 2023 16:16
@TokageItLab
Copy link
Member

I have confirmed that it has been fixed.

The remaining problem is that when you change the loop mode, you will need to update the ImportScene or animation for the preview. The current behavior does not update the loop settings until you open the PostImporter again after changing the loop mode and reimport.

@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from 9bb5e42 to 1d37e94 Compare June 13, 2023 05:36
@Chaosus
Copy link
Member Author

Chaosus commented Jun 13, 2023

@TokageItLab I think I fixed this, check again.

@Chaosus Chaosus force-pushed the import_settings_animation_playback branch from 1d37e94 to 7cd943f Compare June 13, 2023 05:42
Comment on lines +960 to +979
switch (loop_mode) {
case Animation::LOOP_NONE: {
animation_play_button->set_icon(get_theme_icon(SNAME("MainPlay"), SNAME("EditorIcons")));
animation_slider->set_value_no_signal(1.0);
set_process(false);
} break;
case Animation::LOOP_LINEAR: {
animation_player->play(p_name);
} break;
case Animation::LOOP_PINGPONG: {
if (animation_pingpong) {
animation_player->play(p_name);
} else {
animation_player->play_backwards(p_name);
}
animation_pingpong = !animation_pingpong;
} break;
default: {
} break;
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit redundant to have to write the processing for looping outside of the AnimationPlayer, but I think this is the only way to do it for now. I will consider providing a better API in a future refactoring planned as godotengine/godot-proposals#5972.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I like the design. Did not technical review.

@YuriSizov YuriSizov merged commit 9547de5 into godotengine:master Jul 12, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@Chaosus Chaosus deleted the import_settings_animation_playback branch July 14, 2023 18:32
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.

Added playback control to animations in the Advanced Import Settings preview
7 participants