Optimize hit() and _SAT() #1147
Merged
Conversation
This file contains 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
starwed
commented
Jul 1, 2017
| var SAT = this._SAT(this.map, obj.map); | ||
| SAT.obj = obj; | ||
| SAT.type = "SAT"; | ||
| if (SAT) finalresult.push(SAT); |
There was a problem hiding this comment.
Fun fact: when _SAT returned false, this code was previously trying to set the obj and type properties on that value. This 'works' in javascript in the sense that it doesn't produce an error; instead, the boolean primitive is implicitly coerced to an object, the value is set on that object, and then the object is ignored (since it isn't assigned or referenced anywhere.) I think the intermediate object was probably getting optimized away (since I didn't see this code get flagged as a source of allocations) but this was at best very confusing. (Here is an article that describes what happens when you set properties on primitives.)
The main win here is reducing the number of allocations. hit() was also doing an intermediate check between the broad-phase search and the _SAT collision test. I believe this increases the amount of work in the common case - Allow results object to be passed to hit() - Flatten the results of _SAT to reduce allocations a bit when hits are found. - Optimize hit a bit to reduce the amount of work done - Remove an intermediate overlap check; it's unnecessary when we're already using broad-phase + SAT - Assume that the collision component always has map - Return as early as possible
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The main win here is reducing the number of allocations.
hit()was also doing an intermediate check between the broad-phase search and the _SAT collision test. I believe this increases the amount of work in the common case (and in checking performance in Firefox, the check was certainly using up about the same amount of CPU time as the _SAT check itself, so...)nxandnyproperties.