Replacing selectforview#50
Merged
Merged
Conversation
The `EntityFilter` struct will be used to replace the `SelectForView` and most of the `RegistryContainer` code as a simpler way to keep track of entities that should be acted on. Instead of saving a list of what components need to be selected and using the same tag to mark the currently selected ones, any function that wants to work on a subset of entities based on tags needs their caller to assign that entity before calling. The `EntityFilter`'s job is simply to make it easier to run views or groups over those chosen tags without needing to type that tag all over the function. This commit is making sure the filter works by replacing a small `SelectForView` usage with it. The one replaced also tests a feature that adds a tag based on pre-existing components as, on rare occasions, a component that isn't used in the systems being called is part of the filter.
This had a decent speed improvement for simulate turn, but not the other two. That makes sense as the others are not set up to test for speed with calculating crits. Also, the function wasn't tested, so I've add scenarios to the calculate damage tests.
Profiling showed this function was one of the biggest time sinks due to the old selection. Replacing that with the EntityFilter over doubled the speed of the 8000+ input benchmarks.
Those two files and the functions they call no longer use `SelectForView`. The result is a giant speed improvement for large input counts, with the most being the 65536 inputs benchmark becoming 5 times faster! The best part is the larger input counts now no longer greatly increase their time per input times as inputs increase. That's a very good sign and it makes me wonder how much the random input benchmarks would be improving if they could reach those high entity counts.
Analyze effect is just an extension of CalcDamage, so removing SelectForView is both simple and had no speed implications.
This removes all remaining uses of SelectForView and the files that defined/tested it. The big change in Analyze Effect speeds for larger one battle many inputs benchmarks is from changing SkippedInputCount to the correct, larger type. The previous type was too small to hold the correct value, so way more clones were being made than were needed.
I looked over the code to find places the event filters may be incorrect. A few were found, but most of these changes are small ones that should've been addressed. I suspect the benchmark speed improvements come from changing `emplace_or_replace` to `emplace` in CalcDamage.cpp
Contributor
|
🤖 Passed All One Review Bot Checks ✔️ |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #49