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.

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

add cursor event listeners to canvas, not window #3179

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

ngokevin
Copy link
Member

@ngokevin ngokevin commented Oct 19, 2017

Description:

Cursor only cares about actual touch events.

This fixes an issue where vive-controls touchstart bubbled up to window and cursor was receiving them.

dmarcos
dmarcos previously approved these changes Oct 19, 2017
@wmurphyrd
Copy link
Collaborator

Another option would be to register those onMouseMove listeners on the canvas like the onCursorDown/Up listeners, as this would take them out of the bubble path of controllers and other scene elements.

I thought it was odd they were on the window the last time I was in there. Perhaps just an artifact of the merge from the previous mouse cursor component.

@ngokevin
Copy link
Member Author

Thanks, better idea.

@ngokevin ngokevin changed the title have cursor ignore vive-controls/custom touchstart events add cursor event listeners to canvas, not window Oct 21, 2017
@ngokevin ngokevin dismissed dmarcos’s stale review October 21, 2017 01:55

changed approach

@ngokevin
Copy link
Member Author

Needs another review due to changing the solution.

@dmarcos
Copy link
Member

dmarcos commented Oct 23, 2017

The problem with this approach is that if you have an embedded scene that it's small on screen it won't be a good experience.

@dmarcos
Copy link
Member

dmarcos commented Oct 23, 2017

Oh sorry. looking at wrong file

@dmarcos
Copy link
Member

dmarcos commented Oct 23, 2017

Failing tests

@ngokevin
Copy link
Member Author

Fixed. The embedded scene example works fine, same as before for me.

@dmarcos dmarcos merged commit 6959de2 into aframevr:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants