Drastically optimize pause menu elements #300
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Always the first step.
_process runs nothing but this function, and it's not used elsewhere. No need to separate it.
WHOA. Just by doing this, I drop the total process time for this script from ~11 ms to ~4.5 ms. I gotta keep this going!
For use later in refactoring button name fetches.
This drops processing time from ~4.5 ms down to ~0.25 ms. This puts it level with Entity._physics_process in T1R1. Frankly a bit shocking that an options menu should even be that intensive, though. The script already has machinery to mostly operate without _process(). Looking into how to take that to 100% so we can skip that function entirely.
...instead of proactively running update_list() for every rebind option, every frame. This allows us to completely remove the _process() function from rebind_options. Script functions now profile as sub-1 ms on my machine (~0.8).
If we're already caching other input translations, might as well finish the job. Should be zero redundant translation calls now, apart from when translations changed (and I know how to fix that).
Accomplished by statically caching the name of the current locale, only updating the strings if the cache is out of date, and updating the cached locale as soon as the first rebind option finishes (so the others don't see it).
Perhaps it's not taking 11 milliseconds, but this script's process function was up there with water and entities last I checked--which, again, is pretty outrageous for a static pause menu element! Now it only updates when the language changes, not once every frame. While I'm here, refactor the script to potentially support the fancy font in other languages someday. That's really just using a match statement instead of an if, though.... (and commenting it)
Bit of "why use a separate function when a comment will do" here. Also, updating for-loop variable names to accurately describe what's actually contained in them (as far as we care). And commenting, of course.
GTcreyon
suggested changes
Oct 19, 2024
| output = output.trim_suffix(", ") | ||
| return output | ||
|
|
||
| key_list.text = output |
There was a problem hiding this comment.
The game crashes here for me. "Invalid assignment of property or key 'text' with value of type 'String' on a base object of type 'Nil'."
I believe this happens because update_list() is called before @onready can trigger, when the translation change notification is received. I'm not sure why this didn't occur while you were working on it, perhaps Godot updates, or perhaps something to do with my own language settings? (Apologies for the belated response to your PR.)
| refresh_caps() | ||
| func _notification(what): | ||
| if what == NOTIFICATION_TRANSLATION_CHANGED: | ||
| match TranslationServer.get_locale().substr(0, 2): |
There was a problem hiding this comment.
A similar issue occurs here, with a similar solution. (See my other comments.)
Suggested change
| match TranslationServer.get_locale().substr(0, 2): | |
| if level_name == null: | |
| await ready | |
| match TranslationServer.get_locale().substr(0, 2): |
| # already done that). | ||
| if locale_current != TranslationServer.get_locale().substr(0, 2): | ||
| _update_translations() | ||
| # Update the list. | ||
| update_list() |
There was a problem hiding this comment.
The aforementioned crash can be resolved like so:
Suggested change
| update_list() | |
| if key_list == null: | |
| await ready | |
| update_list() |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
On profiling the normal gameplay loop, it was discovered that several pause menu elements had startlingly high performance costs. This PR optimizes them down to reasonable levels.
The bigger offender was the function
_get_joypad_buttonsinrebind_option.gd, which was profiled as using 11(!) ms of CPU time in each frame. This function returns an array of arrays of button names, indexed by logical button index and by controller brand. On inspection, it was found that this function was called once for every binding of every action, every frame, and doing the same 25 string translations every time (which suggests 25 disk reads). Assuming one binding for every action--which I know is lowballing it--that's25 * 25 = 625total translations per frame, literally 98% of which are repeated work.Optimizing the script thus becomes a matter of reusing work. This can be done:
_get_joypad_buttons. Because some buttons share names between brands, some translated strings can be cached before constructing the return array, which skips 15 extra translations per function call. This step alone reduced CPU time from 11ms to 4ms._get_joypad_buttonsdoesn't depend on which action or mapping it's used for--only the selected locale. By storing the result in a static variable, it can be reused between instances, and the disk reads(?) involved in translation lookup can be skipped for all but the first user._get_joypad_buttonsreturns the same result as last frame.rebind_optionalready has machinery to receive the engine's locale-change notification, so it's fairly trivial to only set_get_joypad_buttonswhen the game starts or the locale changes. Combined with the previous step, this reduced CPU time from 4ms to ~0.3ms, about level withEntity._physics_processandDejitterGroup._physics_process.rebind_option's state doesn't change between frames. By adding (and connecting to) a signal to the reset-mappings button, it becomes possible to completely remove_processand instead update rebind options reactively. Naturally, this meansrebind_optionis no longer charting at all on most profiler frames.Some similar string-caching-via-statics work was also done to
get_joypad_motion_name. This is mostly for methodological consistency, but if somebody were to make very strange mapping choices ("wassup YouTube, today I'm gonna be playing SM63R using just the left and right analog stick!"), I could see it saving actual performance.I also chose to optimize
level_info.gd. This was much less of a problem script, but it still seemed appropriate to switch it from detecting locale via_processto detecting via engine notification, especially sincerebind_optionhad shown me how to do that. This saves about 0.15ms--not much, but still seemed excessive for a static menu element.Some mild refactors have also been sprinkled in, including obligatory reordering functions to standard and commenting everything I touch.