tidy up loose ends from #3423 (fixes #3447 and #3483) #3429
Conversation
|
@DigiTec can you confirm whether the Oculus Browser fix has been deployed yet? |
|
From #gearvr slack channel today by |
…3429) (aframevr#3250) * remove object allocations from cursor component * only emit raycaster intersection/intersected events if newly intersected * fix disabling of raycaster.showLine * fix disabling raycaster.line getting overridden by tick line updater * remove arrow fn
machenmusik
changed the title
|
built version is at https://rawgit.com/chenzlabs/aframe/fix-oculus-gearvr-controller-dist/dist/aframe-master.min.js for testing convenience |
|
Let's just keep all the strings and merge it. |
| if (filterIdPrefix && filterIdPrefix.indexOf('|') >= 0) { | ||
| filterIdPrefixes = filterIdPrefix.split('|'); | ||
| filterIdRegex = new RegExp('^' + filterIdPrefix); |
There was a problem hiding this comment.
You don't need to create a Regexp for each time we want to find and match controllers. Can we instantiate and reuse?
There was a problem hiding this comment.
I considered that, but for example https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/compile says it's deprecated so didn't seem great. Thoughts?
There was a problem hiding this comment.
My understanding is that because it is passed to tracked-controls, we can't simply make GAMEPAD_ID_PREFIX a RegExp (which is what both @DigiTec and I would ideally prefer) as that will not carry over properly. As a slightly clunky workaround to prevent subsequent construction, we can probably create a global map that holds the RegExp created for a given prefix string... Is that the sort of thing you had in mind here?
There was a problem hiding this comment.
Can we make filterIdPrefix parameter to take a String or an Array an avoid using RegExps that are always hard to understand and maintain?
There was a problem hiding this comment.
The prefix needs to be passed into tracked-controls, not sure that would work as string-or-array. The original version used split to make the array if it saw pipe delimiter, which also makes garbage, but is easier to read and maintain; would you prefer a different PR that doesn't use RegExp at all, but instead provides an alternative solution to the related issues? (i.e. simple bugfix to previous pipe delimited split)
There was a problem hiding this comment.
I would prefer to avoid RegExp if we can. When I saw var GAMEPAD_ID_PREFIX = '((Gear VR)|(Oculus Go))'; it made me feel confused right off the bat 😄
There was a problem hiding this comment.
Ok, although if we want to avoid garbage from split every time, still need to use a map to memorize... Will take a look at it later this evening.
There was a problem hiding this comment.
regex is fine for me. it's the fastest way. probably just rename the var is good.
as discussed in #3423,
use RegExp and update multi-prefix syntax; remove GearVR
(might be best to merge after the Oculus Browser fix is deployed next Tuesday)