Implement set_time_left in Timer node

[jeronimo-schreyer]

If new time_left is bigger than wait_time, sets wait_time value

Updates
scene\main\timer.cpp
scene\main\timer.h

• Bugsquad edit: This closes Add time_left property setter in Timer node godot-proposals#12655

[lyuma]

It does seem like an oversight, since this functionality is already supported in the newer SceneTreeTimer class.

I would suggest to keep the property setter simple, just like SceneTreeTimer. Always set time_left to what is passed in. There can be reasons to override it. Why disallow this?

Until this is merged, users can use SceneTreeTimer which you can make with get_tree().create_timer(), which does support the property setter

[lodetrick]

How different would this be to just calling $Timer.start(time_left)? It seems like they are filling very similar roles. Also, why does it only change the value if the new value is larger instead of just different than the current time_left?

[fire]

My preference is to approve this pull request because in general properties (time_left) should be read and write. I can approve it officially or wait till all github actions tests pass.

[KoBeWi]

Documentation needs update.

[jeronimo-schreyer]

Also, why does it only change the value if the new value is larger instead of just different than the current time_left?

This change aims to keep the node state as clean and consistent as possible. Setting a time_left larger than wait_time will make the current cycle longer than the rest. I think this also works consistently with a timer functionality, where, if you reduce the wait_time, eventually, you'll expect to hit 0 faster than another regular timer, and trigger the timeout signal. And if you make time_left grow eventually you'll hit wait_time, limiting it.
In the case you want to have one cycle bigger than the rest, then you would need to make it explicit, changing wait_time and then back. I think this corner case makes little sense and somewhat confusing

[jeronimo-schreyer]

I was wondering this is going to need to change if we can change time_left on a stopped timer as well

bool Timer::is_stopped() const {
	return get_time_left() <= 0;
}

Should we use ?

bool Timer::is_stopped() const {
	return !processing;
}

[lyuma]

I mostly want to see it perform the same as SceneTreeTimer, but the implementation of is_stopped() is a bit unexpectedly different. I wouldn't mind seeing it changed to return processing if it has mostly the same effect, since it would be easier to reason about.

For example, think about a case where I set a repeating timer to repeat every 0.1 seconds, but I am at less than 10 FPS. In this case, the timer will become negative and stay negative, and is_stopped() will be false, and get_time_left() will lie and say 0 even though the value is negative, yet a timeout event will be fired every frame. It's unclear what should happen but the current feels a bit wrong to me.

Anyway I'm not saying we should fix everything about the timer class at once, just that we should try our best to make its behavior consistent.

[jeronimo-schreyer]

I think the last check is telling me to deprecate time_left property and create a new one. Is this correct?

We could change it to seconds_left

[jeronimo-schreyer]

sorry about the PR mess

[jeronimo-schreyer]

I think this is more accurate

[KoBeWi]

Setting the property while stopped does nothing and prints an error.