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

Speed up distanceBetweenTwoPoints, and standardize getGestureProgess usage #54

Merged
merged 3 commits into from Aug 30, 2018

Conversation

mvanderkamp
Copy link
Contributor

I decided to develop my own fork in its own direction as a way to gain some JavaScript experience, but I want to keep contributing to this project, so I'm going to maintain a 'mergeback' branch where I'll put updates and fixes that I think could help the project out. I'll make sure to avoid pushing more updates to the branch until any previous PR I've opened from it has been closed.

Here's a couple fixes that I think could be useful:

  • Use Math.hypot() in util.js inside distanceBetweenTwoPoints(). It's what this function is doing anyway, and this internally defined function should be able to solve the problem faster and more reliably. I also got rid of the Math.round() line, because I don't see any benefit to using computational resources to truncate the precision of the function like that. If someone wants only two decimal places in their output they can use Number.prototype.toFixed().
  • Standardize the way that getGestureProgress() is called. Most gestures were passing in this.getId() but a couple were passing in this.type. It seems to me that they should be consistent.

mvanderkamp and others added 3 commits August 30, 2018 09:24
Merge zingtouch master into mergeback branch
Most gestures were passing this.getId() into getGestureProgress, but a
couple of the gestures were passing in this.type. As far as I can tell
the whole 'progress' mechanism depends on consistency across all
Gestures to prevent collisions, so I switched them all to this.getId().
@mvanderkamp mvanderkamp changed the title Updates Speed up distanceBetweenTwoPoints, and standardize getGestureProgess usage Aug 30, 2018
@mike-schultz
Copy link
Collaborator

Hey @mvanderkamp thanks for the contributions to the library! Curious, do you have a specific you case you are using the library for?

@mike-schultz mike-schultz merged commit 371d3ca into zingchart:master Aug 30, 2018
@mvanderkamp
Copy link
Contributor Author

I'm working on a project for an HCI research lab which involves allowing multiple users across many devices to interact with a canvas. Specifically, I've been tasked with building an API that allows others to quickly build apps that do this, so I need a good, simple to use, and robust gesture library. I was told to use Hammer.js, but it doesn't look like it has been maintained for a few years, and I liked the smaller tech stack of this project.

Since we're already deep into breaking changes since the last publish, do you mind if I propose some other changes to the library? Specifically, I'm curious about why the angular measurements are converted to degrees. We could save computation time by just leaving them as radians, which will also make the library easier to use, seeing as CSS and the trig functions inside the Math object seem to use radians.

@mike-schultz
Copy link
Collaborator

Thanks for the background. Yeah feel free to suggest any changes that you might find useful. We started this project a couple years back with the intention of mobile support for one of our libraries but we haven't done much with it so far. As you mentioned, I did up the version to 2 as combining pan/expand is in fact breaking but wanted to add some more functionality for a bigger major version release.

I've typically seen css being used in degrees and added that as more of a convenience, but I agree with just keeping it as radians and letting the user convert to degrees if necessary.

@mvanderkamp
Copy link
Contributor Author

Ahh see this is where I'm still learning. It looks like the CSS transform: rotate() function accepts radians or degrees or "turns" or... gradians?? Interesting!

I'll put together a PR that supplies this change. I might also get rid of a lot of the incidental calculations happening inside those angular functions, unless they're mission critical.

Do you prefer if I just submit the source changes and let you rebuild the distribution code?

@mike-schultz
Copy link
Collaborator

Yeah that's fine. Rebuilding the dist folder is only really critical when a release is actually tagged; otherwise it will just pollute git with a lot of intermittent built code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants