Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tidy up loose ends from #3423 (fixes #3447 and #3483) #3429

Closed
wants to merge 1 commit into from
Closed

tidy up loose ends from #3423 (fixes #3447 and #3483) #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/gearvr-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var GEARVR_CONTROLLER_MODEL_BASE_URL = 'https://cdn.aframe.io/controllers/samsun
var GEARVR_CONTROLLER_MODEL_OBJ_URL = GEARVR_CONTROLLER_MODEL_BASE_URL + 'gear_vr_controller.obj';
var GEARVR_CONTROLLER_MODEL_OBJ_MTL = GEARVR_CONTROLLER_MODEL_BASE_URL + 'gear_vr_controller.mtl';

var GAMEPAD_ID_PREFIX = 'Gear VR|GearVR|Oculus Go';
var GAMEPAD_ID_PREFIX = '((Gear VR)|(Oculus Go))';

/**
* Gear VR controls.
Expand Down
12 changes: 4 additions & 8 deletions src/utils/tracked-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,19 +88,15 @@ function findMatchingController (controllers, filterIdExact, filterIdPrefix, fil
var i;
var matchingControllerOccurence = 0;
var targetControllerMatch = filterControllerIndex || 0;
var filterIdPrefixes;
var filterIdRegex;
if (filterIdPrefix && filterIdPrefix.indexOf('|') >= 0) {
filterIdPrefixes = filterIdPrefix.split('|');
filterIdRegex = new RegExp('^' + filterIdPrefix);
Copy link
Member

@dmarcos dmarcos Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to create a Regexp for each time we want to find and match controllers. Can we instantiate and reuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make filterIdPrefix parameter to take a String or an Array an avoid using RegExps that are always hard to understand and maintain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@dmarcos dmarcos Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regex is fine for me. it's the fastest way. probably just rename the var is good.

}
for (i = 0; i < controllers.length; i++) {
controller = controllers[i];
// Determine if the controller ID matches our criteria
if (filterIdPrefixes) {
var matches = false;
for (var prefix in filterIdPrefixes) {
if (prefix && controller.id.indexOf(prefix) === -1) { matches = true; }
}
if (!matches) { continue; }
if (filterIdRegex) {
if (!filterIdRegex.test(controller.id)) { continue; }
} else
if (filterIdPrefix && controller.id.indexOf(filterIdPrefix) === -1) { continue; }
if (!filterIdPrefix && controller.id !== filterIdExact) { continue; }
Expand Down