Add property change guards to Sprite2D, Sprite3D and AnimatedSprite2D

[miv391]

Implements this proposal: godotengine/godot-proposals#8510

If a property's setter is called with the property's current value, return immediately instead of doing anything costly.

My test creates 10000 Sprite2Ds.

func _ready():
	for y in 100:
		for x in 100:
			var s = SPRITE.instantiate()
			s.position = Vector2(x* 10, y* 10)
			add_child(s)

Each sprite has a script which does some combination of these simple setters (or just pass).

func _process(delta):
	pass
	flip_h = true
	frame = 0

My test version was built with Visual Studio and it's performance seems to be a bit lower than the official version. "% compared to pass" numbers are calculated from release version fps numbers.

	master, debug fps	master, release fps	% compared to pass	PR, debug fps	PR, release fps	% compared to pass
pass	73	105	100%	74	98	100%
flip_h = true	35	48	46%	32	92	94%
frame = 0	50	95	91%	60	91	93%
flip_h = true and frame = 0	31	47	45%	30	89	91%

It may seem odd to just to set flip_h = true, but it is a very common practice to set sprite's direction in every frame like this:

	flip_h = velocity.x > 0

So there are a lot of 2D games which are setting flip_h in every frame without actually changing it.

Production edit: Closes godotengine/godot-proposals#8510

[arkology]

I have one really stupid question about code style. Suggested code follows pattern if (class_variable == func_arg), so it checks if current sprite property is same as newly set. How about to check opposite, if newly set is same as sprite property - so we focus on function argument, not sprite property? Of course this is absolutely the same in terms of functionality but I think it has slight logical difference.
Is there any style recommendations about it in Godot codebase or it doesn't actually make any sense to care about? :)

(And of course I support this guards addition, without it some users could have redraw spam without even knowing about it.)

[KoBeWi]

I checked some classes and we aren't ordering this consistently.

[miv391]

I used the same ordering I had previously seen in some Control nodes. At least I tried to use the same ordering in every function :)

I personally like this order as in the actual assignment the order is same, so it is (at least for me) easy to see how these two lines are connected.

	if (vflip == p_flip) {
	...
	}
	vflip = p_flip;

[akien-mga]

Yeah the order used here is the most common in my experience, and the most readable IMO.

[kleonc]

Could be also added to SpriteBase3D::set_centered etc. (in sprite_3d.cpp).

[AThousandShips]

Also unify code in Sprite3D::set_region_rect, and the different syntax in SpriteBase3D::set_alpha_hash_scale etc.

[miv391]

Could be also added to SpriteBase3D::set_centered etc. (in sprite_3d.cpp).

Done.

Also unify code in Sprite3D::set_region_rect, and the different syntax in SpriteBase3D::set_alpha_hash_scale etc.

Done.

I didn't add a change guard to set_modulate:

godot/scene/3d/sprite_3d.cpp

Lines 324 to 328 in 84692c6

	void SpriteBase3D::set_modulate(const Color &p_color) {
	modulate = p_color;
	_propagate_color_changed();
	_queue_redraw();
	}

That function propagates the color to child nodes and I have no idea what would happen if you add new child nodes between setting modulation color like this:

sprite3d.modulate = Color.RED
sprite3d.add_child(some_node)
sprite3d.modulate = Color.RED

At least now without the change guard the color will be propagated to the child node.

[YuriSizov]

That function propagates the color to child nodes and I have no idea what would happen if you add new child nodes between setting modulation color like this:

It doesn't actually propagate any values, it just triggers a redraw (which makes queue_redraw in this method redundant btw). It's safe to add a guard there, newly added children would of course be drawn on their own.

[YuriSizov]

Other than the above, seems good to me. Feel free to do the adjustments, and then it can be merged.

[miv391]

Adjustments done.

[YuriSizov]

Good work, needs a rebase after #85317. I merged it first since it fixes a problem and is generally more involved.

[YuriSizov]

Thanks!