Performance considerations

[Sch1nken]

First: Thanks for creating this very valuable addon :)

Heya, I am currently investigating some performance optimizations for the ControllerIcons addon.

In my game I have a shop with icons ("buy buttons"). Every now and then these items get refreshed. I chose the lazy method of queue_free()-ing the old buttons and instanciating completely new ones. Part of the Button-Scene is a TextureRect with a ControllerIconTexture.

I've noticed a huge frame time spike in the profiler whenever I refresh these buttons (4 or 8 at a time).

Here are some insights:

• _convert_key_to_path can be easily optimized. Instead of a large match-statement it can be just one small if/else (for mac-os meta key) and then a dictionary lookup. I already did that locally and will probably create a PR soon
• Same for _convert_mouse_button_to_path (though less of a performance uplift).
• DisplayServer.keyboard_get_keycode_from_physical is quite slow on my machine (Linux/X11). Will re-test on my windows machine tomorrow.
• _load_texture_path() gets called A LOT. On a simple scene (just a TextureRect with a ControllerIconTexture, set to be a resource local to scene) it gets called a total of 8 times and (part of that is probably(?) due to Godot internally .duplicate()-ing the ControllerIconTexture on instanciation (due to local to scene being checked)).
• get_matching_event is also a little bit slow (and also gets called quite a lot. For 40 (8 ControllerIconTextures) calls of _load_texture_path() I get 120 calls of get_matching_event

For DisplayServer.keyboard_get_keycode_from_physical I am not sure if caching these can have some unintended side-effects (like OS language/keyboard layout change mid-game resulting in wrong cached values). I will investigate and profile DisplayServer.keyboard_get_keycode_from_physical later this week (probably).

PR incoming soon(tm) for some things on the list.

Edit: I did some tests and it turns out Godot's match-statement is actually pretty optimized (and somehow even faster than a simple dictionary lookup). Not complaining :). I removed the corresponding items from the list.

The massive amount of _load_texture_path() calls are indeed from a local-to-scene-Resource (ControllerIconTexture as part of a TextureRect). This results in massively more calls to _load_texture_path().

Still need to DisplayServer.keyboard_get_keycode_from_physical on windows. But this call is now one of the main functions contributing to the spike in frame time. _get_width() / _get_height() are also somewhat relevant.

I think the most sensible solution (for me, maybe in general) is to have a list of ControllerIconTexture-Resources that just get applied instead of re-instantiating them every time :). Though it might not be feasible for something like a settings menu where all keybinds are listed (for rebinding etc).

I will further investigate the DisplayServer-call. I feel it shouldn't be that slow (though I have little do no overview of how X11 handles stuff like this).

[greenpixels]

I couldn't reproduce the spike on my local machine with recreating icons. (Windows)
So maybe an issue with Linux.

But the amount of calls that an icon texture does in a single tick is still non-neglectible.

@rsubtil
I've locally added functionality that, instead of calling _load_texture_path every time a variable changes, enables a _should_trigger_texture_load variable.

Its setter then looks like this:

var _should_trigger_texture_load = false :
	set(value):
		if _should_trigger_texture_load == value: return
		_should_trigger_texture_load = value
		if _should_trigger_texture_load and ControllerIcons != null:
			ControllerIcons._textures_to_update.append(self)

And in the _process function of ControllerIcons:

for texture in _textures_to_update:
	texture._load_texture_path()
	texture._should_trigger_texture_load = false

This reduced the amount of calls to _load_texture_path in my project by 68% in a single tick.
This introduces a 1-tick delay to texture updates, but I think that is totally okay.

In my sample project when instantiating my controller-remapper it called _load_texture_path 210 times. With this change it reduced it to 69 instead. This could potentially improve the performance for the issues described here aswell, but mainly as a side effect.

Is that something to consider implementing?

[rsubtil]

@greenpixels sorry for the very late response, but I've analysed your solution now, and would like to tinker with it. I also want to make these performance issues more visible with unit-testing, not only to identify hotspots but to also identify regressions.

Are you able to open a MR with your changes? Then I'll also test it locally, and investigate the impact of these changes (the 1-frame delay, while not a major issue, is already another frame delay to the 1-2 existing ones, so adding these up is not viable in the future; I'm especially interested in investigating this to reduce it)

[greenpixels]

Hi, sorry for the late response.
I'll open an MR when I'm back on my computer which should either be tomorrow or the day after.