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

Add an option for tweens to ignore Engine.time_scale #100735

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

tvenclovas96
Copy link
Contributor

@tvenclovas96 tvenclovas96 commented Dec 22, 2024

Implements godotengine/godot-proposals#11404

Adds .set_ignore_time_scale(ignore:bool) method to tweens.

Bugsquad edit: closes godotengine/godot-proposals#11404

<return type="Tween" />
<param index="0" name="ignore" type="bool" default="true" />
<description>
If [param ignore] is [code]true[/code], the speed of tweening will not be affected by [member Engine.time_scale]. This affects all [Tweener]s and their delays. Default value is [code]false[/code].
Copy link
Contributor

Choose a reason for hiding this comment

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

Mixture between this PR's description and #100736 's which is taken from Timer and/or SceneTreeTimer, I believe, so it's consistent.

Suggested change
If [param ignore] is [code]true[/code], the speed of tweening will not be affected by [member Engine.time_scale]. This affects all [Tweener]s and their delays. Default value is [code]false[/code].
If [param ignore] is [code]true[/code], the tween will ignore [member Engine.time_scale] and update with the real, elapsed time. This affects all [Tweener]s and their delays. Default value is [code]false[/code].

@tvenclovas96 tvenclovas96 force-pushed the tween_ignore_time_scale branch from 980ff19 to 00b1754 Compare December 22, 2024 14:17
@@ -161,6 +162,8 @@ class Tween : public RefCounted {
TweenProcessMode get_process_mode();
Ref<Tween> set_pause_mode(TweenPauseMode p_mode);
TweenPauseMode get_pause_mode();
Ref<Tween> set_ignore_time_scale(bool p_ignore);
bool is_ignore_time_scale() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

In a similar PR that does the same thing to Timer nodes, we went with get_ignore_time_scale instead of is_ignore_time_scale. See discussion here : #96626 (comment)
It would make sense to do the same here

Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically bikeshedding but I'd not be against is_ignoring_time_scale() myself for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the points in the discussion, but this method is not exposed, much like the one in SceneTreeTimer.

From the other tween ignore time pull #100736 there is talk about general code cleanup in Tween. I think it would be a good idea to change is_ignore_time_scale both here and in SceneTreeTimer for consistency, alongside the other code polish.

I can implement it, but I'm not sure if this PR should encompass all that.

Copy link
Member

@AThousandShips AThousandShips Dec 22, 2024

Choose a reason for hiding this comment

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

I think it should be is_ignoring_time_scale that's our usual formatting

Copy link
Contributor

Choose a reason for hiding this comment

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

If we feel particularly strongly about it you could make a separate PR for Timer's before we no longer can @AThousandShips

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely will see if I can get that today

@tvenclovas96 tvenclovas96 force-pushed the tween_ignore_time_scale branch from 00b1754 to dbc0cc1 Compare December 22, 2024 16:39
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, will do the changes to Timer soon

@Repiteo Repiteo merged commit 0e00f41 into godotengine:master Dec 30, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 30, 2024

Thanks! Congratulations on your first merged contribution! 🎉

@tvenclovas96 tvenclovas96 deleted the tween_ignore_time_scale branch December 30, 2024 17:37
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.

Add an option for tweens to ignore Engine.time_scale
6 participants