Fix Animation Timeline Go to Next Step / Prev Step which triggered wrong animation display

[VansonLeung]

[FIX] Fix by unifying the behaviours of mouse / command key triggering player->seek: Animation Timeline Go to Next Step / Prev Step is triggering the wrong player->seek

This fix is to tackle with the issue:

Expected result: animation goes smoothly

issue_2_expected_result.mp4

Actual result: animation jumps crazily

issue_3_crazy_movement.mp4

issue_4_backforth.mp4

For details, please refer to this issue item:

• fixes: Animation Timeline Go to Next Step / Prev Step is triggering the wrong player->seek #83394

I am not sure if my fix will break any other thing though (e.g. the most recent onion-skinning capability revival, AnimationMixer). I need you to help me verify.

This fix has the following related merges as the most recent ones:
#80813
#80939

Bugsquad Edited:
Fixes #85129

[AThousandShips]

Please make the title shorter, describe what it fixes not how it is being fixed :)

[AThousandShips]

Why does this pull request exists

Are you asking why you yourself opened a pull request?

[VansonLeung]

I polished the descriptions, thanks

[AThousandShips]

I still can't make sense of what your title means

[VansonLeung]

I used the latest master build. When I used "Go to Next Step" or "Go to Previous Step" in the Animation Timeline Panel, the result is that the player seems to seek the wrong timeframe - i.e. wrong animation is displayed

[TokageItLab]

Perhaps the fundamental cause of this problem is related to #83197.

However, looking at how the processing is done afterwards by this PR, it may be a fine fix, but it makes the p_drag unnecessary so you should remove p_drag from the argument. After that we will have to test again.

Particular attention should be paid when testing to make sure that the audio track does not go haywire and that the animation playback track is properly sequenced.

[VansonLeung]

Perhaps the fundamental cause of this problem is related to #83197.

However, looking at how the processing is done afterwards by this PR, it may be a fine fix, but it makes the p_drag unnecessary so you should remove p_drag from the argument. After that we will have to test again.

Particular attention should be paid when testing to make sure that the audio track does not go haywire and that the animation playback track is properly sequenced.

@TokageItLab

Upon your reminder I cross-checked the effect on audio animation track. I found another new issue when starting onion skinning while having put any audio into my animation track. The Godot commit I am using: [51f81e1]

This newly found issue is independent of my fixes - I will open a new issue for this by today, I am yet to investigate it or look further into the corresponding onion skinning codes.

cd.mp4

Edit: new issue #83426

[VansonLeung]

@TokageItLab regardless of my newly found issue, here is the polished fix by omitting all "p_drag"

015b2af

[AThousandShips]

I'm not sure the signal should be changed, it breaks compatibility and risks causing problems

[VansonLeung]

I'm not sure the signal should be changed, it breaks compatibility and risks causing problems

I'm not sure the signal should be changed, it breaks compatibility and risks causing problems

I may revert the last commit to keep any backward compatibility albeit needing to keep "p_drag" intact.

For those signal changes, I will scan for any potential risks / problems and list here FYI @AThousandShips

[VansonLeung]

The relevant codes can refer to as far as 2022 January.
For compatibility safety it is wise for me to remove my last commit attempting to omit "p_drag" from the function and signals.

I now even think it would be wiser for me to wait for AnimationMixer and Onion Skinning to become stable - before I test and merge / revamp this particular fix. I think my current fix is not good enough it may rather create confusions.

@AThousandShips @TokageItLab

[VansonLeung]

I'm not sure the signal should be changed, it breaks compatibility and risks causing problems

@AThousandShips I have removed the breaking codes for now. I will look more deeply and conclude a better code solution to fix it.

[AThousandShips]

No need to keep tagging me please :) it adds extra notifications and I'll still see the changes anyway

[VansonLeung]

OK, I just committed another approach that will also fix this issue. This approach should be less confusing. I tested with my mini-project fixed success, but I still need your verifications to see if it may break anything else.

[VansonLeung]

This particular line of code caused the animation display to jump elsewhere of the actual pos, so I removed it.

[TokageItLab]

This change is equivalent to the following:

	if (!p_timeline_only) {
		if (player->is_valid() && !p_set) {
			double delta = player->get_current_animation_position();
			player->seek(pos, true, true);
			player->seek(pos + delta, true, true);
		} else {
			if (player->is_playing()) {
				player->stop();
			}
			player->seek(pos, true, true);
		}
	}

↓

	if (!p_timeline_only) {
		player->seek(pos, true, true);
	}

I can't recall anymore why it was necessary in the past to handle drags and seek by key separately, but I think this change probably fine (you just need to rewrite the code as suggested, also p_set argument should be removed if there is never problem). @SaracenOne Do you have any idea?

[VansonLeung]

This particular line of code is actually not returning a delta but rather the actual current position. It will make player->seek(pos + delta, true, true); totally wrong. So I removed it as well.

[YuriSizov]

Superseded by the linked PR. Thanks for your contribution nevertheless! Hope you can find something else for your first contribution 🙂