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 a timer_started signal to Timer node #11229

Open
Burloe opened this issue Nov 24, 2024 · 4 comments
Open

Add a timer_started signal to Timer node #11229

Burloe opened this issue Nov 24, 2024 · 4 comments

Comments

@Burloe
Copy link

Burloe commented Nov 24, 2024

Describe the project you are working on

Plugin development

Describe the problem or limitation you are having in your project

I found that when developing tools and plugins, it would be nice to have a timer_started signal available to sync up systems/features. For example, if you need to stress test some system for X amount of time. Having an _on_timer_started() could help initiate this stress test.

I've also had the need to use it outside of plugin development. Timed UI notifications that might temporarily disable some buttons, ability timers could've been used to initiate something functionality while an ability is active. Instead of making an ability signal in that case, one could both timer_started and timer_timeout to do the same thing.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Frankly, this isn't a overtly necessary or immensely helpful as I've only needed it a couple of times but it would be nice to have. It's also very easy to implement a custom signal that you emit anytime you start the timer, but having a built in signal would be easier and more approachable for beginners I think.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I have no engine dev or C++ experience but I just looked at the code for the timeout signal and just duplicated it to create and emit a started signal when the timer is started. I'm sure there's more needed or I did something incorrect.

timer.cpp

void Timer::_notification(int p_what) { // Line 33
	switch (p_what) {
		case NOTIFICATION_READY: {
			if (autostart) {
#ifdef TOOLS_ENABLED
				if (is_part_of_edited_scene()) {
					break;
				}
#endif
				start();
				autostart = false;

				emit_signal(SNAME("started")); // Added this at line 45
			}
		} break;

		case NOTIFICATION_INTERNAL_PROCESS: {
			if (!processing || timer_process_callback == TIMER_PROCESS_PHYSICS || !is_processing_internal()) {
				return;
			}
			time_left -= get_process_delta_time();

			if (time_left < 0) {
				if (!one_shot) {
					time_left += wait_time;
				} else {
					stop();
				}

				emit_signal(SNAME("timeout"));
			}
		} break;

		case NOTIFICATION_INTERNAL_PHYSICS_PROCESS: {
			if (!processing || timer_process_callback == TIMER_PROCESS_IDLE || !is_physics_processing_internal()) {
				return;
			}
			time_left -= get_physics_process_delta_time();

			if (time_left < 0) {
				if (!one_shot) {
					time_left += wait_time;
				} else {
					stop();
				}
				emit_signal(SNAME("timeout"));
			}
		} break;
	}
}


void Timer::_bind_methods() {  // Line 195
	ClassDB::bind_method(D_METHOD("set_wait_time", "time_sec"), &Timer::set_wait_time);
	ClassDB::bind_method(D_METHOD("get_wait_time"), &Timer::get_wait_time);

	ClassDB::bind_method(D_METHOD("set_one_shot", "enable"), &Timer::set_one_shot);
	ClassDB::bind_method(D_METHOD("is_one_shot"), &Timer::is_one_shot);

	ClassDB::bind_method(D_METHOD("set_autostart", "enable"), &Timer::set_autostart);
	ClassDB::bind_method(D_METHOD("has_autostart"), &Timer::has_autostart);

	ClassDB::bind_method(D_METHOD("start", "time_sec"), &Timer::start, DEFVAL(-1));
	ClassDB::bind_method(D_METHOD("stop"), &Timer::stop);

	ClassDB::bind_method(D_METHOD("set_paused", "paused"), &Timer::set_paused);
	ClassDB::bind_method(D_METHOD("is_paused"), &Timer::is_paused);

	ClassDB::bind_method(D_METHOD("is_stopped"), &Timer::is_stopped);

	ClassDB::bind_method(D_METHOD("get_time_left"), &Timer::get_time_left);

	ClassDB::bind_method(D_METHOD("set_timer_process_callback", "callback"), &Timer::set_timer_process_callback);
	ClassDB::bind_method(D_METHOD("get_timer_process_callback"), &Timer::get_timer_process_callback);
	
	ADD_SIGNAL(MethodInfo("timeout"));
	ADD_SIGNAL(MethodInfo("started")); // Added this at line 219

	ADD_PROPERTY(PropertyInfo(Variant::INT, "process_callback", PROPERTY_HINT_ENUM, "Physics,Idle"), "set_timer_process_callback", "get_timer_process_callback");
	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "wait_time", PROPERTY_HINT_RANGE, "0.001,4096,0.001,or_greater,exp,suffix:s"), "set_wait_time", "get_wait_time");
	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "one_shot"), "set_one_shot", "is_one_shot");
	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "autostart"), "set_autostart", "has_autostart");
	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "paused", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NONE), "set_paused", "is_paused");
	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "time_left", PROPERTY_HINT_NONE, "suffix:s", PROPERTY_USAGE_NONE), "", "get_time_left");

	BIND_ENUM_CONSTANT(TIMER_PROCESS_PHYSICS);
	BIND_ENUM_CONSTANT(TIMER_PROCESS_IDLE);
}

timer.zip

If this enhancement will not be used often, can it be worked around with a few lines of script?

Like I said, this can easily be replicated by creating your own signal in a scene with any timer and emitting it as you call x_timer.start().

Is there a reason why this should be core and not an add-on in the asset library?

Yes, this would be such a small addition to an already existing node that it wouldn't be realistic to add as an add-on/plugin.

@RedMser
Copy link

RedMser commented Nov 24, 2024

I don't think it's a common pattern to emit signals when the user code calls a method, since you have full control over this already. Signals are only helpful if something outside of your control happens (e.g. UI interactions) which you need to be informed about.

For listening on autostart you can use the Node.ready signal in your script also, so this is fairly easy to work around.

@Burloe
Copy link
Author

Burloe commented Nov 24, 2024

I don't disagree and I'm sure there are better ways like you the one you mentioned. I also understand that you have control of when the timer is started outside of autostart through other means(other node signals, InputEvents, method calls etc). But having the option to contain in the same node. Pressing a Delete button for example, which brings up an "Are you sure?" prompt that cancels after 10s of inaction. Then you'd have signals from button nodes and a timeout signal from the timer. Instead you could handle both in the timer signals. That's a bad example but I can't think of a better one right now...

As I said, I haven't had the need for this very often and it's not like it would solve a big issue. In my opinion, it would be a good option to have but it's just a suggestion.

@Mickeon
Copy link

Mickeon commented Nov 25, 2024

This feels tangentially related to

@Calinou Calinou changed the title timer_started Signal to Timer node Add a timer_started signal to Timer node Nov 25, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Nov 25, 2024

Also #6791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants