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

Split pause() from AnimationPlayer's stop() #71218

Merged
merged 2 commits into from
Jan 12, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 11, 2023

Counter-PR to #71208
This is a minimal change, unlike #56645

stop() will function exactly the same as it did.
stop(false) is now pause().
Everything else stays the same.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 11, 2023

I believe _stop_internal() should be implemented on the tween side as well, since pause is just a function that omits some lines in stop, this will reduce the number of duplicates.

void Tween::_stop_internal(bool p_reset) {
	running = false;
	dead = false;
	if (p_reset) {
		started = false;
		total_time = 0;
	}
}

void Tween::stop() {
	_stop_internal(true);
}

void Tween::pause() {
	_stop_internal(false);
}

BTW, I think there is no reason not to set dead to false in pause.

@KoBeWi KoBeWi force-pushed the unlimited_bikeshedding_lets_go branch from 363d151 to d69f091 Compare January 11, 2023 12:46
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 11, 2023

Tween's _stop_internal() would be much shorter than in AnimationPlayer, but why not I guess. Although it should be done in a separate PR.

@YuriSizov
Copy link
Contributor

Docs could benefit from "See also [method the_other_method]."

@TokageItLab
Copy link
Member

TokageItLab commented Jan 11, 2023

Tween::pause() is certainly short, but I am concerned about inconsistency. If this PR changes the AnimationPlayer for making consistent with Tween, I would recommend that the Tween match it as well.

@KoBeWi KoBeWi force-pushed the unlimited_bikeshedding_lets_go branch from d69f091 to da93968 Compare January 11, 2023 13:03
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 11, 2023

Ok I added it as a second commit. I also fixed inconsistent naming of a private method.

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.

I agree with this more than #71208. Without deciding whether or not to implement pause() we cannot go forward. The time has come.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 11, 2023

This is a minimal change, unlike #56645

Does this address the issues linked in there in any way? And how does this PR relate to your own #33733 for that matter?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 11, 2023

Does this address the issues linked in there in any way?

Not really. The issues were mostly about play() vs start()/resume(), but that change has some unwanted side-effects.

I also didn't want to change the behavior of stop(), so #33733 is still relevant (and also stirred some discussion).

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM. I guess adding this to the project converter may be hard due to a very ambiguous name?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 11, 2023

The conversion would have to handlestop(true) and stop(false), so maybe it's not that ambiguous.

EDIT:
Ok converter does not allow that. It would need to be modified.

@akien-mga akien-mga merged commit a8a9892 into godotengine:master Jan 12, 2023
@KoBeWi KoBeWi deleted the unlimited_bikeshedding_lets_go branch January 12, 2023 14:18
@akien-mga
Copy link
Member

Thanks!

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