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

fix mobile touch coordinates #4397

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

yakyouk
Copy link
Contributor

@yakyouk yakyouk commented Jul 8, 2021

Fixes buttons partially clickable or not clickable on mobile
#4075
#4297
#4394

Touch coordinates calculation in shouldMoveCursor was using window.innerWidth and window.innerHeight, but the a-frame canvas is not always filling the window, e.g. with the new UI's toolbar.

Changed window.innerWidth and window.innerHeight to a-canvas element's width and height.
In case the canvas is further embedded in a UI in the future, the code also subtracts the canvas' left and top offsets from touch.clientX and touch.clientY.

Notes:

  • This does not fix the issue where taps on singleActionButton buttons don't work for users who do not have the right to move objects.
  • On an iPhone you have to go to Settings > Safari and turn off Block Pop-ups and Fraudulent Website Warning, or else the link silently fails to open.

┆Issue is synchronized with this Jira Task

Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

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

Yikes good catch. I had assumed the reports we were getting about this were just about our touch controls in general being pretty floaty and unreliable (they need to be reworked) but this fixes a clear regression from adding the toolbar/sidebar. Made some minor suggestions you can apply if you like, but I can also just handle them when merging.

@@ -41,10 +41,11 @@ function shouldMoveCursor(touch, raycaster) {
return true;
}
const rawIntersections = [];
const canvas = document.querySelector(".a-canvas").getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

This only gets called in touchStart anyway so its not a huge deal, but querySelectors can be really slow so we try to avoid them when we can. Also the use of left and top on the next lines threw me because of the variable name, didn't notice the getBoundingClientRect() at first

Suggested change
const canvas = document.querySelector(".a-canvas").getBoundingClientRect();
const canvasRect = AFRAME.scense[0].getBoundingClientRect();

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 wanted to use AFRAME.scene[0] but for some reason it includes the toolbar as well, whereas the canvas doesn't.
image
But we can store a ref to canvas to avoid querying it every time.

Copy link
Contributor Author

@yakyouk yakyouk Jul 10, 2021

Choose a reason for hiding this comment

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

I pushed some changes:
We actually have a ref to canvas in this.canvas, and I store canvas rect in this.canvasRect and update it on resize using ResizeObserver.
Since shouldMoveCursor is outside of the class and does not see this.canvasRect, I pass rect to the function as well.

raycaster.setFromCamera(
{
x: (touch.clientX / window.innerWidth) * 2 - 1,
y: -(touch.clientY / window.innerHeight) * 2 + 1
x: ((touch.clientX - canvas.left) / canvas.width) * 2 - 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
x: ((touch.clientX - canvas.left) / canvas.width) * 2 - 1,
x: ((touch.clientX - canvasRect.left) / canvasRect.width) * 2 - 1,

x: (touch.clientX / window.innerWidth) * 2 - 1,
y: -(touch.clientY / window.innerHeight) * 2 + 1
x: ((touch.clientX - canvas.left) / canvas.width) * 2 - 1,
y: -((touch.clientY - canvas.top) / canvas.height) * 2 + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
y: -((touch.clientY - canvas.top) / canvas.height) * 2 + 1
y: -((touch.clientY - canvasRect.top) / canvasRect.height) * 2 + 1

(touch.clientX / this.canvas.clientWidth) * 2 - 1,
-(touch.clientY / this.canvas.clientHeight) * 2 + 1
((touch.clientX - this.canvasRect.left) / this.canvasRect.width) * 2 - 1,
-((touch.clientY - this.canvasRect.top) / this.canvasRect.height) * 2 + 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated calculation here too

@wmurphyrd
Copy link
Contributor

@netpro2k @yakyouk we also have a fix for this, since the main issue was that shouldMoveCursor was casting a slightly different ray than what start would later set as assignment.cursorPose, I just re-arranged the code to ensure that both places were able to share a single pose calculation.

immers-space@298f8d0

We have this in production on https://vreign.space if you'd like to test.

Dom, let me know if you'd like to see this in its own PR or combined yakyouk's work here

@netpro2k
Copy link
Contributor

netpro2k commented Jul 20, 2021

Dom, let me know if you'd like to see this in its own PR or combined yakyouk's work here

Yep I think a separate PR would be nice, since this is already ready to merge.

@Softvision-GeluHaiduc
Copy link
Contributor

After verifying some mobile issues I noticed that waypoints and links to other rooms or websites don’t work on mobile devices if the “Create and move objects” setting from room settings is disabled, the issues are not reproducible for the creator of the room but all the other users are affected.
I logged issue #4691 to track this.

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.

5 participants