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.
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
Transform Gizmo Refactor #1085
Transform Gizmo Refactor #1085
Changes from 8 commits
e310017
4fa1ccd
faea318
a2b9e63
b2cfa39
8be2b6f
10010b4
bf9c086
0937d76
1b64d2b
6d0bc2f
6f5f2ab
1e9dbcd
9cd80ac
3181c05
ffdf3ff
03a9d3f
980bf10
2f8c826
8f6c6d8
50f1ce3
47b9301
6dbdc35
18b9383
8f4ded3
38495f7
b2b648a
ceaf33f
269d333
b10ccc7
2fc64cc
1177d0d
ec877bf
bcd59b9
306d807
633d20d
710bc8a
fd519b3
e09be3c
7cb3837
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have the
SceneRenderer._gizmosOnMirabuf
map, you should no longer filtersceneObjects
. In this case, you should make it so that you either make_gizmosOnMirabuf
publicly available or have a method in SceneRenderer to test if some object has a gizmo (which would be as simple asthis._gizmosOnMirabuf.has(mirabuf_id)
from inside SceneRenderer).As long as the mirabuf instance id that would be the key in
gizmosOnMirabuf
is available inside this function that I'm commenting on, you can then reduce this from O(n) to O(probably 1) since it would only require a Map lookup instead of an array filter and then foreach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here - use the
_gizmosOnMirabuf
map.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a switch be better here? I'm not sure if the if will be optimized, especially since you're checking inputs in one of the conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure object is always going to be non-null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know
this._parentObject
can only be aMirabufSceneObject
orundefined
but I still prefer a truthiness check rather than checking if it's not exactly undefined. It doesn't really matter very much in this instance though.