Add property change guards to Sprite2D

[miv391]

Describe the project you are working on

My projects are 2D games.

Describe the problem or limitation you are having in your project

A game developer told in a reddit thread (https://www.reddit.com/r/godot/comments/182gdgi/checking_equality_before_assignment_optimization/) how the frame rate increased considerably when a property change guards for Sprite2D were added to the games C# code.

I made a test of my own in GDScript (v4.2.rc1.mono.official [ad72de508]). The test creates 10000 sprites. Then I tested 4 different cases, where the sprites do different things in _process.

	debug fps	release fps
sprite doesn't do anything	73	105
flip_h = true in every frame	35	48
frame = 0 in every frame	50	95
flip_h = true and frame = 0 in every frame	31	47

The cost of setting properties even without actually changing them seems to be huge.

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

My proposal is to add change guards to Sprite2D's properties. I am not very familiar to Godot's source code, but at least some Control use change guards extensively.

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

For example:

void Sprite2D::set_flip_v(bool p_flip) {
	vflip = p_flip;
	queue_redraw();
}

would be changed to

void Sprite2D::set_flip_v(bool p_flip) {
	if (vflip == p_flip) {
		return
	}
	vflip = p_flip;
	queue_redraw();
}

Some properties, like region and frame, already check if the property actually changed, but they still do things like emitting signals:

void Sprite2D::set_frame(int p_frame) {
	ERR_FAIL_INDEX(p_frame, vframes * hframes);

	if (frame != p_frame) {
		item_rect_changed();
	}

	frame = p_frame;

	emit_signal(SceneStringNames::get_singleton()->frame_changed);
}

This would be changed to

void Sprite2D::set_frame(int p_frame) {
	ERR_FAIL_INDEX(p_frame, vframes * hframes);

	if (frame == p_frame) {
		return;		
	}

	item_rect_changed();
	frame = p_frame;
	emit_signal(SceneStringNames::get_singleton()->frame_changed);
}

If this proposal is approved, I can implement this.

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

This can be worked around with a few lines of code, but it will not. Just watch any Godot 2D tutorial video from YouTube and there will be some variation of this:

sprite.flip_h = velocity.x > 0

I'm also assuming that the performance cost of guarding a property change is higher when made with GDScript than when made in Sprite2D's setter.

So this enhancement would be beneficial in very many 2D games. Also, in my opinion it should be Node's responsibility to not to do unnecessary work if properties do not actually change.

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

Cannot be done with an add-on.

[miv391]

Question about coding style (I don't know if this the best place for this kind of question...).

ERR_FAIL_* macros seem to the be first things in the functions. For example current set_frame:

void Sprite2D::set_frame(int p_frame) {
	ERR_FAIL_INDEX(p_frame, vframes * hframes);

	if (frame != p_frame) {
		item_rect_changed();
	}

	frame = p_frame;
	emit_signal(SceneStringNames::get_singleton()->frame_changed);
}

If a change guard is added, this could be written like this:

void Sprite2D::set_frame(int p_frame) {
	ERR_FAIL_INDEX(p_frame, vframes * hframes);
	if (frame == p_frame) {
		return;
	}
	...
}

OR like this:

void Sprite2D::set_frame(int p_frame) {
	if (frame == p_frame) {
		return;
	}
	ERR_FAIL_INDEX(p_frame, vframes * hframes);
	...
}

My opinion is that if nothing is changed, there is no need to pay the performance price of ERR_FAIL_INDEX, no matter how small it is. So I would like to put change guards before ERR_FAIL_*s whenever suitable.

[YuriSizov]

I would suggest you follow established codestyle and keep macros as early as possible. In general always validate the input before you use it.

You also don't need to open a proposal for something like this, it's a minor change and you can add your performance findings to the PR directly.

Edit: All that said, code sample from set_frame seems intentional. Worth looking into the commit history if there is a reason to always set the value.