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

Optimize enemy loading #591

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

WilliamFr0g
Copy link
Contributor

Probably still kinda bad, but it's a lot less bad

I only just quickly swapped some calls for this to work in love 11 because it was built in love 10, it has not been formally tested

Fewer redundant calls to check if files exist (definitely), less string manipulation (I think), more efficient loading order (probably)
Also removes mysterious parsing for a nonexistent "fireenemy" enemy property
For all 1 mappacks that use .PNG instead of .png (looking at you, Aidan)
@alesan99 alesan99 added enhancement Small improvement change An existing feature should be tweaked bug Something isn't working labels May 7, 2024
Copy link
Owner

@alesan99 alesan99 left a comment

Choose a reason for hiding this comment

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

Haven't tested yet but some case-sensitivity code should be fixed and re-added

enemies.lua Show resolved Hide resolved
@qixils qixils added the needs rebase Created for an older version of the game and needs to be update label Aug 13, 2024
@qixils qixils removed the needs rebase Created for an older version of the game and needs to be update label Aug 13, 2024
Copy link
Collaborator

@qixils qixils left a comment

Choose a reason for hiding this comment

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

Given how many times enemies are (re)loaded I definitely think we should have a perpetual in-memory cache (along the lines of a table mapping filepaths to enemies) somewhere to really optimize enemy loading, or at least smarter logic for when to clear the customenemies table, as currently enemies are still loaded on mappack change as well as reloaded on level change. So while this PR certainly cleans up the code and resolves issues, it doesn't feel especially faster (although I haven't benchmarked it, I see the code for it and trust that it was done). However I think it would make sense to save that for a future PR, as managing the cache alongside the base system seems nontrivial.

Tested briefly and everything seems functional to me.

EDIT: Did do a quick benchmark; I get about 0.3s (x2=0.6s) to load the Kirby mappack's enemies on this branch, and 0.4s (x2=0.8s) on the master branch. 33% improvement isn't too bad, but still a total of 0.6s being spent to actually get into a level in the mappack, with half of that being a useless reload, so definitely room to improve in the future.

@qixils qixils added the needs rebase Created for an older version of the game and needs to be update label Aug 13, 2024
@qixils qixils removed the needs rebase Created for an older version of the game and needs to be update label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working change An existing feature should be tweaked enhancement Small improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants