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

simple fix for Gear VR and Oculus Touch controller issues (fixes #3447 and #3483) #3489

Merged
merged 2 commits into from
Mar 23, 2018
Merged

simple fix for Gear VR and Oculus Touch controller issues (fixes #3447 and #3483) #3489

merged 2 commits into from
Mar 23, 2018

Conversation

machenmusik
Copy link
Contributor

Per discussion on #3429, this is a simpler fix avoiding RegExp usage.

@machenmusik machenmusik mentioned this pull request Mar 23, 2018
Closed
dmarcos
dmarcos reviewed Mar 23, 2018
View reviewed changes
src/utils/tracked-controls.js Outdated
if (prefix && controller.id.indexOf(prefix) === -1) { matches = true; }
for (var j = 0; j < filterIdPrefixes.length; j++) {
var prefix = filterIdPrefixes[j];
console.log(prefix, ' ?= ', controller.id);
Copy link
Member

Choose a reason for hiding this comment

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

don't need log

@machenmusik machenmusik changed the title memoize split strings to reduce garbage; fix matching bugs simple fix for Gear VR and Oculus Touch controller issues (fixes #3447 and #3483) Mar 23, 2018
@donmccurdy donmccurdy added this to the 0.8.X milestone Mar 23, 2018
dmarcos
dmarcos reviewed Mar 23, 2018
View reviewed changes
src/utils/tracked-controls.js Outdated

function splitOnPipeOnce (stringToSplit) {
var rtn = splitOnPipeMap[stringToSplit];
if (!rtn) {
Copy link
Member

Choose a reason for hiding this comment

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

Early return if (rtn) { return; }

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'm not sure it actually looks better now, but I understand your stylistic preference.)

@machenmusik
Copy link
Contributor Author

all good now I think

ngokevin
ngokevin requested changes Mar 23, 2018
View reviewed changes
Copy link
Member

@ngokevin ngokevin left a comment

Choose a reason for hiding this comment

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

Let's define/pass the prefixes as an array always, we don't really need to keep it in a string.

@machenmusik
Copy link
Contributor Author

The prefix also needs to be passed into tracked-controls, not sure that will survive as array...?

@machenmusik
Copy link
Contributor Author

If we don't mind doing the split each time, can remove the split once map stuff and it will look much more like what is in there now, just not broken. Is that preferable?

@machenmusik
Copy link
Contributor Author

Have "put the candle back" to provide a minimal fix to what is already in master.
Hopefully we can agree to merge the simple fix, and subsequently discuss optimizations.

@machenmusik
Copy link
Contributor Author

machenmusik commented Mar 23, 2018 •
edited

Travis CI failure, but seems entirely unrelated

@machenmusik
Copy link
Contributor Author

bumped it, travis ok now. @ngokevin what do you think?

@ngokevin ngokevin force-pushed the simple-fix-for-gearvr-touch branch from bd13d1b to f47a61f Compare March 23, 2018 18:28
@ngokevin ngokevin merged commit f69494d into aframevr:master Mar 23, 2018
This was referenced Mar 23, 2018
Closed
Closed
Merged
@machenmusik machenmusik mentioned this pull request Mar 31, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmarcos dmarcos dmarcos left review comments

@ngokevin ngokevin ngokevin requested changes

Assignees
No one assigned
Labels
needs changes
Projects
None yet
Milestone
0.8.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@machenmusik @dmarcos @ngokevin @donmccurdy