Implement world:targets() as a valid method#311
Conversation
Ukendio
left a comment
There was a problem hiding this comment.
This was pretty good! I appreciate that you took the time and also wrote some good unit tests for this.
|
I believe I got everything you wanted changed. If you see anything else give me a shout and I'll take another look. |
…still passes tests)
|
I have since taken a look and realized... you can optimize this a lot further than I thought... return function(): i53?
if nth == end_count then
return nil
end
local target = dense_array[sparse_array[ECS_PAIR_SECOND(archetype_types[nth])].dense]
nth += 1
return target
endI got curious and tried getting rid of most of the function calls I could. It turns out this approach still passes all the tests I wrote. If needed I can revert this, but I believe the idea is optimization at all costs, and thus I will leave it here for you to decide if this black magic can stay. Yes further optimizations by setting |
|
LGTM! |
Brief Description of your Changes.
Closes #309.
This PR introduces the
world:targets()syntax described in that issue.An an example, assuming the following scenario:
The question of "Who does Alice like?" could be many people.
The following is valid syntax under this PR:
The motivation for this change is to reduce the boilerplate for doing the equivalent action with current methods (and hopefully make it a little faster):
A real world example of where this can be applied is timers. See this discord message for where I got this.
This can be converted to:
Impact of your Changes
Initially this PR was meant to reduce the boilerplate required to do this type of operation. On further investigation after implementing unit tests, performance for the new syntax, upon initial benchmarks, shows promising improvements to performance in most cases. The code for this benchmark can be found here.
Since the start of this PR the benchmark has changed in a good way. We were at 115 μs as a 50% and have dropped down to 100μs!
Old Benchmark
Tests Performed
Unit tests have been added and are passing as of submission of the PR (see changes to tests.luau). Previously mentioned in the motivation section, the benchmarks also show this can be more performant than the current method. Additional unit tests can be added as requested and necessary.
Additional Comments
See: alternative syntax proposal
A comment in the initial issue suggested the following syntax be valid (introduce nth in the iterator):
I do not see a reason why this should be included. It is fairly trivial to include if code review later on reveals a reason to include it.