Fix NodeOneShot doesn't respect fade-out when aborting and improvement

[TokageItLab]

Fixes: #53947
Fixes: #73113

Apply same fix to NodeOneShot with #73120.

Also, add a request ONE_SHOT_REQUEST_FADE_OUT to NodeOneshot to Abort with respect to the fade-out time.

This changes the transition timing of the active state. This means that the transition to the next state will occur at the start of the fade in the same way as NodeTransition and NodeState.

However, for the Active state, it should be true if OneShot is still in effect. Also for compatibility.

Therefore, the display (retrieve-able) active state and the internal active state are separated as: active and internal_active.

In addition, the xfade curve can be set as in NodeTransition and StateMachine.

Godot.2023.03.02.-.06.40.14.03.mp4

[lyuma]

In terms of the implementation and fade out curve, I see nothing wrong here.

I'm assuming the main bugfix here is due to calling blend_input(1, ... even while fading, rather than short circuiting with return blend_input(0...) which prevented 1 from being evaluated in the old version.

However, my personal preference is to avoid renames if possible. I cannot evaluate the necessity of renaming against the compatibility breakage in 4.1, so I will leave this to Godot devs who have more experience with backwards compatibility decisions.

[lyuma]

Should clear_remaining_fade also set is_shooting = false?

[TokageItLab]

There can be a condition where the shoot is being performed while clearing the old fade in the first frame of the shoot, so we will not do that here.

[TokageItLab]

If we do not change the NodeOneShot API name now, we will have to set some bad name APIs like ONE_SHOT_REQUEST_FADEOUT instead of ONE_SHOT_REQUEST_FADE_OUT, set_fadeout_curve() instead of set_fade_out_curve() for consistency, and we will accumulate bad name APIs in the future, so I think we should change it now the timing of the changing behavior.

I also believe that these are generally configured from the GUI and it is not frequent to configure them directly in scripts. So I guess it would be quite unlikely to break the project.

[mhilbrunner]

Note that these renames not only break GDScript scripts, but also compatability with GDExtension and C#. Ideally, these renames should have happened before 4.0 final release; now we really need to consider how critical these are, and if in doubt strive to keep compatibility.

[TokageItLab]

Certainly it should not be changed in a patch release like 4.0.x. However, I think it should be changed as needed in minor releases like 4.x, rather than continuing to accumulate badly named APIs until 5.0.

Also, it may not be good to just rename PR, but this PR breaks compatibility since it changes not only the method name but also the behavior of the original functionality in the first place. The behavior of the project will change before and after this PR, and may break projects that depend on the bug (the previous behavior where fades are not reset)¹ or use hacks that workaround the bug.

In such cases, I believe it is acceptable to rename to let them know that changing the behavior (though the renaming should only be an extra bonus).

Footnotes

1. Well, since that is a consistent behavior, we can call it "a previous bad intended behavior" instead of a bug ↩

[fire]

Is it possible to use a complete new set of names for the changed functionality? That would be compatible. We can then deprecate the old functionality.

[TokageItLab]

I had already supported that migration as deprecated for properties' setter/getter, but I followed @fire's suggestion and added deprecated methods with the old method names bound to them.

void AnimationNodeOneShot::_bind_methods() {
#ifndef DISABLE_DEPRECATED
	ClassDB::bind_method(D_METHOD("set_fadein_time", "time"), &AnimationNodeOneShot::set_fade_in_time);
	ClassDB::bind_method(D_METHOD("get_fadein_time"), &AnimationNodeOneShot::get_fade_in_time);
	ClassDB::bind_method(D_METHOD("set_fadeout_time", "time"), &AnimationNodeOneShot::set_fade_out_time);
	ClassDB::bind_method(D_METHOD("get_fadeout_time"), &AnimationNodeOneShot::get_fade_out_time);
...

This should temporarily ensure script compatibility, but I also thought it might be better to add an is_deprecated flag to the binded methods. Node has it and is marked, but, the method cannot tell at a first look that it has been deprecated. Then, for example, we could implement a binding macro for a deprecated method like:

ClassDB::bind_method_deprecated(D_METHOD("set_fadein_time", "time"), &AnimationNodeOneShot::set_fade_in_time);

[TokageItLab]

@mhilbrunner The CSharp test has an error, so I looked into it, and it seems that your point is partially incorrect.

It seems that the method get_fade_in_time (same for get_fade_out_time) has already been converted to the method GetFadeInTime by naming_utils.cpp. However, since the other part naming in CPP are consistent with fade_in, so this "fadein -> FadeIn" conversion seems to be currently only used for NodeOneShot and it looks entirely like a hack.

In summary:

• I added set_fade_in_time in this PR, which is converted correctly to SetFadeInTime without naming_utils.cpp in CSharp.

• Also, I remain the deprecated method set_fadein_time for compatibility. However, naming_utils.cpp will convert it to SetFadeInTime, which will result in duplicated declarations and errors, so we need to remove it (and again, this is only used for NodeOneShot, so removing it here should not be a problem). One problem that occurs as a side effect of that is the addition of a deprecated method SetFadeinTime newly to CSharp.

[raulsntos]

Since this is just a rename and doesn't break compat, I think it's fine for the C# implementation to call the new get_fade_in_time.

As for the new deprecated SetFadeinTime method, it'd be nice to avoid generating it. We can probably skip it like we do with some classes but this can probably be done in a separate PR as long as it gets merged before the new API ships. But I don't think it would be a big deal even if we end up shipping the new deprecated API.

[akien-mga]

I think this PR is going overkill on cosmetic compat breakage of the API where it's not needed. I understand that you might feel that fade_in is better than fadein, but it's not at all critical and as we made it clear multiple times, such compat breakage in 4.x releases is not expected.

I'm not fully closing the door to figuring out a proper way to let the API evolve in 4.x minor releases where it's very needed (fixing outright wrong names or bogus APIs), but this is not the case here. And that way would still need to be defined, agreed upon, and documented, and that's fully outside the scope of this PR.

So I'd appreciate if this PR was amended not to break compatibility, as I don't see why it's justified here.

[TokageItLab]

I would argue that the method conversion hack for CSharp there indicates that the original method name is incorrect and that this should be justified to remove such hacks.

So I'd appreciate if this PR was amended not to break compatibility, as I don't see why it's justified here.

This is already done by binding the old methods so old scripts will not be broken, but has the side effect of adding deprecated methods to CSharp as raulsntos concerned #74190 (comment). This is caused by the hack being implemented only for CSharp, even though it should have corrected the core method name.

[akien-mga]

Whatever hack was added in the C# bindings to make the API nicer for C# users is not a justification for breaking compat.

We released 4.0 with this API, it's not wrong, there's no reason to break compat or introduce a ton of duplicated deprecated methods because you don't like the look of "fadeout". Nobody asked for this change.

This PR claims to be fixing a logic bug - that's all it should do. But we can't merge that fix because it's being mixed with opinionated compat breakage when we made clear that this was not an option after the 4.0 release.

[TokageItLab]

For now, I will be separating only the PR for the renaming.

[TokageItLab]

@akien-mga Modified the old bound method names to leave them unchanged.

[akien-mga]

Thanks!