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

Animation Player no longer calls a method twice when playing the anim… #86645

Closed

Conversation

ysylya
Copy link

@ysylya ysylya commented Dec 30, 2023

Reasons for the bug
There are 3 steps to understand the bug:

  1. In Animation::track_find_key, in "case TYPE_METHOD", the _find method is being called, however the backwards flag is not being passed to it.
    We need to call this function with this flag, so we need to know whether we are playing backwards or not.
  2. Currently, there is no way to pass that info to Animation::track_find_key.
  3. Even if we could pass that info, AnimationMixer::_blend_process, the "bool backward" variable does not have the correct value at the very beginning. (And Animation::track_find_key is called from that method when the bug happens.)

Solution

  1. and 2) Animation::track_find_key now has an other parameter called p_backwards (called like this in _find), which defaults to false. In AnimationMixer::_blend_process, in "case Animation::TYPE_METHOD" the Animation::track_find_key is now called with this parameter.
  2. The backward variable is incorrect because ai.playback_info.delta is 0.0, and not -0.0. To fix this, in AnimationPlayer::_process_playback_data, there are added checks now to make sure if it's correct. Also, in that function when p_started is true, a constant 0.0 was passed as delta, regardless of playing direction.

Possible extensions
In Animation::track_find_key, there are other cases where _find is called without the backwards flag. Might be interesting to check whether they need it as well. This pull request is intended to solve the method call problem only.

@ysylya ysylya requested a review from a team as a code owner December 30, 2023 17:37
@AThousandShips
Copy link
Member

Please open a bug report for this, or link one that exists, to track the issue separately from the PR

@ysylya
Copy link
Author

ysylya commented Dec 30, 2023

Done! #86648

pi.seeked = true;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Unnecessary change, restore

@@ -401,7 +401,7 @@ class Animation : public Resource {
void track_set_key_transition(int p_track, int p_key_idx, real_t p_transition);
void track_set_key_value(int p_track, int p_key_idx, const Variant &p_value);
void track_set_key_time(int p_track, int p_key_idx, double p_time);
int track_find_key(int p_track, double p_time, FindMode p_find_mode = FIND_MODE_NEAREST) const;
int track_find_key(int p_track, double p_time, FindMode p_find_mode = FIND_MODE_NEAREST, bool p_backward = false) const;
Copy link
Member

@AThousandShips AThousandShips Dec 30, 2023

Choose a reason for hiding this comment

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

You also need to update the bind for this, on line 3856 in the cpp file, you also need to register a compatibility method for this, will provide an example for how to do this, this makes me wonder if this is the best solution however

For the C# side you need to add a method in this file: modules/mono/glue/GodotSharp/GodotSharp /Compat.cs

For c++ it's a bit trickier, you need to first add, under static void _bind_methods the following:

#ifndef DISABLE_DEPRECATED
	int _track_find_key_bind_compat_86645(int p_track, double p_time, FindMode p_find_mode = FIND_MODE_NEAREST);
	static void _bind_compatibility_methods();
#endif

Then you need to create a new file called animation.compat.inc and put this in it:

/**************************************************************************/
/*  animation.compat.inc                                                  */
/**************************************************************************/
/*                         This file is part of:                          */
/*                             GODOT ENGINE                               */
/*                        https://godotengine.org                         */
/**************************************************************************/
/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur.                  */
/*                                                                        */
/* Permission is hereby granted, free of charge, to any person obtaining  */
/* a copy of this software and associated documentation files (the        */
/* "Software"), to deal in the Software without restriction, including    */
/* without limitation the rights to use, copy, modify, merge, publish,    */
/* distribute, sublicense, and/or sell copies of the Software, and to     */
/* permit persons to whom the Software is furnished to do so, subject to  */
/* the following conditions:                                              */
/*                                                                        */
/* The above copyright notice and this permission notice shall be         */
/* included in all copies or substantial portions of the Software.        */
/*                                                                        */
/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,        */
/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF     */
/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY   */
/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,   */
/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE      */
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.                 */
/**************************************************************************/

#ifndef DISABLE_DEPRECATED

int Animation::_track_find_key_bind_compat_86645(int p_track, double p_time, FindMode p_find_mode) {
	return track_find_key(p_track, p_time, p_find_mode, false);
}

void Animation::_bind_compatibility_methods() {
	ClassDB::bind_compatibility_method(D_METHOD("track_find_key", "track_idx", "time", "find_mode"), &Animation::_track_find_key_bind_compat_86645, DEFVAL(FIND_MODE_NEAREST));
}

#endif

And include this file in animation.cpp just under #include "animation.h"

@@ -1460,7 +1460,7 @@ void AnimationMixer::_blend_process(double p_delta, bool p_update_only) {
}
TrackCacheMethod *t = static_cast<TrackCacheMethod *>(track);
if (seeked) {
int idx = a->track_find_key(i, time, is_external_seeking ? Animation::FIND_MODE_NEAREST : Animation::FIND_MODE_EXACT);
int idx = a->track_find_key(i, time, is_external_seeking ? Animation::FIND_MODE_NEAREST : Animation::FIND_MODE_EXACT, backward);
Copy link
Member

Choose a reason for hiding this comment

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

This should be done for all the calls to this, not just this one

@@ -1578,7 +1578,7 @@ int Animation::track_find_key(int p_track, double p_time, FindMode p_find_mode)
} break;
case TYPE_METHOD: {
MethodTrack *mt = static_cast<MethodTrack *>(t);
int k = _find(mt->methods, p_time);
int k = _find(mt->methods, p_time, p_backward);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@AThousandShips
Copy link
Member

Thank you for your contribution and welcome to contributing! I've left some details on this as these changes add some things that needs fixing

Someone on the appropriate team will come and look at this when they have the time, I suspect it might take a few days because of the holiday season

@TokageItLab
Copy link
Member

TokageItLab commented Dec 31, 2023

Thanks for your contribution, but this should be mostly fixed by the existing PR as #86227. If you test #86227 and have problems with playback in the backward, you may be possible to send this PR again as different PR which is a follow up after make some modification of removing unneeded changes.

@TokageItLab TokageItLab removed this from the 4.x milestone Dec 31, 2023
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.

AnimationPlayer calls a method twice from a Call Method Track when playing the animation backwards
3 participants