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

cache available recipe list in craft menu #74178

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

mischief
Copy link
Contributor

Summary

Performance "cache available recipe list in craft menu"

Purpose of change

when filling in the recipe result pane with information about derived crafts, get_group_available_recipes and get_available_nested is called every time the ui is navigated. this makes it slow to scroll through the recipes or batch craft list, and i don't think is necessary since the available recipes aren't changing while the menu is open.

Describe the solution

stash the available recipes in the character for reuse and then invalidate the list once we exit the craft menu.

Describe alternatives you've considered

Testing

for me, this makes the craft menu much more responsive, especially on debug/non-lto builds where it is not optimized well.

Additional context

i tried this just as a hack, since i am new to the code, and there is likely a cleaner solution 😀

@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling [C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels May 30, 2024
Copy link
Member

@RenechCDDA RenechCDDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks short and sweet to me, but maybe consider making the cache private with a getter?

when filling in the recipe result pane with information about derived
crafts, get_group_available_recipes and get_available_nested is called
every time the ui is navigated. this makes it slow to scroll through the
recipes or batch craft list, and i don't think is necessary since the
available recipes aren't changing while the menu is open.

stash the available recipes in the character for reuse and then
invalidate the list once we exit the craft menu.

for me, this makes the craft menu much more responsive, especially on
debug/non-lto builds where it is not optimized well.
@mischief mischief marked this pull request as ready for review May 31, 2024 17:47
@mischief
Copy link
Contributor Author

@RenechCDDA PTAL. i've just used the turn counter to invalidate the cache, and don't cache in the tests to avoid the test show_available_recipes_with_item_as_an_ingredient failing.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jun 1, 2024
@dseguin dseguin merged commit 62ffe8d into CleverRaven:master Jun 1, 2024
35 of 43 checks passed
@mischief mischief deleted the craftmenugobrrt branch June 1, 2024 20:38
@RenechCDDA
Copy link
Member

@RenechCDDA PTAL. i've just used the turn counter to invalidate the cache, and don't cache in the tests to avoid the test show_available_recipes_with_item_as_an_ingredient failing.

I am late but this looks fine too! Thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants