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

added Curve in state machine interpolation #63369

Conversation

jeronimo-schreyer
Copy link
Contributor

@jeronimo-schreyer jeronimo-schreyer commented Jul 23, 2022

added Curve in state machine interpolation for better control over blending
theres a proposal for this godotengine/godot-proposals#1402

here you can see the interpolation with and without curve setting

blend_tree_curve.mp4

@jeronimo-schreyer jeronimo-schreyer requested a review from a team as a code owner July 23, 2022 22:25
@Calinou Calinou added this to the 4.0 milestone Jul 23, 2022
@Calinou
Copy link
Member

Calinou commented Jul 23, 2022

@jeronimo-schreyer Remember that for GitHub video previews to work, you need to have a blank line before and after the video URL. I edited your post accordingly, but remember to do this in the future 🙂

@jeronimo-schreyer
Copy link
Contributor Author

I didn't know why I could see it OK in the preview and not in the final posted version. Thanks.

This build error says I need to add documentation for the new Curve property, right?

@Calinou
Copy link
Member

Calinou commented Jul 24, 2022

This build error says I need to add documentation for the new Curve property, right?

Yes, and make sure you're using tab indentation within the XML instead of spaces (except within [codeblock]).

@jeronimo-schreyer
Copy link
Contributor Author

I'm almost certain I use tabs instead of spaces. Is there a way to be sure?

@jeronimo-schreyer jeronimo-schreyer force-pushed the feature-curve-in-transition branch 6 times, most recently from 39ec0be to 049d101 Compare July 24, 2022 04:35
@fire fire requested a review from TokageItLab July 27, 2022 07:09
@TokageItLab
Copy link
Member

TokageItLab commented Jul 27, 2022

IMO it is good, it could be implemented also for NodeTransition, but maybe after this is Approved, we can send it as an additional PR.

@jeronimo-schreyer
Copy link
Contributor Author

jeronimo-schreyer commented Jul 27, 2022

I added this for NodeTransition also on my latest commit. I even changed a property name cross_fade_time for xfade_time to keep consistency in names between them

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

You must follow the rebasing work-flow recommended by Godot. See Pull request workflow.

In brief, check out 74d84d170aab4e692a81165872f040296a2ca7f0 and apply the fixes, then do a --fixup. In the end, there should be only one commit.

Comment on lines +751 to +765
float blend = xfade_time == 0 ? 0 : (prev_xfading / xfade_time);
float curved_blend = (xfade_curve.is_valid()) ? xfade_curve->interpolate(1.0 - blend) : 1.0 - blend;

if (from_start && !p_seek && switched) { //just switched, seek to start of current

rem = blend_input(current, 0, true, p_seek_root, 1.0 - blend, FILTER_IGNORE, true);
rem = blend_input(current, 0, true, p_seek_root, curved_blend, FILTER_IGNORE, true);
} else {
rem = blend_input(current, p_time, p_seek, p_seek_root, 1.0 - blend, FILTER_IGNORE, true);
rem = blend_input(current, p_time, p_seek, p_seek_root, curved_blend, FILTER_IGNORE, true);
}

curved_blend = (xfade_curve.is_valid()) ? xfade_curve->interpolate(blend) : blend;
if (p_seek) {
blend_input(prev, p_time, true, p_seek_root, blend, FILTER_IGNORE, true);
blend_input(prev, p_time, true, p_seek_root, curved_blend, FILTER_IGNORE, true);
time = p_time;
} else {
blend_input(prev, p_time, false, p_seek_root, blend, FILTER_IGNORE, true);
blend_input(prev, p_time, false, p_seek_root, curved_blend, FILTER_IGNORE, true);
Copy link
Member

@TokageItLab TokageItLab Jul 28, 2022

Choose a reason for hiding this comment

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

Interpolation calculation is wrong; you only need to overwrite blend with curve.

Suggestion:

	} else { // cross-fading from prev to current

		float blend = xfade_time == 0 ? 0 : (prev_xfading / xfade_time);
		if (xfade_curve.is_valid()) {
			blend = xfade_curve->interpolate(blend);
		}

		if (from_start && !p_seek && switched) { //just switched, seek to start of current

			rem = blend_input(current, 0, true, p_seek_root, 1.0 - blend, FILTER_IGNORE, true);
		} else {
			rem = blend_input(current, p_time, p_seek, p_seek_root, 1.0 - blend, FILTER_IGNORE, true);
		}

		if (p_seek) {
			blend_input(prev, p_time, true, p_seek_root, blend, FILTER_IGNORE, true);
			time = p_time;
		} else {
			blend_input(prev, p_time, false, p_seek_root, blend, FILTER_IGNORE, true);
			time += p_time;
			prev_xfading -= p_time;
			if (prev_xfading < 0) {
				set_parameter(this->prev, -1);
			}
		}
	}

Comment on lines +435 to +440
float curved_blend = (current_curve.is_valid()) ? current_curve->interpolate(fade_blend) : fade_blend;
float rem = p_state_machine->blend_node(current, p_state_machine->states[current].node, p_time, p_seek, p_seek_root, curved_blend, AnimationNode::FILTER_IGNORE, true);

if (fading_from != StringName()) {
p_state_machine->blend_node(fading_from, p_state_machine->states[fading_from].node, p_time, p_seek, p_seek_root, 1.0 - fade_blend, AnimationNode::FILTER_IGNORE, true);
curved_blend = (current_curve.is_valid()) ? current_curve->interpolate(1.0 - fade_blend) : 1.0 - fade_blend;
p_state_machine->blend_node(fading_from, p_state_machine->states[fading_from].node, p_time, p_seek, p_seek_root, curved_blend, AnimationNode::FILTER_IGNORE, true);
Copy link
Member

@TokageItLab TokageItLab Jul 28, 2022

Choose a reason for hiding this comment

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

Same above.

Suggestion:

	if (current_curve.is_valid()) {
		fade_blend = current_curve->interpolate(fade_blend);
	}
	float rem = p_state_machine->blend_node(current, p_state_machine->states[current].node, p_time, p_seek, p_seek_root, fade_blend, AnimationNode::FILTER_IGNORE, true);

	if (fading_from != StringName()) {
		p_state_machine->blend_node(fading_from, p_state_machine->states[fading_from].node, p_time, p_seek, p_seek_root, 1.0 - fade_blend, AnimationNode::FILTER_IGNORE, true);
	}

@TokageItLab
Copy link
Member

TokageItLab commented Jul 31, 2022

@jeronimo-schreyer This feature is good. Can I salvage it?
We have to do a feature freeze to release the Godot 4 beta (https://godotengine.org/article/godot-4-0-development-enters-feature-freeze), so implementation of the new feature is a bit rushed. If I don't hear back by 8/2, I will send a proxy PR with your name as co-authored.

@akien-mga
Copy link
Member

Superseded by #63802.

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.

4 participants