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

Save allocation by keeping empty lists in _effectsToUpdate #47

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

mosirnik
Copy link
Contributor

Previously, when there were additional bone effects, lists inside _effectsToUpdate had to be reallocated every frame. This patch fixes the issue by keeping empty lists around.

Previously, when there were additional bone effects, lists
inside _effectsToUpdate had to be reallocated every frame.
This patch fixes the issue by keeping empty lists around.
Copy link
Owner

@ManlyMarco ManlyMarco left a comment

Choose a reason for hiding this comment

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

Sorry, I somehow didn't see this PR. It's not perfect but I guess this is a good enough compromise to keep the code fast.

@@ -551,6 +554,12 @@ private void ApplyEffects()
FixBustGravity();
}
}

if (_effectsToUpdate.Count > 2 * numEffects)
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like _effectsToUpdate will be Cleared if there are more than 2 plugins with effects affecting the same bones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is the case. Having another plugin that affects the same bones would not increase _effectsToUpdate.Count. It would increase the sizes of inner lists instead. Also it would increase numEffects, so it would cause _effectsToUpdate to be cleared less often, if anything.

Now admittedly this is all confusing... The idea was that I wanted to prevent _effectsToUpdate from growing indefinitely.

@ManlyMarco ManlyMarco merged commit f34dba2 into ManlyMarco:master Aug 31, 2023
@mosirnik
Copy link
Contributor Author

Sorry, I somehow didn't see this PR. It's not perfect but I guess this is a good enough compromise to keep the code fast.

No worries, thanks for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants