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

Touch handling for sky browser #134

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Touch handling for sky browser #134

wants to merge 3 commits into from

Conversation

ylvaselling
Copy link
Collaborator

@ylvaselling ylvaselling commented Feb 13, 2023

This PR features touch handling for skybrowser

  1. Panning with one finger interaction with wwt
  2. Pinching for zooming in and out over wwt

Pinching needs to be tested on a touchscreen

Copy link
Collaborator

@WeirdRubberDuck WeirdRubberDuck left a comment

Choose a reason for hiding this comment

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

It almost works, but it both moves and zooms when pinching. The zooming is also "jumpy", but I think that might be partly/mainly because of the both the move and pinching is triggered simultaneously. See video

Note that when using multiple fingers, the startInteraction function is called when the first finger touches the screen. I think this might the reason for the behavior, since for the first finger, it will register as dragging... But somehow, some zooming still happens 🤔

I would offer to look at it but can't prioritize this right now, unfortunately

Project.Name.mp4

return position;
}

function scrollZoom(scroll) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "scroll" value here is a bit ambiguous. What doe sit mean? Does it deserve a more describing name?

else { // mouse
position = [interaction.clientX, interaction.clientY];
}
if (!position) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This if-statement seems unnecessary. If !position is true, it means that it's undefined, which means that we will return undefined even if we just return position as is

skybrowserApi.stopAnimations(browserId);
}

function handleDrag(interaction) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is nitpicky, but as this is an event handler function, I would prefer to call the interaction variable event instead. To make it clear that it is actually an object of the javascript event type :) The same goes for all the interaction variables in this file

}

function handleDrag(interaction) {
const end = getClientXY(interaction);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is a position, or rather, multiple positions, the name should reflect that. It's also the current position rather than the end position

Just positions?

@alexanderbock
Copy link
Member

Can confirm. Moving with touch works great, but when pinching, it looks like the movement of the first (?) finger is not disregarded when doing the pinch-to-zoom

@WeirdRubberDuck WeirdRubberDuck marked this pull request as draft September 1, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants